Last Comment Bug 684450 - Remove stop/go/reload button affordance and streamline other location bar icons
: Remove stop/go/reload button affordance and streamline other location bar icons
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Firefox 9
Assigned To: Stephen Horlander [:shorlander]
:
Mentors:
: 593293 (view as bug list)
Depends on: 689079 690910 1292877 691100
Blocks: 593350 593392 593684 686428 691701
  Show dependency treegraph
 
Reported: 2011-09-02 23:17 PDT by Stephen Horlander [:shorlander]
Modified: 2016-08-11 11:33 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot (Pinstripe) - 01 (19.38 KB, image/png)
2011-09-02 23:17 PDT, Stephen Horlander [:shorlander]
no flags Details
WIP Patch - 01 (16.88 KB, patch)
2011-09-02 23:20 PDT, Stephen Horlander [:shorlander]
no flags Details | Diff | Splinter Review
Patch - 02 (45.96 KB, patch)
2011-09-06 20:21 PDT, Stephen Horlander [:shorlander]
no flags Details | Diff | Splinter Review
Screenshot - 02 (54.20 KB, image/png)
2011-09-06 21:31 PDT, Stephen Horlander [:shorlander]
no flags Details
Patch - 03 (38.12 KB, patch)
2011-09-20 11:43 PDT, Stephen Horlander [:shorlander]
no flags Details | Diff | Splinter Review
Patch - 04 (46.84 KB, patch)
2011-09-20 12:57 PDT, Stephen Horlander [:shorlander]
dao+bmo: review-
Details | Diff | Splinter Review
Patch - 05 (46.91 KB, patch)
2011-09-23 14:15 PDT, Stephen Horlander [:shorlander]
dao+bmo: review+
Details | Diff | Splinter Review

Description Stephen Horlander [:shorlander] 2011-09-02 23:17:38 PDT
Created attachment 558040 [details]
Screenshot (Pinstripe) - 01

Bug 673481 removed the separator from the at rest stop/go/reload button. This bug is for cleaning up that area by:

- Updating icons with a consistent style
- Adding a subtle history dropdown icon
- Adjusting for consistent spacing between icons
- Removing button affordance on hover and pressed states
- De-emphasize the history dropdown
- Change starred icon to have filled appearance instead of pressed appearance
Comment 1 Stephen Horlander [:shorlander] 2011-09-02 23:20:19 PDT
Created attachment 558041 [details] [diff] [review]
WIP Patch - 01

WIP patch. Only has Pinstripe.
Comment 2 Stephen Horlander [:shorlander] 2011-09-06 20:21:05 PDT
Created attachment 558718 [details] [diff] [review]
Patch - 02
Comment 3 Stephen Horlander [:shorlander] 2011-09-06 21:31:54 PDT
Created attachment 558725 [details]
Screenshot - 02
Comment 4 Stephen Horlander [:shorlander] 2011-09-07 12:49:20 PDT
Landed on UX for testing: https://hg.mozilla.org/projects/ux/rev/867b27eb486c
Comment 5 Stephen Horlander [:shorlander] 2011-09-14 10:43:18 PDT
Comment on attachment 558718 [details] [diff] [review]
Patch - 02

I was having some trouble getting the icons to have full height in Winstripe so I removed the padding on the #urlbar and negative margins on the #identity-box and the go/reload/stop toolbarbutton. It retains the same height and didn't seem to have any drawbacks.
Comment 6 :Margaret Leibovic 2011-09-14 13:48:02 PDT
(In reply to Stephen Horlander from comment #5) 
> I was having some trouble getting the icons to have full height in Winstripe
> so I removed the padding on the #urlbar and negative margins on the
> #identity-box and the go/reload/stop toolbarbutton. It retains the same
> height and didn't seem to have any drawbacks.

I remember trying to remove the padding and negative margins in bug 634065, and I think we need to also remove the padding from the search box if we do that. See 634065 comment 56.
Comment 7 Dão Gottwald [:dao] 2011-09-15 05:37:40 PDT
Comment on attachment 558718 [details] [diff] [review]
Patch - 02

what Margaret said
Comment 8 Stephen Horlander [:shorlander] 2011-09-20 11:43:06 PDT
Created attachment 561258 [details] [diff] [review]
Patch - 03
Comment 9 Stephen Horlander [:shorlander] 2011-09-20 12:57:47 PDT
Created attachment 561271 [details] [diff] [review]
Patch - 04
Comment 10 Dão Gottwald [:dao] 2011-09-21 01:53:28 PDT
Comment on attachment 561271 [details] [diff] [review]
Patch - 04

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

> .urlbar-icon {
>   cursor: pointer;
>+  padding: 0 2px;
> }

>-#go-button {
>+#go-button,
>+#urlbar > toolbarbutton {
>+  -moz-appearance: none;
>+  border: none;
>+  padding: 0 3px;
>+}
>+
>+#go-button,
>+#urlbar-go-button {
>   padding: 3px 2px 2px 2px;
>   list-style-image: url("chrome://browser/skin/Go-arrow.png");
> }

The Go arrow consumes a smaller horizontal space than the other icons, it needs more padding.

The lack of hover feedback feels strange, I think we should add cursor:pointer like we do for .urlbar-icon.
Comment 11 Dão Gottwald [:dao] 2011-09-21 01:57:40 PDT
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 561271 [details] [diff] [review]
> Patch - 04
> 
> >--- a/browser/themes/gnomestripe/browser/browser.css
> >+++ b/browser/themes/gnomestripe/browser/browser.css
> 
> > .urlbar-icon {
> >   cursor: pointer;
> >+  padding: 0 2px;
> > }
> 
> >-#go-button {
> >+#go-button,
> >+#urlbar > toolbarbutton {
> >+  -moz-appearance: none;
> >+  border: none;
> >+  padding: 0 3px;
> >+}
> >+
> >+#go-button,
> >+#urlbar-go-button {
> >   padding: 3px 2px 2px 2px;
> >   list-style-image: url("chrome://browser/skin/Go-arrow.png");
> > }
> 
> The Go arrow consumes a smaller horizontal space than the other icons, it
> needs more padding.

It's probably simpler if you drop the padding and set width:22px on all buttons.
Comment 12 bogas04 2011-09-22 01:25:03 PDT
Addon Installation notification is messed up 
https://bug674744.bugzilla.mozilla.org/attachment.cgi?id=561643
Comment 13 Stephen Horlander [:shorlander] 2011-09-22 07:07:50 PDT
(In reply to bogas04 from comment #12)
> Addon Installation notification is messed up 
> https://bug674744.bugzilla.mozilla.org/attachment.cgi?id=561643

Thanks! Fixed in latest patch.
Comment 14 Stephen Horlander [:shorlander] 2011-09-23 14:15:44 PDT
Created attachment 562162 [details] [diff] [review]
Patch - 05

(In reply to Dão Gottwald [:dao] from comment #10)
> The Go arrow consumes a smaller horizontal space than the other icons, it
> needs more padding.
> 
> The lack of hover feedback feels strange, I think we should add
> cursor:pointer like we do for .urlbar-icon.

The lack of hover is strange. Although switching to the pointer also seems kind of strange/inconsistent.


(In reply to Dão Gottwald [:dao] from comment #11)
> It's probably simpler if you drop the padding and set width:22px on all
> buttons.

Done.
Comment 15 Dão Gottwald [:dao] 2011-09-24 02:39:19 PDT
Comment on attachment 562162 [details] [diff] [review]
Patch - 05

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

> #urlbar-icons {
>   -moz-box-align: center;
>-  -moz-padding-end: 2px;
> }
> 
> .urlbar-icon {
>   cursor: pointer;
>+  padding: 0 3px;
> }

>-#go-button {
>-  padding: 3px 2px 2px 2px;
>+#go-button,
>+#urlbar > toolbarbutton {
>+  -moz-appearance: none;
>+  padding: 0;
>+  border: none;
>+  cursor: pointer;
>+}

#go-button is an <image> and an .urlbar-icon, which means -moz-appearance:none, border:none and cursor:pointer are redundant for it. The only affect here is that you're removing the padding, which I think you don't actually want. I think you either want to let it keep the original .urlbar-icon padding or add padding-top/bottom in order to enlarge the hit region.
Comment 16 Dão Gottwald [:dao] 2011-09-24 04:03:45 PDT
Comment on attachment 562162 [details] [diff] [review]
Patch - 05

I'll just quickly address my previous comment myself.
Comment 19 Siddhartha Dugar [:sdrocking] 2011-09-24 09:01:59 PDT
Can close Bug 593684
Comment 20 Stephen Horlander [:shorlander] 2011-09-24 19:48:58 PDT
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 562162 [details] [diff] [review] [diff] [details] [review]
> Patch - 05
> 
> I'll just quickly address my previous comment myself.

Thank you!
Comment 21 Dão Gottwald [:dao] 2011-09-26 07:34:15 PDT
For the addon-compat keyword to be useful, you need to explain what kind of extensions are effected here...
Comment 22 Stephen Horlander [:shorlander] 2011-09-26 07:58:47 PDT
(In reply to Dão Gottwald [:dao] from comment #21)
> For the addon-compat keyword to be useful, you need to explain what kind of
> extensions are effected here...

Sorry, I was still investigating :)

It looks like extensions that add items to the #urlbar-icon hbox are affected by this. So far I have only found Omnibar and Read It Later.

Both of them are adding their own padding, which should be fine, but for some reason both of them are adding the .urlbar-icons class to the #urlbar-icon hbox.
Comment 23 u420019 2011-09-27 00:48:15 PDT
I have noticed that the dropdown arrow has no tooltipp text.
Comment 24 Siddhartha Dugar [:sdrocking] 2011-09-27 04:22:45 PDT
(In reply to Jukens from comment #23)
> I have noticed that the dropdown arrow has no tooltipp text.

It doesn't have the tool tip on Firefox 6 also. So nothing related to this bug.
Comment 25 Dão Gottwald [:dao] 2011-10-05 00:36:27 PDT
*** Bug 593293 has been marked as a duplicate of this bug. ***
Comment 26 u420019 2011-11-05 12:04:46 PDT
(In reply to sdrocking from comment #24)
> (In reply to Jukens from comment #23)
> > I have noticed that the dropdown arrow has no tooltipp text.
> 
> It doesn't have the tool tip on Firefox 6 also. So nothing related to this
> bug.

I know that it doesn't have tool tip on Firefox 6 also. Is there a bug related to this topic? When not, this should be implemented IMHO. All the other buttons have tooltips. Only this button has no tool tip. Just to say.
Comment 27 Vova Olar 2011-12-21 01:21:10 PST
If star icon not shown (new tab) and present other extension icons (like Read It Later) - hovering them produces background light at place where star icon should be.
Comment 28 Vova Olar 2011-12-21 01:22:43 PST
Update. Even when star icon is shown hovering any other icon in location bar will produce star icon highlight.
Is there any bug to carry this issue or I should create?
Comment 29 Dão Gottwald [:dao] 2011-12-21 01:33:35 PST
(In reply to Vova Olar from comment #28)
> Update. Even when star icon is shown hovering any other icon in location bar
> will produce star icon highlight.
> Is there any bug to carry this issue or I should create?

Bug 691100. Please report this to the author of Read It Later.
Comment 30 Vova Olar 2011-12-21 10:37:50 PST
I say about blue hover background. And not only with Read It Later. It can be reproduced with any other extension that put it icon to location bar, for example XClear. When I hover any icon in location bar - blue background appears at place where bookmark star icon should be (on blank page). Can anyone confirm?

Note You need to log in before you can comment on or make changes to this bug.