Closed Bug 1351339 Opened 3 years ago Closed 2 years ago

:hover not invalidated properly for display:contents

Categories

(Core :: CSS Parsing and Computation, defect)

54 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: rune, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(3 files)

Attached file hover.html
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170328084734

Steps to reproduce:

Tried to hover a text node with a display:contents ancestor with :hover styling.

Load the attached test and follow the instructions.



Actual results:

The text did not turn green. It did turn green when doing text selection while hovering.



Expected results:

The text should turn green while hovering.
Blocks: 907396
Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Product: Firefox → Core
The testcase works fine for me in Nightly 55.0a1 (2017-03-28) on Linux.
Keywords: testcase
I can confirm that it fails in 54.0a2 and works in 55.0a1.
Hmm, so it fails for me in Linux 55.0a1 (2017-03-20) (64-bit), has it been a recent fix? I'll try to find a regression range.
So it seems this was fixed by bug 1302054, the test case should probably be landed, since it seems that it was not the intention of the patch at all :)
See Also: → 1302054
NI heycam to land the testcase.
Flags: needinfo?(cam)
There are several important questions here:

1)  Why did bug 1302054 change behavior at all?  Bisect confirms that this changeset did it:

changeset:   348572:a60081798b34
user:        Cameron McCormack <cam@mcc.id.au>
date:        Tue Mar 21 16:33:05 2017 +0800
summary:     Bug 1302054 - Part 2: Remove no longer useful nsStyleContext::CalcDifference optimization that handles the same-rule-node case. r=dbaron

which doesn't obviously seem related.

2)  Did the behavior also change (from broken to not) for other dynamic changes (e.g. via attribute) or is this :hover-specific?

3)  What's the stylo story?  The changes in bug 1302054 are Gecko-only, right?  This testcase still fails in stylo.

4)  What's the right behavior of :hover in this case anyway?  The spec is sort of vague enough that any behavior would be ok, and I _can_ buy the "if the text is pointed at then its parent element is in :hover state even if that element has no boxes of its own".  But filed https://github.com/w3c/csswg-drafts/issues/1141 anyway.
I plan to upstream the test btw, though I thought given we still don't import CSS tests from WPT I should probably submit it here too.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6)
> 1)  Why did bug 1302054 change behavior at all?  Bisect confirms that this
> changeset did it:
> 
> changeset:   348572:a60081798b34
> user:        Cameron McCormack <cam@mcc.id.au>
> date:        Tue Mar 21 16:33:05 2017 +0800
> summary:     Bug 1302054 - Part 2: Remove no longer useful
> nsStyleContext::CalcDifference optimization that handles the same-rule-node
> case. r=dbaron
> 
> which doesn't obviously seem related.

Before bug 1302054, nsStyleContext::CalcStyleDifference assumed that if we got the same rule node on the new style context (which is the case here for the text frame), that we can skip calling CalcDifference on any structs that only generate "change hints handled for descendants by ancestors", of which nsChangeHint_RepaintFrame is one.  This is because either (a) this is the root of the restyle, and so there are no new inherited values to take into account (and thus we must have exactly the same style data), or (b) our parent was restyled, and if we got a new inherited property value that would generate a handled-for-descendants-by-ancestors change hint, then it would have been handled by the parent.  In this case, we don't generate a change hint for the display:contents node, so that assumption is wrong.

After bug 1302054, we don't bother skipping calling CalcDifference on structs, and so we correctly generate the RepaintFrame for the text frame.

> 2)  Did the behavior also change (from broken to not) for other dynamic
> changes (e.g. via attribute) or is this :hover-specific?

I didn't test, but yes, it would have.
Flags: needinfo?(cam)
Comment on attachment 8865344 [details]
Bug 1351339: Compute text style changes when the parent is a display: contents element.

https://reviewboard.mozilla.org/r/136914/#review139958

::: layout/base/ServoRestyleManager.cpp:154
(Diff revision 1)
> +  nsChangeHint mComputedHint;
> +
> +  TextPostTraversalState(bool aDisplayContentsParentStyleChanged)
> +    : mShouldPostHints(aDisplayContentsParentStyleChanged)
> +    , mShouldComputeHints(aDisplayContentsParentStyleChanged)
> +    , mComputedHint(nsChangeHint_NeutralChange)

This should be nsChangeHint_Empty instead.  NeutralChange is something that specifically means "there was some change in the values between the old and new style context, but that change has no effect", and is only used to prevent some Gecko-specific optimizations from going awry.

::: layout/base/ServoRestyleManager.cpp:172
(Diff revision 1)
> +    // We rely on the fact that the change hint for a display: contents text
> +    // node is always the same to the one of the primary style of its siblings.

s/same to/same as/

But I got a bit confused by this sentence (although I guessed at its meaning).  Can you reword/clarify?

::: layout/base/ServoRestyleManager.cpp:175
(Diff revision 1)
> +    // TODO(emilio): The above may not be true for ::first-{line,letter}, but
> +    // we'll cross that bridge when we support those in stylo.

Is it a big deal if we just don't have mShouldComputeHints?  We already avoid generating hints for each same-style continuation.  What work are we avoiding here?
Comment on attachment 8865344 [details]
Bug 1351339: Compute text style changes when the parent is a display: contents element.

https://reviewboard.mozilla.org/r/136914/#review139958

> This should be nsChangeHint_Empty instead.  NeutralChange is something that specifically means "there was some change in the values between the old and new style context, but that change has no effect", and is only used to prevent some Gecko-specific optimizations from going awry.

I used this because `NeutralChange` asserts when being pushed to the change list, the idea being that it's always overriden. But sure, that's fine for me :)

> s/same to/same as/
> 
> But I got a bit confused by this sentence (although I guessed at its meaning).  Can you reword/clarify?

Sure

> Is it a big deal if we just don't have mShouldComputeHints?  We already avoid generating hints for each same-style continuation.  What work are we avoiding here?

Well, it would matter in cases like:

```
<div style="display: contents">Text <a href="">foo</a> More text</div>
```

Pretty much everything that has another element in between to text nodes.
Comment on attachment 8865344 [details]
Bug 1351339: Compute text style changes when the parent is a display: contents element.

https://reviewboard.mozilla.org/r/136914/#review139958

> I used this because `NeutralChange` asserts when being pushed to the change list, the idea being that it's always overriden. But sure, that's fine for me :)

I guess this is not a big deal, since `_Empty` would also assert.

> Sure

I've just reworded it, let me know if it's clearer now.

Also, just noticed that we could avoid allocating more than one style context for the children, though I haven't done that optimization yet (not sure it's that important, I guess it could be, so perhaps worth doing it in a followup bug).
Comment on attachment 8865344 [details]
Bug 1351339: Compute text style changes when the parent is a display: contents element.

https://reviewboard.mozilla.org/r/136914/#review140014
Attachment #8865344 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/558c843afbd3
Compute text style changes when the parent is a display: contents element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d4de1e480c2e
Test. r=heycam
https://hg.mozilla.org/mozilla-central/rev/558c843afbd3
https://hg.mozilla.org/mozilla-central/rev/d4de1e480c2e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.