CSS21 attribute selectors not dynamic for xml

• Component: CSS
• Status: RESOLVED FIXED

[Kai Brüning]

When used with an XML DOM, CSS21 attribute selectors of the form [attribute] or [attribute=value] (other forms not tested) do not react properly on changes of the attribute after the document has been loaded. This works with html and xhtml DOMs.

See attached files for a test case.

According to the web inspector, the selector is recognized, but the property is not correctly set in the computed style.

[Kai Brüning]

Created attachment 29305 [details]
Test case for this bug

The .xhtml file show dynamic behavior of the [attribute=value] selector, which does not work with the .xml file.

[Kai Brüning]

See also bug 12519 (https://bugs.webkit.org/show_bug.cgi?id=12519)

[Kai Brüning]

Created attachment 29313 [details]
An attempt to fix the bug including test case

The patch tries to make the minimal change necessary for the fix.
Especially it leaves the code for html/xhtml DOMs unchanged.

[Nikolas Zimmermann]

Comment on attachment 29313 [details]
An attempt to fix the bug including test case

Good evening Kai,

a quick glance over your patch shows some style violations and smaller problems, that I'm going to comment on. NOTE that I did not verify wheter your approach is the best, an expert in that area like Hyatt or Maciej would need to review it. I'm just highlighting some misc. problems:

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 42265)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Kai Brüning  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +		
> +	- Fixed bug 25072: CSS21 attribute selectors not dynamic for xml.
> +		  Copied the relevant part of the WebCore::StyleElement::attributeChanged to WebCore::Element::attributeChanged,
> +		  taking care to not change the behavior when called from WebCore::StyleElement::attributeChanged.
> +
> +        Test: fast/css/attribute-selector-dynamic.xml

Your ChangeLog contains tabs. Please remove them. We usually include the bug url in the ChangeLog, not the number: https://bugs.webkit.org/show_bug.cgi?id=25072.
Your name is also misspelled :-)


> ===================================================================
> --- WebCore/dom/Element.cpp	(revision 42261)
> +++ WebCore/dom/Element.cpp	(working copy)
> @@ -559,6 +559,13 @@ PassRefPtr<Attribute> Element::createAtt
>  
>  void Element::attributeChanged(Attribute* attr, bool)
>  {
> +    // Fix for bug 25072: CSS21 attribute selectors not dynamic for xml
> +    // Mapped attributes are handled by StyledElement::attributeChanged()
> +    if (   ! (attr->isMappedAttribute() && isStyledElement())
> +        && ownerDocument()->attached()
> +        && ownerDocument()->styleSelector()->hasSelectorForAttribute(attr->name().localName()))
> +        setChanged();

You do want to use "document()" instead of "ownerDocument()" here, because it's guaranteed to be non-NULL
(except for document type nodes, that don't live in the document itself, but that's unrelated here).
ownerDocument() would be NULL for a Document object itself, so it's use should be avoided.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 42265)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Kai Brüning  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +		
> +		- test that CSS21 attribute selectors take effect when the attribute is
> +		  dynamically changed in an xml dom.

Again tabs here. You also want to add a pixel test result here, generated using "run-webkit-tests --pixel-tests your/test.xhtml".

> Index: LayoutTests/fast/css/attribute-selector-dynamic.xml
> ===================================================================
> --- LayoutTests/fast/css/attribute-selector-dynamic.xml	(revision 0)
> +++ LayoutTests/fast/css/attribute-selector-dynamic.xml	(revision 0)
> @@ -0,0 +1,28 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<book xmlns="http://docbook.org/ns/docbook" xmlns:xhtml="http://www.w3.org/1999/xhtml" version="5.0"> 
> +	<xhtml:style>
> +		title { background-color: red; display:inline; }
> +		*[test="0"] { background-color: green; }
> +	</xhtml:style>
> +
> +   <title test="1">Should have green background</title>
> +
> +   <xhtml:script>
> +      
> +      function change() {
> +         var element = document.getElementsByTagName('title')[0];
> +         element.attributes.test.value = 0;
> +      	if (window.layoutTestController)
> +         	layoutTestController.notifyDone();
> +      }
> +      
> +      window.onload = function () {
> +      	if (window.layoutTestController)
> +      		layoutTestController.waitUntilDone();
> +         // Trigger an attribute change after all load processing is done. Doing the change
> +         // here immediately does not exhibit the bug.
> +         setTimeout("change();", 100);
> +      }
> +
> +   </xhtml:script>
> +</book>
Again tabs here, they need to be removed.
Does a 0ms timer work here, instead of forcing a 100ms wait? Timing dependant tests should be avoided, if possible.

r- because of the issues above. NOTE again that this needs review from an expert, I'm not sure this is the best approach.

[Kai Brüning]

Hello Nikolas,
thanks for reviewing.

I’ll do the changes and create a new patch (sorry for all the tabs).

Just a few questions and remarks:

> Does a 0ms timer work here, instead of forcing a 100ms wait? Timing dependant
> tests should be avoided, if possible.

Indeed it does, I’ll change this - much better.

> You also want to add a pixel test result here.

Really? I don’t mind, but the dumped render tree does include the background color already. If desired, I can also change the test to display "FAIL" or "PASS".

>  An expert in that area like Hyatt or Maciej would need to review it.

Absolutely. Will this just happen, or do I have to take some action to make it happen?

> Your name is also misspelled :-)

Not if the file is interpreted as utf-8. We’re in the unicode age, aren’t we?

[Nikolas Zimmermann]

(In reply to comment #5)
> Hello Nikolas,
> thanks for reviewing.
You're welcome.

> > You also want to add a pixel test result here.
> 
> Really? I don’t mind, but the dumped render tree does include the background
> color already. If desired, I can also change the test to display "FAIL" or
> "PASS".
Yes, otherwhise pixel tests would always recreate the result for your file. So if you add a new test, just include the pixel test dump as well.

 
> >  An expert in that area like Hyatt or Maciej would need to review it.
> 
> Absolutely. Will this just happen, or do I have to take some action to make it
> happen?
Dave is CCed on this bug. If you uploaded a new version of that patch, he'l either realize it and review. If nothing happens, just mail him directly.

> 
> > Your name is also misspelled :-)
> 
> Not if the file is interpreted as utf-8. We’re in the unicode age, aren’t we?
Heh, sorry - I didn't realize that ;-)

[Kai Brüning]

Created attachment 30768 [details]
Updated patch and test case

As suggested, I changed ownerDocument() to document(). This is inline instead of virtual, so it’s faster, too. But even ownerDocument() would never be null here because we are in an Element which can’t be the document.
Still: why does the code in StyledElement from which I copied this use ownerDocument()?

Additionally, I changed the test to display "FAILED" or "PASSED" and added the pixel result.

[Nikolas Zimmermann]

(In reply to comment #7)
I'm still interessted in Dave/Maciejs view of this patch (is this the right way to fix? If yes, I'd be glad to help with fixing the last style issues before it could be commited).

I'd would be great if any of you could comment, so Kai doesn't loose interesst.

[Dave Hyatt]

Comment on attachment 30768 [details]
Updated patch and test case

Fix the funny style in:

if (   ! (attr->isMappedAttribute() && isStyledElement())

Don't need all those extra spaces.

My big concern with this change is just performance.  Extra virtual function calls being made on every attributeChanged could be an issue.

[Kai Brüning]

> Fix the funny style in:
> if (   ! (attr->isMappedAttribute() && isStyledElement())
> Don't need all those extra spaces.

Was an attempt to make the condition more readable, but of course I will follow WebKit style.
Should I put it all in one line then?

> My big concern with this change is just performance.  Extra virtual function
> calls being made on every attributeChanged could be an issue.

This can be fixed by refactoring StyledElement::attributeChanged() a little and removing the tail call of Element::attributeChanged(). If desired, I値l be happy to create a patch for this.

Remaining question: Nikolas told me to use document() instead of ownerDocument() in Element::attributeChanged(), but StyledElement::attributeChanged() uses ownerDocument(). Which is best?

[Kai Brüning]

Created attachment 32241 [details]
New patch with more refactoring

I reworked the patch with a little refactoring of StyledElement::attributeChanged() and Element::attributeChanged(). Now there are no longer additional virtual function calls beyond the necessary.

Notes:
- The new function Element::recalcStyleIfNeededAfterAttributeChanged() would be a candidate for inlining, but it can’t be inlined in Element.h without adding include files.

- Element::recalcStyleIfNeededAfterAttributeChanged() uses document() instead of ownerdocument() as StyledElement::attributeChanged() used to do. As far as I can see this is completely safe in this case and faster, too. Please double check, though.

[Dave Hyatt]

Comment on attachment 32241 [details]
New patch with more refactoring

This is fine... I dislike the name "otherUpdateAfterAttributeChanged."  I think you could just drop "other" and call it "updateAfterAttributeChanged."

With that name change, r=me. If you don't have commit privs, you can attach a new patch with the name change (or whoever lands it can make the change).

[Kai Brüning]

Created attachment 32315 [details]
Updated patch with the name change suggested by Dave Hyatt

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45937.