Get rid of inherited transition values from after-change style

NEW
Assigned to

Status

()

enhancement
P3
normal
2 years ago
2 years ago

People

(Reporter: bgrins, Assigned: hiro)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Another -moz-window-inactive issue I found in Bug 1420229: if a selector contains this in xul.css (or any UA sheet) then layout/style/test/test_transitions.html begins to permafail on windows with errors like:

> descendant test #3, property text-indent: computed value 128.783px should be between 62.936721px and 63.155104px at time between 1.9429280000000002s and 1.9429280000000002s.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=408b23cf361c61efe8e123ecdfd17d21f4f7b1e6&group_state=expanded&selectedJob=155483515

For the ongoing XBL work (see Bug 1420229) we need to load files that used to be in <resources> tags instead as UA sheets, and we are hitting this failure due to the following rule in menu.css: https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/toolkit/themes/windows/global/menu.css#157-161
Just realized that luckily this isn't Windows-only so updating the title to match. I was only seeing it on Windows in Bug 1420229 because it was the only place that selector was present, but in the try push where the selector is added to xul.css I see the failure on all platforms.

Emilio, you fixed the moz-window-inactive issue in Bug 1428164 - any chance you'd be able to look into this one as well?
Flags: needinfo?(emilio)
Summary: Adding :-moz-window-inactive in a UA sheet like xul.css causes layout/style/test/test_transitions.html to permafail on Windows → Adding :-moz-window-inactive in a UA sheet like xul.css causes layout/style/test/test_transitions.html to permafail
It occurred to me that this probably had nothing to do with UA styles or xul.css.  I just confirmed that if I add `foo:-moz-window-inactive { }` inside test_transitions.html the test fails, so whatever is failing there would be exposed to content pages that include the selector as well.
Summary: Adding :-moz-window-inactive in a UA sheet like xul.css causes layout/style/test/test_transitions.html to permafail → Adding :-moz-window-inactive selector to layout/style/test/test_transitions.html causes it to permafail
Depends on: 1409672
Priority: -- → P3
Yeah, this is a transitions issue though, and hiro and birtles are more familiar than I with those. Maybe worth looking into it? Being restyled without the style changing shouldn't affect which transitions apply I suspect...
Flags: needinfo?(emilio)
FWIW this is Stylo-specific as it passes with Stylo disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62092a548a7048cb45ca21a9d6a6073b02e1184d
Summary: Adding :-moz-window-inactive selector to layout/style/test/test_transitions.html causes it to permafail → Stylo: adding :-moz-window-inactive selector to layout/style/test/test_transitions.html causes it to permafail
Emilio, do you expect the fix in Bug 1409672 / https://github.com/servo/servo/pull/19747 will resolve this issue as well, or should I ask for help from the folks you mention in Comment 4 to look at the transition side of things?
Flags: needinfo?(emilio)
The code in that PR is still unused, need to integrate it on Gecko, but yeah, I expect it to avoid this problem. Though that's kind of a wallpaper I think (unless Birtles says it's fine spec-wise), so I'd rather give them a heads-up, just in case it's something interesting.
Flags: needinfo?(hikezoe)
Flags: needinfo?(emilio)
Flags: needinfo?(bbirtles)
I'm not sure what the specific question here is but as for, "Being restyled without the style changing shouldn't affect which transitions apply I suspect" -- yes, that's right. Transitions are based on comparing the before-change and after-change styles so if nothing changed, there shouldn't be any affect on transitions.

(The relevant spec text is at: https://drafts.csswg.org/css-transitions/#starting)
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #8)
> I'm not sure what the specific question here is but as for, "Being restyled
> without the style changing shouldn't affect which transitions apply I
> suspect" -- yes, that's right. Transitions are based on comparing the
> before-change and after-change styles so if nothing changed, there shouldn't
> be any affect on transitions.
> 
> (The relevant spec text is at:
> https://drafts.csswg.org/css-transitions/#starting)

The issue here is that :-moz-window-inactive causes a full-doc restyle, and that seems to cause failures in test_transitions.html, which looks like a bug.
I can see this weird behavior.  It looks like the transition moves 2x faster if there is 'foo:-moz-window-inactive'.  That's the reason of the failure.  But I don't yet know why (transition duration and end value seem to be correct there).  Anyway I will debug this, though I am not sure this is really a transition issue.
Assignee: nobody → hikezoe
Flags: needinfo?(hikezoe)
What I can tell from what I am seeing is that;

1) there are two elements, a <div> element and a <p> element inside the <div>.
2) two text-indent transitions is triggered by setting a new text-indent to the parent
3) after about 300ms later a traversal with eRestyle_Subtree happens on html element because of foo:-moz-window-inactive
4) in the traversal, the transition on the child element is cancelled
5) then there is only one transition for the parent element whose duration is 4x shorter than the child
6) then the test checks the child text-indent value, it's not expected because there is no transition on the child

So, the faster transition what I saw was actually 4x faster. :)

Anyway, on old style style, we don't restyle the child element in 3).  So I think that's why the test does not fail on gecko, if the child element was traversed on gecko, the test might fail because the logic to trigger/cancel transitions on stylo is a mimic of the logic on gecko.

That's said, cancelling the transition in the traversal 3) seems not be correct to me (I think this is what Emilio supposed in comment 4).  I will check it later.
Oh, one more thing.  If the test runs on non-E10S, the traversal 3) does not happen at all either on stylo or on gecko.  I am not sure this is correct behavior.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Anyway, on old style style, we don't restyle the child element in 3).  So I
> think that's why the test does not fail on gecko, if the child element was
> traversed on gecko, the test might fail because the logic to trigger/cancel

I meant 'the child element were traversed on gecko'.  Is this correct English?
'after-change' style for the child element is not correct in the traversal 3), it should be '150px' which is the value set by parent text-indent in 2), IIUC.  But we got '50px'.
Posted file A relevant test case (obsolete) —
This test case is somewhat weird behavior both on gecko and stylo.  I think after-change style is wrong on both.
Currently in get_after_change_style() we do get rid of transition values on the same element, but we don't get rid of inherited transitioning values.
Summary: Stylo: adding :-moz-window-inactive selector to layout/style/test/test_transitions.html causes it to permafail → Stylo: Get rid of inherited transition values from after-change style
So, it sounds pretty hard to me.  This patch just gets rid of the parent one but we do get rid of all of inherited transitions.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Created attachment 8942577 [details] [diff] [review]
> Get rid of the parent transition values
> 
> So, it sounds pretty hard to me.  This patch just gets rid of the parent one
> but we do get rid of all of inherited transitions.

we *have to* do.
Posted file A reduced test case
This test cases doesn't need to flush whole document, and is a nested element case that attachment 8942577 [details] [diff] [review] doesn't fix.
Attachment #8942569 - Attachment is obsolete: true
So, this is annoying, indeed. Should we avoid cascading transition styles if the traversal is for animation? That looks like what should happen anyway, though perf-wise is kind of annoying.

Blink seems pretty buggy here, too...
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> So, this is annoying, indeed. Should we avoid cascading transition styles if
> the traversal is for animation? That looks like what should happen anyway,
> though perf-wise is kind of annoying.

I mean transition rules, above, sorry :)
I don't think we can do it since some transitioning properties should inherit, color, font-size, etc. can we?  Anyway, this bug has been there on gecko for long time (in the first place I think), so I'd like to deprioritize this if there is a mitigating way to avoid the failure in test_transitions.html and if this transition issue does not block :bgrins' work.
Blerg, indeed... Yeah, I agree this is probably not a priority to fix and I should just land the invalidation stuff which should fix this.
> I meant 'the child element were traversed on gecko'.  Is this correct English?

Yes.
test_transitions.html now passes with the -moz-window-inactive selector thanks to bug 1409672, thank you Emilio. :)

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> > I meant 'the child element were traversed on gecko'.  Is this correct English?
> 
> Yes.

And thank you bz. :)
I did forget to remove stylo prefix in this bug title.
Summary: Stylo: Get rid of inherited transition values from after-change style → Get rid of inherited transition values from after-change style
You need to log in before you can comment on or make changes to this bug.