386228 - Unify back and forward tab history and provide only one drop-down button (IE7 style)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P2)

Description

[Simon Bünzli]

1. Keeping back and forward tab history separate makes you search in two places when one would be enough.

2. Only offering one drop-down button (at least when both buttons are located side-by-side), would reduce visual complexity (cf. http://blog.mozilla.com/faaborg/2007/06/26/quantitative-design/ ).

3. A single list could either be offered in the History menu itself (see bug 329183) or as a single sub-menu along the list of recently closed tabs (allowing a cleaner fix for bug 2877439).

Although the prototype extension linked from the URL unifies both buttons into one, that wouldn't be necessary. Both buttons could merge if they are side-by-side but stand on their own, preferably keeping the unified tab history as drop-down, though (so that you could remove the Forward button to save space and still have access to the odd page located on the wrong side of history).

The unified list would ideally be sorted in reverse order, so that the most recently visited pages are closest to the cursor when the drop-down opens (as IE7 works).

Comment 1

[Simon Bünzli]

Alex: As to <http://blog.mozilla.com/faaborg/2007/11/15/the-shape-of-things-to-come/>, the extension linked from this bug's URL implements a unified session history drop-down for both Back and Forward button.

Should you have a look at it, you might also want to consider the way it handles the overflow case: when more than 11 pages have been visited, it at first rather omits navigation within the same website, so that you can always "fast rewind" to e.g. the website's homepage or the Google search results page you came from.

Comment 2

[Simon Bünzli]

Attachment: WIP

Comment 3

[Simon Bünzli]

This patch merges back and forward button when they're right next to another by hiding the forward button and using a new binding for the back button which includes a forward part and which fills the history drop-down with a unified list.

The main technical issue which I haven't managed to cleanly solve so far is the fact that it currently seems impossible to add a <xul:observes> from inside the XBL (cf. bug 390664) which makes us require slight hackery to keep both button parts correctly enabled/disabled (I guess that'd mostly affect extensions such as IETab which replace webnav's session history). Hints/ideas for a different approach are appreciated.

Opposed to the above listed extension, this patch doesn't modify the buttons when they're separated resp. when one of them is missing; and neither does it include the fast forward/rewind entries in the history drop-down.

Finally, adding an item to open the history sidebar should IMO be a follow up bug (not sure whether that item should go below the drop-downs of the separated buttons as well).

Alex: Please advise if you have any more input design-wise.

Comment 4

[AndrewM]

Oh dear, this is going to be optional right? One of the things I really hate about IE7 is the combined back/forward history. Is it possible to have a pref to keep the current behavior? It's so much clearer and easier to use.

For example, the first argument for this change in comment 0 doesn't really apply to me, I usually have a pretty good idea about whether the page I want to get to is forwards or backwards from where I am at the moment; and I'm sure many other users are the same.

And as for the second point, it should be noted that Safari uses Firefox's current behavior of separate back and forward histories (although without the drop-down arrows, which is yet another reason why I don't use Safari :)

Comment 5

[AndrewM]

On a slightly more positive and constructive note, I just think that it would be far better concentrating on bugs which would offer real usability improvements (such as bug 389478 'Add favicons on back/forward menus'), rather than trying to fix things that are not broken :)

Comment 6

[Simon Bünzli]

Attachment: take 1 (new binding for adjacent back and forward buttons)

The difference with this patch applied should be minor to what we've had for Firefox 2: it's mostly one missing drop-marker and a unified drop-down menu. And as soon as the buttons are moved apart, you're back to normal (Mike, Alex: unless you've got a better plan, of course).

BTW: Themes can easily modify the unified binding and e.g. move the dropmarker button between the other two buttons as suggested in Alex's blog entry mentioned in comment #1.

Comment 7

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

>it at first rather omits navigation within the same website, so that you can >always "fast rewind"

I'm concerned about the case when you get redirected, and hitting the back
button always takes you back to the redirection, taking you to the page you
started on.  Ideally we would be able to detect pages that are redirecting, and
not add them to the back queue.  However in the meantime, optimizing the list
of back pages to fast forward to different sites will occasionally trap users. 
Also, I think interfaces like the sidebar work better for getting a more global
view of history, the drop down is really meant for recent revisitation.

>Finally, adding an item to open the history sidebar should IMO be a follow up
>bug

Sounds good.

>Opposed to the above listed extension, this patch doesn't modify the buttons
>when they're separated 

Will it be possible for us to use different images for the buttons when they
are separated?

Also, what impact will this implementation have on existing themes?  mcdavis
might want to take a look at it since he has expressed some concern that we may
be unintentionally making life harder for themers.

Thanks for working on this!

Comment 8

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

>(such as bug 389478 'Add favicons on back/forward menus')

It would be great to get favicons into the integrated menu.

Comment 9

[Simon Bünzli]

(In reply to comment #7)
> I'm concerned about the case when you get redirected, and hitting the back
> button always takes you back to the redirection,

Me too. Plus it might actually confuse users, which is why I haven't implemented that bit at all.

> Will it be possible for us to use different images for the buttons when they
> are separated?

Of course. The required CSS rules are in the patch as I use CSS myself to decide when to apply the unified binding and when to hide the forward button.

> Also, what impact will this implementation have on existing themes?

I've tried to keep the impact minimal by allowing most rules to continue to work without changes (there are no theme changes in the patch at all). Once a theme relies too much on the exact internal structure of a binding, it'll have to adapt, though (and anyway so).

Themers might want to install the above linked extension and see how their themes behave (the unified binding is almost identical)...

(In reply to comment #8)
> It would be great to get favicons into the integrated menu.

That can happen independently in bug 389478. :) Whichever bug gets fixed second will just have to adapt to the (minor) changes in the other bug.

Comment 10

[Simon Bünzli]

Attachment: take 2 (new binding for adjacent back and forward buttons)

This patch does the same as "take 1" but in a far saner manner (similar to what Dão does in bug 343396). It furthermore fixes one issue and makes it even simpler for themers to detect that the buttons are unified: #back-button[unified] now matches the unified buttons.

Comment 11

[Simon Bünzli]

Attachment: take 2 as extension

Comment 12

[Aaron Leventhal]

Very good that the current item is marked checked in accessibility APIs.

Can we have a keyboard shortcut for this? It's a handy feature for keyboard users as well. (It's always been an issue that we didn't have one). Even better would be a discoverable keyboard shortcut, e.g. as an option in the History menu. 

In fact, why is the list in the History menu different from this list? I find this list much more useful.

Comment 13

[Simon Bünzli]

(In reply to comment #12)
> Can we have a keyboard shortcut for this?

Note that with the current patch you won't get a unified list when the buttons aren't adjacent. Anyway, that'd be bug 287743...

> e.g. as an option in the History menu. 

Adding this as a sub-menu to the History menu similar to the list of recently closed tabs would be quite easy (see also bug 287743 comment#15).

Comment 14

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

>In fact, why is the list in the History menu different from this list? I find
>this list much more useful.

The list in the history menu is across all tabs, while this list is tab specific.  Overall I agree that the current history menu doesn't seem as useful as it potentially could be, I'll try to give that some thought.

Comment 15

[Simon Bünzli]

Gavin, Mike: Any estimate as to when you might get to this?

Alex: As to <http://blog.mozilla.com/faaborg/2007/12/13/a-first-look-at-firefox-3s-icons/>: With this change, the back and forward buttons could be stand-alone as well (they remain individually customizable) - so we'd need another pair of icons.

Comment 16

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

Comment on attachment 291890 [details]
take 2 as extension

ui-r+

Some nits, though:

 - the tooltips will need to be updated properly ("Go back to this page" / "Go forward to this page")
 - it might be nice to see if we can emulate the IE7 style use of -> and <- arrows when hovering over the other menuItems, but I don't think it's necessary.

(Sorry, I meant to do this a while back.)

Comment 17

[Simon Bünzli]

Attachment: nits fixed

(In reply to comment #16)
>  - the tooltips will need to be updated properly ("Go back to this page" / "Go
> forward to this page")

Done. What about the current page? "Currently displayed page"?

>  - it might be nice to see if we can emulate the IE7 style use of -> and <-
> arrows when hovering over the other menuItems

IE7 uses glyphs similar to check-mark and radio-dot which would require two new native appearances in order to get color and size of these arrow right. I'd prefer not having to do that and just use the tooltips instead. I've however added classnames to make it easy for themers to achieve this if they so wish.

Comment 18

[Dan Harkless]

I'm so glad this change is being made, as the separate Back and Forward dropdowns in Firefox constituted a huge degradation in ease-of-use compared to Mozilla / Netscape's Go menu, IMHO (cf https://bugzilla.mozilla.org/show_bug.cgi?id=287743#c7).

When I first saw the single dropdown icon to the right of IE7's Forward button, I first thought Microsoft had made the horrible change of only allowing going multiple steps forward, not multiple steps back.  Of course eventually I played with it and realized it was a resurrection of the old Mozilla Go menu concept.

In retrospect, I realized that having the IE7 glyph of the Back and Forward buttons being together in a little "recessed" area, with the dropdown arrow *outside* of that was intended to convey that the dropdown is for the two buttons collectively.  Though what they intended to convey didn't get through to me, it might to some people, and so it might be worth following Microsoft's lead on this visual metaphor.

Comment 19

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

>In retrospect, I realized that having the IE7 glyph of the Back and Forward
>buttons being together in a little "recessed" area, with the dropdown arrow
>*outside* of that

I think the drop down arrow will visually an conceptually group better with the controls if placed inside of the etched in area surrounding the back and forward button.  Hopefully we will have some initial versions of the control design ready to show soon.

Comment 20

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

(In reply to comment #17)
> Created an attachment (id=295426) [details]
> nits fixed
> 
> (In reply to comment #16)
> >  - the tooltips will need to be updated properly ("Go back to this page" / "Go
> > forward to this page")
> 
> Done. What about the current page? "Currently displayed page"?

Or "Stay on this page"

> >  - it might be nice to see if we can emulate the IE7 style use of -> and <-
> > arrows when hovering over the other menuItems
> 
> IE7 uses glyphs similar to check-mark and radio-dot which would require two new
> native appearances in order to get color and size of these arrow right. I'd
> prefer not having to do that and just use the tooltips instead. I've however
> added classnames to make it easy for themers to achieve this if they so wish.

wfm

Comment 21

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

upping to P2 to match priority of bug 411725 which this blocks

Comment 22

[Simon Bünzli]

Attachment: wrapper binding instead of reusing the #back-button

This fixes the conceptual problem that the the unified button really was just the Back button in disguise (see bug 411725 comment #6) - now it's a wrapper item on its own, containing both Back and Forward button, hiding their dropmarkers and providing its own.

Functionally, this is the same as attachment 295426 [details] [diff] [review] except for Mike's wording from comment #20.

Note: Themes which rely on the OS to draw the bevel might want to add the following rule to visually relate the drop marker to both buttons:

> #unified-back-forward-button { -moz-appearance: toolbarbutton; }

As I understand Alex, this applies to the default theme as well when it's in small-icons mode. I'll leave that change to bug 411725, though.

Gavin: Any ETA for a review? Or would Asaf be a better candidate for this?

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Sorry for not keeping you up to date, I've started reviewing this already. I had some comments on the earlier patch, and it looks like you've addressed one of them already :)  I'll take a look at the new patch now.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 297793 [details] [diff] [review]
wrapper binding instead of reusing the #back-button

>Index: browser/base/content/browser.css

>+/* ::::: Unified Back-/Forward Button ::::: */
>+#unified-back-forward-button {
>+  -moz-binding: url("chrome://browser/content/bindings.xml#unified-back-forward-button-wrapper");
>+}
>+#unified-back-forward-button > toolbarbutton > dropmarker {
>+  display: none; /* we provide our own */
>+}
>+
>+#unified-back-forward-button menuitem[checked],
>+#backMenu menuitem[checked], #forwardMenu menuitem[checked] {
>+  font-weight: bold;
>+}

What's the second selector for? The old backMenu/forwardMenu won't have checked items, will they?

(Why does backMenu exist, anyways? Can't the back button just use context="_child"?)

It would also be better to set a class on the menuitems (in createMenuItem, perhaps?) and use that to style them, rather than using the descendant selector here.

>Index: browser/base/content/browser.js

>+var UnifiedBackForwardButton = {
>+  unify: function() {

>+  separate: function() {

Do these work OK for RTL? Seems like they should be OK, but I'll test later.

>@@ -903,16 +942,17 @@ function delayedStartup()

>+  UnifiedBackForwardButton.unify();

It seems like this has the potential to mess with toolbar initialization (where the toolbar adds its children from the palette). I guess it's probably a safe assumption to make, that given the timeout this will run after the initialization is complete (and indeed UpdateUrlbarSearchSplitterState seems to already make that assumption).

>+        case "unified":

>+          for (var j = end - 1; j >= start; j--) {
>+            entry = sessionHistory.getEntryAtIndex(j, false);
>+            var tooltip = tooltips[j < index ? 0 : j == index ? 1 : 2];
>+            var item = createMenuItem(aParent, j, entry.title || entry.URI.spec, tooltip);

Perhaps the other branches in this function should be changed to pass title || spec to createMenuItem as well, to be consistent?

I'm not done reviewing, but I'm not likely to be able to finish during the weekend so I figured I would post these comments.

Comment 25

[Simon Bünzli]

(In reply to comment #24)
> The old backMenu/forwardMenu won't have checked items, will they?

They will when the buttons are unified (see the checks in BrowserBackMenu and BrowserForwardMenu). If just context="_child"...

> (Can't the back button just use context="_child"?)

Doesn't work for me. Dunno why, though. Would be nice to get rid of those two popups. Then again, it's been like this since day 1.

Comment 26

[Simon Bünzli]

Attachment: updated to comment #24

(In reply to comment #24)
> It would also be better to set a class on the menuitems

Done.

> Do these work OK for RTL?

They do.

> Perhaps the other branches in this function should be changed to pass title ||
> spec to createMenuItem as well, to be consistent?

Done. And while we're at it, they get the new tooltips as well.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #25)
> (In reply to comment #24)
> > The old backMenu/forwardMenu won't have checked items, will they?
> 
> They will when the buttons are unified (see the checks in BrowserBackMenu and
> BrowserForwardMenu). If just context="_child"...

But doesn't the other selector already cover that case? I guess this won't matter assuming you change to using a class, but that part seems to be omitted from your new patch (forgot to diff browser/themes?).

Comment 28

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

Is that a r-, then Gavin?

Comment 29

[Simon Bünzli]

Attachment: fixed an RTL omission

mcdavis941 noted that for bug 411725 the unified dropmarker needs to be RTL-aware as well. Now it is.

(In reply to comment #27)
> that part seems to be omitted from your new patch

It's still there. The rule applies to menuitem.unified-nav-current (right above menuitem.spell-suggestion).

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 298485 [details] [diff] [review]
fixed an RTL omission

>Index: browser/base/content/browser.css

>+menuitem.unified-nav-current {
>+  font-weight: bold;
>+}

Remove the "menuitem." (see http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Don.27t_qualify_ID-categorized_rules_with_tag_names_or_classes )

>Index: browser/base/content/browser.js

> function BrowserForwardMenu(event)
> {
>-  return FillHistoryMenu(event.target, "forward");
>+  var menuType = UnifiedBackForwardButton._unified ? "unified" : "forward";
>+  return FillHistoryMenu(event.target, menuType);
> }

Why does this need changing? Since you're using the back button's popup when unified, I don't see how this could ever be called if UnifiedBackForwardButton._unified is true. 

>Index: browser/base/content/bindings.xml

>+      <method name="_updateUnifiedState">

>+          var selfDisabled = this.hasAttribute("disabled");
>+          
>+          if (canGoBack || canGoForward) {
>+            if (selfDisabled)
>+              this.removeAttribute("disabled");
>+          }
>+          else if (!selfDisabled)
>+            this.setAttribute("disabled", "true");

Why bother with selfDisabled? removeAttribute already checks whether it exists first, and setAttribute when the attribute already exists shouldn't be too bad.

The person who lands this should watch out for a Txul/Ts hit (despite the fact that code's being added to delayedStartup).

Comment 31

[Simon Bünzli]

Attachment: for check-in

(In reply to comment #30)
> Remove the "menuitem."

Done. Should've known it, but got distracted by the following rule...

> Why does this need changing? Since you're using the back button's popup when
> unified, I don't see how this could ever be called if
> UnifiedBackForwardButton._unified is true.

This is needed because #forwardMenu is what is used as the Forward button's context menu.

> Why bother with selfDisabled?

Because there are several places in our code where comments say that unnecessarily messing with DOM attributes is bad performance wise. This ain't no broadcaster, but still. Anyway, I guess we can do without. Removed.

Thanks for the review.

Comment 32

[Simon Bünzli]

(In reply to comment #30)
> The person who lands this should watch out for a Txul/Ts hit
Thanks in advance.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #31)
> > Why bother with selfDisabled?
> 
> Because there are several places in our code where comments say that
> unnecessarily messing with DOM attributes is bad performance wise.

In general, that's right, yes. In this case the DOM code already optimizes for the situations this code would have had an effect (completely in the "removing an attribute that doesn't exist" case, and at least partially in the "setting an attribute to it's current value" case).

Comment 34

[Reed Loden [:reed]]

Checking in browser/base/jar.mn;
/cvsroot/mozilla/browser/base/jar.mn,v  <--  jar.mn
new revision: 1.117; previous revision: 1.116
done
RCS file: /cvsroot/mozilla/browser/base/content/bindings.xml,v
done
Checking in browser/base/content/bindings.xml;
/cvsroot/mozilla/browser/base/content/bindings.xml,v  <--  bindings.xml
initial revision: 1.1
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.48; previous revision: 1.47
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.933; previous revision: 1.932
done
Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.57; previous revision: 1.56
done

Comment 35

[mcdavis941 (sporadically reading bugmail)]

I think we want to check in Bug 411725 around the same time as this bug, but note that 411725 hasn't been reviewed yet.

Comment 36

[Reed Loden [:reed]]

Backed out.

Regressions:
9ms Txul
7ms Ts

Probably better numbers later.

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #35)
> I think we want to check in Bug 411725 around the same time as this bug

Not necessarily, this works without a theme change.

Comment 38

[Simon Bünzli]

We won't have good values for Ts/Txul regressions without having this backing. Re-requesting check-in.

BTW: This will be pretty much impossible to do without any perf regression at all (except if we made the buttons non-separable in the first place). What ms range would be considered acceptable?

(In reply to comment #35)
> I think we want to check in Bug 411725 around the same time as this bug

Except that this patch applies to all platforms while bug 411725 is Windows only... ;-)

Comment 39

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

Tossing to mconnor for the call. The new theme kind of depends on a combined back/forward button, though.

(In reply to comment #38)
> We won't have good values for Ts/Txul regressions without having this backing.
> Re-requesting check-in.

Is there any way we can do an isolated checkin, clobber the nightlies to get a good Ts/Txul number, and then back it out again? I'd like to have as-real-as-possible numbers, here.

Comment 40

[Reed Loden [:reed]]

(In reply to comment #39)
> Is there any way we can do an isolated checkin, clobber the nightlies to get a
> good Ts/Txul number, and then back it out again? I'd like to have
> as-real-as-possible numbers, here.

I'm not sure how clobbering the nightlies helps get better Ts/Txul numbers... I think it was in long enough for Talos to pick it up, so we should look at Talos numbers.

Comment 41

[Simon Bünzli]

Should the perf hit be unacceptable: any suggestions how we might squeeze out a few more ms? I'd really hate to see yet another customizing option gone (the individually removable forward button) and offering both a unified and separate buttons would just be confusing.

Comment 42

[Michael Ventnor]

Index: browser/base/content/browser.css
+#unified-back-forward-button > toolbarbutton > dropmarker {
+  display: none; /* we provide our own */
+}

You really should make this rule platform-specific; Linux depends on -moz-appearance to get a native dropmarker appearance.

Comment 43

[Michael Ventnor]

> You really should make this rule platform-specific; Linux depends on
> -moz-appearance to get a native dropmarker appearance.
> 

Sorry, I'm tired. I didn't realise that actually said display. Still, are you sure this shouldn't be a platform-specific rule?

Comment 44

[Simon Bünzli]

(In reply to comment #43)
> are you sure this shouldn't be a platform-specific rule?

Why would we want to display three equivalent dropmarkers for the unified button on any platform?

Comment 45

[Michael Ventnor]

(In reply to comment #44)
> (In reply to comment #43)
> > are you sure this shouldn't be a platform-specific rule?
> 
> Why would we want to display three equivalent dropmarkers for the unified
> button on any platform?
> 

I see. Never mind my useless comment then.

Comment 46

[Justin Dolske [:Dolske]]

Interestingly, Txul clearly regressed on Linux; OS X didn't budge; and the one number reported on Windows was higher but ambiguously so (Txul is noisy on that box, so without digging more into the numbers and only having 1 run, it's hard to say).

Ts also definitely jumped on Linux, but OS X and Windows didn't budge at all.

Comment 47

[Simon Bünzli]

(In reply to comment #46)
Right, requesting another checkin, then, to get better numbers for Windows and OS X.

Comment 48

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screenshot

1. The unified back and forward buttons has history
2. click the "disabled" unified back/forward button
3. display pull-down menu

Perhaps, this is intended behavior. 
But, I am confused to this behavior. 
Isn't this confusing?

Comment 49

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

>1. The unified back and forward buttons has history
>2. click the "disabled" unified back/forward button
>3. display pull-down menu
>
>Perhaps, this is intended behavior. 
>But, I am confused to this behavior. 
>Isn't this confusing?

Yeah, the drop down button should be the only thing that can display the unified back/forward history menu.

Comment 50

[Simon Bünzli]

Attachment: unbitrotted (and no longer reacts to left-clicks on disabled buttons)

(In reply to comment #49)
> Yeah, the drop down button should be the only thing that can display the
> unified back/forward history menu.

There were two reasons for having this feature:
* The Forward button is most of the time disabled. This behavior made the drop-marker significantly bigger and the popup menu thus much easier to access (per Fitts' Law). Of course you can still right-click to get the popup menu, but now you can't just keep the button down, select a menu item and release the button to navigate...

* It made the popup more discoverable in the first place. Hitting a disabled button not just did nothing, it informed you that what you're looking for might be found in the other navigational direction.

Anyway, since you feel that this is too confusing, it's gone. If you want it back, please file a follow-up bug.

Comment 51

[Reed Loden [:reed]]

Checking in browser/locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--  browser.properties
new revision: 1.60; previous revision: 1.59
done
Checking in browser/base/jar.mn;
/cvsroot/mozilla/browser/base/jar.mn,v  <--  jar.mn
new revision: 1.119; previous revision: 1.118
done
Checking in browser/base/content/bindings.xml;
/cvsroot/mozilla/browser/base/content/bindings.xml,v  <--  bindings.xml
new revision: 1.3; previous revision: 1.2
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.50; previous revision: 1.49
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.943; previous revision: 1.942
done

Comment 52

[Thomas Stache]

(In reply to comment #49)
> Yeah, the drop down button should be the only thing that can display the
> unified back/forward history menu.
> 

(In reply to comment #50)
> There were two reasons for having this feature:
> * The Forward button is most of the time disabled. This behavior made the
> drop-marker significantly bigger and the popup menu thus much easier to access
> (per Fitts' Law). Of course you can still right-click to get the popup menu...
> 
> Anyway, since you feel that this is too confusing, it's gone. If you want it
> back, please file a follow-up bug.
> 

Having used the UniBaFo extension <https://addons.mozilla.org/en-US/firefox/addon/2210> for a long time on Fx2 and Fx3 I must say that I'm quite fond of the feature to hit the disabled forward button. Especially since the extension provides natively themed hover feedback (at least on Windows), that made it quite clear that the whole Back-Forward-Button will be activated when you click.

Comment 53

[Reed Loden [:reed]]

I backed out most of the patch (except the string changes) due to the Ts/Txul regressions... you should have enough numbers now to start working on fixing the problems. Also, there seems to be a problem with the unified back/forward look not displaying until you go to the customize palette and close it. That needs to be figured out, too... Today's nightly has this patch in it for testing, if you want to play with it.

Comment 54

[Justin Dolske [:Dolske]]

The current data looks a bit different than the data I was looking at in comment 46... 

On Talos Twinopen, WinXP shows a clear hit, Linux is up just out of the noise, OS X and Vista (?!) didn't seem to move.

On Talos Ts, OS X jumped by 20% (!), Linux and Vista didn't seem to change out of the noise, and XP... well... qm-mini-xp03 seems to have gone up, and qm-mini-xp01/02 went down. The first box has been historically higher than the other two, so make of that what you will.

On the non-Talos tests, Txul went up on Linux, OS X and Windows might have gone up, but it's hard to see in the usual noise. Ts went up on Linux and Mac, but probably not Windows.

Comment 55

[Simon Bünzli]

As I said in comment #41: I'm pretty much out of realistic (and perf-neutral) options except for making the button unified-only - and that I'd hate to do...

Comment 56

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

(In reply to comment #55)
> As I said in comment #41: I'm pretty much out of realistic (and perf-neutral)
> options except for making the button unified-only - and that I'd hate to do...

Why?

Make a button that's unified only.
Migrate people who have adjescent back/forward layouts (90%+ of our users) to use that new button
Keep support for the old buttons in case other themes want to use them

There's no need for a new binding that causes a perf regression here, IMO. Going forward the unified button will be our primary solution, supporting the older stuff is a secondary goal.

Comment 57

[Dão Gottwald [:dao]]

There's no generic solution for migrating users from a customized toolbarset to a new one, as far as I know.

Comment 58

[Simon Bünzli]

(In reply to comment #56)
> Keep support for the old buttons in case other themes want to use them

Meaning that in the end there will be both one keyhole and two stand-alone buttons in the Customize palette?

> There's no need for a new binding that causes a perf regression here, IMO.

Unless I've missed something, we'll need a new binding anyway, as our current options don't allow for parts of a button to be disabled individually.

> There's no generic solution for migrating users from a customized toolbarset

Wouldn't this just mean removing the original buttons and toolbar.insertItem'ing the new one?

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> > There's no generic solution for migrating users from a customized toolbarset
> 
> Wouldn't this just mean removing the original buttons and
> toolbar.insertItem'ing the new one?

That would only work for toolbarsets that haven't been customized before. We had the same problem when the address/search bar splitter was added to the default toolbarset.

Comment 60

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

> Unless I've missed something, we'll need a new binding anyway, as our current
> options don't allow for parts of a button to be disabled individually.

Right, I think a new binding will be OK, it's just the switching between the bindings depending on control location that's causing the performance hit. I'm a little out of my depth, here, I think Dao might be able to comment more intelligently on this point.

How's this for a plan:

 - let's create a new button type which is a combined back/forward/dropmarker
 - this will use the unified history code you've already written
 - we'll file another bug to change the toolbar config to reset everyone's toolbars when they move to Firefox 3

Dao, can you help out with the new bug and the toolbar config change? We can also use that bug to move the home button to the bookmarks toolbar.

Simon: can Dao help you with the creation of the new binding as well?

(note: neither mconnor nor I are overly concerned with allowing users to tear the combined back/forward buttons apart; we can continue to support the other individual buttons for people who wish to customize their themes that way, I suppose.)

Comment 61

[Simon Bünzli]

(In reply to comment #60)
> Simon: can Dao help you with the creation of the new binding as well?

The binding is already mostly there (see attachment 291888 [details] [diff] [review] above), what's rather missing is the XUL addition to BrowserToolbarPalette and some CSS tweaks for all themes. I won't come to this before tomorrow afternoon, though (or maybe even Thursday). So: All help is welcome!

Comment 62

[Marco Zehe (:MarcoZ)]

(In reply to comment #61)
> The binding is already mostly there (see attachment 291888 [details] [diff] [review] above), what's
> rather missing is the XUL addition to BrowserToolbarPalette and some CSS tweaks
> for all themes.

When you reuse the binding from that attachment, please make sure to expose the accessible property on the binding as either XulPane or NoAccessible. XulPane is preferred of course. This will allow the a11y code to find all children and expose them so that tools such as Orca or Jambu can properly get to and activate them if necessary.

Comment 63

[Aaron Leventhal]

Actually, NoAccessible might be slightly better -- we need to test. NoAccessible should mean the child buttons are exposed, but not the container. Try it and see if those buttons are exposed. You should be able to check with DOM Inspector.

Comment 64

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

>Meaning that in the end there will be both one keyhole and two stand-alone
>buttons in the Customize palette?

I think this will be more usable, people can see and directly drag and drop the exact buttons they want.  I also wonder if we shouldn't add a checkbox similar to "use small icons" that says "separate back and forward"  this checkbox would be disabled if the buttons aren't next to each other, or the user is using small icons, or text.

Comment 65

[Dão Gottwald [:dao]]

Attachment: back/forward toolbaritem, v1

Comment 66

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 300236 [details] [diff] [review]
back/forward toolbaritem, v1

Some followups:
Need to file a bug about fixing the clickToHold code on Mac. Will land this along with bug 404109 so that we can reset the toolbar ID once.

Comment 67

[Mike Connor [:mconnor]]

Comment on attachment 300236 [details] [diff] [review]
back/forward toolbaritem, v1

sr+a=me, looks good

Comment 68

[:Gavin Sharp [email: [e-mail]]]

Attachment: landed patch

Comment 69

[:Gavin Sharp [email: [e-mail]]]

The landed patch includes some Mac CSS fixes to not regress styling on the Mac.

Comment 70

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/browser.css 	1.55
mozilla/browser/base/content/browser.js 	1.950
mozilla/browser/base/content/browser.xul 	1.416
mozilla/browser/themes/pinstripe/browser/browser.css 	1.118
mozilla/browser/themes/winstripe/browser/browser.css 	1.160 

Comment 71

[Frederic Bezies]

Is it normal to get "back and forward" icons removed from main toolbar on linux builds ?!

Comment 72

[Dão Gottwald [:dao]]

(In reply to comment #71)
> Is it normal to get "back and forward" icons removed from main toolbar on linux
> builds ?!

The icons will be back in the next nightly build if everything goes as planned.

Comment 73

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #71)
> Is it normal to get "back and forward" icons removed from main toolbar on linux
> builds ?!

Right, bug 404109's patch (landed a short while ago) should fix that problem.

Comment 74

[Ori Avtalion (salty-horse)]

Attachment: drop-down button in GTK

The drop-down button in GTK is as wide as the other buttons. Shouldn't it be thinner, like the previous version?

Comment 75

[Grey Nicholson (they/them)]

Attachment: Dropdown button against large icons with text; Nautilus

(In reply to comment #74)
> The drop-down button in GTK is as wide as the other buttons. Shouldn't it be
> thinner, like the previous version?
> 

Using large icons (especially with text), it's somewhat thinner than the other buttons, though Nautilus (bottom of the attached picture) is a few pixels thinner still.

Comment 76

[Reed Loden [:reed]]

Comment #74 and comment #75 -- bug 414876 is addressing that problem.

Comment 77

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

>Finally, adding an item to open the history sidebar should IMO be a follow up
>bug

Just filed Bug 415018 to add a history item on the bottom of this menu.

Comment 78

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020423 Minefield/3.0b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020423 Minefield/3.0b4pre

It's in there (tm), we've got spin-off bugs, and we will continue to file more, as needed.

Comment 79

[Stephen Donner [:stephend] Not actively reading bugmail]

VMWare Fusion cut-and-paste: fail; Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008020423 Minefield/3.0b4pre is what I wanted to paste alongside the XP user-agent in comment 77, above.

Comment 80

[Mark J. Reed]

Cool.  But the drop-down button needs a tooltip.

Comment 81

[Axel Grude [:realRaven]]

can we add a about:config option to get proper separate forward & backward history droppers back for people with motor disabilities or small screens, or people who don't want to break the metaphor (right=fwd, left=back) and enjoy having less movement if they only go back 1 or 2 steps and more if they go back 4 or 5?

It still seems to me that deciding to go unified was an arbitrary decision without usability in mind.