Bug 3379 – attr(X) does not work

• Component: CSS
• Status: RESOLVED FIXED

[Gideon M.L. Klok]

If there are two or more elements Y from which you request the attr(X) then it returns the value of 
attribute X from the first element Y. The seconde Y element's attr(X) will return the same value. 

e.g. 

@media print {
  a:after { content: '[' attr(href) ']';
  }
}

Will have the result that all A elements will show the atribute href of the first A element.

[Rémi Zara]

Created attachment 2224 [details]
test case with 3 links

Here is a test case.

The problem seems to come from the
CSSStyleSelector::canShareStyleWithElement(NodeImpl* n) optimisation. If made
to always return false, then the problem does not appear.

[Rémi Zara]

Created attachment 2228 [details]
patch which disabled the optimisation if a content property is present

This patch records the fact that a content property is present, and disables
the canShareStyleWithElement optimisation when present.

[Rémi Zara]

Created attachment 2229 [details]
layout-tests/fast/css-generated-content test case

test case to include in layout-tests/fast/css-generated-content

[Rémi Zara]

Created attachment 2230 [details]
expected result

Expected result for the test case

[Dave Hyatt]

The intent of the original style sharing design was that pseudo-elements should never attempt to share 
style with one another at all, so this patch should not be necessary.  Notice that pseudoStyleForElement 
does not contain any code to do sharing.

I think further investigation is needed to understand how the anchors all sharing the same style is causing 
the pseudo-element of a later anchor to get the wrong value.

[Rémi Zara]

Created attachment 2263 [details]
Don't share style for ::after and ::before

The style was shared in RenderObject::getPseudoStyle(). This patch disables the
sharing only for ::after and ::before pseudo elements, since the content
property can only apply to them in css2.
This patch passes all tests, including of course the new one attached to this
bug.

I think that when you'll implement css3 generated content, you'll have to
disable style sharing when a declaration contains a content property (or at
least those with a content property which contains an attr(X) value), or a
counter-increment or a counter-reset property...

[Dave Hyatt]

addPseudoStyle is a dumb function that just throws the resolved pseudo-style onto the style's list.  If you 
bypass the pseudo-style "cache" by not calling getPseudoStyle, you're going to need to make sure you 
don't add the resultant pseudo-style to the cache either.

Ideally the caching of the before/after styles would only be turned off only if attr(x) is used.  If you added 
some sort of "unshareable" bit to RenderStyles and set it when an attr(x) is used, then rather than asking 
about :before/:after in getPseudoStyle, you could just ask "is the style unshareable".

Then we can, as you say, reuse this unshareable bit in other situations (like counters).

[Rémi Zara]

Created attachment 2459 [details]
Add an unshareable bit to RenderStyle

This patch adds an unshareable bit to RenderStyle, which is set when content:
attr(X) is used, and checked in canShareStyleWithElement().

The patch passes all regression tests.

[Darin Adler]

Comment on attachment 2459 [details]
Add an unshareable bit to RenderStyle

There's no word "unshareable", so this should probably either be named
"sharable" (reversing the sense) or "notSharable" instead.

Otherwise, it looks good to me, but I think a patch in this area really needs
Dave to review it, so setting the flag to bring it to his attention.

[Dave Hyatt]

Comment on attachment 2459 [details]
Add an unshareable bit to RenderStyle

Yeah nearly ready.  Let's use the term "unique."  Also, does the extra bit fit
or will this add 4 bytes to RenderStyles?

[Darin Adler]

I checked. The bit fits.

[Darin Adler]

<rdar://problem/4362407> Safari displays same text for two different cases of CSS-generated text that 
should be different (3379)

[Rémi Zara]

Created attachment 8538 [details]
Add a "unique" bit to RenderStyle

This patch adds a "unique" bit to RenderStyle to disable style sharing when generated content relies on attribute values. Contains a test case and expected results.

[Dave Hyatt]

Comment on attachment 8538 [details]
Add a "unique" bit to RenderStyle

This is really close.  I have two remaining concerns.

(1) Hardcoding of

parentStyle->setUnique();

In CSS3 you'll be able to use the content property without it being in :before/:after, so I think it's better to actually check what you're matching.  If you're matching some pseudo-element, then you need to make parentStyle unique, but if you're not, then you need to make style unique instead.

(2) This second concern could be handled by another patch but is easy to add I think.  Since this patch was last worked on Rob landed some code for us to respond to dynamic attribute changes by registering which attribute affect style.  The attr() construct isn't doing this yet though, so dynamically changing attributes that don't already cause a setChanged() will fail.

Addressing (1) is enough for me to +.  Addressing (2) is extra credit. :)

[Rémi Zara]

Created attachment 8550 [details]
addressed comments

Addressed comments, but I'm not sure if I did it right:
 * I'm not sure that "if (style->styleType()==RenderStyle::NOPSEUDO)" is ok
 * I feel like abusing m_selectorAttrs for point (2) of the review, but it works. So either it works by chance, or maybe this class member should be renamed, maybe to m_affectingAttrs (attributes affecting this style, either in selector or properties) ?

[Dave Hyatt]

Comment on attachment 8550 [details]
addressed comments

r=me

Yeah, we should probably rename the m_selectorAttrs variable (and the method that queries it) at some point.

Another problem I notice is that m_selectorAttrs was coded just to use localName.  This make it a little less precise than it could be (if it were to use the full qualified name instead).

[Darin Adler]

Committed revision 14687.