Bug 25287

I just wanted to confirm that this is a bug, and is actually causing the jQuery javascript library to not be able to disable stylesheets in webkit browsers. http://dev.jquery.com/ticket/6643

Indeed - the property is implemented in bindings, but it has no effect. See also: bug 19958.

<rdar://problem/8072250>

Created attachment 66947 [details] Fix HTMLStyleElement.disabled and also includes test case for the same.

> + // HTML5 specified that we should be able to handle a NULL styleSheet but this never happens. Not even for a detached element? E.g. 'document.createElement("style").disabled = true'?

(In reply to comment #5) > > + // HTML5 specified that we should be able to handle a NULL styleSheet but this never happens. > > Not even for a detached element? E.g. 'document.createElement("style").disabled = true'? Yes, it still returns stylesheet object, for example if you do test = document.createElement("style"); test.sheet It would still return stylesheet object and is not null.

Created attachment 66990 [details] Updated bug fix for 25287 with changelog

Comment on attachment 66990 [details] Updated bug fix for 25287 with changelog Updated bug fix for 25287 with changelog

Attachment 66990 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/html/HTMLStyleElement.cpp:111: Use 0 instead of NULL. [readability/null] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 67718 [details] Updated bug fix for 25287 with fixing style

Comment on attachment 67718 [details] Updated bug fix for 25287 with fixing style View in context: https://bugs.webkit.org/attachment.cgi?id=67718&action=review Overall, this patch is heading in the right direction, but r- to add a test case and to fix the disabled() method. It may be worth writing an XHTML test case as well. I seem to recall that stylesheets are handled somewhat differently in XHTML, but I don't recall the details. > LayoutTests/fast/html/script-tests/htmlstyle-disable.js:7 > +console.appendChild(testParent); This test should also cover a detached element (see Comment #5 and Comment #6). The attached element case looks good! > LayoutTests/fast/html/script-tests/htmlstyle-disable.js:27 > + > + > +successfullyParsed = true; > + > + > + Nit: There's excessive whitespace in this JavaScript file. Usually one blank line is sufficient. > WebCore/html/HTMLStyleElement.cpp:32 > -#include "ScriptableDocumentParser.h" > #include "ScriptEventListener.h" > +#include "ScriptableDocumentParser.h" > + The headers were in the correct order before. Also, please don't add unnecessary whitespace. > WebCore/html/HTMLStyleElement.cpp:112 > + StyleSheet* styleSheet = this->sheet(); > + ASSERT(styleSheet); > + // HTML5 specified that we should be able to handle a null styleSheet but this never happens. > + return styleSheet->disabled(); Instead of an ASSERT() here, you need to check that the styleSheet is not NULL. I don't see where StyleSheet::createSheet() guarantees that a stylesheet will be created. if (StyleSheet* styleSheet = this->sheet()) { // HTML5 specified that we should be able to handle a null styleSheet but this never happens. return styleSheet->disabled(); } return false; > + // HTML5 specified that we should be able to handle a null styleSheet but this never happens. I think this comment is wrong. The StyleSheet::createSheet() method does not guarantee that a sheet object is returned. Note that you may want to check the spec to see what disabled should return if there is no stylesheet. I'm just assuming that false is the correct thing to do here.

*** Bug 52124 has been marked as a duplicate of this bug. ***

Created attachment 78341 [details] Patch

Comment on attachment 78341 [details] Patch Looks great. Please add a test for non-CSS <style> elements, e.g.: <style type="foo/bar"></style> Am I right thinking that your test fully passes in Firefox?

Generally speaking, this should probably work the same for SVG style element, too. I don't know if the general state of HTML/SVG compatibility in WebKit is such that fixing this right away makes sense.

Hmm, please compare this to HTMLLinkElement.disabled implementation. It uses [Reflect], and HTMLLinkElement::parseMappedAttribute() calls setDisabledState() when the value changes - but it's also broken per bug 26673. In particular, HTMLLinkElement.setDisabledState() is a much more complex function than we have here.

I think HTMLLinkElement.setDisabledState() is necessarily more complex because it's referencing an external resource. As for SVGStyleElement, <http://www.w3.org/TR/SVG/styling.html#StyleElement> doesn't mention a disabled attribute, nor does <http://www.w3.org/TR/1999/REC-html401-19991224/present/styles.html#h-14.2.3>, but DOM1 and DOM2 do, for HTML <http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-16428977>. I agree that we should probably just implement it, though. I filed bug 52130.

(In reply to comment #14) > (From update of attachment 78341 [details]) > Looks great. Please add a test for non-CSS <style> elements, e.g.: > > <style type="foo/bar"></style> Done. > Am I right thinking that your test fully passes in Firefox? Yes, it does. http://trac.webkit.org/changeset/75352