596428 - Stop button shouldn't be ghosted, since this adds N ms to page load time

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

The way we ghost the stop button to avoid accidentally clicking the reload button if you hit it just as it switches state has the unfortunate side effect of making the page load seem slower than it really is.

Instead, we should ghost/disable the reload button for this time period instead, so the stop button doesn't linger until reload can be clicked. This is also likely to be less visually noisy.

Comment 1

[David Regev]

There is another related issue with the ghosting: it looks broken, since people don’t understand why the button is disabled for a second. I think I’ve even seen this issue being reported as a bug. What about not showing any icon during the transition (neither Stop nor Reload) but, instead, make the button yellow (preferably a muted yellow, so it’s not too visually noisy). That should make it clearer that the button is not broken and that this is some sort of precautionary transition period, just like on traffic lights, even if users don’t necessarily understand the purpose of this precaution. Alternatively, show the throbber during this period, which is what I’ve been using myself.

Comment 2

[Alex Limi (:limi) — Firefox UX Team]

No, showing more movement or more color is the opposite of what we want here. Delaying the disabling to after the page finishes and applying it to the reload button instead of the stop button makes it more subtle, transparent, and makes it feel like the page finishes faster.

It might even be an option to not have the reload button change icon at all, but just be disabled for the short period as an error prevention technique.

Comment 3

[David Regev]

(In reply to comment #2)
> It might even be an option to not have the reload button change icon at all,
> but just be disabled for the short period as an error prevention technique.

I don’t understand. I thought that’s what you were proposing in the first place: disabled Reload instead of disabled Stop (which I agree is better). I was saying that this still has the negative consequence that people will think the button is broken, because it takes a moment to switch states. A transition state that is more obvious would solve that issue.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>It might even be an option to not have the reload button change icon at all,
>but just be disabled for the short period as an error prevention technique.

how about we only display the disabled state of reload if the mouse happens to be hovering the button.  This way:

1) if the user has their mouse outside of the button, odds are they aren't going to be me able to get there in time to see the disabled state (but if they somehow do, they will)
2) if the user has their mouse inside of the button, they will get feedback that their soon to hit downclick isn't going to work
3) if they have no interest in hitting the button, they don't get any of the extra visual noise

Comment 5

[David Regev]

(In reply to comment #4)
> how about we only display the disabled state of reload if the mouse happens to
> be hovering the button.

Do you mean showing an empty colour-less glyph-less button when not hovered over? That would actually solve both the noise issue and the broken-appearance issue.

Comment 6

[Frank Yan (:fryn)]

(In reply to comment #4)
> >It might even be an option to not have the reload button change icon at all,
> >but just be disabled for the short period as an error prevention technique.
> 
> how about we only display the disabled state of reload if the mouse happens to
> be hovering the button.

This is an improvement over what Alex and I had discussed a couple months ago. I prefer this solution for its lack of visual noise in the common case.

I really don't find reloading out of impatience to be much of a problem.

Comment 7

[Dave Garrett]

(In reply to comment #6)
> I really don't find reloading out of impatience to be much of a problem.

That's what I initially thought too, a couple years ago when I installed an extension to combine the stop and reload buttons. But after a bit of use there were plenty of times that I accidentally hit refresh because I just missed the stop button. It's one of those little quirks that really gets you once you've used the feature for a while.

The fact that this problem was foreseen here and is being intentionally worked around actually impresses me a tiny bit. ;)

I think switching immediately to a non-functioning refresh button on completion (or even near completion) with a disabled effect on hover makes the most sense and least noise.

Comment 8

[Alex Limi (:limi) — Firefox UX Team]

Yup, we can definitely make the disabled state only appear on hover.

Comment 9

[Alex Limi (:limi) — Firefox UX Team]

Just a quick comment: now that about:home is in the nightlies, this is the perfect test case that shows how slow it feels with the ghosting of the stop button — the page is done instantaneously, but you feel like you have to wait for the red to go away. :)

Enter "about:home" in your URL bar and reload it a couple of times, and you'll see it very clearly.

Comment 10

[Alex Limi (:limi) — Firefox UX Team]

Requesting blocking for this, since it adversely affects our perceived performance.

Comment 11

[Michael Ventnor]

Perhaps we could have a very slow fade-transition from stop button to reload button, and the button is disabled during this animation?

Comment 12

[George K.]

Wouldn't fading between Stop and Reload be similar to the original complaint? The Stop button would still "hang around" after the page load has actually finished.

Just 2c: What about fading the Reload button from the disabled to the enabled state (on hover or always)?

Comment 13

[[not reading bugmail]]

Well the delay was added when the two buttons where allowed to merge into one, but it doesn't complete its cycle right away, its rather slower then the page completion. How about we just remove the delay or the make it less, as its been more annoying to have to wait to click it again.

Comment 14

[Peter Kasting]

The reason we purposefully stuck with a disabled Stop instead of a disabled Reload in Chrome 6 is because disabling Reload is misleading.  The reload functionality isn't actually disabled (e.g. hit F5).  The stop functionality, which the user was aiming to trigger, is what's become disabled.  If I target a stop button, and right as I mouse over it and click it turns into a disabled reload, what am I to think happened?  I hit reload and now the page is going to load again and the browser is stopping me from clicking further?

So showing a disabled Stop button is what most accurately conveys the state of the user interface.

Comment 15

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #14)
> The reason we purposefully stuck with a disabled Stop instead of a disabled
> Reload in Chrome 6 is because disabling Reload is misleading. 

Interfaces lie all the time in the name of efficiency and making things seem faster, e.g. progress bars.

The goal here is 1) error prevention, 2) avoid making the page load seem slower than it is.

The best way to do that is to disable the reload button for a short period if it is hovered when the page completes.

Comment 16

[Dave Garrett]

(In reply to comment #14)
> So showing a disabled Stop button is what most accurately conveys the state of
> the user interface.

People interface with the interface, thus the interface needs to be geared for the people. In other words, meaningful is more important than literal. It's more than just "little white UI lies" as Limi is saying. It's simply about speaking to the user truthfully about what's going on and what they need to know, not the underlying technical state they don't care about.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Those two views aren't exclusive in this case, it isn't a lie for us to remove the stop button after the page has finished loading (they can't actually stop anything).  We just need to prevent the user from hitting reload when they meant to hit stop.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Agree with Limi; we're disabling the control not because the function's disabled, but because we don't want the accidental misclick. Honestly, we could just disable the control for those few ms and we'd be in the same place, and it'd be less jittery.

Good perception of performance win, agreed that it should block.

(Peter: thanks for the context on Chrome!)

Comment 19

[Frank Yan (:fryn)]

Busier than expected. Unassigning myself for now.

I noticed that an additional theme bug:
both #reload-button[disabled]:hover and #urlbar-reload-button[disabled]:hover should not show the blue hovered state.

Comment 20

[:Margaret Leibovic]

(In reply to comment #8)
> Yup, we can definitely make the disabled state only appear on hover.

Fortunately, the enabled state (blue background) for the reload button is only identifiable on hover anyway.

(In reply to comment #19)
> I noticed that an additional theme bug:
> both #reload-button[disabled]:hover and #urlbar-reload-button[disabled]:hover
> should not show the blue hovered state.

Shouldn't the disabled/hover styles for #reload-button just be taken care of by the general toolbar button styling?

I also noticed another bug. The #urlbar > toolbarbutton:active:hover selector should really be #urlbar > toolbarbutton:not([disabled]):active:hover, since we do not want to apply pressed-looking styles to disabled buttons.

Comment 21

[:Margaret Leibovic]

Attachment: patch

Comment 22

[:Margaret Leibovic]

Attachment: patch

Oops, I made a mistake in the gnomestripe CSS. Fixed now.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 482726 [details] [diff] [review]
patch

>+    // Temporarily disable the reload button to prevent the user from
>+    // accidentally reloading the page when intending to click the stop button
>+    this.reload.setAttribute("disabled", "true");

Can you set this.reload.disabled = true; here?

In other cases where you're removing the 'disabled' attribute, you need to sync it with the reload command's disabled state instead (e.g. via XULBrowserWindow.reloadCommand.getAttribute("disabled") == "true").

Comment 24

[Frank Yan (:fryn)]

(In reply to comment #23)
> Comment on attachment 482726 [details] [diff] [review]
> patch
> 
> >+    // Temporarily disable the reload button to prevent the user from
> >+    // accidentally reloading the page when intending to click the stop button
> >+    this.reload.setAttribute("disabled", "true");
> 
> Can you set this.reload.disabled = true; here?
> 
> In other cases where you're removing the 'disabled' attribute, you need to sync
> it with the reload command's disabled state instead (e.g. via
> XULBrowserWindow.reloadCommand.getAttribute("disabled") == "true").


Margaret, this is what I was talking about on IRC. On pages like about:blank, the existing already disables reload, so we don't want to disable it temporarily and then re-enable it for those pages.

////

Possible solution if CombinedStopReload.switchToReload runs before those pages disable the reload command:

in browser.xul:
<toolbarbutton id="(urlbar-)reload-button" observes="Browser:ReloadOrDuplicate"...

in browser.js:
this.reload.disabled = true;
setTimeout(...
  if (XULBrowserWindow.reloadCommand.getAttribute("disabled") != "true")
    this.reload.disabled = false; ...

////

Possible solution if CombinedStopReload.switchToReload runs after those pages disable the reload command:

in browser.xul:
<toolbarbutton id="(urlbar-)reload-button" observes="Browser:ReloadOrDuplicate"...

in browser.js:
if (XULBrowserWindow.reloadCommand.getAttribute("disabled") != "true") {
  this.reload.disabled = true;
  setTimeout(... this.reload.disabled = false; ...

Comment 25

[:Margaret Leibovic]

(In reply to comment #24) 
> Possible solution if CombinedStopReload.switchToReload runs after those pages
> disable the reload command:
> 
> in browser.xul:
> <toolbarbutton id="(urlbar-)reload-button"
> observes="Browser:ReloadOrDuplicate"...
> 
> in browser.js:
> if (XULBrowserWindow.reloadCommand.getAttribute("disabled") != "true") {
>   this.reload.disabled = true;
>   setTimeout(... this.reload.disabled = false; ...

CombinedStopReload.switchToReload does run after those pages are disabled. I don't understand why I would need to add that observer in browser.xul. Just adding that check in browser.js seems to be doing the right thing.

Comment 26

[:Margaret Leibovic]

Attachment: patch v2

Comment 27

[Frank Yan (:fryn)]

Comment on attachment 482900 [details] [diff] [review]
patch v2

Tried it out. I guess the observes isn't needed in this case. Cool.

Patch looks good to me.
Amusing asymmetric use of the JS property and DOM attribute, but it works.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 482900 [details] [diff] [review]
patch v2

>   switchToReload: function (aDelay) {
>     if (!this._initialized)
>       return;
> 
>+    this.reload.removeAttribute("displaystop");
>+
>     if (!aDelay || this._stopClicked) {
>       this._stopClicked = false;
>       this._cancelTransition();
>-      this.reload.removeAttribute("displaystop");
>+      // Only enable the reload button if the reload command is enabled
>+      if (XULBrowserWindow.reloadCommand.getAttribute("disabled") != "true")
>+        this.reload.removeAttribute("disabled");

How about:

this.reload.disabled = XULBrowserWindow.reloadCommand.getAttribute("disabled") == "true";

>       return;
>     }
> 
>     if (this._timer)
>       return;
> 
>-    this._timer = setTimeout(function (self) {
>-      self._timer = 0;
>-      self.reload.removeAttribute("displaystop");
>-    }, 650, this);
>+    // Temporarily disable the reload button to prevent the user from
>+    // accidentally reloading the page when intending to click the stop button
>+    if (XULBrowserWindow.reloadCommand.getAttribute("disabled") != "true") {
>+      this.reload.disabled = true;
>+      this._timer = setTimeout(function (self) {
>+        self._timer = 0;
>+        self.reload.removeAttribute("disabled");
>+      }, 650, this);
>+    }
>   },

Always setting the timer and doing this.reload.disabled = ... like I did above would make this code more predictable, I think. Also, a convenient getter for XULBrowserWindow.reloadCommand.getAttribute("disabled") == "true" might make sense (CombinedStopReload accesses it three times now, I think).

Comment 29

[Frank Yan (:fryn)]

If you incorporate Dao's comments, don't forget to change [disabled] to [disabled=true] in the CSS. ;)

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #29)
> If you incorporate Dao's comments, don't forget to change [disabled] to
> [disabled=true] in the CSS. ;)

Shouldn't make a difference.

Comment 31

[Frank Yan (:fryn)]

(In reply to comment #30)
> (In reply to comment #29)
> > If you incorporate Dao's comments, don't forget to change [disabled] to
> > [disabled=true] in the CSS. ;)
> 
> Shouldn't make a difference.

Oh wow, node.disabled = false; actually removes the attribute. Dao's right. Cool.

Comment 32

[:Margaret Leibovic]

Attachment: patch v3

There were only two instances of XULBrowserWindow.reloadCommand.getAttribute("disabled") == "true" (because we unconditionally set the timer now), so I decided not to make a getter for it.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 482954 [details] [diff] [review]
patch v3

>+      // Only enable the reload button if the reload command is enabled
>+      this.reload.disabled = XULBrowserWindow.reloadCommand
>+                                             .getAttribute("disabled") == "true";

>+      // Only enable the reload button if the reload command is enabled
>+      self.reload.disabled = XULBrowserWindow.reloadCommand
>+                                             .getAttribute("disabled") == "true";

The comment doesn't really add any information, I think you can drop it.

Comment 34

[:Margaret Leibovic]

(In reply to comment #33)
> Comment on attachment 482954 [details] [diff] [review]
> patch v3
> 
> >+      // Only enable the reload button if the reload command is enabled
> >+      this.reload.disabled = XULBrowserWindow.reloadCommand
> >+                                             .getAttribute("disabled") == "true";
> 
> >+      // Only enable the reload button if the reload command is enabled
> >+      self.reload.disabled = XULBrowserWindow.reloadCommand
> >+                                             .getAttribute("disabled") == "true";
> 
> The comment doesn't really add any information, I think you can drop it.

Okay, will do.

Comment 35

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/95b0a1632878