Closed Bug 1422903 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox59 --- verified
firefox60 --- verified

People

(Reporter: mehmet.sahin, Assigned: johannh)

References

Details

Attachments

(2 files)

Attached video 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.
Blocks: 1401955
59.0a1 (2017-12-04) (64-Bit)
Windows 10 x64

This issue occur on Win10 too.
Is this issue also "Depends on: 1422074" ?
No, this one is just a regression that we should fix :)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Blocks: 1423702
Marking P3 for regular priority.
Priority: -- → P3
Blocks: 1425449
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?
(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 :)
Component: Private Browsing → Tabbed Browser
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).
See Also: → 1423256
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...
Attachment #8936560 - Flags: review?(florian) → review+
(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.
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.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a618b142d9dc
Prevent about:privatebrowsing favicon from flickering. r=florian
https://hg.mozilla.org/mozilla-central/rev/a618b142d9dc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
FYI: I created a follow up bug report (bug 1427186) regarding the "New Tab" Title which appears, before the title "Private Browsing" appears.
Blocks: 1422962
Depends on: 1428873
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1434922
You need to log in before you can comment on or make changes to this bug.