Closed Bug 995300 Opened 6 years ago Closed 6 years ago

When the location bar grows vertically due to an increased font size, the forward button should grow as well to match the location bar

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Screenshot: http://i.imgur.com/XGuOF8i.png

This probably affects all platforms but doesn't matter on OS X where users presumably can't change the system font size.
Assignee: nobody → dao
Depends on: 997131
Duplicate of this bug: 1003776
Attached patch patchSplinter Review
Quick and dirty. This stretches the icon, which isn't ideal but better than the current situation where the borders don't connect.

Fixing this without stretching the icon would involve moving the button styling from the .toolbarbutton-icon to #forward-button.
Attachment #8416461 - Flags: review?(mdeboer)
(In reply to Dão Gottwald [:dao] from comment #2)
> Fixing this without stretching the icon would involve moving the button
> styling from the .toolbarbutton-icon to #forward-button.

What happens when you give the #forward-button > .toolbarbutton-icon a fixed height?
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > Fixing this without stretching the icon would involve moving the button
> > styling from the .toolbarbutton-icon to #forward-button.
> 
> What happens when you give the #forward-button > .toolbarbutton-icon a fixed
> height?

It has an implied fixed height right now, causing this bug.
Status: NEW → ASSIGNED
Comment on attachment 8416461 [details] [diff] [review]
patch

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

We'd need to move the styling to the #forward-button indeed.

Even though the stretched icon is visually jarring to me, I still feel it's an improvement, like you said.

Can you file a follow-up for that?

::: browser/themes/linux/browser.css
@@ +739,5 @@
>    transform: scaleX(-1);
>  }
>  
>  #forward-button {
> +  -moz-box-align: stretch; /* let the button shape grow vertically with the location bar */

nit: can you put the comment above this line?

::: browser/themes/windows/browser.css
@@ +876,5 @@
>  
>  /* unified back/forward button */
>  
>  #forward-button {
> +  -moz-box-align: stretch; /* let the button shape grow vertically with the location bar */

nit: same here.
Attachment #8416461 - Flags: review?(mdeboer) → review+
> >  #forward-button {
> > +  -moz-box-align: stretch; /* let the button shape grow vertically with the location bar */
> 
> nit: can you put the comment above this line?

Oops, I imported the patch and meant to make the above change, but then I a got distracted and ended up pushing it unmodified:

https://hg.mozilla.org/integration/fx-team/rev/5e1f22e16fa4

I did originally put that comment on the same line on purpose, to make it clearer that it's only for that property rather than the entire rule...
*slap*

I'll let it slide, only this one time... :-P
Depends on: 1005132
https://hg.mozilla.org/mozilla-central/rev/5e1f22e16fa4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment on attachment 8416461 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not sure...
User impact if declined: the forward button may have a different height than the location bar (http://i.imgur.com/XGuOF8i.png)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8416461 - Flags: approval-mozilla-aurora?
Attachment #8416461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite-
Flags: in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.