1111094 - Arrowpanel menus on about:newtab and about:home should use the same style as arrowpanels in the main chrome

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: menu inconsistencies

The menus on about:home and about:newtab use a custom styling that is inconsistent with other arrow panels in Firefox. Let's make them use the same styling as the other menus. The developer tools menu is a very good reference for what those menus should look like (see attachment).

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Fixes styling on about:newtab and removes dead code.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8560602 [details] [diff] [review]
Patch

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

::: browser/base/content/newtab/newTab.css
@@ -460,4 @@
>  }
>  
>  .newtab-customize-panel-item,
> -.newtab-search-panel-engine,

Doesn't look dead to me:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js#191

ditto for the image and label after it that you're changing.

@@ +463,3 @@
>  #newtab-search-manage {
> +  padding: 2px 24px;
> +  background-color: hsla(210,4%,10%,0);

All this styling won't work well with high contrast themes and is in the content package instead of skin/themes, so that seems wrong... also, why aren't we making some of this reusable?

@@ +465,5 @@
> +  background-color: hsla(210,4%,10%,0);
> +  border-radius: 2px;
> +  border-width: 1px;
> +  border-style: solid;
> +  border-color: hsla(210,4%,10%,0);

Nit: Why not just use a single border: 1px solid hsla(...); here?

@@ +491,5 @@
>  .newtab-customize-panel-item[selected],
>  .newtab-search-panel-engine[selected] {
> +  background-image: url("chrome://global/skin/menu/shared-menu-check.png");
> +  background-position: center left 4px;
> +  background-repeat: no-repeat;

Why touch this at all?

Comment 3

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8560602 [details] [diff] [review]
> Patch
> 
> Review of attachment 8560602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/newtab/newTab.css
> @@ -460,4 @@
> >  }
> >  
> >  .newtab-customize-panel-item,
> > -.newtab-search-panel-engine,
> 
> Doesn't look dead to me:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> search.js#191
> 
> ditto for the image and label after it that you're changing.
Will restore that code then.

> @@ +463,3 @@
> >  #newtab-search-manage {
> > +  padding: 2px 24px;
> > +  background-color: hsla(210,4%,10%,0);
> 
> All this styling won't work well with high contrast themes and is in the
> content package instead of skin/themes, so that seems wrong... also, why
> aren't we making some of this reusable?
The panel related styling was in browser/base, so I decided to stick with the same file, but I'll move the styling to themes/ in that case. For high contrast, should I add a media query ? Also, I'm not sure how I can make this code reusable for the australis panels + the new tab page + other content pages... 

> @@ +465,5 @@
> > +  background-color: hsla(210,4%,10%,0);
> > +  border-radius: 2px;
> > +  border-width: 1px;
> > +  border-style: solid;
> > +  border-color: hsla(210,4%,10%,0);
> 
> Nit: Why not just use a single border: 1px solid hsla(...); here?
Will do.

> @@ +491,5 @@
> >  .newtab-customize-panel-item[selected],
> >  .newtab-search-panel-engine[selected] {
> > +  background-image: url("chrome://global/skin/menu/shared-menu-check.png");
> > +  background-position: center left 4px;
> > +  background-repeat: no-repeat;
> 
> Why touch this at all?
Because otherwise the shorthand property will override background-color, which is definitively not wanted.

Comment 4

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Comment on attachment 8560602 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8560602 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/base/content/newtab/newTab.css
> > @@ -460,4 @@
> > >  }
> > >  
> > >  .newtab-customize-panel-item,
> > > -.newtab-search-panel-engine,
> > 
> > Doesn't look dead to me:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> > search.js#191
> > 
> > ditto for the image and label after it that you're changing.
> Will restore that code then.
> 
> > @@ +463,3 @@
> > >  #newtab-search-manage {
> > > +  padding: 2px 24px;
> > > +  background-color: hsla(210,4%,10%,0);
> > 
> > All this styling won't work well with high contrast themes and is in the
> > content package instead of skin/themes, so that seems wrong... also, why
> > aren't we making some of this reusable?
> The panel related styling was in browser/base, so I decided to stick with
> the same file, but I'll move the styling to themes/ in that case. For high
> contrast, should I add a media query 

We should mimic what is being done by :Paenglab in bug 1079098 (and incorporate Philipp's feedback inasmuch as it applies here).

> ? Also, I'm not sure how I can make
> this code reusable for the australis panels + the new tab page + other
> content pages... 

If it's in browser/themes, seems like we could create a panel styling file under shared/ and use CSS variables or %defines for the hover and background colors and the border-radius stuff, and %include that file in the places that use it.

Might be something for a followup if it gets too involved?

> > @@ +491,5 @@
> > >  .newtab-customize-panel-item[selected],
> > >  .newtab-search-panel-engine[selected] {
> > > +  background-image: url("chrome://global/skin/menu/shared-menu-check.png");
> > > +  background-position: center left 4px;
> > > +  background-repeat: no-repeat;
> > 
> > Why touch this at all?
> Because otherwise the shorthand property will override background-color,
> which is definitively not wanted.

OK. Now that I'm looking at it again, this is probably broken in RTL mode... can we check/fix that while we're here?