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

NEW
Unassigned
(NeedInfo from)

Status

()

defect
P3
normal
5 years ago
6 months ago

People

(Reporter: ntim, Unassigned, NeedInfo)

Tracking

({leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
No description provided.
Reporter

Comment 1

5 years ago
This makes no styling changes, and only moves the styling to shared
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8530888 - Flags: review?(dolske)
Reporter

Updated

5 years ago
Reporter

Comment 2

5 years ago
Forgot to include the shared file in the previous patch
Attachment #8530888 - Attachment is obsolete: true
Attachment #8530888 - Flags: review?(dolske)
Attachment #8530890 - Flags: review?(dolske)
Reporter

Comment 3

5 years ago
Forgot to pre-process the files in my previous patch.
Attachment #8530890 - Attachment is obsolete: true
Attachment #8530890 - Flags: review?(dolske)
Attachment #8530892 - Flags: review?(dolske)
Reporter

Comment 4

5 years ago
Attachment #8530902 - Flags: review?(dao)
Reporter

Comment 5

5 years ago
I also took care of high contrast when I updated the styling.
Reporter

Comment 6

5 years ago
Uploaded wrong patch as part 2.
Attachment #8530902 - Attachment is obsolete: true
Attachment #8530902 - Flags: review?(dao)
Attachment #8530905 - Flags: review?(dao)
Reporter

Comment 7

5 years ago
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
Reporter

Comment 8

5 years ago
Cleaner try build (canceled the dirty ones) :
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=93e8b23fde24
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.
Reporter

Comment 10

5 years ago
(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.
Reporter

Comment 11

5 years ago
Fixed issue florian pointed out.
Attachment #8530912 - Flags: review?(dao)
Reporter

Updated

5 years ago
Attachment #8530905 - Attachment is obsolete: true
Attachment #8530905 - Flags: review?(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?
Attachment #8530912 - Flags: review?(dao) → review-
Reporter

Comment 13

5 years ago
(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.
(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.
Reporter

Comment 15

5 years ago
With threedlightshadow as border color.
Attachment #8530912 - Attachment is obsolete: true
Attachment #8532695 - Flags: review?(dao)
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.
Attachment #8530892 - Flags: review?(dolske) → review+
Reporter

Comment 17

5 years ago
Checkin-needed for part 1
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).
Attachment #8530892 - Flags: review-
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.
(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 :-/.
Flags: needinfo?(cbook)
Reporter

Comment 22

5 years ago
(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.
(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.
(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
Flags: needinfo?(cbook)
Reporter

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
Reporter

Comment 25

5 years ago
(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.
(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.
Reporter

Comment 27

5 years ago
(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 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?
Attachment #8532695 - Flags: review?(dao)
Duplicate of this bug: 1105750
Reporter

Updated

5 years ago
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Whiteboard: [fxsearch]
Priority: -- → P3
Tim, I guess we can probably close it now, right?
Flags: needinfo?(ntim.bugs)
Reporter

Comment 31

6 months ago
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.
Flags: needinfo?(ntim.bugs) → needinfo?(dao+bmo)
Summary: Search dropdown footer and open search items have an inconsistent styling with the australis panels → Search dropdown footer and open search items have an inconsistent styling with the arrow panels
You need to log in before you can comment on or make changes to this bug.