772304 - Work - Implement loading / progress indicators for Metro URL bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P1)

Comment 1

[Jonathan Wilde [:jwilde]]

Sorry for missing description.

Currently, Metro Firefox lacks any indication of page load status, besides the combined stop-reload button.  This is confusing.  We need some sort of progress indicator so that users know what the status of the page is.

Comment 2

[Jim Mathies [:jimm]]

Do we have a design on this?

Maybe we could resurrect the never implemented fx desktop cylon tab animation? :)

Comment 3

[Jim Mathies [:jimm]]

The metro video player has a nice content overlay that matches the dots animation of metro. Might look nice to do something like that across content below the address bar so it is always visible to the user.

Comment 4

[Jonathan Wilde [:jwilde]]

Yuan and I talked about this today.  This will likely take the form of a progress line at the bottom of the URL toolbar.

We're looking at having the line be animated similarly to the address bar progress bar in Windows 8's Windows Explorer or Mountain Lion's Safari:

  - Initially, it'll move along like a normal progress bar/line, but the progress line will be scaled such that at 100%, it's only covers 70% of its container.
  - When it hits 100%, it rapidly wooshes along to fill the rest of the container and simultaneously fades out.

Comment 5

[Jonathan Wilde [:jwilde]]

Also, we need to get an updated stop button icon from shorlander for the URL bar and add that.

Comment 7

[Matt Brubeck (:mbrubeck)]

Yuan and/or Stephen, will you be able to produce design specs or mockups for this soon?

Comment 8

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I could work on this, and share with Stephen to get his opinion as soon as I finish. 
it's probably the fastest way. I know he's has a long to-do list and gonna travel this week.

Comment 9

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: Progress Line Spec

Comment 10

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Interaction on the progress indicator:
Whenever a new page is opened and loading, URL bar should automatically go down. The progress line is supposed to go across the screen. The same time, Refresh icon should change to Stop icon. Once it finishes loading, Stop icon changes back to refresh. '

Once user starts scrolling/interacting with site content, the URL bar will hide automatically. 


We should try to implement accelerating progress bar. This visual trick actually changes users' perception on duration(11% faster potentially). Read the scientific study here: http://www.chrisharrison.net/index.php/Research/ProgressBars2

Comment 11

[Frank Yan (:fryn)]

I don't think the progress bar is the right place for us to be promoting our brand colors, since orange and red usually imply safety/caution and error, respectively, as color indicators.

Comment 12

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

That's a good point. I am okay with changing to a neutral color, like blue. 
Looking into One Mozilla Style(http://mozilla.seanmartell.com/guide/index.php?directory=.&currentPic=16)
Could use this gradient: #00539F to #0095DD. Would like Stephen to review this.

Comment 13

[Matt Brubeck (:mbrubeck)]

Frank, any update on this?  A work-in-progress patch would be useful to start doing some testing and UX feedback.

As a temporary measure to improve the dogfooding experience, I checked in a tiny patch to show a throbber during page load.  This is *just* a temporary quick fixa we will back it out when the real progress bar is ready to land:
https://hg.mozilla.org/projects/elm/rev/5644ac3f7e66

Comment 14

[Jim Mathies [:jimm]]

Attachment: work week wip

Comment 15

[Jim Mathies [:jimm]]

Attachment: work week wip

Comment 16

[Jim Mathies [:jimm]]

Attachment: progress patch v.1

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 704343 [details] [diff] [review]
progress patch v.1

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

Looks good, just a bunch of nit-picky comments and some questions below.

Do you know if nsBrowserStatusFilter runs automatically to throttle/smooth the progress notifications, or do we need to do something to enable it?

::: browser/metro/base/content/BrowserProgressListener.js
@@ +5,5 @@
> +// Progress heartbeat timer duration (ms)
> +const kHeartbeatDuration = 1000;
> +// Start and end progress screen margins
> +const kProgressMarginStart = 30;
> +const kProgressMarginEnd = 70;

Can you add a comment that these are percentages?

@@ +27,5 @@
> +    switch (aMessage.name) {
> +      case "Content:StateChange": {
> +        if (json.stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) {
> +          if (json.stateFlags & Ci.nsIWebProgressListener.STATE_START)
> +            this._windowStart(json, tab);

Is this guaranteed to work even though we don't register for NOTIFY_STATE_WINDOW in bindings/browser.js?  Can we just use the STATE_IS_DOCUMENT notifications to control the progress bar instead?

@@ +63,5 @@
> +      }
> +    }
> +  },
> +
> +  _securityChange: function _securityChange(json, tab) {

Style nit: aJson, aTab.  Or you can something like "aParams" if "aJson" looks too weird.

Same nit for several other functions in this file.  Thanks for splitting these into functions, by the way - sorry to punish you by giving you extra refactoring work. :)

@@ +141,5 @@
> +
> +  _documentStop: function _documentStop(aTab) {
> +    // Make sure the URLbar is in view. If this were the selected tab,
> +    // _waitForLoad would scroll to top.
> +    aTab.pageScrollOffset = { x: 0, y: 0 };

I suspect this code is no longer needed and can be removed.  If so, you can do that here, or I can open a follow-up bug.

@@ +188,5 @@
> +  _progressStep: function _progressStep() {
> +    if (!this._progressActive)
> +      return;
> +    this._stepProgressCount();
> +    document.getElementById("progress-control").style.width = this._progressCount + "%";

Please add a lazy getter for the #progress-control element (e.g. "this.progressBar").  That should make the code look a little cleaner everywhere and might save us some CPU cycles here.

@@ +208,5 @@
> +    document.getElementById("progress-control").setAttribute("fade", true);
> +    document.getElementById("progress-control").style.width = "100%"; 
> +  },
> +
> +  progressTransEnd: function progressTransEnd(data) {

Nit: This should probably have an underscore, and I'd prefer a slightly more readable name (maybe "_progressTransitionEnded").

::: browser/metro/base/content/bindings/browser.js
@@ +37,5 @@
>      sendAsyncMessage("Content:StateChange", json);
>    },
>  
> +  // Currently not sent due to overhead
> +  onProgressChange: function onProgressChange(aWebProgress, aRequest,

This comment is no longer true and should be removed, right?

@@ +45,5 @@
> +      contentWindowId: content.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID,
> +      aCurSelfProgress:       aCurSelfProgress,
> +      aMaxSelfProgress:       aMaxSelfProgress,
> +      aCurTotalProgress:       aCurTotalProgress,
> +      aMaxTotalProgress:       aMaxTotalProgress,

Nit: Don't use the "a" prefix for the property names within the JSON object.

Question: Should we even bother sending this data if we aren't currently using it?

@@ +48,5 @@
> +      aCurTotalProgress:       aCurTotalProgress,
> +      aMaxTotalProgress:       aMaxTotalProgress,
> +    };
> +
> +    sendAsyncMessage("Content:ProgressChange", json);

Very minor stylistic hint: I prefer to pass the "json" literal directly when possible, rather than binding it to a local variable first.  Like this:

  sendAsyncMessage("Content:ProgressChange", {
    property: value,
    // ...
  });

Totally up to you whether you want to make this change.  Our existing code is inconsistent about this.

@@ +53,4 @@
>    },
>  
> +  onRefreshAttempted: function onRefreshAttempted(aWebProgress, aRefreshURI, aMillis, aSameURI) {
> +  },

I don't think you need this, since this object is not implementing nsIWebProgressListener2.

::: browser/metro/base/content/browser-scripts.js
@@ +83,5 @@
>  /*
>   * Browser scripts
>   */
>  [
> +  ["WebProgress", "chrome://browser/content/BrowserProgressListener.js"],

I'm starting a movement to make our filenames more closely related to the symbols they define/export.  Could you please change the filename to "WebProgress.js", or change the global to "BrowserProgressListener"?

::: browser/metro/base/content/browser.xul
@@ +275,5 @@
>              <box id="start-autocomplete"/>
>            </scrollbox>
>          </hbox>
> +
> +        <hbox id="progress-control" layer="true"></hbox>

Is the "layer" attribute needed? It's fine if it is, but it seems to be non-standard and not very well documented...

::: browser/metro/theme/browser.css
@@ +23,5 @@
> +  display: block;
> +  height: @progress_height@;
> +  max-height: @progress_height@;
> +  opacity: 1;
> +  background: -moz-linear-gradient(left,  @progress_start_color@,  @progress_end_color@);

We should add a #progress-control:-moz-dir(rtl) rule that sets a gradient in the opposite direction for RTL locales.

You can test this in English by setting "intl.uidirection.en" = "rtl" in about:config, though you'll see that the progress bar is the least of our worries. :)

Comment 18

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> Comment on attachment 704343 [details] [diff] [review]
> progress patch v.1
> 
> Review of attachment 704343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just a bunch of nit-picky comments and some questions below.
> 
> Do you know if nsBrowserStatusFilter runs automatically to throttle/smooth
> the progress notifications, or do we need to do something to enable it?

I looked at that but didn't find much use for it based on the logic in the code. What I've implemented a basic filter mechanism locally.


> @@ +27,5 @@
> > +    switch (aMessage.name) {
> > +      case "Content:StateChange": {
> > +        if (json.stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) {
> > +          if (json.stateFlags & Ci.nsIWebProgressListener.STATE_START)
> > +            this._windowStart(json, tab);
> 
> Is this guaranteed to work even though we don't register for
> NOTIFY_STATE_WINDOW in bindings/browser.js?  Can we just use the
> STATE_IS_DOCUMENT notifications to control the progress bar instead?

This came over from the original fennec code. I rely on _windowStart/Stop for the progress so they are firing.

> > +
> > +  _documentStop: function _documentStop(aTab) {
> > +    // Make sure the URLbar is in view. If this were the selected tab,
> > +    // _waitForLoad would scroll to top.
> > +    aTab.pageScrollOffset = { x: 0, y: 0 };
> 
> I suspect this code is no longer needed and can be removed.  If so, you can
> do that here, or I can open a follow-up bug.

Sure, I can remove it. Will clip it out and test.

> > +    document.getElementById("progress-control").setAttribute("fade", true);
> > +    document.getElementById("progress-control").style.width = "100%"; 
> > +  },
> > +
> > +  progressTransEnd: function progressTransEnd(data) {
> 
> Nit: This should probably have an underscore, and I'd prefer a slightly more
> readable name (maybe "_progressTransitionEnded").


Is that right? This is a public event callback?


> 
> ::: browser/metro/base/content/bindings/browser.js
> @@ +37,5 @@
> >      sendAsyncMessage("Content:StateChange", json);
> >    },
> >  
> > +  // Currently not sent due to overhead
> > +  onProgressChange: function onProgressChange(aWebProgress, aRequest,
> 
> This comment is no longer true and should be removed, right?

No it's accurate. I'm not using progress change events (I don't register for them so this will never be called) but I left the handler. I can take it out.

> Is the "layer" attribute needed? It's fine if it is, but it seems to be
> non-standard and not very well documented...

My impression is that it improves performance since only the progress layer will need to be repainted, and everything else can remain as a texture in memory. I have no data to back that up yet though. I'd like to do some testing if and when we ever get talos tests automated.

> We should add a #progress-control:-moz-dir(rtl) rule that sets a gradient in
> the opposite direction for RTL locales.

ah, nice catch.

Comment 19

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #18)
> > Do you know if nsBrowserStatusFilter runs automatically to throttle/smooth
> > the progress notifications, or do we need to do something to enable it?
> 
> I looked at that but didn't find much use for it based on the logic in the
> code. What I've implemented a basic filter mechanism locally.

One advantage of nsBrowserStatusFilter is that it prevents any JavaScript code from running at all on updates that it filters out, so it might be a bigger perf-win than equivalent filtering in JS.  We should file a follow-up bug to investigate it.

> > Is this guaranteed to work even though we don't register for
> > NOTIFY_STATE_WINDOW in bindings/browser.js?  Can we just use the
> > STATE_IS_DOCUMENT notifications to control the progress bar instead?
> 
> This came over from the original fennec code. I rely on _windowStart/Stop
> for the progress so they are firing.

We should add NOTIFY_STATE_WINDOW to our addProgressListener in bindings/browser.js then, to make sure this keeps working.  (I assume it's working now because other flags are set on the same message, but I don't think this is guaranteed.)

> > > +  progressTransEnd: function progressTransEnd(data) {
> > 
> > Nit: This should probably have an underscore, and I'd prefer a slightly more
> > readable name (maybe "_progressTransitionEnded").
> 
> Is that right? This is a public event callback?

The callback is set from within the same file.  No code outside of this file needs to refer to this function.  It's not part of the public "API" of this object.

> > > +  // Currently not sent due to overhead
> > > +  onProgressChange: function onProgressChange(aWebProgress, aRequest,
> > 
> > This comment is no longer true and should be removed, right?
> 
> No it's accurate. I'm not using progress change events (I don't register for
> them so this will never be called) but I left the handler. I can take it out.

Oh, I get it now.  Yes, please take this out if it's unused.

> > Is the "layer" attribute needed? It's fine if it is, but it seems to be
> > non-standard and not very well documented...
> 
> My impression is that it improves performance since only the progress layer
> will need to be repainted, and everything else can remain as a texture in
> memory.

Got it, thanks for the explanation.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #19)
> (In reply to Jim Mathies [:jimm] from comment #18)
> > > Do you know if nsBrowserStatusFilter runs automatically to throttle/smooth
> > > the progress notifications, or do we need to do something to enable it?
> > 
> > I looked at that but didn't find much use for it based on the logic in the
> > code. What I've implemented a basic filter mechanism locally.
> 
> One advantage of nsBrowserStatusFilter is that it prevents any JavaScript
> code from running at all on updates that it filters out, so it might be a
> bigger perf-win than equivalent filtering in JS.  We should file a follow-up
> bug to investigate it.


I'm not sure which has more overhead - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp#89

I think once we get talos going we'll want to revisit the impact the meter has. I'll file a bug on it to be sure we do.

Comment 21

[Jim Mathies [:jimm]]

Attachment: progress patch v.2

Comment 22

[Jim Mathies [:jimm]]

Attachment: progress patch v.2

Comment 23

[Jim Mathies [:jimm]]

http://hg.mozilla.org/projects/elm/rev/955c2b24c4ad

Comment 24

[juan becerra [:juanb]]

Checking to see if there are existing tests for this. Otherwise we'll triage for in-qa-testsuite to add Mozmill tests.