1402290 - Notification bar text is vertically misaligned

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[roxleitan]

Attachment: Notification warning bar.png

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170921220243

[Affected versions]: 
Nightly 57.0a1

[Affected platforms]:
Windows 10 x64, Ubuntu 16.04 x64, Mac OS X 10.12

[Steps to reproduce]:
1.Launch Nightly 57.0a1 with a new profile.
2.Install an ad blocker add-on
3.Navigate to http://www.popuptest.com/popuptest1.html

[Expected result]:
The notification bar should match the specifications:
https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252114

[Actual result]:
The notification bar does not match the specs. (please see the attachment)

Comment 1

[roxleitan]

Attachment: Notification bar.png

In this attachment can be observed the differences between Expected and Actual results:
- notification icon is different
- Learn more button is missing
- One "button" is missing
- Different text size

Comment 2

[Dale Harvey (:daleharvey)]

Attachment: Bug 1402290 - Update notification alignment.

Review commit: https://reviewboard.mozilla.org/r/187714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187714/

Comment 3

[Dale Harvey (:daleharvey)]

I wasnt sure where to put the .notification-button styling, inside notification.inc.css it doesnt have any effect on the button, notification > button is whats giving that the current styling but didnt want to change it globally

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

https://reviewboard.mozilla.org/r/187714/#review193042

::: toolkit/themes/linux/global/global.css:280
(Diff revision 1)
>  
> +.notification-button {
> +  border-radius: 0;
> +  padding: 6px 10px;
> +  margin-inline-end: 12px;
> +}

This looks completely out of place and I'm not even sure what problem you're trying to fix here. Please revert.

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187714/diff/1-2/

Comment 6

[Dale Harvey (:daleharvey)]

I wasnt sure whether to only fix the alignment or to try to get it to look like the spec, right now the buttons are rounded and bar is much smaller which the notification-button changes fixed

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

https://reviewboard.mozilla.org/r/187714/#review193062

::: toolkit/themes/shared/notification.inc.css:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  notification {
>    padding: 2px 0 3px;

Your earlier patch evened out this padding, which made sense to me. Why did you revert this?

Comment 8

[Dale Harvey (:daleharvey)]

> Your earlier patch evened out this padding, which made sense to me. Why did you revert this?

Its fairly subtle but without the extra height if felt like the description was too low with even padding, with 2px the cap height (N) is exactly centered, but the ascenders are taller than the cap height so I think it can go either way, is your call

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: Screen Shot 2017-10-10 at 10.19.30.png

2px on the left, 3px on the right

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

https://reviewboard.mozilla.org/r/187714/#review193140

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54c3651423b2
Update notification alignment. r=dao

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

Approval Request Comment
[Feature/Bug causing the regression]: Photon polish
[User impact if declined]: Slightly misaligned text
[Is this code covered by automated tests?]: nope
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: nope
[Why is the change risky/not risky?]: very minor css change to text alignment
[String changes made/needed]:

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/54c3651423b2

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916575 [details]
Bug 1402290 - Update notification alignment.

Photon polish, Beta57+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2220c5f79afb

Comment 16

[Alexandru Simonca [:asimonca], Release Desktop QA]

Verified fixed in 57.0b9 on Windows 10 x64, macOS 10.13.1 and Ubuntu 16.04 x64.

Comment 17

[roxleitan]

Verified fixed using the latest Nightly 58.0a1 (2017-10-18) on Windows 10 x64, macOS 10.12 and Ubuntu 16.04 x64.