Closed
Bug 571454
Opened 14 years ago
Closed 13 years ago
Back button does not work along edge of screen (would like to see Fitts Law applied when on left most edge of the toolbars)
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 8
People
(Reporter: kurt_dixon150, Assigned: jaws)
References
Details
(Keywords: verified-beta, Whiteboard: [qa!][testday-20110930])
Attachments
(3 files, 9 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
shorlander
:
ui-review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100610 Minefield/3.7a5pre
I'm using the current nightly build and noticed that the back button behavior is different than in all previous versions of Firefox (from 1.0 until 3.6.3)
With a maximized window, the back button should be click-able from the extreme left edge of the screen, next to the button. This is to take advantage of Fitts's law. This is how it works in all previous Firefox versions (and most other browsers), and I consider it a bug in the current nightly version. This report is just to ensure awareness, I realize there are hundreds of more pressing issues at the moment.
Reproducible: Always
Steps to Reproduce:
1. Maximize window
2. Make sure the back button is active (ie, there's a previous page to go to)
3. Click with mouse directly against the left edge of the screen, at the same vertical position as the back button
Actual Results:
Nothing happens.
Expected Results:
The back button should be activated the same as if I clicked directly on it.
Comment 1•14 years ago
|
||
I can reproduce this on 3.7a6pre for both small and large icons. It has probably been around since the new navigation icons landed.
Comment 2•14 years ago
|
||
I should have noted that this was on WinXP.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100614 Minefield/3.7a6pre
Comment 3•14 years ago
|
||
Here is a mockup for a possible solution to this issue. The back button is moved slightly left so that the clickable area is on the edge of the screen. It does look somewhat awkward though.
The only other solution that comes to mind would be to increase the size of the clickable areas for the toolbar buttons to pre-3.7/4.0 levels. I'm not sure which one would be easier to implement.
Sorry for the triple post.
Comment 4•14 years ago
|
||
For similar reasons the Bookmarks button (or whatever button happens to be the right-most) should probably be aligned to the right screen edge.
Comment 5•14 years ago
|
||
We don't have to move the actual button, we should just make the clickable area larger.
Stephen, can you make sure this is connected to the theme bugs?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•14 years ago
|
||
Interesting I noticed this to. I agree with limi, I would only increase the click-able area as moving or increasing the size of the actual icon will make it look and feel crammed.
Comment 7•14 years ago
|
||
This should also apply to the bookmarks toolbar.
Reporter | ||
Comment 8•14 years ago
|
||
As of Firefox 4 beta 9, this bug still exists. Any progress on fixing it?
Comment 9•14 years ago
|
||
(In reply to comment #8)
> As of Firefox 4 beta 9, this bug still exists. Any progress on fixing it?
This is the only bug I've seen posted for this issue. So unless someone is assigned and actively working on getting this fixed, probably not. Its not a blocker yet.. but it could fall under papercuts or polish.
Summary: Back button in 3.7a5 minefield does not work along edge of screen → Back button does not work along edge of screen (would like to see Fitts Law applied when on left most edge of the toolbars)
Updated•14 years ago
|
Version: unspecified → Trunk
Comment 10•14 years ago
|
||
I would like us to try to take a fix for this before RC, even just off of the circle it doesn't activate as a hit right now.
Comment 11•14 years ago
|
||
Still present in release, and this is a constant annoyance.
Updated•13 years ago
|
Component: Toolbars → Theme
QA Contact: toolbars → theme
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•13 years ago
|
||
Can you please take a look at my proposed change? I have not made the required changes to pinstripe and gnomestripe yet, but I have it very close to working on winstripe.
There is an issue with the overlapping area of the back and forward buttons. It appears that the forward button has a higher z-index, and I have not come up with a way to fix this. Please let me know if you have any ideas.
Thanks!
Attachment #541411 -
Flags: feedback?(dao)
Assignee | ||
Comment 13•13 years ago
|
||
This is an update of the current progress for the fix of this bug. The clickable area is painted red and the z-index issue has been fixed.
This still needs to be ported to gnomestripe and pinstripe, as well as moving theme-specific styles out of the generic browser.css file.
Attachment #541411 -
Attachment is obsolete: true
Attachment #541592 -
Flags: feedback?(dao)
Attachment #541411 -
Flags: feedback?(dao)
Comment 14•13 years ago
|
||
Nothing needs to be done for gnomestripe, as far as I can tell. Pinstripe should be a separate bug.
Assignee | ||
Comment 15•13 years ago
|
||
This patch increases the clickable area for the back button on Windows and Mac. Gnomestripe had to be updated because browser.xul changed.
Attachment #541592 -
Attachment is obsolete: true
Attachment #542971 -
Flags: review?(dao)
Attachment #541592 -
Flags: feedback?(dao)
Assignee | ||
Comment 16•13 years ago
|
||
Dão, do you think we'll be able to land this patch in time for Firefox 7?
Comment 17•13 years ago
|
||
Probably not. I'm not happy with this patch, the XUL markup in particular doesn't seem sound.
Assignee | ||
Updated•13 years ago
|
Attachment #542971 -
Flags: feedback?(fryn)
Assignee | ||
Comment 18•13 years ago
|
||
I have redone the patch without modifying browser.xul by repurposing the nested toolbarbutton-icon. This patch only applies the changes to winstripe.
I think this is a much cleaner solution for the bug than the previous patch that I submitted. Thanks Dão for pushing me :)
Attachment #542971 -
Attachment is obsolete: true
Attachment #543546 -
Flags: review?(dao)
Attachment #543546 -
Flags: feedback?(fryn)
Attachment #542971 -
Flags: review?(dao)
Attachment #542971 -
Flags: feedback?(fryn)
Comment 19•13 years ago
|
||
Comment on attachment 543546 [details] [diff] [review]
Patch for bug 571454 - version 2
>+#navigator-toolbox[iconsize="small"][mode="icons"] > #nav-bar #back-button.toolbarbutton-1 {
>+ -moz-margin-start: 2px;
>+ -moz-margin-end: 0;
>+}
Why this change?
>-#back-button {
>+#back-button > .toolbarbutton-icon {
> -moz-image-region: rect(0, 18px, 18px, 0);
> }
>
> #forward-button {
> -moz-image-region: rect(0, 36px, 18px, 18px);
> }
>
>-#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button {
>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button > .toolbarbutton-icon {
> -moz-image-region: rect(18px, 20px, 38px, 0);
> }
>
> #back-button:-moz-locale-dir(rtl) > .toolbarbutton-icon,
> #forward-button:-moz-locale-dir(rtl),
> #forward-button:-moz-locale-dir(rtl) > .toolbarbutton-text {
> -moz-transform: scaleX(-1);
> }
>
>-#nav-bar #back-button {
>+#nav-bar #back-button > .toolbarbutton-icon {
> -moz-margin-end: 0 !important;
> }
And why these changes?
>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button > .toolbarbutton-icon {
> border-radius: 10000px;
>- padding: 0;
>+ padding: 5px;
> width: 30px;
> height: 30px;
width/height aren't needed anymore when you set the padding, are they?
Assignee | ||
Comment 20•13 years ago
|
||
I have fixed the issues that you pointed out in your previous review. Many of the child selectors for |.toolbarbutton-icon| were unnecessary.
However, there is an issue with this patch that I am not sure how to fix. Maybe you will have an idea:
With large icons and the back button disabled, the list-item-image opacity is not properly changed to |opacity: .5|. This is because |.toolbarbutton-icon| has been re-purposed for the visual state of the button itself, and thus needs |opacity: .8|. Do you know if/how I could set the list-item-image to |opacity: .5|?
Attachment #543546 -
Attachment is obsolete: true
Attachment #543697 -
Flags: feedback?(dao)
Attachment #543546 -
Flags: review?(dao)
Attachment #543546 -
Flags: feedback?(fryn)
Assignee | ||
Comment 21•13 years ago
|
||
One way I have thought would be possible would be to just change the opacity of the |.toolbarbutton-icon| to .5 and then adjust the background-colors, box-shadows, and border-colors to fit, but that seems like it will be pretty hard to get correct.
Comment 22•13 years ago
|
||
We could stop reducing the icon's opacity on top of reducing the button's opacity. I.e. just use opacity:0.4 for the button and leave the icon alone.
Updated•13 years ago
|
Attachment #543697 -
Flags: feedback?(dao)
Comment 23•13 years ago
|
||
Comment on attachment 543697 [details] [diff] [review]
Patch for bug 571454 - version 3 (with minor regression)
> #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button {
>+ margin: -5px 0;
>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button > .toolbarbutton-icon {
> margin-top: -2px;
> margin-bottom: -2px;
Why are you setting negative margins on the icon?
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to comment #23)
> Comment on attachment 543697 [details] [diff] [review] [review]
> Patch for bug 571454 - version 3 (with minor regression)
>
> > #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button {
> >+ margin: -5px 0;
>
> >+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button > .toolbarbutton-icon {
> > margin-top: -2px;
> > margin-bottom: -2px;
>
> Why are you setting negative margins on the icon?
Previously, the #back-button had the -2px margins. The styling was moved to the icon, so that's where the -2px comes from.
The #back-button itself has the larger negative margins so that the clickable area will extend slightly above and below the icon.
Comment 25•13 years ago
|
||
I still don't see why the -2px margins need to move over to the icon when the button gets larger negative margins.
Assignee | ||
Comment 26•13 years ago
|
||
(In reply to comment #25)
> I still don't see why the -2px margins need to move over to the icon when
> the button gets larger negative margins.
Good catch. I just removed the negative margins on the icon and there is no change in appearance. I should have tried it before.
Assignee | ||
Comment 27•13 years ago
|
||
I have removed the negative margins on the icon.
I tried changing the opacity of the button to .4 and removing the opacity changes of the icon. This change gave a washed out look to the back button that is different than the disabled look of the other toolbar buttons.
One approach for changing the opacity of the list-style-image, is simply to include a "disabled" state of the large back arrow in Toolbar.png. I have made a crude attempt at demonstrating it in this patch. While we don't include any explicit disabled states of images in the Toolbar.png, it would be a simple solution for this problem. What do you think?
Attachment #544326 -
Flags: feedback?(dao)
Comment 28•13 years ago
|
||
> I tried changing the opacity of the button to .4 and removing the opacity
> changes of the icon. This change gave a washed out look to the back button
> that is different than the disabled look of the other toolbar buttons.
I meant doing this for all buttons, not just the back button.
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #544333 -
Flags: review?(dao)
Assignee | ||
Comment 30•13 years ago
|
||
Comment on attachment 544333 [details] [diff] [review]
Patch for bug 571454 - version 3.2
I have made all toolbar buttons have an opacity of .4 when disabled.
Assignee | ||
Updated•13 years ago
|
Attachment #544326 -
Attachment is obsolete: true
Attachment #544326 -
Flags: feedback?(dao)
Assignee | ||
Updated•13 years ago
|
Attachment #543697 -
Attachment is obsolete: true
Assignee | ||
Comment 31•13 years ago
|
||
I would like to double check to see if there are any qualms about the change to the opacity on the disabled toolbar buttons. I have included a screenshot of disabled buttons along with the Paste button enabled.
Attachment #544360 -
Flags: ui-review?(shorlander)
Comment 32•13 years ago
|
||
Comment on attachment 544333 [details] [diff] [review]
Patch for bug 571454 - version 3.2
> #nav-bar .toolbarbutton-1[disabled="true"] {
>- opacity: .8;
>+ opacity: .4;
> }
.5 might be sufficient as well.
> #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled="true"]):not(:active):hover,
> #nav-bar .toolbarbutton-1:not([open="true"]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled="true"]),
>-#nav-bar .toolbarbutton-1:not([type="menu-button"]):not([disabled="true"]):not([checked="true"]):not([open="true"]):not(:active):hover {
>+#nav-bar .toolbarbutton-1:not([type="menu-button"]):not([disabled="true"]):not([checked="true"]):not([open="true"]):not(:active):hover,
>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button:not([disabled="true"]):not(:active):hover > .toolbarbutton-icon {
This new selector also needs :not([open]).
>-#nav-bar #back-button {
>+#navigator-toolbox[iconsize="small"][mode="icons"] > #nav-bar #back-button {
> -moz-margin-end: 0 !important;
> }
This change looks unnecessary. You're affecting mode=text and mode=full here.
> #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button {
>- border-radius: 10000px;
>- padding: 0;
>- width: 30px;
>- height: 30px;
>+ margin: -5px 0;
>+ padding-top: 0;
>+ padding-bottom: 0;
>+ -moz-padding-start: 5px;
>+ -moz-padding-end: 0;
> position: relative;
> z-index: 1;
>- margin-top: -2px;
>- margin-bottom: -2px;
>+ border-radius: 0 10000px 10000px 0;
>+ background: transparent;
>+ border: none;
>+ box-shadow: none;
>+ text-shadow: none;
>+}
text-shadow:none isn't needed, since there's no text.
Attachment #544333 -
Flags: review?(dao) → review+
Assignee | ||
Comment 33•13 years ago
|
||
I have made the requested changes. Pending shorlander's approval, can you land this for me Dão?
Attachment #544333 -
Attachment is obsolete: true
Assignee | ||
Comment 34•13 years ago
|
||
I switched to an opacity of .5 for the disabled buttons, and have included a screenshot of the buttons at .4 opacity for comparison.
Stephen: Can you please let me know if you are OK with this change?
Attachment #544360 -
Attachment is obsolete: true
Attachment #544883 -
Flags: ui-review?(shorlander)
Attachment #544360 -
Flags: ui-review?(shorlander)
Comment 35•13 years ago
|
||
Comment on attachment 544883 [details]
Screenshot of different opacities for disabled buttons
.4 Opacity works well. It is actually an improvement because it makes it more obvious :)
Attachment #544883 -
Flags: ui-review?(shorlander) → ui-review+
Assignee | ||
Comment 36•13 years ago
|
||
Attachment #544882 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 37•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Comment 38•13 years ago
|
||
This patch appears to fail when small toolbar icons are used. Is this intentional?
Assignee | ||
Comment 39•13 years ago
|
||
(In reply to comment #38)
> This patch appears to fail when small toolbar icons are used. Is this
> intentional?
This was not intentional per se, but it was known. I have filed bug 670565 to track this issue with small icons.
Comment 40•13 years ago
|
||
I'm not sure if this is the appropriate place to ask this question, but I'm using a userchrome.css file that contains the following code:
#nav-bar {
padding: 6px !important;
}
This patch fails unless the above code segment is commented out.
What should I do to preserve the effects of the above code AND this patch at the same time?
Assignee | ||
Comment 41•13 years ago
|
||
(In reply to comment #40)
> I'm not sure if this is the appropriate place to ask this question, but I'm
> using a userchrome.css file that contains the following code:
I don't think this is the appropriate place for this discussion. Unfortunately, I'm not sure where the appropriate place would be either.
> #nav-bar {
> padding: 6px !important;
> }
>
> This patch fails unless the above code segment is commented out.
>
> What should I do to preserve the effects of the above code AND this patch at
> the same time?
I'm not sure what else is in the userchrome.css file, but you could try adding this to the line below your padding rule:
#nav-bar {
padding: 6px !important;
-moz-padding-start: 0 !important;
}
You would then have to add the following rule to your userchrome.css file:
#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button {
-moz-padding-start: 11px;
}
Comment 42•13 years ago
|
||
Thanks; appreciate the help.
Comment 43•13 years ago
|
||
Surely a better idea would have been to include some spacers on the far ends of the nav bar that copy the onClick events from the toolbar buttons before or after them (depending on whether they are a start or end spacer). This way it is all automatic.
I feel the current method is somewhat "hackish" because it is skinning the .toolbarbutton-icon, which means all themes will be incompatible, and add-ons like Stratiform have a very hard time keeping up with a change like this, which doesn't even work for any buttons other than the back button.
I'd give some code or a patch but I'm horrible with JavaScript.
Comment 44•13 years ago
|
||
Sorry if this is a stupid question, but is the fix here simply moving the button away from the edge of the screen? If so, I think I can call this verified on Firefox 8.0b1.
Whiteboard: [qa+][testday-20110930]
Comment 45•13 years ago
|
||
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #44)
> Sorry if this is a stupid question, but is the fix here simply moving the
> button away from the edge of the screen? If so, I think I can call this
> verified on Firefox 8.0b1.
No. Here's how you verify it:
1. Maximize the window.
2. Make the back button enabled if it isn't already (by following a link, etc.).
3. Click part of the left edge of the screen that is to the left of the back button.
If the back button activates, this bug is fixed.
Comment 46•13 years ago
|
||
So I can confirm this is the behaviour on Windows 7 but not any other platforms. Should this not be a unified experience across all platforms?
Assignee | ||
Comment 47•13 years ago
|
||
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #46)
> So I can confirm this is the behaviour on Windows 7 but not any other
> platforms. Should this not be a unified experience across all platforms?
This bug was only meant to be fixed for Windows XP and higher.
Other platforms don't have these types of affordances (such as Mac OSX where dragging part of the toolbar will drag the window).
OS: Windows 7 → Windows XP
Comment 48•13 years ago
|
||
(In reply to Jared Wein [:jwein] from comment #47)
> Other platforms don't have these types of affordances (such as Mac OSX where
> dragging part of the toolbar will drag the window).
How unfortunate...
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [qa+][testday-20110930] → [qa!][testday-20110930]
Comment 49•13 years ago
|
||
Great that this has been fixed in Firefox 8 beta!
Any chance though that this can be extended to work for *any* toolbar item that sits adjacent to *either* the left or right screen edges? (the Google Chrome "wrench" menu, for example, can be activated from the right screen edge)
Comment 50•13 years ago
|
||
(In reply to broccauley from comment #49)
> Any chance though that this can be extended to work for *any* toolbar item
> that sits adjacent to *either* the left or right screen edges?
Yes, that makes total sense. Can you file a new bug for that?
Assignee | ||
Comment 51•11 years ago
|
||
Bug 941046 added a test for this.
You need to log in
before you can comment on or make changes to this bug.
Description
•