1372518 - The area on the left side of the location bar flickers right after first paint

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Florian Quèze [:florian]]

Attachment: first-paint-flickering.mp4

See the attached screen recording of first paint on the quantum reference hardware.

We are loading about:home as the homepage here.

The first frame showing the browser UI has:
- "New Tab" as the tab title.
- reload button disabled
- light grey (i) icon in the location bar.

~40 frames later, we have:
- the reload button is replaced by an enabled stop button
- the (i) icon disappears.
- 'Nightly' appears in the identity area of the location bar, causing the location bar placeholder to shift to the right.
- the tab title becomes "Nightly Start Page"

After 8 frames, we display the favicon in the tab.
Another 2 frames and we display the globe icon next to "Nightly".

After another 10 frames, the stop button is replaced by a disabled reload button.

~30 more frames and the reload button becomes enabled. 


I think we can significantly improve this. It's fine to have icons painted a bit later, but things should be at the correct place immediately.

The things I think we should fix:
- the stop button should never appear while loading about:home
- "Nightly" should already be visible in the identity area before first paint, to avoid the flicker when the "Search or enter address" placeholder shifts to the right.
- we should not display the grey (i) icon (it's fine if the globe icon takes slightly longer to appear)
- we should never display "New Tab" as the tab title. Either we should already show the final page title at first paint, or we should leave the tab empty until we have the correct title.
- (maybe) make the new tab white "+" button appear at first paint

I suspect some of these things can be fixed at once, but if some of this turns out to be unrelated code, we can turn this into a meta bug.

Comment 1

[Johann Hofmann [:johannh]]

Related to this, the paths for the small brand icons might be optimize-able, see bug 1365973.

Comment 2

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)

> The things I think we should fix:
> - the stop button should never appear while loading about:home

Fixed in bug 1376892.

> - "Nightly" should already be visible in the identity area before first
> paint, to avoid the flicker when the "Search or enter address" placeholder
> shifts to the right.
> - we should not display the grey (i) icon (it's fine if the globe icon takes
> slightly longer to appear)

I'll refocus this bug on these two points, as they are the only that remain.

> - we should never display "New Tab" as the tab title. Either we should
> already show the final page title at first paint, or we should leave the tab
> empty until we have the correct title.

This is bug 1362774.

> - (maybe) make the new tab white "+" button appear at first paint

This has been fixed, I don't have the bug number, but I think it's when the '+' png icon got replaced by an svg one.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8895499 [details] [diff] [review]
Patch

Review of attachment 8895499 [details] [diff] [review]:
-----------------------------------------------------------------

Thank your for this patch. It makes window opening much nicer. I can't see any trouble with the general approach right now (famous last words), I just had a couple of remarks on the code.

::: browser/base/content/browser.js
@@ +1398,5 @@
>      ToolbarIconColor.init();
>  
>      gRemoteControl.updateVisualCue(Marionette.running);
>  
> +    gIdentityHandler.initIdentityBlock(this._getUriToLoad());

You probably know this better than me, but _getURIToLoad() is so far only called in delayedStartup and gets two services. Are you sure it's fine to use this in onLoad?

@@ +7699,5 @@
>      // Update per-site permissions section.
>      this.updateSitePermissions();
>    },
>  
> +  secureInternalUIWhitelist: /^(?:accounts|addons|cache|config|crashes|customizing|downloads|healthreport|home|license|newaddon|permissions|preferences|privatebrowsing|rights|searchreset|sessionrestore|support|welcomeback)(?:[?#]|$)/i,

Nit: move this to the top of gIdentityHandler. Since it's not exposed publicly it can probably get an underscore in front.

@@ +7704,5 @@
>    setURI(uri) {
> +    if (this._initialURI) {
> +      if (uri.spec == "about:blank")
> +        return;
> +      this._initialURI = undefined;

This whole block is very confusing to understand, even as someone familiar with the code. It would probably help to

a) give initialURI a better name (see below)
b) make a comment

@@ +7751,5 @@
> +
> +    let uri = Services.io.newURI(initialURI);
> +    if (this.secureInternalUIWhitelist.test(uri.pathQueryRef)) {
> +      this._isSecureInternalUI = true;
> +      this._initialURI = initialURI;

Unless I misunderstand something this._initialURI can be made a bool, right? May I suggest renaming it to something that reflects it functionality better, like _suppressAboutBlankDuringLoad or _showChromeUIDuringFirstLoad? Those names are a bit longer than I'd like but you get the idea.

Also, if I'm not mistaken, all member variables of gIdentityHandler are declared and given a default value at the top.

@@ +7754,5 @@
> +      this._isSecureInternalUI = true;
> +      this._initialURI = initialURI;
> +      this.refreshIdentityBlock();
> +      // The identity label won't be visible without setting this.
> +      gURLBar.setAttribute("pageproxystate", "valid");

Are you not using SetPageProxyState("valid") for any particular reason? Otherwise you might want to do it.

Comment 5

[Florian Quèze [:florian]]

(In reply to Johann Hofmann [:johannh] from comment #4)

> ::: browser/base/content/browser.js
> @@ +1398,5 @@
> >      ToolbarIconColor.init();
> >  
> >      gRemoteControl.updateVisualCue(Marionette.running);
> >  
> > +    gIdentityHandler.initIdentityBlock(this._getUriToLoad());
> 
> You probably know this better than me, but _getURIToLoad() is so far only
> called in delayedStartup and gets two services. Are you sure it's fine to
> use this in onLoad?

These 2 services are:

    Cc["@mozilla.org/browser/sessionstartup;1"]
      .getService(Ci.nsISessionStartup);

This is currently loaded during app-startup, and even if we could delay it a bit, it would still be before first paint, as we want to have session data asap during startup, see bug 1369456.

    Cc["@mozilla.org/browser/clh;1"]
      .getService(Ci.nsIBrowserHandler)

This is the service that opens the browser window in the first place, so it's definitely already loaded, see the code at http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/components/nsBrowserContentHandler.js#773

The one concern I had when writing the patch is that _getURIToLoad() would now be called twice per window instead of once. If it turns out to have a significant cost, it should be easy to cache the result, but for now I don't think it's worth the extra complexity.

> @@ +7751,5 @@
> > +
> > +    let uri = Services.io.newURI(initialURI);
> > +    if (this.secureInternalUIWhitelist.test(uri.pathQueryRef)) {
> > +      this._isSecureInternalUI = true;
> > +      this._initialURI = initialURI;
> 
> Unless I misunderstand something this._initialURI can be made a bool, right?
> May I suggest renaming it to something that reflects it functionality
> better, like _suppressAboutBlankDuringLoad or _showChromeUIDuringFirstLoad?
> Those names are a bit longer than I'd like but you get the idea.

It could be a bool, yes. I initially decided to make it contain the URL to be able to only reset it once we have finally loaded the expected URL. But I then got concerned that if for some reason we end up loading a slightly different URL, that would make us swallow any future about:blank load, so I decided to reset this field on the first non-about:blank load.

> Also, if I'm not mistaken, all member variables of gIdentityHandler are
> declared and given a default value at the top.

I also hesitated for that. It's not very useful as it's a short lived field. Btw, _isURILoadedFromFile isn't declared at the top (it's finding that existing exception that made me not declare it. Maybe not a very solid reason :-)).

> @@ +7754,5 @@
> > +      this._isSecureInternalUI = true;
> > +      this._initialURI = initialURI;
> > +      this.refreshIdentityBlock();
> > +      // The identity label won't be visible without setting this.
> > +      gURLBar.setAttribute("pageproxystate", "valid");
> 
> Are you not using SetPageProxyState("valid") for any particular reason?
> Otherwise you might want to do it.

I was concerned about the possible side effects of these lines:
    gLastValidURLStr = gURLBar.value;
    gURLBar.addEventListener("input", UpdatePageProxyState);
especially as gURLBar.value has most likely not been set yet.

Also, the UpdatePopupNotificationsVisibility call would be useless overhead.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> The one concern I had when writing the patch is that _getURIToLoad() would
> now be called twice per window instead of once. If it turns out to have a
> significant cost, it should be easy to cache the result, but for now I don't
> think it's worth the extra complexity.

Could be a smart getter:

get _uriToLoad() {
  delete this._uriToLoad;
  this._uriToLoad = ...;
}

There's hardly any extra complexity really.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8896024 [details] [diff] [review]
Patch v2

Review of attachment 8896024 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me! Thank you.

Comment 9

[Florian Quèze [:florian]]

Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09bd400d06055390578ff9cb89556f3211e2e51

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7bd8419732
the identity block shouldn't flicker when loading a secure internal page in a new window, r=johannh.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a7bd8419732

Comment 12

[Adrian Florinescu [:aflorinescu]]

Verified on 57.0a1 20170906220306: Windows 10x64 Pro, Windows 7 x32, Mac OSX10.12 for:

-window - first open
-window - new window
-window - new private window
-  +drag and drop
-  +high contrast themes
-  +shortcuts