1106569 - Search dropdown footer and open search items have an inconsistent styling with the arrow panels

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect, P3)

Comment 1

[Tim Nguyen :ntim]

Attachment: Part 1 : Move one-off search styling to browser/themes/shared

This makes no styling changes, and only moves the styling to shared

Comment 2

[Tim Nguyen :ntim]

Attachment: Part 1 : Move one-off search styling to shared directory (v1.1)

Forgot to include the shared file in the previous patch

Comment 3

[Tim Nguyen :ntim]

Attachment: Part 1 : Move one-off search styling to shared directory (v1.2)

Forgot to pre-process the files in my previous patch.

Comment 4

[Tim Nguyen :ntim]

Attachment: Part 2 : Styling updates to one-off panel

Comment 5

[Tim Nguyen :ntim]

Attachment: High contrast screenshot

I also took care of high contrast when I updated the styling.

Comment 6

[Tim Nguyen :ntim]

Attachment: Part 2 : Styling updates to one-off panel (v2)

Uploaded wrong patch as part 2.

Comment 7

[Tim Nguyen :ntim]

I've only tried the patches on Windows, so if anyone can try it on Linux and OSX, that would be awesome.

Try build :
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4284b0b41ad3
or
https://tbpl.mozilla.org/?tree=Try&rev=4284b0b41ad3

Comment 8

[Tim Nguyen :ntim]

Cleaner try build (canceled the dirty ones) :
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=93e8b23fde24

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8530905 [details] [diff] [review]
Part 2 : Styling updates to one-off panel (v2)

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

::: browser/themes/shared/searchbar.inc.css
@@ +123,5 @@
> +
> +.addengine-item:hover,
> +.search-setting-button:hover {
> +  background-color: hsla(210,4%,10%,.15);
> +  border-top-color: 1px solid hsla(210,4%,10%,.14);

I don't think "1px solid" should be in border-top-color.

@@ +129,5 @@
> +
> +.addengine-item:active,
> +.search-setting-button:active {
> +  background-color: hsla(210,4%,10%,.19);
> +  border-top-color: 1px solid hsla(210,4%,10%,.14);

same here.

Comment 10

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Comment on attachment 8530905 [details] [diff] [review]
> Part 2 : Styling updates to one-off panel (v2)
> 
> Review of attachment 8530905 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/searchbar.inc.css
> @@ +123,5 @@
> > +
> > +.addengine-item:hover,
> > +.search-setting-button:hover {
> > +  background-color: hsla(210,4%,10%,.15);
> > +  border-top-color: 1px solid hsla(210,4%,10%,.14);
> 
> I don't think "1px solid" should be in border-top-color.
> 
> @@ +129,5 @@
> > +
> > +.addengine-item:active,
> > +.search-setting-button:active {
> > +  background-color: hsla(210,4%,10%,.19);
> > +  border-top-color: 1px solid hsla(210,4%,10%,.14);
> 
> same here.

Whoops, thanks ! Gonna remove that.

Comment 11

[Tim Nguyen :ntim]

Attachment: Part 2 : Styling updates to one-off panel (v2.1)

Fixed issue florian pointed out.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8530912 [details] [diff] [review]
Part 2 : Styling updates to one-off panel (v2.1)

> .search-panel-current-engine {
>   border-top: none !important;
>-  border-bottom: 1px solid #ccc;
>+  border-bottom: 1px solid hsla(210,4%,10%,.12);
>   -moz-box-align: center;
> }

Use threedshadow?

>+.search-setting-button,
> .addengine-item {
>   -moz-appearance: none;
>-  border: none;
>-  height: 32px;
>+  border-bottom: none;
>+  border-left: none;
>+  border-right: none;
>+  -moz-border-top-colors: none;

Why did you replace border: none; with this?

Comment 13

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8530912 [details] [diff] [review]
> Part 2 : Styling updates to one-off panel (v2.1)
> 
> > .search-panel-current-engine {
> >   border-top: none !important;
> >-  border-bottom: 1px solid #ccc;
> >+  border-bottom: 1px solid hsla(210,4%,10%,.12);
> >   -moz-box-align: center;
> > }
> 
> Use threedshadow?
It looks way too dark, that corresponds to #A0A0A0, and I'm aiming a color similar to #ddd. Also, hsla values don't break high contrast.

> >+.search-setting-button,
> > .addengine-item {
> >   -moz-appearance: none;
> >-  border: none;
> >-  height: 32px;
> >+  border-bottom: none;
> >+  border-left: none;
> >+  border-right: none;
> >+  -moz-border-top-colors: none;
> 
> Why did you replace border: none; with this?
It was used for the .search-setting-button rules, but since border: none; works fine as well, I'm gonna restore that.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > Comment on attachment 8530912 [details] [diff] [review]
> > Part 2 : Styling updates to one-off panel (v2.1)
> > 
> > > .search-panel-current-engine {
> > >   border-top: none !important;
> > >-  border-bottom: 1px solid #ccc;
> > >+  border-bottom: 1px solid hsla(210,4%,10%,.12);
> > >   -moz-box-align: center;
> > > }
> > 
> > Use threedshadow?
> It looks way too dark, that corresponds to #A0A0A0,

How about threedlightshadow?

> Also, hsla values don't break high contrast.

Well, I don't actually see the border in attachment 8530903 [details]. Maybe I'm looking at the wrong thing.

Comment 15

[Tim Nguyen :ntim]

Attachment: Part 2 : Styling updates to one-off panel (v2.2)

With threedlightshadow as border color.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 8530892 [details] [diff] [review]
Part 1 : Move one-off search styling to shared directory (v1.2)

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

r+ assuming this was just moving identical code into searchbar.inc.css.

Comment 17

[Tim Nguyen :ntim]

Checkin-needed for part 1

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8530892 [details] [diff] [review]
Part 1 : Move one-off search styling to shared directory (v1.2)

(In reply to Justin Dolske [:Dolske] from comment #16)
> Comment on attachment 8530892 [details] [diff] [review]
> Part 1 : Move one-off search styling to shared directory (v1.2)
> 
> Review of attachment 8530892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming this was just moving identical code into searchbar.inc.css.

No, they were not identical (it seems the version that has been used here as the 'shared' version is the Windows one; I suspect this means there'll likely be no icon shown on Linux anymore).

Comment 20

[Florian Quèze [:florian]]

Here is the diff between Windows (-) and Linux (+) for the relevant part of searchbar.css:

 .searchbar-search-button-container {
   -moz-box-align: center;
-  padding: 3px 4px;
+  padding: 2px 3px;
   -moz-padding-end: 2px;
 }
 
@@ -129,8 +118,8 @@
   font-weight: normal;
   background-color: rgb(245, 245, 245);
   border-top: 1px solid #ccc;
-  margin: 0;
-  padding: 3px 6px;
+  margin: 0 1px;
+  padding: 3px 5px;
   color: #666;
 }
 
@@ -186,6 +175,8 @@
 }
 
 .searchbar-engine-one-off-item > .button-box > .button-icon {
+  display: -moz-box;
+  -moz-margin-end: 0;
   width: 16px;
   height: 16px;
 }
@@ -230,7 +221,7 @@
 }
 
 .search-panel-tree > .autocomplete-treebody::-moz-tree-cell {
-  -moz-padding-start: 15px;
+  -moz-padding-start: 16px;
   border-top: none !important;
 }


The diff between Windows and Mac is larger because:
- the Mac CSS was written first, so it doesn't contain some of the rules that were written for Windows and are not needed for Mac but would be OK to share (they would be harmless).
- there are margin/padding tweaks (like between Windows and Linux).
- the panel has a border radius on Mac, and only on Mac.
- the HiDPI stuff that your existing patch didn't remove is showing in the diff.

Comment 21

[Florian Quèze [:florian]]

(In reply to Carsten Book [:Tomcat] from comment #18)
> https://hg.mozilla.org/integration/fx-team/rev/bfee51abf471

Can you please back this out before it gets included in a nightly?

Justin, Tim had mentioned in comment 7 that the patches had only been tested on Windows :-/.

Comment 22

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> (In reply to Carsten Book [:Tomcat] from comment #18)
> > https://hg.mozilla.org/integration/fx-team/rev/bfee51abf471
> 
> Can you please back this out before it gets included in a nightly?
> 
> Justin, Tim had mentioned in comment 7 that the patches had only been tested
> on Windows :-/.

Actually, I let it land on Nightly so it can bake there and so regressions can be fixed early.

Comment 23

[Florian Quèze [:florian]]

(In reply to Tim Nguyen [:ntim] from comment #22)

> Actually, I let it land on Nightly so it can bake there and so regressions
> can be fixed early.

At the time you added the checkin-needed keyword, nobody had mentioned yet why it would visibly regress on Mac and Linux. I still think we should backout, and land something that works.

By the way, thanks for cleaning this up! Merging the duplicated content of these files is definitely something I wanted to do, but it wasn't really possible while we were hacking quickly on beta.

Comment 24

[Carsten Book [:Tomcat]]

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> (In reply to Tim Nguyen [:ntim] from comment #22)
> 
> > Actually, I let it land on Nightly so it can bake there and so regressions
> > can be fixed early.
> 
> At the time you added the checkin-needed keyword, nobody had mentioned yet
> why it would visibly regress on Mac and Linux. I still think we should
> backout, and land something that works.
> 
> By the way, thanks for cleaning this up! Merging the duplicated content of
> these files is definitely something I wanted to do, but it wasn't really
> possible while we were hacking quickly on beta.

backed out in https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=9a4487ecb029

Comment 25

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> Here is the diff between Windows (-) and Linux (+) for the relevant part of
> searchbar.css:
> 
>  .searchbar-search-button-container {
>    -moz-box-align: center;
> -  padding: 3px 4px;
> +  padding: 2px 3px;
>    -moz-padding-end: 2px;
>  }
>  
> @@ -129,8 +118,8 @@
>    font-weight: normal;
>    background-color: rgb(245, 245, 245);
>    border-top: 1px solid #ccc;
> -  margin: 0;
> -  padding: 3px 6px;
> +  margin: 0 1px;
> +  padding: 3px 5px;
>    color: #666;
>  }
>  
> @@ -186,6 +175,8 @@
>  }
>  
>  .searchbar-engine-one-off-item > .button-box > .button-icon {
> +  display: -moz-box;
> +  -moz-margin-end: 0;
>    width: 16px;
>    height: 16px;
>  }
> @@ -230,7 +221,7 @@
>  }
>  
>  .search-panel-tree > .autocomplete-treebody::-moz-tree-cell {
> -  -moz-padding-start: 15px;
> +  -moz-padding-start: 16px;
>    border-top: none !important;
>  }
> 
> 
> The diff between Windows and Mac is larger because:
> - the Mac CSS was written first, so it doesn't contain some of the rules
> that were written for Windows and are not needed for Mac but would be OK to
> share (they would be harmless).
> - there are margin/padding tweaks (like between Windows and Linux).
> - the panel has a border radius on Mac, and only on Mac.
> - the HiDPI stuff that your existing patch didn't remove is showing in the
> diff.
For the padding tweaks, 1 or 2px of difference shouldn't break the layout if it's coded properly. The HDPI stuff hasn't been moved (even though it should), because the HDPI files are not packaged for Windows and Linux. The panel border-radius was kept Mac only as wanted.

Comment 26

[Florian Quèze [:florian]]

(In reply to Tim Nguyen [:ntim] from comment #25)

> For the padding tweaks, 1 or 2px of difference shouldn't break the layout if
> it's coded properly.

This is a strong assumption. In the area of the one-off buttons, the layout has some calculations made in JS, so being off by just one pixel can make us wrap things improperly (which makes the UI look like attachment 8527957 [details]).

> The panel border-radius was kept Mac only as wanted.

Attachment 8530892 [details] [diff] removed 2 (out of 4) border-radius rules from the Mac CSS.

Comment 27

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> (In reply to Tim Nguyen [:ntim] from comment #25)
> 
> > For the padding tweaks, 1 or 2px of difference shouldn't break the layout if
> > it's coded properly.
> 
> This is a strong assumption. In the area of the one-off buttons, the layout
> has some calculations made in JS, so being off by just one pixel can make us
> wrap things improperly (which makes the UI look like attachment 8527957 [details]
> [details]).
I think calculations should be done via css calc or flexbox or via similar means. It's less css tweaks like this, and also works.

> > The panel border-radius was kept Mac only as wanted.
> 
> Attachment 8530892 [details] [diff] removed 2 (out of 4) border-radius rules
> from the Mac CSS.

Ah, looks like I forgot to add overflow hidden on the popup (which should hide overflowing sharp corners).

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8532695 [details] [diff] [review]
Part 2 : Styling updates to one-off panel (v2.2)

It seems that we need an updated part 1 before part 2 can be reviewed?

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Tim, I guess we can probably close it now, right?

Comment 31

[Tim Nguyen :ntim]

While bug 1301758 and bug 1298659 do most of the work, the existing patch adds an :active state to the one-off buttons and the one-off footer. Also, the font of the arrow panels is still bigger than the search popup font. I don't know if these are still things that we still want fixed though.

Comment 32

[Dão Gottwald [:dao]]

I think we're good here.