1705215 - With "Bookmarks toolbar: only show on new tab", tab-switches to/from a new-tab cause the viewport-height to change (and cause layout/paint jank) **in all background tabs**

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Ensure you have "Bookmarks toolbar: only show on new tab" (this is the default)
2. Open two tabs, side-by-side. The first tab should be about:home (the new-tab page), and the second tab should be the attached testcase.
3. Switch back and forth between the two tabs.

ACTUAL RESULTS:
Every time you switch to the testcase, you can see that it has logged to indicate that its viewport changed height. (This height-change is from the toolbar appearing and/or disappearing while that tab is focused.)

EXPECTED RESULTS:
The toolbar-hiding-behavior should not cause the viewport of the testcase to change height. Conceptually, the toolbar should only be present for the new-tab page; the testcase shouldn't incur layout-jank as a result of that toolbar being hidden from the tab-switch operation.

I ran into this "in the wild" at Google Photos -- I was viewing a large photo which is scaled to fit my viewport, and I noticed that there was a visible "jump" in the photo rendering whenever I switched from a new-tab to the Google Photos tab. (It would briefly render at the smaller toolbar-smooshed size, and then render at the full viewport size.)

Comment 1

[Daniel Holbert [:dholbert]]

Simpler phrasing of "EXPECTED RESULTS":

You should never see more than a single number in the testcase, including after switching tabs away and back (to/from a new-tab where the bookmarks toolbar is visible)

If any additional numbers appear (they do right now), then that indicates that we've resized the viewport in a way that affects the testcase's layout; and this causes unnecessary relayout work for us, and it also may cause visual changes/flashes which are startling to the user, when they switch tabs.

Comment 2

[Daniel Holbert [:dholbert]]

This is actually a bit worse than I thought. It seems the bookmarks-toolbar here causes a viewport change in all background tabs, which is expensive and can cause unexpected side effects/breakage.

STR to demonstrate effect on background tabs:

1. Create a window with the following three tabs:
   (a) this bug's testcase https://bugzilla.mozilla.org/attachment.cgi?id=9215907
   (b) https://www.example.org/
   (c) a New Tab (with the bookmarks toolbar auto-showing)

2. Focus your testcase-tab and reload it, so that it only shows a single number.

3. Switch directly to your example.org tab (e.g. by clicking it) and then back to the testcase.
   ---> Note that the testcase doesn't show any additional numbers; still just one.

4. Now switch to example.org tab, and then to the new tab, and then back to example.org, and then back to your testcase.

ACTUAL RESULTS:
The testcase shows two additional height measurements, indicating that it was able to measure the viewport-height-change when you switched to the newtab page, even though it was in the background and was not even the tab that you were looking at directly before or directly after the newtab page.

EXPECTED RESULTS:
The testcase should not show any additional measurements. Its viewport should not change size.

This seems to cause actual site-breakage at https://www.purpleair.com/map if I switch from a new tab to another non-new tab to Purple Air (some of the time at least). I end up with a tiny 300x400 map-canvas in my Purple Air tab, apparently because the Purple Air site thinks the viewport resized, and its resize-handling code trips over something and doesn't work properly (maybe from being iframe-encapsulated?) when it reacts to this resize while it's in a background tab.

Comment 3

[Daniel Holbert [:dholbert]]

Here's a profile showing this problem at Purple Air:
Call tree: https://share.firefox.dev/3jE007H
Marker chart filtered for "Resize": https://share.firefox.dev/3lNTb64
(I'm basically performing STR step 4 but with Purple Air instead of my testcase. As you can see in the screenshots tab, I start on Purple air, and then switch tabs to example.org, and then to a new tab, and then back to example.org and then back to Purple Air.)

In the marker chart filtered for "Resize", you can see that we're posting two resize events for the Purple Air tab: one when we tab-switch from example.org to the new-tab, and another one just before we end up back at the purple air tab. This is happening despite the fact that we're never directly actually modifying that tab's viewport, and we're never even switching directly from that tab to another tab with a differently-sized viewport.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of broken purple air

Also, in the screenshots tab of comment 3's profiles, you can see that Purple Air ends up with a broken rendering at the end, with a 300x400 canvas area, as a result of this background-tab window-resize activity. Here's a larger screenshot showing that resulting brokenness.

Comment 5

[Daniel Holbert [:dholbert]]

(I filed bug 1724788 on the broken rendering at Purple Air -- that's a distinct underlying bug, and this bookmarks-toolbar thing just happens to be one way of triggering it, as a side effect of causing background-tab viewport-size changes.)

Comment 6

[Mark Banner (:standard8)]

Moving this across to the toolbars component as we believe this is more of a front-end issue, that is not specific to bookmarks. Any toolbar acting in a similar way would cause the same problem.

This will hopefully also give more visibility to the issue as under bookmarks it is less likely to be checked.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[:Gijs (he/him)]

I actually don't know how we'd go about fixing this.

All the toolbars are in a separate container to the stack of browsers (and there is no overlap between the two). AIUI we don't want to change this also for webrender reasons (there used to be an automated test that checked this, even).

To hide/show a toolbar, obviously the size of the toolbox (set of toolbars) changes, and as a result the size of the complete stack of browsers also changes, and with them, the associated viewports.

I don't know if or how we could disentangle this in such a way that we could keep the background tab content areas the same size while the frontmost tab changes size, while also maintaining the invariants about there not being overlap for the benefit of graphics optimization etc. Daniel or Emilio, do you have advice on how frontend should tackle this Gordian knot?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

All the toolbars are in a separate container to the stack of browsers (and there is no overlap between the two). AIUI we don't want to change this also for webrender reasons (there used to be an automated test that checked this, even).

I'm moderately sure the thing we need to do is to guarantee there's no overlap between the active tab browser and the toolbars. The background tab browsers are completely hidden to WebRender until they get painted.

I don't know if or how we could disentangle this in such a way that we could keep the background tab content areas the same size while the frontmost tab changes size, while also maintaining the invariants about there not being overlap for the benefit of graphics optimization etc. Daniel or Emilio, do you have advice on how frontend should tackle this Gordian knot?

It seems it should be fixable, but indeed it seems somewhat annoying to fix without awful special cases... Maybe we can tolerate overlap only on about:newtab? Then it feels like it should be possible to hack our way around this. But that'd involve probably having a ResizeObserver on the bookmarks toolbar since its size is not really fixed, etc...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I think moving the toolbar inside the browserContainer of the new tab page when using this mode should be feasible, but probably needs a bunch of changes to both the CSS and the front-end code to deal with the toolbar moving around...

Comment 11

[Dão Gottwald [:dao]]

I think this should be doable using absolute positioning?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Probably? If we abspos it then it'd overlap the about:newtab page right? That's probably fine?

Comment 13

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Probably? If we abspos it then it'd overlap the about:newtab page right? That's probably fine?

Even for about:newtab, we'd probably want to push the content down to avoid messing with layout (e.g. the cog icon on the top right) - but unfortunately this also has to work reliably for extension new tab pages and we can't easily do the same thing there.

I expect (but Dão can clarify perhaps) that the suggestion would be that we'd still resize the browser that has the new tab page in it, but if we made the toolbar abspos then we could do that independently of the toolbar appearing/disappearing so it'd fix this problem. Of course it's possible I misinterpreted the comment...

Comment 14

[Dão Gottwald [:dao]]

Attachment: WIP: Bug 1705215 - Let the bookmarks toolbar overlap the content area when showing only on new tab.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #13)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Probably? If we abspos it then it'd overlap the about:newtab page right? That's probably fine?

Even for about:newtab, we'd probably want to push the content down to avoid messing with layout (e.g. the cog icon on the top right) - but unfortunately this also has to work reliably for extension new tab pages and we can't easily do the same thing there.

I'm thinking we can just overlap the new tab page when the toolbox is hovered or has focus, see the attached WIP. I'll try to make some time to discuss this with UX and a11y and finish the patch.

Comment 17

[:Gijs (he/him)]

Given Dão assigned this, going to mark P1 as well.

Comment 18

[Dão Gottwald [:dao]]

Attachment: Bug 1705215 - Stop resizing all background browsers when showing the bookmarks toolbar only on the new tab page. r=gijs

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68d611170a4e
Stop resizing all background browsers when showing the bookmarks toolbar only on the new tab page. r=Gijs,tabbrowser-reviewers,mak

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/68d611170a4e

Comment 21

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on Windows 10, Mac 13, Ubuntu 22.04 with Firefox 119.0a1 (2023-08-29), 118.0b2, 117.0. Followed comment#1 and comment#3 usecases into checking that viewport-height doesn't change when default setting show bookmarks toolbar on newtab is set.