Closed Bug 873626 Opened 7 years ago Closed 6 years ago

Adjust height of navbar to 40px per Australis specs on Linux and Windows

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: brandon.cheng, Assigned: brandon.cheng)

References

()

Details

(Whiteboard: [Australis:M9][Australis:P4])

Attachments

(2 files, 7 obsolete files)

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
Component: Untriaged → Theme
OS: Linux → Windows 7
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [Australis:M6]
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #751287 - Flags: review?(mnoorenberghe+bmo)
Assignee: nobody → bcheng.gt
Status: NEW → ASSIGNED
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.
Attachment #751287 - Flags: review?(mnoorenberghe+bmo) → feedback+
(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.
Attached patch Patch v1.5 (obsolete) — Splinter Review
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.
Attachment #751287 - Attachment is obsolete: true
(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. :)
Blocks: 873662
Hi Brandon! Any progress on this issue? I'd love to get my hand on a finished clip-path! ;)
Attached patch Patch v2 (obsolete) — Splinter Review
This patch adds changes for OS X and Linux platforms.
Attachment #751396 - Attachment is obsolete: true
Attachment #757054 - Flags: review?(jaws)
Attached patch Patch v2.1 (obsolete) — Splinter Review
I accidentally backed out the windows changes in v2, whoops. Don't worry, I check this one! :)
Attachment #757054 - Attachment is obsolete: true
Attachment #757054 - Flags: review?(jaws)
Attachment #757074 - Flags: review?(jaws)
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.
Attached patch Patch v2.2 (obsolete) — Splinter Review
Not completely sure what happened in the last patch. I went ahead and generated this one from scratch.
Attachment #757074 - Attachment is obsolete: true
Attachment #757074 - Flags: review?(jaws)
Attachment #757139 - Flags: review?(jaws)
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?
Attachment #757139 - Flags: review?(jaws) → feedback+
(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.
(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?
Whiteboard: [Australis:M6] → [Australis:M7]
(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.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Any updates here Brandon?
Flags: needinfo?(bcheng.gt)
(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.
Flags: needinfo?(bcheng.gt)
(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?
Flags: needinfo?(bcheng.gt)
(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.
Flags: needinfo?(bcheng.gt)
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?
Flags: needinfo?(bcheng.gt)
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P4]
(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.
Flags: needinfo?(bcheng.gt)
Attached patch Patch v3 (obsolete) — Splinter Review
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. :)
Attachment #757139 - Attachment is obsolete: true
Attachment #829832 - Flags: review?(jaws)
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?
Attachment #829832 - Flags: review?(jaws) → review+
Attached patch Patch v3.1 (obsolete) — Splinter Review
Remove tabsontop selector
Attachment #829832 - Attachment is obsolete: true
Attachment #830602 - Flags: review?(jaws)
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.
Attachment #830602 - Flags: review?(jaws) → review+
Attached patch Patch v3.2Splinter Review
Merged #navbar properties. Nice catch. It was a bit obvious, but I still missed it. Thanks.
Attachment #830602 - Attachment is obsolete: true
Attachment #830678 - Flags: review?(jaws)
I'll check this in tomorrow, thanks!
Flags: needinfo?(jaws)
https://hg.mozilla.org/projects/ux/rev/4c2d984c2094
Flags: needinfo?(jaws)
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M9][Australis:P4][fixed-in-ux]
Summary: Adjust height of navbar to 40px per Australis specs → Adjust height of navbar to 40px per Australis specs on Linux and Windows
Forgot the : in   1.12 +  padding-top 2px;
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P4][fixed-in-ux] → [Australis:M9][Australis:P4]
Target Milestone: --- → Firefox 28
Attached patch Linux fix.patchSplinter Review
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
Attachment #8334255 - Flags: review?(jaws)
Flags: needinfo?(bcheng.gt)
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.
Attachment #8334255 - Flags: review?(jaws) → review+
Removing checkin-needed since the patch needs a more correct summary and the reviewer listed on the summary is incorrect.
Keywords: checkin-needed
(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 :)
Depends on: 940935
You need to log in before you can comment on or make changes to this bug.