User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release) Build ID: 20130512193902 Steps to reproduce: The specifications from Stephen Horlander call for a 40px tall navigation bar. It is currently 36px. http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html#sectionNavBar

This is a one-liner that adds 2px to the top and bottom of each button's padding (thereby increasing the navbar by 4px). There is suppose to be 8px above and below each button. 7px appears in the code since the border is not factored in. When it is, we get 8px and all is well.

Comment on attachment 751287 [details] [diff] [review] Patch v1 Review of attachment 751287 [details] [diff] [review]: ----------------------------------------------------------------- With this patch I only get a navbar height of 38px. On Windows there isn't a border for the button, the border is on the inner .toolbarbutton-icon. I do however get the 40px height with a top & bottom padding of 8px. Also, can you make the same change for OS X and Linux? I or someone else can test out the change for you if you don't have access to those environments.

(In reply to Jared Wein [:jaws] from comment #2) > Comment on attachment 751287 [details] [diff] [review] > Patch v1 > > Review of attachment 751287 [details] [diff] [review]: > ----------------------------------------------------------------- > > With this patch I only get a navbar height of 38px. On Windows there isn't a > border for the button, the border is on the inner .toolbarbutton-icon. I do > however get the 40px height with a top & bottom padding of 8px. > > Also, can you make the same change for OS X and Linux? I or someone else can > test out the change for you if you don't have access to those environments. Ah, you're right. I guess it was actually 34px high before rather than 36px. I should have gone in and actually measured it. I should have access to the other systems, but I do need to fix my Mac before I have access to OS X. That should be quick though. :)

Comment on attachment 751287 [details] [diff] [review] Patch v1 This change causes windows-urlbar-back-button-clip-path to be misaligned.

This patch actually makes the navigation bar 40px, whereas the last one only made it 38px. This also updates the clip-path clip 2px lower than it originally did. These are windows only changes at the moment, so no review needed for now. Somethings I discovered worthy of note: 1. The navbar was indeed originally 36px. However, 6px of padding had to be added to the primary toolbar buttons to extend it to 40px. The 2px missing here compressed the toolbar button's height (from 26px to 24px). I believe this should have been compressed originally. So "padding: 5px 2px;" should have been "padding: 6px 2px;". The buttons visually stay 24px tall regardless though. 2. The clip-path had hard-coded absolute paths to assume the urlbar was 36px tall. I basically just moved all references 2px down after the urlbar became 40px tall.

(In reply to Brandon Cheng from comment #5) > 2. The clip-path had hard-coded absolute paths to assume the urlbar was 36px > tall. I basically just moved all references 2px down after the urlbar became > 40px tall. It might be better to move the clip-path from #urlbar-container to #urlbar. However, you then need to take care of RTL which currently depends on the clip-path being on the container: http://hg.mozilla.org/mozilla-central/annotate/4ac6c72b06c8/browser/themes/windows/browser.css#l1332

(In reply to Dão Gottwald [:dao] from comment #6) > It might be better to move the clip-path from #urlbar-container to #urlbar. > However, you then need to take care of RTL which currently depends on the > clip-path being on the container: > http://hg.mozilla.org/mozilla-central/annotate/4ac6c72b06c8/browser/themes/ > windows/browser.css#l1332 I'll file another bug on this and fix it after this one. :)

Hi Brandon! Any progress on this issue? I'd love to get my hand on a finished clip-path! ;)

This patch adds changes for OS X and Linux platforms.

I accidentally backed out the windows changes in v2, whoops. Don't worry, I check this one! :)

Comment on attachment 757074 [details] [diff] [review] Patch v2.1 Review of attachment 757074 [details] [diff] [review]: ----------------------------------------------------------------- There's something weird about the attachment here. I see two disjoint edits to the linux/browser.css in the attachment, which is getting the online review tool that I use confused.

Not completely sure what happened in the last patch. I went ahead and generated this one from scratch.

Comment on attachment 757139 [details] [diff] [review] Patch v2.2 Review of attachment 757139 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/browser.css @@ +101,5 @@ > } > > +#nav-bar[tabsontop=true] { > + padding-top: 5px !important; > + padding-bottom: 5px !important; Is it possible to remove the !important here? My guess is that this is currently needed because there is another selector that has higher specificity. Maybe this selector just needs to be made more specific or moved after the rule that it is trying to override?

(In reply to Jared Wein [:jaws] from comment #13) > Comment on attachment 757139 [details] [diff] [review] > Patch v2.2 > > Review of attachment 757139 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/osx/browser.css > @@ +101,5 @@ > > } > > > > +#nav-bar[tabsontop=true] { > > + padding-top: 5px !important; > > + padding-bottom: 5px !important; > > Is it possible to remove the !important here? My guess is that this is > currently needed because there is another selector that has higher > specificity. Maybe this selector just needs to be made more specific or > moved after the rule that it is trying to override? That's exactly right. I spent a while on this and thought the !important may have been the best thing to do without changing (breaking!) other code. The issue is that the code above is trying to override a selector that is using !important to override yet another selector (which has 3 #id's and a 301 specificity count). Even if I change the code above to include the #browser-panel, the !important of the second selector (which overrides the 301) trumps. So that would also have to be changed. The code in question is themes/osx/browser.css lines 81 to 104. Here's a link. https://hg.mozilla.org/projects/ux/file/c197150217d6/browser/themes/osx/browser.css#l81 I agree that the !important is ugly and I would like to remove it. I just don't see a much better alternative. There are other ways to increase the nav-bar height, but it makes sense to increase the already existing 1px top+bottom padding. Are we planning on dropping the toolbar button outlines on OS X? Implementing the toolbar buttons the same way we have at the moment on Windows might be better way to get the extra 2px in.

I made an oops. It starts at line #70. https://hg.mozilla.org/projects/ux/file/c197150217d6/browser/themes/osx/browser.css#l70

(In reply to Brandon Cheng from comment #14) > Are we planning on dropping the toolbar button outlines > on OS X? Yes, see bug 856665. Can you see how the patch in that bug will affect this change?

(In reply to Jared Wein [:jaws] from comment #16) > (In reply to Brandon Cheng from comment #14) > > Are we planning on dropping the toolbar button outlines > > on OS X? > > Yes, see bug 856665. Can you see how the patch in that bug will affect this > change? I was looking to change the padding/margin on the toolbar buttons (as on Windows), and the outline would be a visual issue. Turns out I was wrong. Changing the margin on line #437's selector would also work. I have no idea what #restore-button is though, and toolbar-button1 may not be selective enough..? https://hg.mozilla.org/projects/ux/file/c197150217d6/browser/themes/osx/browser.css#l437

Removing the items from M7 that do not block us from landing on m-c.

Any updates here Brandon?

(In reply to Jared Wein [:jaws] from comment #19) > Any updates here Brandon? Yes, hopefully soon (aiming for tomorrow). I need to try making this change through the method I described in comment #17.

A few things: 1) Linux had the new bookmark star pushed out recently, and the problem with it is that it's stretching the navigation bar height. With the default toolbar set, Linux is actually 4px over the 40px we're looking for. I think another bug should be filed on this (if not already), and just leave the change we have here since it's working when this huge button isn't on the toolbar. 2) OS X has some proportion issues that I'm just noticing now. Mainly, the back button radius is 15px when it should be 16px, and the urlbar is also 2px shorter than spec. I can apply the navigation bar change here to OS X, but I think this and those proportion changes should be pushed to another bug. 3) Another small thing I'm not sure why I didn't notice before: OS X's navigation bar is currently 38px, but it is 37px measured due to the way the border at the bottom is implemented. That would have to be changed. In other words, I think the patch here should be pushed to Linux and Windows, with further work on OS X moved.

(In reply to Brandon Cheng from comment #21) > A few things: > > 1) Linux had the new bookmark star pushed out recently, and the problem with > it is that it's stretching the navigation bar height. With the default > toolbar set, Linux is actually 4px over the 40px we're looking for. I think > another bug should be filed on this (if not already), and just leave the > change we have here since it's working when this huge button isn't on the > toolbar. Yes, please file another bug. > 2) OS X has some proportion issues that I'm just noticing now. Mainly, the > back button radius is 15px when it should be 16px, and the urlbar is also > 2px shorter than spec. I can apply the navigation bar change here to OS X, > but I think this and those proportion changes should be pushed to another > bug. This is bug 893661, if I understand correctly. Is there other OS X work that needs to happen here?

(In reply to :Gijs Kruitbosch from comment #22) > This is bug 893661, if I understand correctly. Is there other OS X work that > needs to happen here? There should not be. I'll remove the OS X changes in the current patch and regenerate it on the latest UX.

Is there a follow up bug for OS X or do you plan to fix this in this bug? If so, please update the platform field.

Brandon, are you still working on this?

(In reply to :Gijs Kruitbosch from comment #25) > Brandon, are you still working on this? I've started working on this again. The clip-path has been modified since the last patch here, so I need to double check that before submitting a patch. I hope to have it by tomorrow.

This patch has been simplified a lot. The OS X changes have been backed out and another but will be filed (comment #21). The clip-path on Windows has been made relative to the urlbar, so adjusting padding no longer shifts the clip-path. Sorry this took so long. And thanks to Jared, Gijs, and Dao so far. :)

Comment on attachment 829832 [details] [diff] [review] Patch v3 Review of attachment 829832 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/linux/browser.css @@ +42,5 @@ > padding-top: 1px; > padding-bottom: 1px; > } > > +#nav-bar[tabsontop=true] { [tabsontop=true] should always be valid since bug 755593 was fixed. Can you drop that attribute selector?

Remove tabsontop selector

Comment on attachment 830602 [details] [diff] [review] Patch v3.1 Review of attachment 830602 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/linux/browser.css @@ +47,5 @@ > + padding-top: 2px; > + padding-bottom: 2px; > +} > + > +#nav-bar { These can now be merged.

Merged #navbar properties. Nice catch. It was a bit obvious, but I still missed it. Thanks.

I'll check this in tomorrow, thanks!

Forgot the : in 1.12 + padding-top 2px;

Thanks for catching that! Pushed a follow-up for it: https://hg.mozilla.org/projects/ux/rev/1ff54a14fbb7

I apologize, there's another issue I just discovered. Some changes have appeared to have been made and the specificity of the two vertical padding attributes for the Linux side of things is not enough to override the CSS just a few lines above. https://hg.mozilla.org/projects/ux/file/672ff858678a/browser/themes/linux/browser.css#l40 The easiest way to solve this would be to add a separate #nav-bar selector with !important and a comment explaining it. Would this be enough to allow !important into the code? Or do you perhaps see a better solution? This is what I'm suggesting: https://gist.github.com/gluxon/e72678654f682c72b69f

(In reply to Brandon Cheng from comment #36) > Or do you perhaps see a better solution? You simply need to change "#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar)" to "#navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar)".

https://hg.mozilla.org/mozilla-central/rev/4c2d984c2094 https://hg.mozilla.org/mozilla-central/rev/1ff54a14fbb7

Additional/Follow-up fix for the Linux specificity issue that I overlooked. Using fix from Dao in comment 37. And thanks Dao, it didn't occur to me that I could do that. :P

Comment on attachment 8334255 [details] [diff] [review] Linux fix.patch As a general guideline, please file new bugs for extra patches if the original patch already landed, such that we can properly keep track of things.

Removing checkin-needed since the patch needs a more correct summary and the reviewer listed on the summary is incorrect.

Comment on attachment 8334255 [details] [diff] [review] Linux fix.patch https://hg.mozilla.org/integration/fx-team/rev/97a25ccd3bf8

(In reply to Dão Gottwald [:dao] from comment #40) > Comment on attachment 8334255 [details] [diff] [review] > Linux fix.patch > > As a general guideline, please file new bugs for extra patches if the > original patch already landed, such that we can properly keep track of > things. Noted for next time. Sorry about that. (In reply to Jared Wein [:jaws] from comment #41) > Removing checkin-needed since the patch needs a more correct summary and the > reviewer listed on the summary is incorrect. I listed you (:jaws) for review. Should I have done Dão? And should I add a better summary in a comment while I can now?

(In reply to Brandon Cheng from comment #44) > (In reply to Jared Wein [:jaws] from comment #41) > > Removing checkin-needed since the patch needs a more correct summary and the > > reviewer listed on the summary is incorrect. > > I listed you (:jaws) for review. Should I have done Dão? And should I add a > better summary in a comment while I can now? Yeah that was fine, you initially requested review from me and then Dao got to it before I did. I fixed the summary and checked it in for you, no worries. I only left the comment about the commit message needing updating to explain why I was removing the checkin-needed flag :)