893661 - Back button and toolbar slightly smaller than design spec

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Comment 1

[kazukigot1]

Attachment: Screenshot of the problem

See screenshot.

Comment 2

[Justin Dolske [:Dolske]]

What are these screenshots comparing?

Are you comparing UX to Nightly or Firefox 22? (Possibly hidpi settings?)

Comment 3

[Dão Gottwald [:dao]]

The screenshots are comparing UX builds pre and post bug 875488.

Comment 4

[kazukigot1]

(In reply to Justin Dolske [:Dolske] from comment #2)
> What are these screenshots comparing?
> 
> Are you comparing UX to Nightly or Firefox 22? (Possibly hidpi settings?)

I am compared the UX to live mockup of shorlander. Link to a live mockup is this.

http://people.mozilla.com/ ~ shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html

Comment 5

[kazukigot1]

Path was incorrect

http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html

Comment 6

[kazukigot1]

This looks smaller in OSX on.

Comment 7

[Brandon Cheng]

The fix on Windows is to change the padding on #back-button from 5px to 6px on line 653 of browser.css. I've double checked that this results in a new size that matches the spec images. My only concern is that the outline/border quality may be affected, but it doesn't appear to be visually.

https://hg.mozilla.org/projects/ux/file/672ff858678a/browser/themes/windows/browser.css#l653

I haven't looked at it specifically, but I imagine OS X's fixes to be similar. Although OS X also needs 2 additional pixels on the urlbar.

Assign me please. :)

Comment 8

[Brandon Cheng]

The outline on the OS X back button is rendered as an image. I'm not sure where the original layered version (psd?) of this image exists. Even with it, I'm don't think I have the ability to recreate it with a 16px radius instead of 15px.

https://hg.mozilla.org/projects/ux/file/672ff858678a/browser/themes/osx/keyhole-circle.png

Do we want to recreate the PNG, or perhaps re-implement the outline in CSS? (The latter of which I could do.)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Stephen created that image, and he should be able to increase the radius of the circle to 16px. Flagging Stephen for needinfo...

Comment 10

[Michael Maslaney [:mmaslaney] ([e-mail])]

Jared, the current OSX back button is 30px and 60px for retina display. Would you like me to increase it to 32px/64px?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Yes, that would be great.

Comment 12

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: back.zip

The spec states that the back button should be a measure of 30x30 pixels. After looking through the layered psd files, the radius of the button is indeed 30x30 pixels, but with the stroke and dropshadow included, the final measurement is actually 32x33 (standard) and 64x65 (retina).

Let me know if the attached assets work.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Brandon, can you pick this up now?

Comment 14

[Brandon Cheng]

Sorry, I haven't forgotten about this. The attached assets don't seem to include a depressed and inactive state like the original. Also, I don't think the arrow should be included in the keyhole-circle png, as that's provided by toolbar.png

https://hg.mozilla.org/projects/ux/raw-file/672ff858678a/browser/themes/osx/keyhole-circle.png

Comment 15

[:Gijs (he/him)]

(In reply to Brandon Cheng from comment #14)
> Sorry, I haven't forgotten about this. The attached assets don't seem to
> include a depressed and inactive state like the original. Also, I don't
> think the arrow should be included in the keyhole-circle png, as that's
> provided by toolbar.png
> 
> https://hg.mozilla.org/projects/ux/raw-file/672ff858678a/browser/themes/osx/
> keyhole-circle.png

Michael, can you fix the assets here?

Comment 16

[Stephen Horlander [:shorlander]]

Attachment: Keyhole — OS X

Comment 17

[Stephen Horlander [:shorlander]]

Attachment: Keyhole — OS X @2x

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Brandon, any update on this?

Comment 19

[Brandon Cheng]

(In reply to Jared Wein [:jaws] from comment #18)
> Brandon, any update on this?

Yeah. I'm trying to test this, but Nightly on OS X has been displaying only a white frame for me for the last few days. Is this unique to my build or is there currently a bug about this?

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

It sounds unique to your build. I didn't get any confirmation of other people seeing it on osx and I've been unable to reproduce it on my Mac Mini.

Comment 21

[Brandon Cheng]

I'll have access to another OS X machine today. I'll finish the patch and generate it on that. Thanks for finding that bug Gijs.

Comment 22

[Brandon Cheng]

I've got the back button resized to the correct size on normal and HiDPI (tested) for both Windows and OS X. Still working on getting the urlbar and forward button up to 24px on OS X.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Hey Brandon, any update on this? This is the last week that we can fix this. If you don't think you'll have time to finish it this week, could you upload your patch and I can help bring it to completion? Thanks!

Comment 24

[Brandon Cheng]

Attachment: Patch v1.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> Hey Brandon, any update on this? This is the last week that we can fix this.
> If you don't think you'll have time to finish it this week, could you upload
> your patch and I can help bring it to completion? Thanks!

Here's the patch. I uploaded it as-is since you need this soon. Two things about it at the moment.

1) Since this requires the SVG clip curve on the urlbar to be 2px taller, I ended up redoing the SVG clipping with an arc path instead of the arbitrary cubic. (It's a lot cleaner and logical now,) It's been tested on in a custom jsfiddle but not actually on Firefox. My guess is that Windows and OS X will need tweaking as to where the clipping starts (x-axis). http://jsfiddle.net/gluxon/A7TAG/7/

2) To bring the urlbar up 2px on OS X, I added padding to the top and bottom. It looks like the spec (font size is good too), but I'm not sure if this is how we want to do this.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8388158 [details] [diff] [review]
Patch v1.patch

> #urlbar,
> .searchbar-textbox {
>   font: icon;
>   -moz-appearance: none;
>   box-shadow: 0 1px rgba(255, 255, 255, 0.2), inset 0 1px hsla(0,0%,0%,.05);
>   margin: 0 4px;
>-  padding: 0;
>+  padding-top: 1px;
>+  padding-bottom: 1px;

Seems like this change adds left and right paddings of 1px. I suspect you didn't want this...

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8388158 [details] [diff] [review]
Patch v1.patch

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

I'm uploading a new patch which tweaks the origin on Windows and fixes the issue that Dao pointed out.

::: browser/base/content/browser.xul
@@ +1168,5 @@
>      <svg:clipPath id="windows-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox">
>        <svg:path d="M 0,0 C 0.16,0.11 0.28,0.29 0.28,0.5 0.28,0.71 0.16,0.89 0,1 L 1,1 1,0 0,0 z"/>
>      </svg:clipPath>
>      <svg:clipPath id="windows-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse">
> +      <svg:path d="M 0,0 a 16 16 0 0 1 0 24 l 10000,0 l 0,-24 l -10000,0 z"/>

yeah this is a much simpler path. thanks!

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Because of bug 981714, I've uploaded my diff to pastebin. Shorlander, does this look OK to you on OSX?

http://pastebin.mozilla.org/4542227

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 29

[Brandon Cheng]

Notes for myself that need to be done when I get a chance to generate another patch.

#1) Update the Windows clip-path. It should be -1px to the left and bumped up to 26px height due to #urlbar-container having 1px padding all around.

#2) OS X has a urlbar shadow on focus. The clip-path height needs to be updated so this isn't cut off.

#3) Forward button needs to be wider and taller on OS X. Clippings also need to be updated for this.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Brandon, I change #1 in the attached patch. I haven't gotten to #2 or #3 yet. Are you planning on doing those? If so, that'd be great!

Comment 31

[Brandon Cheng]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> Brandon, I change #1 in the attached patch. I haven't gotten to #2 or #3
> yet. Are you planning on doing those? If so, that'd be great!

I was. #2 is taken care of and tested. For #3, I'm realizing that I can't find any specs on how wide the forward button should be. Do you know have information on how wide it should be? Thanks.

Comment 32

[Stephen Horlander [:shorlander]]

(In reply to Brandon Cheng from comment #31)
> I was. #2 is taken care of and tested. For #3, I'm realizing that I can't
> find any specs on how wide the forward button should be. Do you know have
> information on how wide it should be? Thanks.

This is an older mockup but it is 24px wide from back-button border to the end of the forward-border: file:///Users/shorlander/Desktop/Stuff/People-Organizing/people-organizing/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28ConditionalForward%29.jpg

zoomed: http://people.mozilla.org/~shorlander/bugs/Bug-893661-forward-button-width.png

I think it's 20px now.

Comment 33

[Stephen Horlander [:shorlander]]

Well that's not what I meant to post :)

http://people.mozilla.org/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28ConditionalForward%29.jpg

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Cool, Brandon, I'll let you make those other changes.

Comment 35

[Brandon Cheng]

Attachment: Patch v2

This should be it unless there's issues. Differences from Patch v1.1 include the forward button fixes and an updated SVG patch to include the urlbar shadow when focused.

I used the git-patch-to-hg-patch tool to generate this since I'm a lot more familiar with Git. Let me know if this is an issue and I can regenerate it in Mercurial.

Comment 36

[Brandon Cheng]

And thanks again to Stephen and Jared for the forward button specs and Windows alignment fix respectively.

Comment 37

[:Gijs (he/him)]

I tried this on OS X and I'm seeing the left (start) border of the fwd button bleed through the back button... :-(

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8391974 [details] [diff] [review]
Patch v2

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

The hover state of the forward button isn't getting clipped by the back button. See http://screencast.com/t/9id98IEhv3QY for a screenshot of what I see on Windows 8.

::: browser/base/content/browser.xul
@@ +1176,3 @@
>      </svg:clipPath>
>      <svg:clipPath id="osx-urlbar-back-button-clip-path" clipPathUnits="userSpaceOnUse">
> +      <svg:path d="M -12,-5 a 16 16 0 0 1 0,34 l 10000,0 l 0,-34 l -10000,0 z"/>

Why does the path need to start at such an offset and end at such a lower point than the keyhole-forward path?

Comment 39

[Brandon Cheng]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #38)
> The hover state of the forward button isn't getting clipped by the back
> button. See http://screencast.com/t/9id98IEhv3QY for a screenshot of what I
> see on Windows 8.

That's weird. I have tested this and I'm not seeing that at all. http://i.imgur.com/9r8AAeo.png
I'm not seeing Gijs issue either if I'm right about what he's referring to.

I think the git patch to hg patch might not have worked so well with the forward button svg clippings. I'll just regenerate the patch with Mercurial.

> Why does the path need to start at such an offset and end at such a lower
> point than the keyhole-forward path?

The SVG clipping is applied to #urlbar-wrapper. #urlbar-wrapper has a large blue shadow when its focused. It extends 5px above and below, so I had to extend the clipping area 10px in height (from 24px to 34px) and move it left and up.

An ideal solution would be to move the clipping to the #urlbar. This would allow it to have the same clipping as the forward button. I haven't tested if this is possible. I feel like there might be conflicts with the site identity elements (but again I'm really not sure). I'll try it since I don't like that weird path start either.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Brandon Cheng from comment #39)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #38)
> > The hover state of the forward button isn't getting clipped by the back
> > button. See http://screencast.com/t/9id98IEhv3QY for a screenshot of what I
> > see on Windows 8.
> 
> That's weird. I have tested this and I'm not seeing that at all.
> http://i.imgur.com/9r8AAeo.png
> I'm not seeing Gijs issue either if I'm right about what he's referring to.
> 
> I think the git patch to hg patch might not have worked so well with the
> forward button svg clippings. I'll just regenerate the patch with Mercurial.

Yeah, that's probably why.

> > Why does the path need to start at such an offset and end at such a lower
> > point than the keyhole-forward path?
> 
> The SVG clipping is applied to #urlbar-wrapper. #urlbar-wrapper has a large
> blue shadow when its focused. It extends 5px above and below, so I had to
> extend the clipping area 10px in height (from 24px to 34px) and move it left
> and up.
> 
> An ideal solution would be to move the clipping to the #urlbar. This would
> allow it to have the same clipping as the forward button. I haven't tested
> if this is possible. I feel like there might be conflicts with the site
> identity elements (but again I'm really not sure). I'll try it since I don't
> like that weird path start either.

You should probably leave it unchanged so that we can expedite this patch, since timing is very short now.

Comment 41

[:Gijs (he/him)]

Attachment: (Australis) Back button and toolbar slightly smaller than design spec

I rebased this, and it does WFM on Windows. Retesting OS X with this in a bit.

Comment 42

[:Gijs (he/him)]

Attachment: (Australis) Back button and toolbar slightly smaller than design spec

Of course, I managed to mess it up and neglect to migrate the OS X clip path change.

Comment 43

[:Gijs (he/him)]

Attachment: (Australis) Back button and toolbar slightly smaller than design spec

And half the Windows changes. Sigh. Time for sleep, methinks.

Comment 44

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #43)
> Created attachment 8392574 [details] [diff] [review]
> (Australis) Back button and toolbar slightly smaller than design spec
> 
> And half the Windows changes. Sigh. Time for sleep, methinks.

Thanks a lot for doing that Gijs!

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8392574 [details] [diff] [review]
(Australis) Back button and toolbar slightly smaller than design spec

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

r=me for the browser/base && /browser/themes/windows changes. I'll need Gijs or someone else on OSX to test the patch there.

::: browser/themes/windows/browser.css
@@ +823,3 @@
>    border-left-style: none !important;
>    border-radius: 0 !important;
> +  padding-left: 9px !important;

why is the padding-left different than the margin-left?

Comment 46

[Brandon Cheng]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #45)
> Comment on attachment 8392574 [details] [diff] [review]
> ::: browser/themes/windows/browser.css
> @@ +823,3 @@
> >    border-left-style: none !important;
> >    border-radius: 0 !important;
> > +  padding-left: 9px !important;
> 
> why is the padding-left different than the margin-left?

It's to bring the forward button up to the 24px measured in Stephen's mockups. The other option to do this is to increase the width of the button itself. Although I'm not sure if that will properly center the icon since I haven't tried this either.

Comment 47

[Brandon Cheng]

To elaborate a bit more, margin-left was reduced by one since the forward button didn't really need to be that far back. It also allows the clip to start at a nice 0,0 as a bonus. Before the clip was M 1,0 so we were just drawing a row of pixels only to have it cut off. This was a bit harder to tell with the clip-path before this patch.

padding-left at 9px gives a 3px difference between margin-left. This pushes the forward icon 3px out and brings the forward button up from 21px to 24px.

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

Thanks.

Comment 49

[:Gijs (he/him)]

Comment on attachment 8392574 [details] [diff] [review]
(Australis) Back button and toolbar slightly smaller than design spec

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

r=me. As a followup, we should make the back button and the url field be joined together. On 28, there is no 'break' between the dark grey of the url field and the back button's borders, but on current nightly and after this patch, there's an outer light shadow/accent which overlaps it. I'm not sure if that's the image's antialiasing at work or the clip path, but either way. Maybe it's even intentional? Question for Stephen, I guess. Anyway, this patch doesn't make it worse/bad, so we should ship this.

Not sure about the risk for beta though. Does this alter the navbar's height, Brandon? If so, I'd at least want to bake it on nightly for a bit before uplifting to beta. Less fussed about aurora seeing as that's only just merged up.

Comment 50

[:Gijs (he/him)]

Oh, one more nit, though: we should remove the non-lion keyhole (that wasn't updated here) and map this (lion) one to the old path. Otherwise the clip path and the image probably won't match.

Comment 51

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #49)
> Comment on attachment 8392574 [details] [diff] [review]
> (Australis) Back button and toolbar slightly smaller than design spec
> 
> Review of attachment 8392574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. As a followup, we should make the back button and the url field be
> joined together. On 28, there is no 'break' between the dark grey of the url
> field and the back button's borders, but on current nightly and after this
> patch, there's an outer light shadow/accent which overlaps it. I'm not sure
> if that's the image's antialiasing at work or the clip path, but either way.
> Maybe it's even intentional? Question for Stephen, I guess. Anyway, this
> patch doesn't make it worse/bad, so we should ship this.

Hm... are you talking about how the border doesn't hit the back button exactly on Windows? http://i.imgur.com/UxSQl6G.png


> Not sure about the risk for beta though. Does this alter the navbar's height,
> Brandon? If so, I'd at least want to bake it on nightly for a bit before
> uplifting to beta. Less fussed about aurora seeing as that's only just merged
> up.

It does on OS X. The 2 additional pixels on the back button extend the navigation toolbar 2px from 38px to 40px. It inadvertently fixes bug 937734.

> Oh, one more nit, though: we should remove the non-lion keyhole (that wasn't
> updated here) and map this (lion) one to the old path. Otherwise the clip
> path and the image probably won't match.

I noticed that. I was going to ask if that png was being used anywhere I didn't know about. It's not mapped anywhere in the manifest file. I don't think I understand the part about the clip-path and the old non-lion png matching..? As far as I know, it's unused.

Comment 52

[Brandon Cheng]

(In reply to Brandon Cheng from comment #51)
> It does on OS X. The 2 additional pixels on the back button extend the
> navigation toolbar 2px from 38px to 40px. It inadvertently fixes bug 937734.

Actually, give me around 45 minutes to reconfirm this on a build from scratch.

Comment 53

[:Gijs (he/him)]

(In reply to Brandon Cheng from comment #51)
> (In reply to :Gijs Kruitbosch from comment #49)
> > Comment on attachment 8392574 [details] [diff] [review]
> > (Australis) Back button and toolbar slightly smaller than design spec
> > 
> > Review of attachment 8392574 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me. As a followup, we should make the back button and the url field be
> > joined together. On 28, there is no 'break' between the dark grey of the url
> > field and the back button's borders, but on current nightly and after this
> > patch, there's an outer light shadow/accent which overlaps it. I'm not sure
> > if that's the image's antialiasing at work or the clip path, but either way.
> > Maybe it's even intentional? Question for Stephen, I guess. Anyway, this
> > patch doesn't make it worse/bad, so we should ship this.
> 
> Hm... are you talking about how the border doesn't hit the back button
> exactly on Windows? http://i.imgur.com/UxSQl6G.png

I was talking about OS X. I've not investigated on Windows.

> > Not sure about the risk for beta though. Does this alter the navbar's height,
> > Brandon? If so, I'd at least want to bake it on nightly for a bit before
> > uplifting to beta. Less fussed about aurora seeing as that's only just merged
> > up.
> 
> It does on OS X. The 2 additional pixels on the back button extend the
> navigation toolbar 2px from 38px to 40px. It inadvertently fixes bug 937734.

Right. So I'd want to bake on Nightly and Aurora before beta uplift at the end of this week / beginning of next, assuming we get the approvals and such.

> > Oh, one more nit, though: we should remove the non-lion keyhole (that wasn't
> > updated here) and map this (lion) one to the old path. Otherwise the clip
> > path and the image probably won't match.
> 
> I noticed that. I was going to ask if that png was being used anywhere I
> didn't know about. It's not mapped anywhere in the manifest file. I don't
> think I understand the part about the clip-path and the old non-lion png
> matching..? As far as I know, it's unused.

It is used. It's used on 10.6:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#46

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#445

So this basically involves removing these two lines, and making this line:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#421

use the path of the first line I linked to. I think the only consequence of not doing so would be the back button's background being scaled from the current non-lion icon on 10.6? Not ideal, though, so we should just fix this as part of this patch. Can you do the above? :-)

Comment 54

[:Gijs (he/him)]

bzexport is dumb sometimes, and I am dumb when I am sleepy. :-)

Comment 55

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #53)
> So this basically involves removing these two lines, and making this line:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#421
> 
> use the path of the first line I linked to. I think the only consequence of
> not doing so would be the back button's background being scaled from the
> current non-lion icon on 10.6? Not ideal, though, so we should just fix this
> as part of this patch. Can you do the above? :-)

Sure thing. Do we want to rename keyhole-circle-lion.png to keyhole-circle.png too?

Comment 56

[:Gijs (he/him)]

(In reply to Brandon Cheng from comment #55)
> (In reply to :Gijs Kruitbosch from comment #53)
> > So this basically involves removing these two lines, and making this line:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#421
> > 
> > use the path of the first line I linked to. I think the only consequence of
> > not doing so would be the back button's background being scaled from the
> > current non-lion icon on 10.6? Not ideal, though, so we should just fix this
> > as part of this patch. Can you do the above? :-)
> 
> Sure thing. Do we want to rename keyhole-circle-lion.png to
> keyhole-circle.png too?

Good point. Same for the @2x one.

Comment 57

[Brandon Cheng]

Attachment: Patch v2.1

Uniform keyhole-circle across OS X.

Comment 58

[:Gijs (he/him)]

Comment on attachment 8393132 [details] [diff] [review]
Patch v2.1

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

Nice! However, we should rm the lion ones and add the @2x one, which isn't in this patch. I can just fix that when I land this, though.

Comment 59

[:Gijs (he/him)]

(In reply to Brandon Cheng from comment #52)
> (In reply to Brandon Cheng from comment #51)
> > It does on OS X. The 2 additional pixels on the back button extend the
> > navigation toolbar 2px from 38px to 40px. It inadvertently fixes bug 937734.
> 
> Actually, give me around 45 minutes to reconfirm this on a build from
> scratch.

So is it two pixels higher? :-)

Comment 60

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #59)
> (In reply to Brandon Cheng from comment #52)
> > (In reply to Brandon Cheng from comment #51)
> > > It does on OS X. The 2 additional pixels on the back button extend the
> > > navigation toolbar 2px from 38px to 40px. It inadvertently fixes bug 937734.
> > 
> > Actually, give me around 45 minutes to reconfirm this on a build from
> > scratch.
> 
> So is it two pixels higher? :-)

(checked, yes it is)

Comment 61

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/67ded00b8ca7

Comment 62

[Brandon Cheng]

Thanks to Gijs and Jared for reviewing! You guys are awesome. :)

Comment 63

[Jared Wein [:jaws] (please needinfo? me)]

Thanks! Nice patch :)

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67ded00b8ca7

Comment 65

[:Gijs (he/him)]

Comment on attachment 8393132 [details] [diff] [review]
Patch v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: back button is a little on the small side
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): medium-low; some SVG path simplifications, some CSS changes on Windows and OS X for the default theme. I'd like to land this on aurora and bake for a bit, and only then request beta approval, basically.
String or IDL/UUID changes made by this patch: none

Comment 66

[:Gijs (he/him)]

Sylvestre, I've seen you do a bunch of a+'s since I requested this - is this on your/someone's radar and did you not approve yet because you wanted it to bake a bit longer, or is it falling through the cracks because of some reason? Ideally, for consistency's sake, I'd like this to eventually make 29, but I figured we should start with Aurora and move to beta after more baking...

Comment 67

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8393132 [details] [diff] [review]
Patch v2.1

Gijs, well, you tagged it only for aurora. Lukas being in charge of 30 (ie aurora), I am not supposed to look aurora approval.
Anyway, I am going to approve it in aurora if you want to see it in 29.

Comment 68

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #67)
> Comment on attachment 8393132 [details] [diff] [review]
> Patch v2.1
> 
> Gijs, well, you tagged it only for aurora. Lukas being in charge of 30 (ie
> aurora), I am not supposed to look aurora approval.
> Anyway, I am going to approve it in aurora if you want to see it in 29.

Ah, sorry! Thanks for clarifying. :-)

Comment 69

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4fdbb6be0bc9

Comment 70

[:Gijs (he/him)]

Comment on attachment 8393132 [details] [diff] [review]
Patch v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: back button is a little on the small side
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low; some SVG path simplifications, some CSS changes on Windows and OS X for the default theme. This has now baked for a while on both m-c and aurora with 0 regressions, so I think we should just take this on beta for consistency.
String or IDL/UUID changes made by this patch: none

Comment 71

[Mike de Boer [:mikedeboer]]

Pushed to Beta as: https://hg.mozilla.org/releases/mozilla-beta/rev/59ea52090736

Comment 72

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified this on Windows 7 and Mac OSX (non-retina) with 31.0a1 Nightly (20140403030201), 30.0a2 Aurora (20140403004002) and 29.0b4 Beta. Buttons (Back, Fw) and Navbar are much better aligned with spec, with small exceptions where there is a 1 px difference:
- Win 7 x64 
    - Back=32x32px - OK
    - Fw=23px - NOT OK (expected 24px)
    - URL bar=40px - OK
- Mac 10.9 - Back=32x32, Fw=24, URL bar=39
    - Back=32x32px - OK
    - Fw=24px - OK
    - URL bar=39px - NOT OK (expected 40px)

Retina display still needs to be tested, but I would like to know whether the non-retina 1 px mismatches still need fixing or are acceptable as is.

See screenshots attached.

Comment 73

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Attachment: Forward button on Win 7 is 23 px wide (1 px shorter).

Comment 74

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Attachment: Navbar on Mac is 39 px (1 px shorter).

Comment 75

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Could someone verify this on Retina display, as we do not have any MacBooks available?
Are the 1px mismatches from comment 72 acceptable, or should that be fixed?

Comment 76

[Mike de Boer [:mikedeboer]]

Brandon, can you verify and explain Florin's findings in comment 72?

Comment 77

[Brandon Cheng]

(In reply to Mike de Boer [:mikedeboer] from comment #76)
> Brandon, can you verify and explain Florin's findings in comment 72?

Yes. I can verify the OS X one. I have to inspect it and see why it's off. I think the darker border is really an inset shadow (but should be a border) making the nav-bar element 40px but not visually.

I don't have Windows 7 to verify the other one. I can confirm that Windows 8 at least has it right.

Comment 78

[Brandon Cheng]

Attachment: Forward Button on Win 8 is aligned

Comment 79

[Brandon Cheng]

(In reply to Mike de Boer [:mikedeboer] from comment #76)
> Brandon, can you verify and explain Florin's findings in comment 72?

I can verify Florin's findings for the forward button on Windows 7. Filed as bug 998157.

Comment 80

[u279076]

(In reply to Brandon Cheng from comment #77)
> (In reply to Mike de Boer [:mikedeboer] from comment #76)
> > Brandon, can you verify and explain Florin's findings in comment 72?
> 
> Yes. I can verify the OS X one. I have to inspect it and see why it's off. 

Brandon, have you made any progress on this? Should it be handled in a follow-up bug or should this bug be reopened?

Comment 81

[Brandon Cheng]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #80)
> (In reply to Brandon Cheng from comment #77)
> > (In reply to Mike de Boer [:mikedeboer] from comment #76)
> > > Brandon, can you verify and explain Florin's findings in comment 72?
> > 
> > Yes. I can verify the OS X one. I have to inspect it and see why it's off. 
> 
> Brandon, have you made any progress on this? Should it be handled in a
> follow-up bug or should this bug be reopened?

I'll file a follow-up bug for the OS X issue.

Comment 82

[u279076]

Marking this verified fixed based on recent comments.