Bug 93665 – Changing class attribute is not reflected in the classList property

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

classList is defined in Element but setting the class attribute needs to trigger classAttributeChanged which is only available on StyledElement

[Erik Arvidsson]

Created attachment 158898 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158898 [details]
Patch

Attachment 158898 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13516539

New failing tests:
css3/selectors3/xml/css3-modsel-139.xml
css3/selectors3/xhtml/css3-modsel-113.xml
fast/dom/getElementsByClassName/011.xml
css3/selectors3/xhtml/css3-modsel-113b.xml
fast/parser/xhtml-alternate-entities.xml
css3/selectors3/xhtml/css3-modsel-139.xml
css3/selectors3/xml/css3-modsel-139b.xml
css3/selectors3/xml/css3-modsel-113b.xml
css3/selectors3/xhtml/css3-modsel-139b.xml
css3/selectors3/xml/css3-modsel-113.xml

[WebKit Review Bot]

Created attachment 158920 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Erik Arvidsson]

Comment on attachment 158898 [details]
Patch

I'll look into the failures tomorrow

[Erik Arvidsson]

Created attachment 159948 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159948 [details]
Patch

Attachment 159948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13567305

New failing tests:
fast/parser/xhtml-alternate-entities.xml

[WebKit Review Bot]

Created attachment 159956 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 159948 [details]
Patch

Attachment 159948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13569299

New failing tests:
fast/parser/xhtml-alternate-entities.xml

[WebKit Review Bot]

Created attachment 159965 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Erik Arvidsson]

Created attachment 159983 [details]
Patch

[Ojan Vafai]

Comment on attachment 159983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159983&action=review

> Source/WebCore/dom/ClassNodeList.cpp:57
> +    // TODO: DOM4 allows getElementsByClassName to return non StyledElement.

s/TODO/FIXME

[Ryosuke Niwa]

Comment on attachment 159983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159983&action=review

> Source/WebCore/ChangeLog:9
> +        Class attribute logic needs to be moved to Element (from StyledElement)
> +        https://bugs.webkit.org/show_bug.cgi?id=93665
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Before this change classAttributeChanged was only called for StyledElement. With this refactoring
> +        it gets called for all Elements when the class attribute changes.

Does this fix some bug or is it just a refactoring? Please clarify.
Ideally the bug title is changed to reflect whatever behavior being fixed if there is any behavior change.

> Source/WebCore/dom/ClassNodeList.cpp:57
> +    // TODO: DOM4 allows getElementsByClassName to return non StyledElement.

We use FIXME instead of TODO.

[Ryosuke Niwa]

Comment on attachment 159983 [details]
Patch

I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code.

[Ojan Vafai]

(In reply to comment #13)
> (From update of attachment 159983 [details])
> I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code.

I thought it was clear from the test that it makes classList work for svg/xml elements. You just want that clearly spelled out in the ChangeLog?

[Ojan Vafai]

What I mean to say, it it makes setting the class attribute and then reading out the classList work for svg/xml elements.

[Ryosuke Niwa]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 159983 [details] [details])
> > I don't think we can land the patch as is since it's unclear whether this patch is changing the behavior or just refactoring the code.
> 
> I thought it was clear from the test that it makes classList work for svg/xml elements. You just want that clearly spelled out in the ChangeLog?

Well, it wasn't clear from the bug title & the change log entry whether this was a simple refactoring with an addition of test or this was fixing some bug for which we're adding a test.

[Erik Arvidsson]

Created attachment 159990 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 159990 [details]
Patch

You should probably use Ojan's r+ but r=me anyhow.

[Erik Arvidsson]

Created attachment 159991 [details]
Patch for landing

[Erik Arvidsson]

(In reply to comment #18)
> (From update of attachment 159990 [details])
> You should probably use Ojan's r+ but r=me anyhow.

You are confusing me... I  just did --reviewer "Ojan Vafai". I guess it is not a big deal but I can redo it if you really care

[WebKit Review Bot]

Comment on attachment 159991 [details]
Patch for landing

Clearing flags on attachment: 159991

Committed r126349: <http://trac.webkit.org/changeset/126349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 159990 [details] [details])
> > You should probably use Ojan's r+ but r=me anyhow.
> 
> You are confusing me... I  just did --reviewer "Ojan Vafai". I guess it is not a big deal but I can redo it if you really care

Yeah, that's what I meant to ask you :) I just set r+ on the patch for the bugzilla's sake.