Bug 25287 - HTMLStyleElement.disabled doesn't work (affects jQuery)
: HTMLStyleElement.disabled doesn't work (affects jQuery)
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Trivial
Assigned To: Nobody
http://pie.googlecode.com/svn/trunk/t...
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-19 08:10 PDT by HE Shi-Jun
Modified: 2011-01-09 09:13 PST (History)
7 users (show)

See Also:


Attachments
Fix HTMLStyleElement.disabled and also includes test case for the same. (3.61 KB, patch)
2010-09-08 15:12 PDT, Tarun Nainani
no flags Details | Formatted Diff | Diff
Updated bug fix for 25287 with changelog (5.03 KB, patch)
2010-09-08 19:17 PDT, Tarun Nainani
no flags Details | Formatted Diff | Diff
Updated bug fix for 25287 with fixing style (4.89 KB, patch)
2010-09-15 14:46 PDT, Tarun Nainani
no flags Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2011-01-08 22:07 PST, Simon Fraser (smfr)
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description HE Shi-Jun 2009-04-19 08:10:56 PDT
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
Comment 1 Josh Powell 2010-06-07 09:40:09 PDT
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
Comment 2 Alexey Proskuryakov 2010-06-08 13:48:00 PDT
Indeed - the property is implemented in bindings, but it has no effect.

See also: bug 19958.
Comment 3 Alexey Proskuryakov 2010-06-08 13:48:22 PDT
<rdar://problem/8072250>
Comment 4 Tarun Nainani 2010-09-08 15:12:12 PDT
Created attachment 66947 [details]
Fix HTMLStyleElement.disabled and also includes test case for the same.
Comment 5 Alexey Proskuryakov 2010-09-08 15:19:43 PDT
> +    // 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'?
Comment 6 Tarun Nainani 2010-09-08 15:47:14 PDT
(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.
Comment 7 Tarun Nainani 2010-09-08 19:17:22 PDT
Created attachment 66990 [details]
Updated bug fix for 25287 with changelog
Comment 8 Tarun Nainani 2010-09-15 07:55:46 PDT
Comment on attachment 66990 [details]
Updated bug fix for 25287 with changelog

Updated bug fix for 25287 with changelog
Comment 9 WebKit Review Bot 2010-09-15 08:03:53 PDT
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.
Comment 10 Tarun Nainani 2010-09-15 14:46:11 PDT
Created attachment 67718 [details]
Updated bug fix for 25287 with fixing style
Comment 11 David Kilzer (:ddkilzer) 2010-10-24 07:50:57 PDT
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.
Comment 12 Simon Fraser (smfr) 2011-01-08 21:33:41 PST
*** Bug 52124 has been marked as a duplicate of this bug. ***
Comment 13 Simon Fraser (smfr) 2011-01-08 22:07:49 PST
Created attachment 78341 [details]
Patch
Comment 14 Alexey Proskuryakov 2011-01-09 00:36:33 PST
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?
Comment 15 Alexey Proskuryakov 2011-01-09 00:39:15 PST
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.
Comment 16 Alexey Proskuryakov 2011-01-09 02:03:57 PST
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.
Comment 17 Simon Fraser (smfr) 2011-01-09 09:09:57 PST
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.
Comment 18 Simon Fraser (smfr) 2011-01-09 09:13:32 PST
(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