1422903 - [Private Browsing] Regression: Tab Title and Tab Favicon of NewTab flicker

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Mehmet]

Attachment: PrivateMode.mov

59.0a1 (2017-12-04) (64-Bit)
macOS 10.12.6

STR:

1.) Open a Private Window
2.) Open New Tabs in this Private Window
3.) Take a look at the favicon and the title

Actual: Flicker is to see.

Expected: To see no flicker. Maybe you can suppress the Globe icon and the default New Tab title in Private Mode and show instead the Private Window Favicon and the Private Session Title directly.

Comment 1

[u577973]

59.0a1 (2017-12-04) (64-Bit)
Windows 10 x64

This issue occur on Win10 too.

Comment 2

[Josh Matthews [:jdm]]

This looks like it's caused by the favicon being set after the DOMContentLoaded event: https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/browser/components/privatebrowsing/content/aboutPrivateBrowsing.js#45-63

Comment 3

[Mehmet]

Is this issue also "Depends on: 1422074" ?

Comment 4

[Johann Hofmann [:johannh]]

No, this one is just a regression that we should fix :)

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.

We apply a custom workaround to show about:home and about:newtab favicons
as early as possible for perceived performance, but didn't previously consider
the about:privatebrowsing page (which can act as both about:home and about:newtab).

Review commit: https://reviewboard.mozilla.org/r/207300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207300/

Comment 6

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57ffbe07594003429de27d7611d4d0324d9c8154

Comment 7

[Luke Crouch [:groovecoder]]

Marking P3 for regular priority.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8936560 [details]
Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207300/diff/1-2/

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8936560 [details]
Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.

https://reviewboard.mozilla.org/r/207300/#review213780

::: browser/base/content/tabbrowser.xml:809
(Diff revision 2)
> +                    // Don't switch to the default icon on about:home or about:newtab,
> +                    // since these pages get their favicon set in browser code to
> +                    // improve perceived performance.
> +                    let isNewTab = originalLocation &&
> +                      (originalLocation.spec == BROWSER_NEW_TAB_URL ||
> +                       originalLocation.spec == "about:home");

I just tried this patch (before the == "about:home" check was just added) testing bug 1425449, and the first new tab (about:home) still flickers but the second new tab (not-preloaded about:newtab) does not flicker.

I wonder if we just want to say all about pages don't use a default icon `!(originalLocation && originalLocation.scheme == "about")`?

A quick test from about:about seems to show no icon for about:credits and about:logo. Acceptable to have no favicon for those without needing to list out these specs?

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Ed Lee :Mardak from comment #9)
> Comment on attachment 8936560 [details]
> Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.
> 
> https://reviewboard.mozilla.org/r/207300/#review213780
> 
> ::: browser/base/content/tabbrowser.xml:809
> (Diff revision 2)
> > +                    // Don't switch to the default icon on about:home or about:newtab,
> > +                    // since these pages get their favicon set in browser code to
> > +                    // improve perceived performance.
> > +                    let isNewTab = originalLocation &&
> > +                      (originalLocation.spec == BROWSER_NEW_TAB_URL ||
> > +                       originalLocation.spec == "about:home");
> 
> I just tried this patch (before the == "about:home" check was just added)
> testing bug 1425449, and the first new tab (about:home) still flickers but
> the second new tab (not-preloaded about:newtab) does not flicker.

Can you try it again? Note that there could still be a minimal flicker (I still can't seem to satisfy the automated tests), but it should be much better (I can't notice it with my eyes anymore).

> I wonder if we just want to say all about pages don't use a default icon
> `!(originalLocation && originalLocation.scheme == "about")`?

That sounds unnecessary to me, we're just special-casing about:home and about:newtab and I'd like to avoid unintended side effects on the other pages :)

Comment 11

[Ed Lee :Mardak]

I noticed in bug 1423256 comment 7 attachment 8937531 [details] that about:blank doesn't have a favicon. Should it be consistent with the other "New Tab"s or maybe it should stay "blank" (other than the title).

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8936560 [details]
Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.

https://reviewboard.mozilla.org/r/207300/#review214518

r=me because when testing locally this is such a nice improvement :-).

But I have some concerns about the code...

Also, do we have any idea of what still causes the flicker caught by the test? I would feel much better about this once the whitelist entry is gone in the flickering test.
And are you still considering declaring the favicons of about: urls in a central location so that we can stop having all these hacks in front-end code?

::: browser/base/content/tabbrowser.xml:808
(Diff revision 2)
>                    if (!this.mBrowser.mIconURL && !ignoreBlank) {
> +                    // Don't switch to the default icon on about:home or about:newtab,
> +                    // since these pages get their favicon set in browser code to
> +                    // improve perceived performance.
> +                    let isNewTab = originalLocation &&
> +                      (originalLocation.spec == BROWSER_NEW_TAB_URL ||

This use of BROWSER_NEW_TAB_URL makes me uncomfortable, as the new tab page could have been customized and be an https URL. I wonder if we should just check for about:newtab || about:privatebrowsing instead.

::: browser/base/content/tabbrowser.xml:2824
(Diff revision 2)
>                throw e;
>              }
>  
>              // Hack to ensure that the about:newtab favicon is loaded
>              // instantaneously, to avoid flickering and improve perceived performance.
>              if (aURI == BROWSER_NEW_TAB_URL) {

But we already had this problem here...

Comment 13

[Johann Hofmann [:johannh]]

(In reply to Florian Quèze [:florian] from comment #12)
> Comment on attachment 8936560 [details]
> Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.
> 
> https://reviewboard.mozilla.org/r/207300/#review214518
> 
> r=me because when testing locally this is such a nice improvement :-).
> 
> But I have some concerns about the code...

Hmmm, looking into BROWSER_NEW_TAB_URL you may be right that it's safer to use about:newtab instead. It's really what we want. I'll change it!

> Also, do we have any idea of what still causes the flicker caught by the
> test? I would feel much better about this once the whitelist entry is gone
> in the flickering test.

Right, I agree, I will give it another look before pushing this.

> And are you still considering declaring the favicons of about: urls in a
> central location so that we can stop having all these hacks in front-end
> code?

Yes, I would file a new bug (and maybe another one for about:preferences) because I'd like to finish this bug in 59 to fix the associated issues.

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8936560 [details]
Bug 1422903 - Prevent about:privatebrowsing favicon from flickering.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207300/diff/2-3/

Comment 15

[Johann Hofmann [:johannh]]

I think the remaining flicker on about:home (which is really hard to notice by now) comes from the late promise resolution we're talking about in bug 1403648. So that bug is basically blocking removing the two flicker whitelist items for the tab favicon.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a618b142d9dc
Prevent about:privatebrowsing favicon from flickering. r=florian

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a618b142d9dc

Comment 18

[Mehmet]

FYI: I created a follow up bug report (bug 1427186) regarding the "New Tab" Title which appears, before the title "Private Browsing" appears.

Comment 19

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly (2017-12-04) on Windows 10 x64 using the steps from comment 0.
I retested everything using Latest Nightly and beta 59.0b6 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 and the bug is not reproducing anymore.