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

VERIFIED FIXED in Firefox 57

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
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.
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Comment 2

2 years ago
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 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.
Attachment #8911366 - Flags: review?(jhofmann) → review-
Comment hidden (mozreview-request)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
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!
Attachment #8911366 - Flags: review?(jhofmann) → review+

Comment 6

2 years ago
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 <mozilla@kaply.com>
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 hidden (mozreview-request)

Comment 8

2 years ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/a44a81c459ab
Add border between notification buttons when highlight is disabled. r=johannh
https://hg.mozilla.org/mozilla-central/rev/a44a81c459ab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 10

2 years ago
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.
Attachment #8911366 - Flags: approval-mozilla-beta?
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
Attachment #8911366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 13

2 years ago
Posted image 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.

Updated

2 years ago
Status: RESOLVED → VERIFIED
Let's make sure this works as intended on Beta 57 as well.
Flags: qe-verify+

Comment 15

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.