Closed Bug 1106569 Opened 7 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: ntim, Unassigned)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files, 5 obsolete files)

No description provided.
This makes no styling changes, and only moves the styling to shared
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8530888 - Flags: review?(dolske)
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)
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)
Attachment #8530902 - Flags: review?(dao)
I also took care of high contrast when I updated the styling.
Uploaded wrong patch as part 2.
Attachment #8530902 - Attachment is obsolete: true
Attachment #8530902 - Flags: review?(dao)
Attachment #8530905 - Flags: review?(dao)
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
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.
(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.
Fixed issue florian pointed out.
Attachment #8530912 - Flags: review?(dao)
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-
(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.
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+
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)
(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)
Whiteboard: [fixed-in-fx-team]
(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.
(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
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Whiteboard: [fxsearch]
Priority: -- → P3
Tim, I guess we can probably close it now, right?
Flags: needinfo?(ntim.bugs)
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

I think we're good here.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(dao+bmo)
Keywords: leave-open
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.