Closed Bug 1421497 Opened 7 years ago Closed 6 years ago

Combine searchbar.css platform-specific files into one shared file

Categories

(Firefox :: Theme, enhancement)

Product:
Firefox
Component:
Theme
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.
59 bytes, text/x-review-board-request
dao
: review+
Details
Text alignment screenshot
6.59 KB, image/png
Details 
Prerequisite for bug 1417883
Attachment #8933891 - Flags: review?(dao+bmo)
Attachment #8933892 - Flags: review?(dao+bmo)
Tested on Linux and macOS so far.
Also tested on Windows.
Also looks fine on Windows 125% DPI
Comment on attachment 8933891 [details]
Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.

https://reviewboard.mozilla.org/r/204826/#review212540

::: browser/themes/osx/searchbar.css
(Diff revision 6)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -.searchbar-textbox {
> +%include ../shared/searchbar.inc.css
> -  border-radius: 10000px;
> -}

Where did this go?
I don't understand the second patch / what it has to do with this bug...
(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8933891 [details]
> Bug 1421497 - Combine searchbar.css platform-specific files into one shared
> file.
> 
> https://reviewboard.mozilla.org/r/204826/#review212540
> 
> ::: browser/themes/osx/searchbar.css
> (Diff revision 6)
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > -.searchbar-textbox {
> > +%include ../shared/searchbar.inc.css
> > -  border-radius: 10000px;
> > -}
> 
> Where did this go?

This rule is currently dead on macOS, because it is overriden by another CSS rule, so I assumed we didn't want it anymore: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/urlbar-searchbar.inc.css#30
(In reply to Dão Gottwald [::dao] from comment #17)
> I don't understand the second patch / what it has to do with this bug...

It's a nice improvement over the current way of sizing the one-offs, but I'd be fine with removing it from this bug.
Comment on attachment 8933891 [details]
Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.

https://reviewboard.mozilla.org/r/204826/#review217470

I tested this patch on Linux and it makes text in the popup too small.

This would probably easier to review if searchbar.inc.css was based on searchbar.css from Windows or Linux.

::: browser/themes/shared/searchbar.inc.css:82
(Diff revision 7)
>  }
>  
>  .searchbar-engine-one-off-item {
>    -moz-appearance: none;
>    display: inline-block;
> +  border: none;

This shouldn't be needed anymore thanks to bug 1334429.
Attachment #8933891 - Flags: review?(dao+bmo) → review-
Comment on attachment 8933891 [details]
Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.

https://reviewboard.mozilla.org/r/204826/#review217478

(In reply to Dão Gottwald [::dao] from comment #23)
> This would probably easier to review if searchbar.inc.css was based on
> searchbar.css from Windows or Linux.

So I diffed this locally for Linux, and searchbar.inc.css still brings a bunch of changes from Mac. Most of these are probably okay but some look suspicious.
Attachment #8933891 - Flags: review?(dao+bmo)
Comment on attachment 8933891 [details]
Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.

https://reviewboard.mozilla.org/r/204826/#review217504

::: browser/themes/shared/searchbar.inc.css:70
(Diff revision 9)
>  
>  .search-panel-one-offs {
>    margin: 0 !important;
>    border-top: 1px solid var(--panel-separator-color);
>    background-color: var(--arrowpanel-dimmed);
> +  line-height: 0;

Might be worth adding a reference to bug 1108841 here.

::: browser/themes/shared/searchbar.inc.css:129
(Diff revision 9)
>  .searchbar-engine-one-off-item > .button-box > .button-text {
>    display: none;
>  }
>  
>  .searchbar-engine-one-off-item > .button-box > .button-icon {
>    display: -moz-box;

display: -moz-box probably isn't needed anymore as of bug 1302157.

::: browser/themes/shared/searchbar.inc.css:144
(Diff revision 9)
>  
>  .addengine-item {
>    -moz-appearance: none;
>    background-color: transparent;
>    color: inherit;
>    border: none;

background-color and border can probably be removed here.

::: browser/themes/shared/searchbar.inc.css:209
(Diff revision 9)
> -  padding-inline-start: 4px;
> -}
> -
>  .search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> -  padding-inline-start: 5px;
> +  padding-inline-start: 4px;
> +  padding-inline-end: 2px;

Text alignment seems off by two pixels. It was also off before but in the opposite direction...

::: browser/themes/windows/searchbar.css
(Diff revision 9)
> -  border-bottom: 1px solid var(--panel-separator-color);
> -}
> -
> -.searchbar-engine-one-off-item:-moz-focusring {
> -  outline: none;
> -}

Is this not needed anymore?
Attachment #8933891 - Flags: review?(dao+bmo)
> >  .search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> > -  padding-inline-start: 5px;
> > +  padding-inline-start: 4px;
> > +  padding-inline-end: 2px;
> 
> Text alignment seems off by two pixels.
(In reply to Dão Gottwald [::dao] from comment #30)
> Created attachment 8941474 [details]
> Text alignment screenshot
> 
> > >  .search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> > > -  padding-inline-start: 5px;
> > > +  padding-inline-start: 4px;
> > > +  padding-inline-end: 2px;
> > 
> > Text alignment seems off by two pixels.

Your latest patch update makes this worse.
(In reply to Dão Gottwald [::dao] from comment #33)
> (In reply to Dão Gottwald [::dao] from comment #30)
> > Created attachment 8941474 [details]
> > Text alignment screenshot
> > 
> > > >  .search-panel-tree > .autocomplete-treebody::-moz-tree-image {
> > > > -  padding-inline-start: 5px;
> > > > +  padding-inline-start: 4px;
> > > > +  padding-inline-end: 2px;
> > > 
> > > Text alignment seems off by two pixels.
> 
> Your latest patch update makes this worse.

More precisely, it moves the suggestions further away from the header text. Part of the problem is that the header text and input field aren't aligned either.
Comment on attachment 8933891 [details]
Bug 1421497 - Combine searchbar.css platform-specific files into one shared file.

https://reviewboard.mozilla.org/r/204826/#review217572
Attachment #8933891 - Flags: review?(dao+bmo) → review+
Comment on attachment 8933892 [details]
Bug 1421497 - Change engines per row to be a CSS variable.

https://reviewboard.mozilla.org/r/204828/#review217574

This code has a history of being fragile. Let's not touch it in this bug.
Attachment #8933892 - Flags: review?(dao+bmo)
Attachment #8933892 - Attachment is obsolete: true
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/56528e7c9dac
Combine searchbar.css platform-specific files into one shared file. r=dao
https://hg.mozilla.org/mozilla-central/rev/56528e7c9dac
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: