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?

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.

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

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

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?

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.

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)

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

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

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.

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.

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.

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.