1305976 - Outline is only updated on repaint

Status: NEW

(Core :: Layout, defect, P3)

Description

[Christian Sonne [:cers]]

Attachment: test case

If a child of an element with an outline is affected by - for example - an animated transform in a way that should alter the outline, this is only reflected when forced to repaint.

If nothings forces the repaint (like mouse movement or animating another property), then the outline fails to update.

STR:
1) load attached test case
2) hold mouse cursor still
3) observe outer blue outlines
4) move mouse cursor around (over same window)
5) observe outer blue outlines

Expected result:
The two examples should look the same regardless of mouse movement

Actual result:
Examples only look similar (slightly jerkier for the left one) when mouse is moving

Through mozregression, I've narrowed down the cause to this range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=883e17fc475f&tochange=ab0490972e1e , and my best guess is that bug 1061364 is to blame.

Comment 1

[Markus Stange [:mstange]]

Bug 1061364 sounds like a good guess. Brian, want to take a look?

Comment 2

[Brian Birtles (:birtles)]

Wow, that's crazy. I didn't know outline was supposed to respect transforms (it doesn't in Chrome or Edge). Perhaps we need some special handling like we have for scroll bars where we updated the transform on the main thread every 200ms when an outline is applied on an ancestor?

I wouldn't want to back out bug 1061364 for this since, assuming it is the cause, it just goes to demonstrate how much unnecessary work we were doing before that landed.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I've never been looking into bug 1061364 deeply, it seems to affect only transitions. No? I guess bug 980770 is the one in the range in comment 0.

Comment 4

[Brian Birtles (:birtles)]

Oh, I didn't even see bug 980770 in there! That's definitely the cause (as confirmed by loading the test case with layers.offmainthreadcomposition.async-animations set to false).

I suppose our options include:

* Update the main thread periodically when we have an outline on an ancestor like we do for scroll bars (will probably look pretty bad)
* Make outline not incorporate transforms (to match Chrome/Edge) -- I'm guessing we chose to include them deliberately though
* Force animations on elements with a visible outline on an ancestor to the main thread

Comment 5

[Christian Sonne [:cers]]

(In reply to Brian Birtles (:birtles) from comment #2)
> Wow, that's crazy. I didn't know outline was supposed to respect transforms
> (it doesn't in Chrome or Edge).

The spec isn't terribly clear to me at least, but the most relevant part I can find says:

> Outlines may be non-rectangular. For example, if the element is broken across several lines, the outline should be an outline or minimum set of outlines that encloses all the element’s boxes.

So the question I guess is really if the box moves when something is transformed? Perhaps someone better versed in the spec can say...

Comment 6

[Hiroyuki Ikezoe (:hiro)]

https://drafts.csswg.org/css-ui-3/#outline-props

 The rendering of applying transforms to outlines is left explicitly undefined in CSS3-UI.

(In reply to Brian Birtles (:birtles) from comment #4)
> I suppose our options include:
> 
> * Update the main thread periodically when we have an outline on an ancestor
> like we do for scroll bars (will probably look pretty bad)
> * Make outline not incorporate transforms (to match Chrome/Edge) -- I'm
> guessing we chose to include them deliberately though

So,

> * Force animations on elements with a visible outline on an ancestor to the
> main thread

We should take this for now, I think.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 49, but we might still take a patch on beta.

Comment 9

[Andrew Overholt [:overholt]]

This feels like something that we wouldn't uplift a fix for all the way to beta. I'm assuming it's very much too late to back out bug 1061364 at this point?

Comment 10

[Brian Birtles (:birtles)]

(In reply to Andrew Overholt [:overholt] from comment #9)
> This feels like something that we wouldn't uplift a fix for all the way to
> beta. I'm assuming it's very much too late to back out bug 1061364 at this
> point?

The regressing bug here is bug 980770 (i.e. async animations) not bug 1061364 and I'm sure we won't back out bug 980770 for this.

This regression has been shipping since Firefox 41. We'll get to it but it's not our highest priority at the moment (we have other regressions from async animations like bug 1301305 that are higher priority).

Comment 11

[Andrew Overholt [:overholt]]

Thanks for clarifying, Brian.

Comment 12

[Astley Chen (inactive)]

By comment 10, wont fix on FF51.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

This bug has graduated beyond regression :)