1401009 - disableHighlight in PopupNotifications doesn't work properly for more than one button

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P3)

Description

[Mike Kaply [:mkaply]]

Attachment: Screenshot of problem

The new disableHighlight feature added in bug 1377006 doesn't work properly when there is more than one button.

See attachment.

Solution should be to add a border, but it will be on either the left or right depending on Windows/Mac.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1401009 - Add border between notification buttons when highlight is disabled.

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

Comment 2

[Mike Kaply [:mkaply]]

Because the patch is platform specific (left or right border), I had to put it in the platform popupnotification css files.

Not sure if there is a way to do this in a platform independent way.

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

https://reviewboard.mozilla.org/r/182840/#review188738

All these rules should be border-inline-{start,end} instead, to account for rtl. Putting them inside the platform specific files sounds fine to me.

Comment 4

[Mike Kaply [:mkaply]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

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

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

https://reviewboard.mozilla.org/r/182840/#review188948

Thank you!

Comment 6

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 3 changes to 3 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev d9274643bde4 needs "Bug N" or "No bug" in the commit message.
remote: Michael Kaply <[e-mail]>
remote: Bug #1401009 - Add border between notification buttons when highlight is disabled. r=johannh
remote: 
remote: MozReview-Commit-ID: 4YGdUZstiDk
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a44a81c459ab
Add border between notification buttons when highlight is disabled. r=johannh

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/a44a81c459ab

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1377006
[User impact if declined]: Vertical line missing in popup notification.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Add a search engine using the new is_default API
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Changes CSS for this one case
[String changes made/needed]: None

This is a purely visual fix that would be nice to have. Very low risk.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911366 [details]
Bug 1401009 - Add border between notification buttons when highlight is disabled.

Polish the UI, taking it.
Should be in 57b4, gtb later today

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2399e64d6af2

Comment 13

[marius.santa]

Attachment: line separator.jpg

This bug is verified on Firefox 58.0a1 (20170927100120) under Windows 10 64-bit and Mac OS X 10.12.3.

Please see the attached screenshot.

Comment 14

[Andrei Vaida [:avaida]]

Let's make sure this works as intended on Beta 57 as well.

Comment 15

[marius.santa]

Attachment: separator correct.PNG

This bug is verified on Firefox 57.0b13 (20171030163911) under Windows 10 64-bit and Mac OS X 10.12.3.

Please see the attached screenshot.