Closed Bug 596428 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: limi, Assigned: Margaret)

Details

(Keywords: ux-userfeedback)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → fryn
Status: NEW → ASSIGNED
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.
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.
(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.
>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
(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.
(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.
(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.
Yup, we can definitely make the disabled state only appear on hover.
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.
Requesting blocking for this, since it adversely affects our perceived performance.
blocking2.0: --- → ?
Perhaps we could have a very slow fade-transition from stop button to reload button, and the button is disabled during this animation?
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)?
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.
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.
(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.
(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.
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.
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!)
blocking2.0: ? → betaN+
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.
Assignee: fryn → nobody
Assignee: nobody → 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.
Attached patch patch (obsolete) — Splinter Review
Attachment #482725 - Flags: review?(dao)
Attached patch patch (obsolete) — Splinter Review
Oops, I made a mistake in the gnomestripe CSS. Fixed now.
Attachment #482725 - Attachment is obsolete: true
Attachment #482726 - Flags: review?(dao)
Attachment #482725 - Flags: review?(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").
Attachment #482726 - Flags: review?(dao) → review-
Component: Theme → General
QA Contact: theme → general
(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; ...
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #482726 - Attachment is obsolete: true
Attachment #482900 - Flags: feedback?(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.
Attachment #482900 - Flags: feedback?(fryn) → feedback+
Attachment #482900 - Flags: review?(dao)
Whiteboard: [needs review 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).
If you incorporate Dao's comments, don't forget to change [disabled] to [disabled=true] in the CSS. ;)
(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.
(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.
Attached patch patch v3Splinter Review
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.
Attachment #482900 - Attachment is obsolete: true
Attachment #482954 - Flags: review?(dao)
Attachment #482900 - Flags: review?(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.
Attachment #482954 - Flags: review?(dao) → review+
(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.
Whiteboard: [needs review dao]
http://hg.mozilla.org/mozilla-central/rev/95b0a1632878
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.