Bug 25287 - HTMLStyleElement.disabled doesn't work (affects jQuery)
: HTMLStyleElement.disabled doesn't work (affects jQuery)
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Trivial
Assigned To:
: http://pie.googlecode.com/svn/trunk/t...
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2009-04-19 08:10 PST by
Modified: 2011-01-09 09:13 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-04-19 08:10:56 PST
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 From 2010-06-07 09:40:09 PST -------
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 From 2010-06-08 13:48:00 PST -------
Indeed - the property is implemented in bindings, but it has no effect.

See also: bug 19958.
------- Comment #3 From 2010-06-08 13:48:22 PST -------
<rdar://problem/8072250>
------- Comment #4 From 2010-09-08 15:12:12 PST -------
Created an attachment (id=66947) [details]
Fix HTMLStyleElement.disabled and also includes test case for the same.
------- Comment #5 From 2010-09-08 15:19:43 PST -------
> +    // 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 From 2010-09-08 15:47:14 PST -------
(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 From 2010-09-08 19:17:22 PST -------
Created an attachment (id=66990) [details]
Updated bug fix for 25287 with changelog
------- Comment #8 From 2010-09-15 07:55:46 PST -------
(From update of attachment 66990 [details])
Updated bug fix for 25287 with changelog
------- Comment #9 From 2010-09-15 08:03:53 PST -------
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 From 2010-09-15 14:46:11 PST -------
Created an attachment (id=67718) [details]
Updated bug fix for 25287 with fixing style
------- Comment #11 From 2010-10-24 07:50:57 PST -------
(From update of attachment 67718 [details])
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 From 2011-01-08 21:33:41 PST -------
*** Bug 52124 has been marked as a duplicate of this bug. ***
------- Comment #13 From 2011-01-08 22:07:49 PST -------
Created an attachment (id=78341) [details]
Patch
------- Comment #14 From 2011-01-09 00:36:33 PST -------
(From update of attachment 78341 [details])
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 From 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 From 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 From 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 From 2011-01-09 09:13:32 PST -------
(In reply to comment #14)
> (From update of attachment 78341 [details] [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