Closed Bug 1401009 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(4 files)

Attached image 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.
Priority: -- → P3
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-
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+
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
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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+
Attached 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.
Status: RESOLVED → VERIFIED
Let's make sure this works as intended on Beta 57 as well.
Flags: qe-verify+
Attached image 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.
You need to log in before you can comment on or make changes to this bug.