Closed
Bug 1218789
Opened 7 years ago
Closed 7 years ago
Close button and gear button are misaligned in notifications
Categories
(Core :: DOM: Push Notifications, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: phlsa, Assigned: jaws)
References
Details
Attachments
(4 files, 1 obsolete file)
34.68 KB,
image/png
|
Details | |
65.01 KB,
image/png
|
Details | |
46.88 KB,
image/png
|
Details | |
5.76 KB,
patch
|
jaws
:
review+
Gijs
:
feedback+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The close button and the gear button in a notification are not aligned correctly. Needs UX input on how exactly they should be aligned
Flags: needinfo?(philipp)
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Additional problems (see attachments) - On mouseover and click of gear icon, the highlight alternates between a circle and a dotted line square. - the highlighted gray shade of the dismiss (x) and gear icon are different.
Comment 3•7 years ago
|
||
Reporter | ||
Comment 4•7 years ago
|
||
OK, I just did the measurements: - The origin line should move down by 5px - The gear should move to the right by 5px I also agree with the issues that Bill has raised. The background of the gear hover effect should match the close button hover effect from the Windows 10 close asset.
Flags: needinfo?(philipp)
Assignee | ||
Comment 5•7 years ago
|
||
This patch fixes the alignment of the button, the positioning of the origin text (to be lower as requested in comment #4), and also removes the focusring as requested in comment #2. I also added a new style to utilities.svg to have an inverted gear. I tried using `filter: invert(100%)` for that rule but it just made the gear appear very dull-grey, not the inverse of `fill: #424f5a`. I used #ddd because #fff was extremely strong and the extra high contrast caused the gear to appear visually larger when hovered (dominating the hover and active background colors). In the future we could change the selector for `use[id$="-inverted"]` to use the *= attribute selector to support "utilities-inverted-native" but we don't have a use for that now so I left the selector as $=
Updated•7 years ago
|
status-firefox44:
--- → affected
Comment 6•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > and also removes the focusring as requested in comment #2. The focus ring wasn't the main problem in comment #2, it was that the border-radius only applied on :hover, not :active. For accessibility I'm not sure if we should remove the ring.
Comment 7•7 years ago
|
||
Comment on attachment 8684412 [details] [diff] [review] Patch Review of attachment 8684412 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Not sure what to do about the focus ring. I think we normally only want it when keyboard navigation was used but this patch seems to remove it for all methods of focus.
Attachment #8684412 -
Flags: review?(MattN+bmo)
Attachment #8684412 -
Flags: review+
Attachment #8684412 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 8•7 years ago
|
||
Comment on attachment 8684412 [details] [diff] [review] Patch Review of attachment 8684412 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/alert-common.css @@ +126,5 @@ > + background-color: rgb(102,102,102); > +} > + > +#alertSettings:-moz-focusring > .button-box { > + border-color: transparent; I'm not familiar with this code so I don't understand why this is necessary. moz-focusring is only drawn when focus ring drawing is enabled, which if all we're doing is interacting with this thing via the mouse, it shouldn't be. Removing this style is going to cause issues for bug 1210833. But also, I would have expected it to have been using outline, not border... Do we know why this is currently showing up? Is something telling the focus manager keyboard focus moved, or something?
Attachment #8684412 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 9•7 years ago
|
||
While writing this patch I wrote separate styles that set different colors for border-color and outline-color, and confirmed that border was being used to draw this focusring. For the time being, I put in a somewhat ugly hack that only removes the border if the button was clicked on. "focus" will come before "click" since it is triggered via mousedown and click comes after mouseup. See also https://groups.google.com/a/chromium.org/forum/#!topic/chromium-reviews/nmtkCoWR5qk
Attachment #8684412 -
Attachment is obsolete: true
Attachment #8687565 -
Flags: review+
Attachment #8687565 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 10•7 years ago
|
||
Comment on attachment 8687565 [details] [diff] [review] Patch with better a11y Review of attachment 8687565 [details] [diff] [review]: ----------------------------------------------------------------- It might be worth filing a followup bug and asking Neil or someone else who knows focus code to check why this is happening. We shouldn't need this kind of hack.
Attachment #8687565 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/909241ae2daf3fd56ed3e4151ee09874e89ef086 Bug 1218789 - Close button and gear button are misaligned in notifications. r=MattN
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/909241ae2daf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8687565 [details] [diff] [review] Patch with better a11y Approval Request Comment [Feature/regressing bug #]: new feature polish work for push notifications released in 44 [User impact if declined]: notifications on windows and linux won't look as good [Describe test coverage new/current, TreeHerder]: on m-c for some time now, visual and automated tests [Risks and why]: none expected [String/UUID change made/needed]: none
Attachment #8687565 -
Flags: approval-mozilla-aurora?
Comment on attachment 8687565 [details] [diff] [review] Patch with better a11y This patch has been in Nightly for ~2 weeks, and since push notifications is planned for FF44, makes sense to uplift to Aurora44.
Attachment #8687565 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/74dc63cabfbf
Comment 16•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/74dc63cabfbf
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•