859950 - Work - Overlay back button

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect)

Description

[Frank Yan (:fryn)]

Attachment: patch

One half of bug 836791.

Comment 1

[Matt Brubeck (:mbrubeck)]

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

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

This looks sweet!  Just a few functional concerns:

::: browser/metro/base/content/browser-ui.js
@@ +1764,5 @@
> +  back: function() {
> +    if (document.getElementById("cmd_back").disabled)
> +      CommandUpdater.doCommand("cmd_newTab");
> +    else
> +      CommandUpdater.doCommand("cmd_back");

I find this confusing.  Can we remove the cmd_newTab part for now, and add it in a different form when the new tab button actually lands?

::: browser/metro/theme/browser.css
@@ +972,5 @@
> +  box-shadow: none;
> +  transform: translateX(-54px);
> +}
> +
> +.overlay-button:not([startpage]):hover {

Instead of [startpage], we should use a [disabled] attribute (since about:start might not be the first page in the tab's history).

If we use a XUL element then we get this for free by using command="cmd_back".  I'm not sure if this works for an <html:div> or if you'd need to explicitly set an attribute in BrowserUI._updateButtons.

Comment 2

[Frank Yan (:fryn)]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Comment on attachment 735328 [details] [diff] [review]
> patch
> 
> Review of attachment 735328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks sweet!  Just a few functional concerns:
> 
> ::: browser/metro/base/content/browser-ui.js
> @@ +1764,5 @@
> > +  back: function() {
> > +    if (document.getElementById("cmd_back").disabled)
> > +      CommandUpdater.doCommand("cmd_newTab");
> > +    else
> > +      CommandUpdater.doCommand("cmd_back");
> 
> I find this confusing.  Can we remove the cmd_newTab part for now, and add
> it in a different form when the new tab button actually lands?
> 
> ::: browser/metro/theme/browser.css
> @@ +972,5 @@
> > +  box-shadow: none;
> > +  transform: translateX(-54px);
> > +}
> > +
> > +.overlay-button:not([startpage]):hover {
> 
> Instead of [startpage], we should use a [disabled] attribute (since
> about:start might not be the first page in the tab's history).
> 
> If we use a XUL element then we get this for free by using
> command="cmd_back".  I'm not sure if this works for an <html:div> or if
> you'd need to explicitly set an attribute in BrowserUI._updateButtons.

What I want to do here is never include about:start in tab history and just always take the user back to the start page when there is no more tab history left. This perfectly solves the problem of <a target="_blank"> links opening pages in new tabs and then users getting confused as to why they can't go back any more, because they are actually in a different tab now.

That's why checking [startpage] should work perfectly every time, whereas checking cmd_back doesn't give us the desired behavior.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Frank Yan (:fryn) from comment #2)
> What I want to do here is never include about:start in tab history and just
> always take the user back to the start page when there is no more tab
> history left. This perfectly solves the problem of <a target="_blank"> links
> opening pages in new tabs and then users getting confused as to why they
> can't go back any more, because they are actually in a different tab now.

Well, I'm not sure it "perfectly" solves the problem -- the user still can't press "back" to get to the previous page.  Instead they press "back" and end up in a new state -- one where they don't even have a back button.  It would be more helpful if "back" in this case returned them to the previous tab (this is what Android browsers do with the system Back button).

> That's why checking [startpage] should work perfectly every time, whereas
> checking cmd_back doesn't give us the desired behavior.

One problem with this is when a user visits Firefox Start *after* visiting some other page (for example, if they bookmarked about:start).  Then they can no longer go back in that tab.

I think this general approach could work, but we'd need to make other UX changes like preventing users from navigating to the start page in an existing tab, plus the code changes to actually add about:start to the beginning of every tab's history.  Let's separate those changes out into a different bug.

Comment 4

[Frank Yan (:fryn)]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> (In reply to Frank Yan (:fryn) from comment #2)
> > What I want to do here is never include about:start in tab history and just
> > always take the user back to the start page when there is no more tab
> > history left. This perfectly solves the problem of <a target="_blank"> links
> > opening pages in new tabs and then users getting confused as to why they
> > can't go back any more, because they are actually in a different tab now.
> 
> Well, I'm not sure it "perfectly" solves the problem -- the user still can't
> press "back" to get to the previous page.  Instead they press "back" and end
> up in a new state -- one where they don't even have a back button.  It would
> be more helpful if "back" in this case returned them to the previous tab
> (this is what Android browsers do with the system Back button).

That's true. Does it close the "child" tab?

> One problem with this is when a user visits Firefox Start *after* visiting
> some other page (for example, if they bookmarked about:start).  Then they
> can no longer go back in that tab.

That's a very good point.

> I think this general approach could work, but we'd need to make other UX
> changes like preventing users from navigating to the start page in an
> existing tab, plus the code changes to actually add about:start to the
> beginning of every tab's history.  Let's separate those changes out into a
> different bug.

Yeah, making the overlay back button do the same thing as the app bar back button makes sense. Is it easier to add about:start to the beginning of every tab's history or to simply not add about:start to the beginning of every tab's history (like we currently do for about:newtab on desktop (though that's something we actually want to fix on desktop))?

I'll make a new patch revision. Thank you for the prompt, thoughtful review.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Frank Yan (:fryn) from comment #4)
> > It would be more helpful if "back" in this case returned them to the
> > previous tab(this is what Android browsers do with the system Back button).
> 
> That's true. Does it close the "child" tab?

Yes -- which of course can also produce unexpected behavior (e.g. you can't go "forward" again), though this doesn't seem to be a major usability problem in practice.

If we had a nice zooming tab interface like you've proposed, it might be nice to just zoom out to show both tabs when the user tries to go "back" from a child tab, or something like that.

> Yeah, making the overlay back button do the same thing as the app bar back
> button makes sense. Is it easier to add about:start to the beginning of
> every tab's history or to simply not add about:start to the beginning of
> every tab's history (like we currently do for about:newtab on desktop
> (though that's something we actually want to fix on desktop))?

I'm not certain which one will make more sense; we may need to experiment and think a bit.

Comment 6

[Frank Yan (:fryn)]

Attachment: patch v2

oncommand doesn't get triggered on clicks for html:divs, but I also don't want to incur the overhead of XBL.

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> If we had a nice zooming tab interface like you've proposed, it might be
> nice to just zoom out to show both tabs when the user tries to go "back"
> from a child tab, or something like that.

That makes sense. I have a WIP patch for this zooming interface actually. I'll post it in a separate bug.

> > Yeah, making the overlay back button do the same thing as the app bar back
> > button makes sense. Is it easier to add about:start to the beginning of
> > every tab's history or to simply not add about:start to the beginning of
> > every tab's history (like we currently do for about:newtab on desktop
> > (though that's something we actually want to fix on desktop))?
> 
> I'm not certain which one will make more sense; we may need to experiment
> and think a bit.

I just realized that we might need to exclude about:start from the tab history in order to show a show a useful page and tab thumbnail in the tab strip after the following steps:
1. Click back with no more non-start-page history entries left. Now the start page is showing.
2. Switch to another tab and do the same thing.
3. The tab strip is much more useful, showing non-start-pages for both tabs, and we can achieve this by not including the start page in the tab history, though we'll need to make sure tab selection doesn't get all wonky and change handling of about:start in the URL bar as needed.
This is all for a separate bug (that I will file) of course, but I just wanted to post this here before I forget all of it.

Comment 7

[Matt Brubeck (:mbrubeck)]

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

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

Nice and simple - I like it. :)

Comment 8

[Frank Yan (:fryn)]

Thank you for the quick review!
I'm filing the relevant followups now. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/efd8efab8212

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/efd8efab8212

Comment 10

[Em-a-il]

Since this and #859997 (overlay plus button) landed I have the problem that there is a black bar across the page. (http://imageshack.us/photo/my-images/545/screenshot2ss.png)
It disappears when I hover one of the two new buttons.