1106239 - Fix up the styling for about:home's and about:newtab's search panel and about:newtab's customize panel

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Fixes:

- use the child selector
- use shorthand properties
- remove the hardcoded background as it doesn't match the panel arrow (the severity of the mismatch depends on the OS and OS theme)
- remove the hardcoded text color as it isn't guaranteed to be visible on the standard arrow panel background (we could use graytext but I think this is too faint and looks broken)
- remove pointless -moz-box-align: center;
- remove the redundant abouthome-search-panel-item class
- don't overload the newtab-search-panel-engine class

Comment 1

[Ed Lee :Mardak]

Comment on attachment 8530502 [details] [diff] [review]
patch

>+++ b/browser/base/content/newtab/newTab.css
> .newtab-customize-panel-item:not(:last-child),
>-.newtab-search-panel-engine:not(:last-child) {
>-  border-bottom: 1px solid #ccc;
>+.newtab-search-panel-engine {
>+  border-bottom: 1px solid threedshadow;
> }
Was the removal of :not(:last-child) for search-panel-engine intentional? It would cause the border to appear at the bottom now. And I'm not familiar with the recent search selection changes, but it seems like we always only show just one item anyway with useNewUI, so we should be able to just remove this styling for search panel.

> - use the child selector
Just to be clear, is this to optimize css matching to be more specific and faster? This isn't to have a more specific/"important" rule overriding a less specific one.

f+ in general and for the newtab-customize stuff which just copies the search panel styling. (I don't have official r+, so deferring to adw.)

Comment 2

[Dão Gottwald [:dao]]

(In reply to Ed Lee :Mardak from comment #1)
> >+++ b/browser/base/content/newtab/newTab.css
> > .newtab-customize-panel-item:not(:last-child),
> >-.newtab-search-panel-engine:not(:last-child) {
> >-  border-bottom: 1px solid #ccc;
> >+.newtab-search-panel-engine {
> >+  border-bottom: 1px solid threedshadow;
> > }
> Was the removal of :not(:last-child) for search-panel-engine intentional?

Yes, it's related to my last point, "don't overload the newtab-search-panel-engine class". The last child always is #newtab-search-manage, and with my patch it doesn't get the "engine" class.

> > - use the child selector
> Just to be clear, is this to optimize css matching to be more specific and
> faster? This isn't to have a more specific/"important" rule overriding a
> less specific one.

Exactly.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8530502 [details] [diff] [review]
patch

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0778496d34f5

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8530502 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1101122, bug 1101147
[User impact if declined]: panel background doesn't match the panel arrow
[Describe test coverage new/current, TBPL]: none
[Risks and why]: straightforward, mostly CSS-only fixes, low risk
[String/UUID change made/needed]: none

Comment 7

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ba461354557
https://hg.mozilla.org/releases/mozilla-beta/rev/8c1802629a0c