Closed Bug 1218789 Opened 4 years ago Closed 4 years ago

Close button and gear button are misaligned in notifications

Categories

(Core :: DOM: Push Notifications, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: phlsa, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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)
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.
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)
Attached patch Patch (obsolete) — Splinter Review
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 $=
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8684412 - Flags: review?(MattN+bmo)
(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 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 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)
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 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+
https://hg.mozilla.org/integration/fx-team/rev/909241ae2daf3fd56ed3e4151ee09874e89ef086
Bug 1218789 - Close button and gear button are misaligned in notifications. r=MattN
https://hg.mozilla.org/mozilla-central/rev/909241ae2daf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1227717
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+
You need to log in before you can comment on or make changes to this bug.