Brian Grinstead [:bgrins] Reporter
	Description • 7 years ago

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

	Brian Grinstead [:bgrins] Reporter
	Comment 1 • 7 years ago

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?

	Brian Grinstead [:bgrins] Reporter
	Comment 2 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 7 years ago

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...

	Brian Grinstead [:bgrins] Reporter
	Comment 5 • 7 years ago

FWIW this is Stylo-specific as it passes with Stylo disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62092a548a7048cb45ca21a9d6a6073b02e1184d

	Brian Grinstead [:bgrins] Reporter
	Comment 6 • 7 years ago

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?

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 7 years ago

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.

	Brian Birtles (:birtles)
	Comment 8 • 7 years ago

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)

	Emilio Cobos Álvarez (:emilio)
	Comment 9 • 7 years ago

(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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 10 • 7 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 11 • 7 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 12 • 7 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 13 • 7 years ago

(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?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 14 • 7 years ago

'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'.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 15 • 7 years ago

This test case is somewhat weird behavior both on gecko and stylo.  I think after-change style is wrong on both.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 16 • 7 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 17 • 7 years ago

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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 18 • 7 years ago

(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.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 19 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 20 • 7 years ago

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...

	Emilio Cobos Álvarez (:emilio)
	Comment 21 • 7 years ago

(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 :)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 22 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 23 • 7 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 7 years ago

> I meant 'the child element were traversed on gecko'.  Is this correct English?

Yes.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 25 • 7 years ago

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. :)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 26 • 7 years ago

I did forget to remove stylo prefix in this bug title.