Back button and toolbar slightly smaller than design spec

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: kazukigot1, Assigned: Brandon Cheng)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 31
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed, firefox31 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(8 attachments, 6 obsolete attachments)

12.88 KB, image/jpeg
Details
9.08 KB, application/zip
Details
2.16 KB, image/png
Details
5.89 KB, image/png
Details
14.73 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
34.98 KB, image/png
Details
70.89 KB, image/png
Details
(deleted), image/png
Details
Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 775502 [details]
Screenshot of the problem

See screenshot.
(Reporter)

Updated

5 years ago
Blocks: 875488
(Reporter)

Updated

5 years ago
Blocks: 872617
What are these screenshots comparing?

Are you comparing UX to Nightly or Firefox 22? (Possibly hidpi settings?)
The screenshots are comparing UX builds pre and post bug 875488.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Component: Toolbars and Customization → Theme
Keywords: regression
(Reporter)

Comment 4

5 years ago
(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
Summary: [Australis] back button have a small size → Back button and toolbar slightly smaller than design spec
Whiteboard: [Australis:P3]
(Reporter)

Comment 6

5 years ago
This looks smaller in OSX on.
Whiteboard: [Australis:P3] → [Australis:P3][Australis:M?]

Updated

4 years ago
Blocks: 931108
(Assignee)

Comment 7

4 years ago
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. :)
(Assignee)

Comment 8

4 years ago
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.)
Stephen created that image, and he should be able to increase the radius of the circle to 16px. Flagging Stephen for needinfo...
Assignee: nobody → bcheng.gt
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Flags: needinfo?(shorlander) → needinfo?(mmaslaney)
Jared, the current OSX back button is 30px and 60px for retina display. Would you like me to increase it to 32px/64px?
Flags: needinfo?(mmaslaney) → needinfo?(jaws)
Yes, that would be great.
Flags: needinfo?(jaws)
Created attachment 8355550 [details]
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.
Brandon, can you pick this up now?
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 14

4 years ago
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
Flags: needinfo?(bcheng.gt)

Comment 15

4 years ago
(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?
Flags: needinfo?(mmaslaney)
Flags: needinfo?(shorlander)
Flags: needinfo?(mmaslaney)
Created attachment 8366975 [details]
Keyhole — OS X
Flags: needinfo?(shorlander)
Created attachment 8366976 [details]
Keyhole — OS X @2x
Brandon, any update on this?
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 19

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

Updated

4 years ago
Depends on: 974007
(Assignee)

Comment 21

4 years ago
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.
(Assignee)

Comment 22

4 years ago
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.
Flags: needinfo?(bcheng.gt)
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!
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 24

4 years ago
Created attachment 8388158 [details] [diff] [review]
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.
Attachment #8388158 - Flags: review?(jaws)
Flags: needinfo?(bcheng.gt)
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 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!
Attachment #8388158 - Flags: review?(jaws) → review+
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
Flags: needinfo?(shorlander)
Created attachment 8388694 [details] [diff] [review]
Patch v1.1
Attachment #8388158 - Attachment is obsolete: true
Attachment #8388694 - Flags: review?(shorlander)
(Assignee)

Comment 29

4 years ago
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.
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!
Flags: needinfo?(shorlander)
(Assignee)

Comment 31

4 years ago
(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.
(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.
Whiteboard: [Australis:P3][Australis:M?] → [Australis:P3]
Cool, Brandon, I'll let you make those other changes.
(Assignee)

Comment 35

4 years ago
Created attachment 8391974 [details] [diff] [review]
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.
Attachment #8391974 - Flags: review?(jaws)
(Assignee)

Comment 36

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

Comment 37

4 years ago
I tried this on OS X and I'm seeing the left (start) border of the fwd button bleed through the back button... :-(
Attachment #8388694 - Attachment is obsolete: true
Attachment #8388694 - Flags: review?(shorlander)
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?
Attachment #8391974 - Flags: review?(jaws) → review-
(Assignee)

Comment 39

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

4 years ago
Created attachment 8392564 [details] [diff] [review]
(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.
Attachment #8392564 - Flags: review?(jaws)

Updated

4 years ago
Assignee: bcheng.gt → gijskruitbosch+bugs

Comment 42

4 years ago
Created attachment 8392570 [details] [diff] [review]
(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.
Attachment #8392570 - Flags: review?(jaws)

Updated

4 years ago
Attachment #8392564 - Attachment is obsolete: true
Attachment #8392564 - Flags: review?(jaws)

Comment 43

4 years ago
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.
Attachment #8392574 - Flags: review?(jaws)

Updated

4 years ago
Attachment #8392570 - Attachment is obsolete: true
Attachment #8392570 - Flags: review?(jaws)
(Assignee)

Comment 44

4 years ago
(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 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?
Attachment #8392574 - Flags: review?(jaws)
Attachment #8392574 - Flags: review?(gijskruitbosch+bugs)
Attachment #8392574 - Flags: review+
Attachment #8391974 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
(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.
(Assignee)

Comment 47

4 years ago
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 49

4 years ago
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.
Attachment #8392574 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 50

4 years ago
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.
(Assignee)

Comment 51

4 years ago
(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.
(Assignee)

Comment 52

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

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

4 years ago
bzexport is dumb sometimes, and I am dumb when I am sleepy. :-)
Assignee: gijskruitbosch+bugs → bcheng.gt
(Assignee)

Comment 55

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

4 years ago
(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.
(Assignee)

Comment 57

4 years ago
Created attachment 8393132 [details] [diff] [review]
Patch v2.1

Uniform keyhole-circle across OS X.
Attachment #8393132 - Flags: review?(gijskruitbosch+bugs)

Comment 58

4 years ago
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.
Attachment #8393132 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 59

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

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

4 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/67ded00b8ca7
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]

Updated

4 years ago
Blocks: 937734

Updated

4 years ago
Attachment #8392574 - Attachment is obsolete: true
(Assignee)

Comment 62

4 years ago
Thanks to Gijs and Jared for reviewing! You guys are awesome. :)
Thanks! Nice patch :)
https://hg.mozilla.org/mozilla-central/rev/67ded00b8ca7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31

Comment 65

4 years ago
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
Attachment #8393132 - Flags: approval-mozilla-aurora?
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed

Comment 66

4 years ago
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...
Flags: needinfo?(sledru)
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.
Attachment #8393132 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(sledru)

Comment 68

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

4 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4fdbb6be0bc9
status-firefox30: affected → fixed

Comment 70

4 years ago
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
Attachment #8393132 - Flags: approval-mozilla-beta?
Attachment #8393132 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed to Beta as: https://hg.mozilla.org/releases/mozilla-beta/rev/59ea52090736
status-firefox29: affected → fixed
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.
Created attachment 8401810 [details]
Forward button on Win 7 is 23 px wide (1 px shorter).
Created attachment 8401811 [details]
Navbar on Mac is 39 px (1 px shorter).
Attachment #8401811 - Attachment description: Navbar_1px_short_Mac.png → Navbar on Mac is 39 px (1 px shorter).
Attachment #8401810 - Attachment description: FW_1px_short_Win.png → Forward button on Win 7 is 23 px wide (1 px shorter).
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?

Updated

4 years ago
Depends on: 989466
Brandon, can you verify and explain Florin's findings in comment 72?
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 77

4 years ago
(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.
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 78

4 years ago
Created attachment 8406469 [details]
Forward Button on Win 8 is aligned
(Assignee)

Comment 79

4 years ago
(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.
Depends on: 998157
(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?
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 81

4 years ago
(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.
Flags: needinfo?(bcheng.gt)
(Assignee)

Updated

4 years ago
Blocks: 1028785
Marking this verified fixed based on recent comments.
Status: RESOLVED → VERIFIED
status-firefox31: fixed → verified
You need to log in before you can comment on or make changes to this bug.