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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-performance])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Related to this, the paths for the small brand icons might be optimize-able, see bug 1365973.
See Also: → 1365973
Whiteboard: [photon-performance] → [photon-performance] [triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: adrian.florinescu
Whiteboard: [photon-performance] [triage] → [photon-performance]
Depends on: 1376892
Depends on: 1376893
Depends on: 1362774
(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.
Posted patch Patch (obsolete) — Splinter Review
Attachment #8895499 - Flags: review?(jhofmann)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
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.
Attachment #8895499 - Flags: review?(jhofmann) → review-
(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.
(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.
Posted patch Patch v2Splinter Review
Attachment #8896024 - Flags: review?(jhofmann)
Attachment #8895499 - Attachment is obsolete: true
Comment on attachment 8896024 [details] [diff] [review]
Patch v2

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

This works for me! Thank you.
Attachment #8896024 - Flags: review?(jhofmann) → review+
Pushed by florian@queze.net:
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.
https://hg.mozilla.org/mozilla-central/rev/3a7bd8419732
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1393702
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.