1890261 - The content inside the New Tab page is shaking after opening a new tab

Given bug 1890261 and talking to Emilio, this is likely something to do with the viewport size changing and that change somehow happening after the page's initial paint. However, it's not clear to me why that would be the case, despite some digging. Trying to make sure all the relevant attributes get set for the preloaded tabs also didn't help. Emilio, any chance you could help crystallize what is going on here?

Flags: needinfo?(emilio)

Emilio Cobos Álvarez [:emilio]

Comment 5

•

1 year ago

Is there an easy way to reproduce this? I haven't been able to notice the shift myself, which makes it hard to investigate...

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

Emilio Cobos Álvarez [:emilio]

Comment 6

•

1 year ago

On a debug build, I see the bookmarks toolbar sometimes disappear briefly when opening a new tab, which I suspect is related tho...

:Gijs (he/him)

Comment 7

•

1 year ago

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

Is there an easy way to reproduce this? I haven't been able to notice the shift myself, which makes it hard to investigate...

I can trivially reproduce by just hammering ctrl-t (cmd-t on my mac, but same deal...). Does that not work for you?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

:Gijs (he/him)

Comment 8

•

1 year ago

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

On a debug build, I see the bookmarks toolbar sometimes disappear briefly when opening a new tab, which I suspect is related tho...

This also seems unexpected to me if the bar was visible before, and I'm not sure why it would happen - but that would indeed likely be related.

Emilio Cobos Álvarez [:emilio]

Comment 9

•

1 year ago

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

I can trivially reproduce by just hammering ctrl-t (cmd-t on my mac, but same deal...). Does that not work for you?

No, that reproduces the bookmarks toolbar issue mentioned in comment 6, not comment 0. But ok, I think given that and some logging, what seems to be going on is that briefly, we think that we have about:blank rather than about:newtab.

Logging:

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
index 7f27f70c2fea3..662368874dbf4 100644
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6116,6 +6116,10 @@ function setToolbarVisibility(
             } catch (ex) {}
           }
         }
+        dump(currentURI?.spec + "\n");
+        if (currentURI?.spec == "about:blank") {
+          dump(new Error().stack + "\n");
+        }
         isVisible = BookmarkingUI.isOnNewTabPage(currentURI);
         document.documentElement.toggleAttribute(overlapAttr, isVisible);
         break;

I get entries like:

about:blank
setToolbarVisibility@chrome://browser/content/browser.js:6121:16
updateBookmarkToolbarVisibility@chrome://browser/content/browser.js:749:23
onLocationChange@chrome://browser/content/browser.js:4692:7
callListeners@chrome://browser/content/tabbrowser.js:983:31
_callProgressListeners@chrome://browser/content/tabbrowser.js:997:22
updateCurrentBrowser@chrome://browser/content/tabbrowser.js:1226:12
_setupEventListeners/<@chrome://browser/content/tabbrowser.js:6064:16
set selectedIndex@chrome://global/content/elements/tabbox.js:236:14
set selectedPanel@chrome://global/content/elements/tabbox.js:246:54
set selectedIndex@chrome://global/content/elements/tabbox.js:599:9
set selectedItem@chrome://global/content/elements/tabbox.js:617:35
set selectedTab@chrome://global/content/elements/tabbox.js:80:11
set selectedTab@chrome://browser/content/tabbrowser.js:423:7
addTab@chrome://browser/content/tabbrowser.js:2844:9
openLinkIn@resource:///modules/URILoadingHelper.sys.mjs:570:41
openTrustedLinkIn@resource:///modules/URILoadingHelper.sys.mjs:727:10
openTrustedLinkIn@chrome://browser/content/utilityOverlay.js:122:20
openTab/<.wrappedJSObject<@chrome://browser/content/browser-commands.js:339:28
openTab@chrome://browser/content/browser-commands.js:325:26
oncommand@chrome://browser/content/browser.xhtml:1:17

I think that should explain the issue? So now the question I guess is why the currentURI is about:blank... I can dig into that if you need?

Flags: needinfo?(emilio)

:Gijs (he/him)

Comment 10

•

1 year ago

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

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

I can trivially reproduce by just hammering ctrl-t (cmd-t on my mac, but same deal...). Does that not work for you?

No, that reproduces the bookmarks toolbar issue mentioned in comment 6, not comment 0. But ok, I think given that and some logging, what seems to be going on is that briefly, we think that we have about:blank rather than about:newtab.

Do you have the "recommended by pocket" stories and recent activity disabled (using the settings bit on the new tab page)? That's the thing that just confused me.

Logging:

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
index 7f27f70c2fea3..662368874dbf4 100644
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6116,6 +6116,10 @@ function setToolbarVisibility(
             } catch (ex) {}
           }
         }
+        dump(currentURI?.spec + "\n");
+        if (currentURI?.spec == "about:blank") {
+          dump(new Error().stack + "\n");
+        }
         isVisible = BookmarkingUI.isOnNewTabPage(currentURI);
         document.documentElement.toggleAttribute(overlapAttr, isVisible);
         break;

I get entries like:

about:blank
setToolbarVisibility@chrome://browser/content/browser.js:6121:16
updateBookmarkToolbarVisibility@chrome://browser/content/browser.js:749:23
onLocationChange@chrome://browser/content/browser.js:4692:7
callListeners@chrome://browser/content/tabbrowser.js:983:31
_callProgressListeners@chrome://browser/content/tabbrowser.js:997:22
updateCurrentBrowser@chrome://browser/content/tabbrowser.js:1226:12
_setupEventListeners/<@chrome://browser/content/tabbrowser.js:6064:16
set selectedIndex@chrome://global/content/elements/tabbox.js:236:14
set selectedPanel@chrome://global/content/elements/tabbox.js:246:54
set selectedIndex@chrome://global/content/elements/tabbox.js:599:9
set selectedItem@chrome://global/content/elements/tabbox.js:617:35
set selectedTab@chrome://global/content/elements/tabbox.js:80:11
set selectedTab@chrome://browser/content/tabbrowser.js:423:7
addTab@chrome://browser/content/tabbrowser.js:2844:9
openLinkIn@resource:///modules/URILoadingHelper.sys.mjs:570:41
openTrustedLinkIn@resource:///modules/URILoadingHelper.sys.mjs:727:10
openTrustedLinkIn@chrome://browser/content/utilityOverlay.js:122:20
openTab/<.wrappedJSObject<@chrome://browser/content/browser-commands.js:339:28
openTab@chrome://browser/content/browser-commands.js:325:26
oncommand@chrome://browser/content/browser.xhtml:1:17

I think that should explain the issue? So now the question I guess is why the currentURI is about:blank... I can dig into that if you need?

I added logging to updateCurrentBrowser and do not see it being hit with about:blank. I can see some occasionally in setToolbarVisibility but they do not correlate with the flickering and if I add an early return there for the about:blank case, I don't see any improvement.

Flags: needinfo?(emilio)

Emilio Cobos Álvarez [:emilio]

Comment 11

•

1 year ago

Do you have the "recommended by pocket" stories and recent activity disabled (using the settings bit on the new tab page)? That's the thing that just confused me.

Gah, I did not, and that explains it. The issue is that those add up enough content for the center-aligned flex item in the new tab page to overflow the viewport. So it doesn't shift on resize.

So if I early-return (well, early-break) to the about:blank case in setToolbarVisibility I definitely see an improvement, which is that the bookmarks toolbar no longer flickers when hammering Ctrl+T.

However, I can repro this now consistently, and I see what's going on. The issue is that the browser gets dynamically resized, and that the content in the new tab page is center aligned. So the shift is just what you see when you add and remove this padding.

In fact, if you do this, which I thought was a clever simplification:

diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js
index f319fd5d46c64..9627ff1f1ae2c 100644
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -3050,10 +3050,6 @@
         usingPreloadedContent,
       };

-      if (BookmarkingUI.isOnNewTabPage(uri)) {
-        this.getPanel(b).classList.add("newTabBrowserPanel");
-      }
-
       // Hack to ensure that the about:newtab, and about:welcome favicon is loaded
       // instantaneously, to avoid flickering and improve perceived performance.
       this.setDefaultIcon(tab, uri);
@@ -7091,13 +7087,6 @@
             this.mTab.linkedBrowser.mute();
           }

-          gBrowser
-            .getPanel(this.mBrowser)
-            .classList.toggle(
-              "newTabBrowserPanel",
-              BookmarkingUI.isOnNewTabPage(aLocation)
-            );
-
           if (gBrowser.isFindBarInitialized(this.mTab)) {
             let findBar = gBrowser.getCachedFindBar(this.mTab);

diff --git a/browser/themes/shared/browser-shared.css b/browser/themes/shared/browser-shared.css
index 9e513830a9d9e..35646a6ea4704 100644
--- a/browser/themes/shared/browser-shared.css
+++ b/browser/themes/shared/browser-shared.css
@@ -315,7 +315,7 @@
     z-index: 1;
   }

-  .newTabBrowserPanel,
+  .browserSidebarContainer.deck-selected,
   #sidebar-box {
     /* Make room for the bookmarks toolbar so that it won't visibly overlap the
        new tab page and sidebar contents. We do not put this padding on #browser

You also see it when ctrl-tabbing between newtab pages.

So it's just really a race between when the next layout happens in the parent process (which determines when the resize in content happens) and the ongoing load in the child process...

So I think the most reliable way to prevent this from happening would be to just move that padding to the child process, so that the newtab page applies it unconditionally when that setting is on, rather than async...

There are other possible fixes tho... But most of them would require some coordination between newtab and the browser anyways so...

Flags: needinfo?(emilio)

Emilio Cobos Álvarez [:emilio]

Comment 12

•

1 year ago

Attached file WIP: Bug 1890261 - Account for bookmark toolbar visibility on the newtab page itself. — Details

Something like this (I had to manually apply the change to
activity-stream-linux.css to test it, ugh) seems to work, but would
still need to pass down the settings (maybe via contentTheme.js?).

:Gijs (he/him)

Comment 13

•

1 year ago

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

So I think the most reliable way to prevent this from happening would be to just move that padding to the child process, so that the newtab page applies it unconditionally when that setting is on, rather than async...

There are other possible fixes tho... But most of them would require some coordination between newtab and the browser anyways so...

Hm. Unfortunately the resizing applies also to custom new tab pages (provided by web extensions) and to blank tabs. Obviously for blank tabs it doesn't make any visual difference when we resize or overlap, so that part is fine. But webextension new tab pages are a different matter - if the BMT ends up overlapping the top of those (which AIUI is what the WIP would cause to happen as-is?) that is likely to interfere with their styling (page would get cut off at the top). Or am I misunderstanding the patch?

The thing is, because we preload the new tab page, I would have thought that if we set the correct size when creating the browser in which we preload (or the new browser created when preload is off / we haven't created a new preload browser yet after using the last few), that would fix this, and then there wouldn't be any kind of race possible? New tab loads are supposed to be very fast because of this preloading, and I would have thought we could deal with this as part of that...
If we do this for newly created tabs that we know are "new tab"s (ie besides the preloading, which is turned off if the new tab page isn't provided by us), then that'd even address this in the webextension case.

Flags: needinfo?(emilio)

Donal Meehan [:dmeehan]

Updated

•

1 year ago

status-firefox125: fix-optional → wontfix

status-firefox126: fix-optional → wontfix

Emilio Cobos Álvarez [:emilio]

Comment 14

•

1 year ago

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

Hm. Unfortunately the resizing applies also to custom new tab pages (provided by web extensions) and to blank tabs. Obviously for blank tabs it doesn't make any visual difference when we resize or overlap, so that part is fine. But webextension new tab pages are a different matter - if the BMT ends up overlapping the top of those (which AIUI is what the WIP would cause to happen as-is?) that is likely to interfere with their styling (page would get cut off at the top). Or am I misunderstanding the patch?

No, that's correct, wasn't aware of that.

The thing is, because we preload the new tab page, I would have thought that if we set the correct size when creating the browser in which we preload (or the new browser created when preload is off / we haven't created a new preload browser yet after using the last few), that would fix this, and then there wouldn't be any kind of race possible? New tab loads are supposed to be very fast because of this preloading, and I would have thought we could deal with this as part of that...
If we do this for newly created tabs that we know are "new tab"s (ie besides the preloading, which is turned off if the new tab page isn't provided by us), then that'd even address this in the webextension case.

Yeah, that sounds right. It'd basically be a matter of adding the .newTabBrowserPanel class earlier or so...

Flags: needinfo?(emilio)

Haik Aftandilian [:haik]

Updated

•

1 year ago

status-firefox127: affected → wontfix

status-firefox128: --- → affected

Liz Henry (:lizzard)

Comment 15

•

1 year ago

Gijs, I notice this regression has a fair amount of discussion last month, but also has a low severity and priority set.
Do you think the impact/severity should be higher? If so then maybe you can help find an owner for this.

But otherwise, let's mark it fix-optional across all current releases so that it will drop from weekly regression triage. Thanks!

status-firefox129: --- → affected

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Comment 16

•

1 year ago

I think the severity of this is probably accurate in terms of the immediately-user-felt effect of the flicker. However, I was hoping to come back to it because I suspect there are performance implications to this bug (as we're resizing the new tab page and that takes CPU and GPU work that we shouldn't be doing) that we'd benefit from addressing more than the S4 severity might suggest. I'll keep the needinfo and request a perf evaluation, I guess.

Performance Impact: --- → ?

status-firefox128: affected → fix-optional

status-firefox129: affected → fix-optional

:Gijs (he/him)

Comment 17

•

1 year ago

Moving this to a reminder.

Flags: needinfo?(gijskruitbosch+bugs)

Henry Wilkes (they/them) [:henry-x]

Comment 18

•

1 year ago

I think we have a similar issue in Tor Browser: switching from a web page to a content page (our home page) causes the home page content to jump.

Bas Schouten (:bas.schouten)

Comment 19

•

1 year ago

As per Gijs' indication about resource use.

The Performance Impact Calculator has determined this bug's performance impact to be medium. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Resource impact: Severe
[x] Able to reproduce locally

Performance Impact: ? → medium

Emilio Cobos Álvarez [:emilio]

Comment 20

•

1 year ago

I don't think this is severe nowadays fwiw after bug 1917458, but also bug 1917458 changed how this code works significantly so are we sure this is still an issue? I can't repro comment 0 locally...

Updated

•

1 year ago

Summary: The content inside the New Tab page is shaking after opening a new tab. → The content inside the New Tab page is shaking after opening a new tab

Henry Wilkes (they/them) [:henry-x]

Comment 21

•

1 year ago

Attached video Screencast of new tab layout jank — Details

I can still reproduce something similar in my development build by switching between a new tab and a web page.

It seems to be much rarer when switching between new tabs though.

Daniel Holbert [:dholbert]

Updated

•

11 months ago

Blocks: 1920587

Denis Palmeiro [:denispal]

Updated

•

4 months ago

Performance Impact: medium → none

new_tab.gif 2 years ago Alexandru Trif, Desktop Test Engineering [:atrif] 474.03 KB, image/gif		Details
WIP: Bug 1890261 - Account for bookmark toolbar visibility on the newtab page itself. 1 year ago Emilio Cobos Álvarez [:emilio] 48 bytes, text/x-phabricator-request		Details \| Review
Screencast of new tab layout jank 1 year ago Henry Wilkes (they/them) [:henry-x] 223.87 KB, video/mp4		Details