Closed Bug 1050807 Opened 6 years ago Closed 6 years ago

[10.10] Update URL and search bar border/box-shadow for OS X Yosemite

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Mockup: attachment 8440728 [details].
Points: --- → 3
QA Whiteboard: [qa+]
Flags: firefox-backlog+
QA Whiteboard: [qa+]
Flags: qe-verify+
I'm hoping you've managed to set up Yosemite successfully... :-)
Attachment #8479845 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Marco, can you add this, too? :-)
Iteration: --- → 34.3
Flags: needinfo?(mmucci)
Added to IT 34.3
Flags: needinfo?(mmucci)
Blocks: 1052534
Comment on attachment 8479845 [details] [diff] [review]
style urlbar and search box appropriately on on OS X Yosemite,

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

The patch looks fine. A couple notes:
1. If the font-size increases, the boxes don't increase in size and clip the text on the top and bottom.
2. The boxes look to increase in size when the window goes from inactive to active (and decrease in the other direction). It is most noticeable between the search bar and the location bar. I looked at Safari and App Store, and both look to be doing something close, but I can't quite tell if their input boxes change size with the same magnitude.
Attachment #8479845 - Flags: review?(jaws) → review+
Comment on attachment 8479845 [details] [diff] [review]
style urlbar and search box appropriately on on OS X Yosemite,

> @media not all and (-moz-mac-lion-theme) {
>   #urlbar:-moz-window-inactive,
>   .searchbar-textbox:-moz-window-inactive {
>     border-color: @toolbarbuttonInactiveBorderColor@;
>   }
> }
> 
>+@media (-moz-mac-yosemite-theme) {
>+  .searchbar-textbox,
>+  #urlbar {
>+    border: 1px none #dbdbdb;
>+    border-radius: 3px;
>+    height: 24px;
>+    box-shadow: 0px 1px 0 0 #aeaeae, 1px 2px 0 0 #d8d8d8;
>+    background-image: none;
>+  }
>+  .searchbar-textbox:-moz-window-inactive,
>+  #urlbar:-moz-window-inactive {
>+    box-shadow: none;
>+    border-style: solid;
>+  }

Are you sure #dbdbdb is used as the border color for :-moz-window-inactive, as opposed to @toolbarbuttonInactiveBorderColor@ which is specified above with a more specific selector?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> The patch looks fine. A couple notes:
> 1. If the font-size increases, the boxes don't increase in size and clip the
> text on the top and bottom.

Please do not hardcode the height like this.

> 2. The boxes look to increase in size when the window goes from inactive to
> active (and decrease in the other direction).

This is because the border goes from 1px solid to none (where the thickness doesn't matter, the border just isn't rendered at all). This probably needs to be solved differently.
Attachment #8479845 - Flags: review-
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8479845 [details] [diff] [review]
> style urlbar and search box appropriately on on OS X Yosemite,
> 
> > @media not all and (-moz-mac-lion-theme) {
> >   #urlbar:-moz-window-inactive,
> >   .searchbar-textbox:-moz-window-inactive {
> >     border-color: @toolbarbuttonInactiveBorderColor@;
> >   }
> > }
> > 
> >+@media (-moz-mac-yosemite-theme) {
> >+  .searchbar-textbox,
> >+  #urlbar {
> >+    border: 1px none #dbdbdb;
> >+    border-radius: 3px;
> >+    height: 24px;
> >+    box-shadow: 0px 1px 0 0 #aeaeae, 1px 2px 0 0 #d8d8d8;
> >+    background-image: none;
> >+  }
> >+  .searchbar-textbox:-moz-window-inactive,
> >+  #urlbar:-moz-window-inactive {
> >+    box-shadow: none;
> >+    border-style: solid;
> >+  }
> 
> Are you sure #dbdbdb is used as the border color for :-moz-window-inactive,
> as opposed to @toolbarbuttonInactiveBorderColor@ which is specified above
> with a more specific selector?

I sampled it from a native app. So yes, I'm quite sure.

> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > The patch looks fine. A couple notes:
> > 1. If the font-size increases, the boxes don't increase in size and clip the
> > text on the top and bottom.
> 
> Please do not hardcode the height like this.

Please suggest something else, then. Would 2em make you happier? It's not like font size is configurable on OS X...

> > 2. The boxes look to increase in size when the window goes from inactive to
> > active (and decrease in the other direction).
> 
> This is because the border goes from 1px solid to none (where the thickness
> doesn't matter, the border just isn't rendered at all). This probably needs
> to be solved differently.

It doesn't need to be "solved" - this is how the rest of the OS behaves; it's by design. Which Jared actually noted in his comment, but you snipped that part of his review.
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Comment on attachment 8479845 [details] [diff] [review]
> > style urlbar and search box appropriately on on OS X Yosemite,
> > 
> > > @media not all and (-moz-mac-lion-theme) {
> > >   #urlbar:-moz-window-inactive,
> > >   .searchbar-textbox:-moz-window-inactive {
> > >     border-color: @toolbarbuttonInactiveBorderColor@;
> > >   }
> > > }
> > > 
> > >+@media (-moz-mac-yosemite-theme) {
> > >+  .searchbar-textbox,
> > >+  #urlbar {
> > >+    border: 1px none #dbdbdb;
> > >+    border-radius: 3px;
> > >+    height: 24px;
> > >+    box-shadow: 0px 1px 0 0 #aeaeae, 1px 2px 0 0 #d8d8d8;
> > >+    background-image: none;
> > >+  }
> > >+  .searchbar-textbox:-moz-window-inactive,
> > >+  #urlbar:-moz-window-inactive {
> > >+    box-shadow: none;
> > >+    border-style: solid;
> > >+  }
> > 
> > Are you sure #dbdbdb is used as the border color for :-moz-window-inactive,
> > as opposed to @toolbarbuttonInactiveBorderColor@ which is specified above
> > with a more specific selector?
> 
> I sampled it from a native app. So yes, I'm quite sure.

I now see that I misread this comment - sorry. But confusingly, -moz-mac-lion-theme is true on 10.10, and so the media query "@media not all and (-moz-mac-lion-theme)" does not match. So the fact that the selector is less specific is OK.
(In reply to :Gijs Kruitbosch from comment #6)
> > Please do not hardcode the height like this.
> 
> Please suggest something else, then. Would 2em make you happier? It's not
> like font size is configurable on OS X...

No, with 2em it's still fragile, e.g. add-ons could extend the location bar to include other content that might increase the height. I think you'll need to use padding, although I'm not sure off-hand on which part of the location bar it should be set.

> > > 2. The boxes look to increase in size when the window goes from inactive to
> > > active (and decrease in the other direction).
> > 
> > This is because the border goes from 1px solid to none (where the thickness
> > doesn't matter, the border just isn't rendered at all). This probably needs
> > to be solved differently.
> 
> It doesn't need to be "solved" - this is how the rest of the OS behaves;
> it's by design. Which Jared actually noted in his comment, but you snipped
> that part of his review.

Snipped intentionally, as Jared wasn't sure and just from how I read the effect's description it didn't seem intentional...

> I now see that I misread this comment - sorry. But confusingly,
> -moz-mac-lion-theme is true on 10.10, and so the media query "@media not all
> and (-moz-mac-lion-theme)" does not match.

This sounds like a bug? Given how different 10.10 is, it's probably not useful to subsume it under "lion or later". At the very least it seems like a footgun since the media query's name doesn't imply "or later"...
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #6)
> > > Please do not hardcode the height like this.
> > 
> > Please suggest something else, then. Would 2em make you happier? It's not
> > like font size is configurable on OS X...
> 
> No, with 2em it's still fragile, e.g. add-ons could extend the location bar
> to include other content that might increase the height. I think you'll need
> to use padding, although I'm not sure off-hand on which part of the location
> bar it should be set.

The location bar, in my testing, is already the right size here. The problem is that the search bar's size should match. Adjusting just its padding wouldn't fix that for the case you describe. What would you prefer to do here, considering that? Just adjust padding and hope that add-ons will test and adjust padding on the search bar, too, if they change the url-bar?

> > I now see that I misread this comment - sorry. But confusingly,
> > -moz-mac-lion-theme is true on 10.10, and so the media query "@media not all
> > and (-moz-mac-lion-theme)" does not match.
> 
> This sounds like a bug? Given how different 10.10 is, it's probably not
> useful to subsume it under "lion or later". At the very least it seems like
> a footgun since the media query's name doesn't imply "or later"...

Perhaps, but it's certainly something we've relied on lots already - lion was 10.7, and those rules apply on 10.8 (mountain lion) and 10.9 (mavericks) as well, so it's a bit late to change it (and would probably cause bugs). We could rename the media query and do a  mass-search/replace across the tree, but I'm not sure that's valuable. I expect the yosemite query works the same, though, and we are probably still in time to adjust that one without breaking add-ons or other such things? D'you want (me) to file a followup for that?
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #9)
> The location bar, in my testing, is already the right size here. The problem
> is that the search bar's size should match.

Where does this discrepancy come from? Has one of your style adjustments introduced this?

> Adjusting just its padding
> wouldn't fix that for the case you describe. What would you prefer to do
> here, considering that?

We should just let them have different heights in such situations, since AFAIK that's what we already do on Windows, Linux, and other versions of OS X. If we want to change this such that the smaller field would automatically grow to match the taller one, we can discuss this, but it's scope creep for this bug.

> > This sounds like a bug? Given how different 10.10 is, it's probably not
> > useful to subsume it under "lion or later". At the very least it seems like
> > a footgun since the media query's name doesn't imply "or later"...
> 
> Perhaps, but it's certainly something we've relied on lots already - lion
> was 10.7, and those rules apply on 10.8 (mountain lion) and 10.9 (mavericks)
> as well, so it's a bit late to change it (and would probably cause bugs). We
> could rename the media query and do a  mass-search/replace across the tree,
> but I'm not sure that's valuable. I expect the yosemite query works the
> same, though, and we are probably still in time to adjust that one without
> breaking add-ons or other such things? D'you want (me) to file a followup
> for that?

I don't have a strong opinion about this.
Flags: needinfo?(dao)
This works just as well and doesn't set a height.
Attachment #8481278 - Flags: review?(dao)
Attachment #8479845 - Attachment is obsolete: true
Comment on attachment 8481278 [details] [diff] [review]
style urlbar and search box appropriately on on OS X Yosemite,

>+    box-shadow: 0px 1px 0 0 #aeaeae, 1px 2px 0 0 #d8d8d8;

nit: there's another 0px where you can just use 0
Attachment #8481278 - Flags: review?(dao) → review+
w/ nit:
remote:   https://hg.mozilla.org/integration/fx-team/rev/a8d97e40a778
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a8d97e40a778
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
QA Contact: jbecerra → catalin.varga
Verified as fixed using the following environment:

FF 34
Build Id: 20141002004001
OS: Mac Os X 10.10
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.