Closed Bug 1185362 Opened 5 years ago Closed 5 years ago

Fix and consolidate margin and padding styling around the identity block

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8635798 - Flags: review?(paolo.mozmail)
Assignee: nobody → dao
Status: NEW → ASSIGNED
(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?
(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.
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.
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?
(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.
(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.
(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.
(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.
Attached patch patch v2Splinter Review
set the top and bottom padding to 2px
Attachment #8635798 - Attachment is obsolete: true
Attachment #8635798 - Flags: review?(paolo.mozmail)
Attachment #8637192 - Flags: review?(paolo.mozmail)
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?
Attachment #8637192 - Flags: review?(paolo.mozmail)
(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 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!
Attachment #8637192 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2bb4fef68522
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
No longer depends on: 1189704
You need to log in before you can comment on or make changes to this bug.