1185362 - Fix and consolidate margin and padding styling around the identity block

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

As of bug 1180202, the identity block looks kind of broken without a label, because some necessary margins / paddings are only set when there's a label because that's the only case we used to draw the separator for.

The attached patch should fix that and generally consolidate the styling across OSes for better maintainability.

This should be a big step towards bug 1180213, although you may still have some work to do.

Comment 1

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #0)
> This should be a big step towards bug 1180213, although you may still have
> some work to do.

This is fantastic, thanks! It's indeed a big help.

I'll take some time to review this, in the meantime I have one quick question: do we still need two separate rules to define the margins in the LTR and RTL case for the transition to work correctly with the new logical properties?

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #1)
> I'll take some time to review this, in the meantime I have one quick
> question: do we still need two separate rules to define the margins in the
> LTR and RTL case for the transition to work correctly with the new logical
> properties?

I believe the new properties work just like -moz-margin-start/end (those are actually aliases now), so I don't think anything changes for us. Might be worth testing though.

Comment 3

[:Paolo Amadini]

I've done some preliminary testing on various platforms first and looks good overall, although there's a regression in the height of the location bar in popup windows (no navigation) on Windows and Linux only. I haven't linked that to specific code as I still have to review the patch in detail, which will likely happen tomorrow.

Comment 4

[:Paolo Amadini]

Just wanted to say that I'm still reviewing, there's a lot to check so most probably won't be finished today.

The reason for the reduced height without the navigation is likely the removed padding from the URL bar textbox, so I guess that previously the font size plus the padding determined the textbox height in that case. In the case with navigation, the height was probably determined by the elements inside the textbox since when I tested it ended up having the correct height instead.

Dão, should we let the elements inside the textbox dictate its height, or maybe at this point we should just set an explicit height in pixels?

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #4)
> Dão, should we let the elements inside the textbox dictate its height,

I think that's fine. In non-popup windows the location bar is expected to match the height of the search bar and toolbar buttons, but that's not the case for popups.

I can probably set the identity box's vertical padding to 2px like it was before on Windows and Linux. Not sure if that would cause problems on OS X, though.

Comment 6

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #5)
> In non-popup windows the location bar is expected to
> match the height of the search bar and toolbar buttons, but that's not the
> case for popups.

Yeah, in popup windows there is no visual element that aligns vertically with the location bar, although it having the same height in all windows is definitely convenient from a styling standpoint.

> I can probably set the identity box's vertical padding to 2px like it was
> before on Windows and Linux. Not sure if that would cause problems on OS X,
> though.

That sounds good, let's do the simplest thing that allows us to have a consistent height across windows. Even though I haven't finished the review, the path generally looks good, so feel free to attach an updated version if you want.

I've also tested that some of the rules that are now separate for LTR and RTL can be simplified using logical properties, though "transition" still has to refer to the physical properties.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #6)
> I've also tested that some of the rules that are now separate for LTR and
> RTL can be simplified using logical properties, though "transition" still
> has to refer to the physical properties.

Let's file a separate bug on this, don't need to take care of every possible simplification in one patch.

Comment 8

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Paolo Amadini from comment #6)
> > I've also tested that some of the rules that are now separate for LTR and
> > RTL can be simplified using logical properties, though "transition" still
> > has to refer to the physical properties.
> 
> Let's file a separate bug on this, don't need to take care of every possible
> simplification in one patch.

Sure, it's even better.

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v2

set the top and bottom padding to 2px

Comment 10

[:Paolo Amadini]

Comment on attachment 8637192 [details] [diff] [review]
patch v2

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

This looks great, it rationalizes how we handle margins in the location bar, and I've seen how this fixes quite a few bugs. Reviewing this has also helped me understand the structure of the navigation bar much better.

I've tested the height on Windows and Linux and the margin is still one pixel too small (height is two pixels less in total). I'd like to test a new version before landing so clearing the review flag for now.

::: browser/themes/linux/browser.css
@@ +18,5 @@
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-wrapper
>  %define conditionalForwardWithUrlbarWidth 30
>  
>  :root {
> +  --backbutton-urlbar-overlap: 5px;

We might want to set this to 0px in devedition.css.

::: browser/themes/linux/searchbar.css
@@ -9,5 @@
>  
> -.searchbar-textbox {
> -  min-height: 22px;
> -  background-color: -moz-field;
> -}

Tested the height in various customization targets and seems to works correctly.

Is the removal of background-color intentional?

Comment 11

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #10)
> I've tested the height on Windows and Linux and the margin is still one
> pixel too small (height is two pixels less in total). I'd like to test a new
> version before landing so clearing the review flag for now.

I honestly don't think this is worth worrying about. I'm not sure why you think it's important for the location bar to have the exact same height in popup windows and non-popup windows. The popup UI looks nothing like the full browser window UI anyway.

> >  :root {
> > +  --backbutton-urlbar-overlap: 5px;
> 
> We might want to set this to 0px in devedition.css.

Possibly. devedition.css should already be resetting those margins by other means, so this patch shouldn't regress anything.

> ::: browser/themes/linux/searchbar.css
> @@ -9,5 @@
> >  
> > -.searchbar-textbox {
> > -  min-height: 22px;
> > -  background-color: -moz-field;
> > -}
> 
> Tested the height in various customization targets and seems to works
> correctly.
> 
> Is the removal of background-color intentional?

Yes, it's already the default background.

Comment 12

[:Paolo Amadini]

Comment on attachment 8637192 [details] [diff] [review]
patch v2

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

(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to :Paolo Amadini from comment #10)
> > I've tested the height on Windows and Linux and the margin is still one
> > pixel too small (height is two pixels less in total). I'd like to test a new
> > version before landing so clearing the review flag for now.
> 
> I honestly don't think this is worth worrying about. I'm not sure why you
> think it's important for the location bar to have the exact same height in
> popup windows and non-popup windows. The popup UI looks nothing like the
> full browser window UI anyway.

I think having two versions of the textbox with different heights does create additional work for the user experience team and for our engineering team, but I agree with the updated padding this isn't a usability regression. I can handle that in a follow-up as needed.

> > We might want to set this to 0px in devedition.css.
> 
> Possibly. devedition.css should already be resetting those margins by other
> means, so this patch shouldn't regress anything.

The Developer Edition has an extra margin on the left since there is no overlap space, it was probably difficult to get rid of it before, but thanks to the refactoring it's easier so I can fix that in a follow-up as well.

Thanks!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2bb4fef68522

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bb4fef68522