Closed Bug 1106239 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.1
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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
Attachment #8530502 - Flags: review?(felipc)
Flags: qe-verify-
Flags: firefox-backlog+
Iteration: --- → 37.1
Points: --- → 3
Attachment #8530502 - Flags: review?(edilee)
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.)
Attachment #8530502 - Flags: review?(edilee)
Attachment #8530502 - Flags: review?(adw)
Attachment #8530502 - Flags: feedback+
(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.
Attachment #8530502 - Flags: review?(felipc) → review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0778496d34f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
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
Attachment #8530502 - Flags: approval-mozilla-beta?
Attachment #8530502 - Flags: approval-mozilla-aurora?
Attachment #8530502 - Flags: approval-mozilla-beta?
Attachment #8530502 - Flags: approval-mozilla-beta+
Attachment #8530502 - Flags: approval-mozilla-aurora?
Attachment #8530502 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.