REGRESSION (r96393): In some cases, generated content is never shown

[Alan Hogan]

The last paragraph should have a pilcrow.

This generated content does not display, but it does in other browsers.

Inspecting the paragraph and clicking its element in Inspector will cause a redraw and the pilcrow will be shown.

[Alexey Proskuryakov]

Regressed in <http://trac.webkit.org/changeset/96393>.

[Arpita Bahuguna]

Created attachment 148092 [details]
Patch

[Arpita Bahuguna]

Element's with attribute-selector specified on them should not share the style of any of their previous (matching) siblings.
In canShareStyleWithElement() we do check whether the element (sibling) being compared with has attribute selector specified on it or not (affectedByUncommonAttributeSelectors()) and if so we return from the function and the style is not shared.
Similarly, we need to verify the same for the current element as well. Since affectedByUncommonAttributeSelectors() can only be called on the element's renderStyle which is not available for the current element. 
Hence have added a function which checks whether the current element has any attribute selector applied on it or not.

[Antti Koivisto]

Comment on attachment 148092 [details]
Patch

The fix looks wrong or at least way too wide. As far as I see the bug is about using the attr() function in content attribute, yet this disables style sharing for all elements that may match some attribute selector.

[Arpita Bahuguna]

(In reply to comment #4)
> (From update of attachment 148092 [details])
> The fix looks wrong or at least way too wide. As far as I see the bug is about using the attr() function in content attribute, yet this disables style sharing for all elements that may match some attribute selector.

I completely missed out on verifying the issue with other styles. Apologize for my mistake. I shall try and upload another patch with hopefully the correct fix this time as soon as possible.

[Arpita Bahuguna]

Created attachment 148539 [details]
Patch

[Arpita Bahuguna]

The problem here is that an attribute selector appended by a pseudo-element does not return any matched rules for the call: (matchesRuleSet(m_uncommonAttributeRuleSet.get())) in locateSharedStyle().

This is because of the way pseudo-elements are handled in SelectorChecker::checkOneSelector(). For a pseudo-element selector we check for the condition (!context.elementStyle && m_mode == ResolvingStyle) and since currently the SelectorChecker mode is set to ResolvingStyle and since the current element's style is still not available we fail this check and (wrongly) return false from checkOneSelector().

Have thus introduced another enum value: SharingRules (not sure about the name though) for SelectorChecker's mode. Before making the locateSharedStyle() call we can set the SelectorChecker mode to SharingRules and then reset the same post the call.

Existing mode value "CollectingRules" although appropriate cannot be used in this scenario because we also don't want to set any value to dynamicPseudo for this flow.
if (pseudoId != NOPSEUDO)
    dynamicPseudo = pseudoId;

[Antti Koivisto]

Comment on attachment 148539 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        (WebCore::SelectorChecker::checkOneSelector):
> +        While obtaining the shared style (if any) for the current element we return false from
> +        checkOneSelector() for the pseudo-element case. This is because our SelectorChecker's
> +        mode is set to that of ResolvingStyle and we thus fail the initial condition check.
> +
> +        To avoid the same have added another value (SharingRules) to the Mode enum and the same
> +        can be set for the SelectorChecker before calling on locateSharedStyle().

I don't understand the logic of this fix. The explanation here is not particularly enlightening. What exactly is the problem and how does the patch solve it?

[Arpita Bahuguna]

Created attachment 149911 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #8)
> (From update of attachment 148539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148539&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        (WebCore::SelectorChecker::checkOneSelector):
> > +        While obtaining the shared style (if any) for the current element we return false from
> > +        checkOneSelector() for the pseudo-element case. This is because our SelectorChecker's
> > +        mode is set to that of ResolvingStyle and we thus fail the initial condition check.
> > +
> > +        To avoid the same have added another value (SharingRules) to the Mode enum and the same
> > +        can be set for the SelectorChecker before calling on locateSharedStyle().
> 
> I don't understand the logic of this fix. The explanation here is not particularly enlightening. What exactly is the problem and how does the patch solve it?

Thanks for the review Antti. I agree, the explanation here was not really sufficient; my apologies for that.
Have uploaded another patch with a more detailed explanation in the ChangeLog which hopefully shall provide some more insight into the fix.

[Andreas Kling]

This needs to be looked at by Antti.

[Antti Koivisto]

Comment on attachment 149911 [details]
Patch

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

The fix looks good but I think this can be factored nicer.

> Source/WebCore/css/StyleResolver.cpp:1715
> +        m_checker.setMode(SelectorChecker::SharingRules);
>          RenderStyle* sharedStyle = locateSharedStyle();
> +        m_checker.setMode(SelectorChecker::ResolvingStyle);

I think you should set and restore the mode nearer to the SelectorChecker call site. The most natural place would probably be matchesRuleSet() function (which is only used by locateSharedStyle() and should be the only way to call the SelectorChecker there). That function should also be renamed to something more descriptive, styleSharingCandidateMatchesRuleSet() for example.

[Arpita Bahuguna]

Created attachment 168192 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #12)
> (From update of attachment 149911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149911&action=review
> 
Hi Antti, Thank-you for the review comments.
Have uploaded another patch with the suggested changes.

> The fix looks good but I think this can be factored nicer.
> 
> > Source/WebCore/css/StyleResolver.cpp:1715
> > +        m_checker.setMode(SelectorChecker::SharingRules);
> >          RenderStyle* sharedStyle = locateSharedStyle();
> > +        m_checker.setMode(SelectorChecker::ResolvingStyle);
> 
> I think you should set and restore the mode nearer to the SelectorChecker call site. The most natural place would probably be matchesRuleSet() function (which is only used by locateSharedStyle() and should be the only way to call the SelectorChecker there). That function should also be renamed to something more descriptive, styleSharingCandidateMatchesRuleSet() for example.

Moving the SelectorChecker Mode set/reset closer to the call definitely makes more sense. Am now setting the same before making our collectMatchingRules() call from matchesRuleSet() which too has now been renamed to styleSharingCandidateMatchesRuleSet() as per your suggestion.

[Antti Koivisto]

Comment on attachment 168192 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 168192 [details]
Patch

Clearing flags on attachment: 168192

Committed r131047: <http://trac.webkit.org/changeset/131047>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.