1384435 - Stylo: Size labels missing on hover in webpack-bundle-analyzer

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Espen H]

Attachment: missing_numbers.png

It's not 100% consistent, but with stylo enabled in nightly, many times the size labels in webpack-bundle-analyzer do not appear. I have never been able to reproduce this with stylo disabled.

Steps to reproduce:

- Set up a webpack project with https://github.com/th0r/webpack-bundle-analyzer
- Run a build and have it open the interactive zoomable treemap in firefox
- Observe that with layout.css.servo.enabled = true, the sizes on hover DO NOT consistently display
  - If they appear, try switching to another tab and back
- Observe that with layout.css.servo.enabled = false, the sizes on hover DO consistently display

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Can you still reproduce? I suspect a lot it's the same root cause as bug 1384065.

Comment 2

[Espen H]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Can you still reproduce? I suspect a lot it's the same root cause as bug
> 1384065.

Is this in nightly? I'm afraid I am still seeing this issue in 56.0a1 (2017-07-30) (64-bit).

It seems pretty consistent when I move the cursor outside the window, then directly to a cell in the tab. Another interesting finding is that when I move the cursor from a cell which has missing text in its tooltip, then off screen, the text appears as the tooltip is fading out.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yup, should be.

Anyways, thanks for checking, will try to repro (some more precise STR would be helpful, since it's ages I haven't touch anything npm-related, but will try to find my way, thanks!)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, I can still repro. They're setting the text content directly on the tooltip, so I don't see off-hand where the bug is. It seems we're failing to construct frames for the text node, but will need to check more carefully. I'm trying to reduce this.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase

So, here's a test-case that reproduces the problem most of the time for me. It's pretty much a poor-man's version of the analyzer plugin.

Seems like if I remove the transition the issue goes away, and I'm pretty sure the animation restyles are the issue.

Hiro, any chance you can take a look at this today? Otherwise I can do it tomorrow.

Thanks! (And thanks for reporting it again!)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> So, here's a test-case that reproduces the problem most of the time for me.
> It's pretty much a poor-man's version of the analyzer plugin.

I wanted to write "analyzer tooltip" instead. Oh well.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Great test case!  This is definitely animation bug, I suspect visibility handling is something wrong. Taking.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Oh, no? This happens only on strong elements.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

While debugging the test case, I noticed *RESTYLE_CSS_ANIMATIONS* are fired on the test case even though there is no CSS animation. The animation restyle hint comes from Gecko_UpdateAnimations[1].  It seems to me that the restyle hint is culprit of this bug (dropping this call solved this bug). 

CCing Boris, he might know something.

[1] https://hg.mozilla.org/mozilla-central/file/87824406b9fe/layout/style/ServoBindings.cpp#l720

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8892142 [details]
testcase

>target.onmousemove = function(e) {
>  tooltip.style.top = e.offsetY + "px";
>  tooltip.style.left = e.offsetX + "px";

The CascadeResults task is created by this style change.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I believe bug  1367975 fixes this issue, but it's not yet clear to me why the needless RESTYLE_CSS_ANIMATIONS caused this.  It needs to be checked.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> I believe bug  1367975 fixes this issue, but it's not yet clear to me why
> the needless RESTYLE_CSS_ANIMATIONS caused this.  It needs to be checked.

I think now I understand what's going on there. Another culprit is unset_animation_only_dirty_descendants() in recalc_style_at() [1]. It's the remnant of bug 1356141. To reproduce this bug, there needs a transition and a needless RESTYLE_CSS_ANIMATIONS from a SequentialTask.

1) restyling a transition element
2) 1) leads to restyling descendant elements and set animation-only dirty bit to some descendant elements (e.g. div elements which are the parent of strong elements in the test case in comment 10) because the transition restyle propagates MustCascadeChildren
3) in a subsequent normal traverse, a SequentialTask is created and post RESTYLE_CSS_ANIMATIONS to the transition element due to bug 1356141
4) as a result of RESTYLE_CSS_ANIMATIONS, second animation-only restyle is triggered
5) re-restyling the transition element in the second animation-only restyle
6) the transition restyle propagated *CanSkipCascade* this time
7) descendant elements are traversed since they have animation-only dirty bit marked at 2) and clear the dirty bit because of [1] and never re-marked the dirty bit again since they are *CanSkipCascade*

This explanation might be not accurate in detail but this is roughly what's going on there.

[1] https://hg.mozilla.org/mozilla-central/file/52285ea5e54c/servo/components/style/traversal.rs#l561

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=738a10502cd41bb4ea622574e84f62c875014aca

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Finally I could write a reftest for this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d3dfa40378b34fc5b9db77001673f69a5490711

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1384435 - Don't clear animation only dirty bit during style recalc (unless the element is in a display:none subtree).

After bug 1356141, the setup of animation-only dirty bit should have matched
to normal dirty bit's one (Though they don't match in post traversal due to
throttled animation flush). An unset_animation_only_dirty_descendants call
removed in this patch cleared dirty bits which are needed for post traversal if
there are a second animation-only traversal and if there is no need to restyle
for the second animation-only traversal.

The reftest in this patch fails without either this fix or the fix for bug
1367975.

See bug 1384435 comment 12 for more detail what's going on at that time.

Review commit: https://reviewboard.mozilla.org/r/164170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164170/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8893168 [details]
Bug 1384435 - Don't clear animation only dirty bit during style recalc (unless the element is in a display:none subtree).

https://reviewboard.mozilla.org/r/164170/#review169524

::: layout/reftests/css-transitions/style-change-during-transition.html:24
(Diff revision 1)
> +    requestAnimationFrame(() => {
> +      requestAnimationFrame(() => {

Honestly I don't quite understand why two frames is needed here, but without this two frames this test did not fail.

Comment 17

[Darkspirit]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> I believe bug  1367975 fixes this issue, but it's not yet clear to me why the needless RESTYLE_CSS_ANIMATIONS caused this.  It needs to be checked.

Can't reproduce the bug with attachment 8892142 [details] anymore. Nightly 57 x64 20170804100354 @ Debian Testing.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8893168 [details]
Bug 1384435 - Don't clear animation only dirty bit during style recalc (unless the element is in a display:none subtree).

https://reviewboard.mozilla.org/r/164170/#review170958

::: commit-message-6f191:1
(Diff revision 1)
> +Bug 1384435 - Don't clear animation only dirty bit except for the element gets in display:none subtree. r?birtles

I guess "except for" here means "unless", i.e.

Don't clear animation-only dirty bit during style recalc (unless the element is in a display:none subtree)

?

(And for my own reference, the display:none case is handled by the call to clear_descendant_data earlier in that function. I do wonder if it's necessary to mention that in the changeset summary, however.)

::: commit-message-6f191:7
(Diff revision 1)
> +
> +After bug 1356141, the setup of animation-only dirty bit should have matched
> +to normal dirty bit's one (Though they don't match in post traversal due to
> +throttled animation flush). An unset_animation_only_dirty_descendants call
> +removed in this patch cleared dirty bits which are needed for post traversal if
> +there are a second animation-only traversal and if there is no need to restyle

Nit: there is a second animation-only...

(i.e. s/are/is/)

::: commit-message-6f191:13
(Diff revision 1)
> +for the second animation-only traversal.
> +
> +The reftest in this patch fails without either this fix or the fix for bug
> +1367975.
> +
> +See bug 1384435 comment 12 for more detail what's going on at that time.

Since this patch contains Servo-side changes, perhaps when we land it we should say "Gecko bug 1384435 comment 12" and perhaps even link-ify it, e.g. [Gecko bug 1384435 comment 12](https://bugzilla.mozilla.org/show_bug.cgi?id=1384435#c12)

::: layout/reftests/css-transitions/style-change-during-transition.html:24
(Diff revision 1)
> +    requestAnimationFrame(() => {
> +      requestAnimationFrame(() => {

I wouldn't worry about it too much -- it might be due to paint throttling which we'd need to include the paint listener machinery to test for and that probably complicates the test too much.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #18)
> Comment on attachment 8893168 [details]
> Bug 1384435 - Don't clear animation only dirty bit except for the element
> gets in display:none subtree.
> 
> https://reviewboard.mozilla.org/r/164170/#review170958
> 
> ::: commit-message-6f191:1
> (Diff revision 1)
> > +Bug 1384435 - Don't clear animation only dirty bit except for the element gets in display:none subtree. r?birtles
> 
> I guess "except for" here means "unless", i.e.

Right! Thank you!

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8893168 [details]
Bug 1384435 - Don't clear animation only dirty bit during style recalc (unless the element is in a display:none subtree).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164170/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7953d781a9bf
Don't clear animation only dirty bit during style recalc (unless the element is in a display:none subtree). r=birtles

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7953d781a9bf