Open Bug 1387296 Opened 7 years ago Updated 2 years ago

Hovering background tabs causes the whole chrome to repaint

Categories

(Core :: Web Painting, defect, P3)

defect

Tracking

()

Tracking Status
firefox57 --- affected

People

(Reporter: mstange, Unassigned)

References

Details

Attachments

(2 files)

Steps to reproduce:
 1. Set nglayout.debug.paint_flashing_chrome to true.
 2. Hover over a background tab.

The whole chrome repaints.

This is probably due to the transform animation on .tab-line that was added in bug 1349555.

There may be something we can do to reduce the layerization changes triggered by this animation. I've already tried setting overflow:hidden on .tab-background, which seemed to help a bit in other ways but didn't fix the invalidation.
Should this be treated as a layout bug or as a front-end one?
Flags: needinfo?(mstange)
Whiteboard: [photon-visual][triage]
Possibly both? I'd say layout first, and the layout investigation might yield suggestions that should be applied to front-end code.
Flags: needinfo?(mstange)
Any suggestion who could help with the layout investigation? :)
Flags: needinfo?(mstange)
Whiteboard: [photon-visual][triage] → [reserve-photon-visual][p4]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
I'm doing the investigation.

The general problem is that whenever there's an off-main-thread transform animation, main thread layerization needs to account for that animation by seperating out everything around the animated element into two groups: content that will always be below the animated element, and content that will always be above the animated element. Layerization does not try to predict what values the transform on the animated element will attain; it simply assumes that anything that has a transform animation can potentially move *anywhere*.
Content can put the element with the transform animation inside of an element that has overflow:hidden. This way, layerization knows that the animated element can not move a certain box. And any content which is above the animated element in z-order but does not intersect the overflow:hidden box does not need to be put on a separate layer above the animated element.

Some notes:
Setting overflow:hidden on .tab-background isn't enough because .tab-background slightly overlaps with the navbar's top border.
Next I tried adding a wrapper element with overflow:hidden that wraps *just the tab line*, but that's also not enough. And that's because .tab-content overlaps the tab line, and the "hitregion" event region for .tab-content causes the creation of a layer. This can be worked around by setting pointer-events: none on .tab-content (and pointer-events: auto on the interactive elements inside .tab-content).
Once I had done those things, and *also* set pointer-events: none on .tab-background, layerization started to be what you'd want and the invalidations were gone. I'm not sure why that last part was necessary.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Priority: P3 → P1
Blocks: 1389476
FWIW, the medium term fix on the platform side is the move to WebRender. WebRender doesn't group things into layers the way we do at the moment, so layerization-related effects like the ones in this bug will be much rarer.
So, how bad is this bug's impact in practice? Could we just live with it until WebRender saves us? It's a bit annoying that the front-end workaround isn't self-contained at all.
Flags: needinfo?(mstange)
(In reply to Dão Gottwald [::dao] from comment #7)
> So, how bad is this bug's impact in practice?

Very bad. All the tabs to the right of the hovered background tab flicker, it's very distracting.

> Could we just live with it
> until WebRender saves us?

Only if WebRender is fully guaranteed to be in 57.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)

> All the tabs to the right of the hovered background tab flicker,

More specifically, on Mac it looks like there's a change in the font weight or opacity of tab labels.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > So, how bad is this bug's impact in practice?
> 
> Very bad. All the tabs to the right of the hovered background tab flicker,
> it's very distracting.

Is this Mac-specifc, related to vibrancy or something? I've never seen this on Ubuntu or Win 10.
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)
> 
> > All the tabs to the right of the hovered background tab flicker,
> 
> More specifically, on Mac it looks like there's a change in the font weight
> or opacity of tab labels.

I think that would be bug 1389518.
Note that https://bugzilla.mozilla.org/show_bug.cgi?id=1389476#c13 just reported seeing a similar effect on both Linux and Windows.
I'm not exactly sure of the difference between this bug and bug 1389476 (the latter describes exactly what I'm seeing).

I tried the patch here, and it seems to improve things but doesn't fully fix the bug: instead of having all the tabs to the right of the hovered tab changing appearance, it's only 2-3 tabs that get repainted, but it's still visibly broken. And the urlbar and searchbar placeholders still change too.
(In reply to Florian Quèze [:florian] [:flo] (away until August 28th) from comment #13)

> And the urlbar and searchbar placeholders still change too.

This part is unnoticeable on a retina screen, I only notice it on the external lowdpi screen. But enabling paint flashing makes it obvious on both screens.
(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> I'm not exactly sure of the difference between this bug and bug 1389476 (the
> latter describes exactly what I'm seeing).

This one is reproducible by enabling paint flashing on any platform and it's one of the factors playing into bug 1389476.
(In reply to Dão Gottwald [::dao] from comment #7)
> So, how bad is this bug's impact in practice? Could we just live with it
> until WebRender saves us? It's a bit annoying that the front-end workaround
> isn't self-contained at all.

The repaint itself is unfortunate but we could probably live with it. The impact on the text antialiasing is much worse. And the patch in this bug looks to be the only way to get rid of the text flashing on non-Mac platforms.
On Mac, we can fix the text flashing bug in the tabs just using the approach from bug 1387594, by hardcoding the font smoothing background color for the tab text so that the text is rendered the same way regardless of layerization. But that approach does not work on non-Mac platforms because non-Mac platforms don't have a way of overriding the font smoothing background color. (This is also something that will probably change with webrender, but for now, our options are limited.)

So I strongly believe we should take this patch, even though it's rather invasive.

However, comment 13 makes me think this patch is incomplete, at least on Linux. The remaining issues that comment 13 lists do not happen for me on Mac with this patch applied, so there's probably something subtly different in the Linux photon theme with respect to elements overlapping.
I think we should take this patch first, and in the meantime I'll do some more investigation on Linux to see if there are any other CSS tweaks that will address the remaining problems there.

I will also prepare patches for bug 1387594 because the patch in this bug is not enough on its own to fix the Mac troubles; the hovered tab itself is still affected, and scrolling the tab strip also causes the issue to appear, and bug 1387594 will fix those problems.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #16)
> On Mac, we can fix the text flashing bug in the tabs just using the approach
> from bug 1387594, by hardcoding the font smoothing background color for the
> tab text so that the text is rendered the same way regardless of
> layerization. But that approach does not work on non-Mac platforms because
> non-Mac platforms don't have a way of overriding the font smoothing
> background color. (This is also something that will probably change with
> webrender, but for now, our options are limited.)
> 
> So I strongly believe we should take this patch, even though it's rather
> invasive.
> 
> However, comment 13 makes me think this patch is incomplete, at least on
> Linux. The remaining issues that comment 13 lists do not happen for me on
> Mac with this patch applied, so there's probably something subtly different
> in the Linux photon theme with respect to elements overlapping.

I'm confused. I haven't seen any of these problems on Linux or Windows without enabling paint flashing. I believe Florian is on Mac?
Oops, I'm confused as well then. Somebody was reporting the bug on Windows or Linux as well. Let me double check.
Oh yeah, it was bug 1389476 comment 13, as mentioned by djc in comment 12.

That still leaves the mystery of the remaining issues from comment 13 on Mac.
I can reproduce this issue on Windows 10 using Intel HD iGPU.
The whole URL text in URL bar is something like repainting and also on some favicons in tabs it is visible. 
I can attach a video but unfortunately it is not very visible there. You have to look very closely on the text. The most visible part is on the text "Mozilla" especially if you look at "ll".

.tab-line:not([selected=true]) {
  transition: none !important;
}
Completely solved the issue as suggested in bug 1389476.

Bug 1401037 is about favicon issue, but I think root cause for all 3 bugs here is the same.

Will be nice to have it fixed for 57, since it is definitely noticeable and a bit distracting.
Attached video repainting bug.webm
Comment on attachment 8897085 [details]
Bug 1387296 - Improve layerization and reduce invalidations in the tab bar.

https://reviewboard.mozilla.org/r/168374/#review187040

With bug 1389476 and bug 1401037 fixed, it's my understanding that we don't need this anymore and can wait for WebRender to reduce the overhead.
Attachment #8897085 - Flags: review?(dao+bmo)
I reluctantly agree.
Status: ASSIGNED → NEW
Priority: P1 → P3
Assignee: mstange → nobody
Whiteboard: [reserve-photon-visual][p4]
See Also: → 1401122
See Also: → 1404366
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: