Bug 25287 – HTMLStyleElement.disabled doesn't work (affects jQuery)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[HE Shi-Jun]

See testcase: http://pie.googlecode.com/svn/trunk/test/bugs/HTMLStyleElement-disabled.html

As html5 spec 4.2.7, The disabled DOM attribute ... must return the value of the StyleSheet interface's disabled attribute on getting, and forward the new value to that same attribute on setting.

Safari 3.2.2 and Chrome 1.0.154 don't pass the test and IE6, FF3, Opera9 all ok.

Workaround: using HTMLStyleElement.sheet.disabled

[Josh Powell]

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

[Alexey Proskuryakov]

Indeed - the property is implemented in bindings, but it has no effect.

See also: bug 19958.

[Alexey Proskuryakov]

<rdar://problem/8072250>

[Tarun Nainani]

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

[Alexey Proskuryakov]

> +    // 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'?

[Tarun Nainani]

(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.

[Tarun Nainani]

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

[Tarun Nainani]

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

Updated bug fix for 25287 with changelog

[WebKit Review Bot]

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.

[Tarun Nainani]

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

[David Kilzer (:ddkilzer)]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Created attachment 78341 [details]
Patch

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

(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