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

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Toolbars and Customization
P2
enhancement
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Simon Bünzli, Assigned: dao)

Tracking

(Blocks: 1 bug, {addon-compat, late-l10n})

Trunk
Firefox 3 beta3
addon-compat, late-l10n
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 10 obsolete attachments)

3.79 KB, application/x-xpinstall
beltzner
: ui-review+
Details
20.33 KB, image/png
Details
28.01 KB, patch
Details | Diff | Splinter Review
2.19 KB, image/png
Details
14.42 KB, image/png
Details
(Reporter)

Description

10 years ago
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).
(Reporter)

Comment 1

10 years ago
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.
Flags: blocking-firefox3?

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P4
Target Milestone: --- → Firefox 3 M10
(Reporter)

Comment 2

10 years ago
Created attachment 291754 [details] [diff] [review]
WIP
(Reporter)

Comment 3

10 years ago
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.
Keywords: helpwanted

Comment 4

10 years ago
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

10 years ago
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 :)
(Reporter)

Comment 6

10 years ago
Created attachment 291793 [details] [diff] [review]
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.
Assignee: nobody → zeniko
Attachment #291754 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #291793 - Flags: ui-review?(beltzner)
Attachment #291793 - Flags: review?(gavin.sharp)
>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!
>(such as bug 389478 'Add favicons on back/forward menus')

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

Comment 9

10 years ago
(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.
(Reporter)

Comment 10

10 years ago
Created attachment 291888 [details] [diff] [review]
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.
Attachment #291793 - Attachment is obsolete: true
Attachment #291888 - Flags: review?(gavin.sharp)
Attachment #291793 - Flags: ui-review?(beltzner)
Attachment #291793 - Flags: review?(gavin.sharp)
(Reporter)

Comment 11

10 years ago
Created attachment 291890 [details]
take 2 as extension
Attachment #291890 - Flags: ui-review?(beltzner)

Comment 12

10 years ago
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.
(Reporter)

Comment 13

10 years ago
(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).
Keywords: helpwanted
(Reporter)

Updated

10 years ago
Blocks: 287743
>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.
(Reporter)

Comment 15

10 years ago
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.
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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.)
Attachment #291890 - Flags: ui-review?(beltzner) → ui-review+
(Reporter)

Comment 17

10 years ago
Created attachment 295426 [details] [diff] [review]
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.
Attachment #291888 - Attachment is obsolete: true
Attachment #295426 - Flags: review?(gavin.sharp)
Attachment #291888 - Flags: review?(gavin.sharp)
(Reporter)

Updated

10 years ago
Keywords: uiwanted
Whiteboard: [has patch][need review gavin]

Comment 18

10 years ago
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.
>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.

Updated

10 years ago
Priority: P4 → P3
(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
upping to P2 to match priority of bug 411725 which this blocks
Blocks: 411725
Priority: P3 → P2
(Reporter)

Comment 22

10 years ago
Created attachment 297793 [details] [diff] [review]
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?
Attachment #295426 - Attachment is obsolete: true
Attachment #297793 - Flags: review?(gavin.sharp)
Attachment #295426 - Flags: review?(gavin.sharp)
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 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.
(Reporter)

Comment 25

10 years ago
(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.
(Reporter)

Comment 26

10 years ago
Created attachment 297883 [details] [diff] [review]
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.
Attachment #297793 - Attachment is obsolete: true
Attachment #297883 - Flags: review?(gavin.sharp)
Attachment #297793 - Flags: review?(gavin.sharp)
(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?).
Is that a r-, then Gavin?
(Reporter)

Comment 29

10 years ago
Created attachment 298485 [details] [diff] [review]
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).
Attachment #297883 - Attachment is obsolete: true
Attachment #298485 - Flags: review?(gavin.sharp)
Attachment #297883 - Flags: review?(gavin.sharp)
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).
Attachment #298485 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin] → [has patch]
(Reporter)

Comment 31

10 years ago
Created attachment 298711 [details] [diff] [review]
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.
Attachment #298485 - Attachment is obsolete: true
(Reporter)

Comment 32

10 years ago
(In reply to comment #30)
> The person who lands this should watch out for a Txul/Ts hit
Thanks in advance.
Keywords: checkin-needed
(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).
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch]
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.
Backed out.

Regressions:
9ms Txul
7ms Ts

Probably better numbers later.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 37

10 years ago
(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.
(Reporter)

Comment 38

10 years ago
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... ;-)
Keywords: checkin-needed
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.
(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.
(Reporter)

Comment 41

10 years ago
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.
Keywords: checkin-needed

Comment 42

10 years ago
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

10 years ago
> 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?

(Reporter)

Comment 44

10 years ago
(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

10 years ago
(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.
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.
(Reporter)

Comment 47

10 years ago
(In reply to comment #46)
Right, requesting another checkin, then, to get better numbers for Windows and OS X.
Keywords: checkin-needed
Created attachment 299479 [details]
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?
>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.
(Reporter)

Comment 50

10 years ago
Created attachment 299496 [details] [diff] [review]
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.
Attachment #298711 - Attachment is obsolete: true
Blocks: 405605
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
Keywords: checkin-needed
Keywords: late-l10n

Comment 52

10 years ago
(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.

Updated

10 years ago
Depends on: 414362

Updated

10 years ago
Depends on: 414366

Updated

10 years ago
Depends on: 414370
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.
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.
(Reporter)

Comment 55

10 years ago
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...
Keywords: helpwanted
Whiteboard: [patch has Ts/Txul issues]
(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.
(Assignee)

Comment 57

10 years ago
There's no generic solution for migrating users from a customized toolbarset to a new one, as far as I know.
(Reporter)

Comment 58

10 years ago
(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?
(Assignee)

Comment 59

10 years ago
(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.
> 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.)
(Reporter)

Comment 61

10 years ago
(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

10 years ago
(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

10 years ago
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.
Assignee: zeniko → dao
Status: REOPENED → NEW
>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.
(Assignee)

Comment 65

10 years ago
Created attachment 300236 [details] [diff] [review]
back/forward toolbaritem, v1
Attachment #299496 - Attachment is obsolete: true
Attachment #300236 - Flags: review?(gavin.sharp)
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.
Attachment #300236 - Flags: superreview?(mconnor)
Attachment #300236 - Flags: review?(gavin.sharp)
Attachment #300236 - Flags: review+
Attachment #300236 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Keywords: late-compat
Comment on attachment 300236 [details] [diff] [review]
back/forward toolbaritem, v1

sr+a=me, looks good
Attachment #300236 - Flags: superreview?(mconnor)
Attachment #300236 - Flags: superreview+
Attachment #300236 - Flags: approval1.9?
Attachment #300236 - Flags: approval1.9+
Created attachment 300302 [details] [diff] [review]
landed patch
Attachment #300236 - Attachment is obsolete: true
The landed patch includes some Mac CSS fixes to not regress styling on the Mac.
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [patch has Ts/Txul issues]
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 
(Assignee)

Updated

10 years ago
Blocks: 414367
(Assignee)

Updated

10 years ago
No longer depends on: 414370

Comment 71

10 years ago
Is it normal to get "back and forward" icons removed from main toolbar on linux builds ?!
(Assignee)

Comment 72

10 years ago
(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.
(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.
Created attachment 300390 [details]
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?

Updated

10 years ago
Blocks: 414876
Created attachment 300420 [details]
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 #74 and comment #75 -- bug 414876 is addressing that problem.

Updated

10 years ago
Depends on: 414923

Updated

10 years ago
No longer blocks: 414876
Depends on: 414876
>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.

Updated

10 years ago
Depends on: 414842
(Assignee)

Updated

10 years ago
No longer depends on: 414842
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.
Status: RESOLVED → VERIFIED
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

10 years ago
Cool.  But the drop-down button needs a tooltip.
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.
You need to log in before you can comment on or make changes to this bug.