1218789 - Close button and gear button are misaligned in notifications

Status: RESOLVED FIXED

(Core :: DOM: Push Notifications, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Screen Shot 2015-10-27 at 14.35.03.png

The close button and the gear button in a notification are not aligned correctly.

Needs UX input on how exactly they should be aligned

Comment 1

[Bill Maggs (bmaggs)]

Attachment: squarehighlitgear.png

Comment 2

[Bill Maggs (bmaggs)]

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

[Bill Maggs (bmaggs)]

Attachment: Screenshot 2015-10-28 16.09.08.png

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 5

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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 $=

Comment 6

[Matthew N. [:MattN]]

(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

[Matthew N. [:MattN]]

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.

Comment 8

[:Gijs (he/him)]

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?

Comment 9

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch with better a11y

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

Comment 10

[:Gijs (he/him)]

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.

Comment 11

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/909241ae2daf3fd56ed3e4151ee09874e89ef086
Bug 1218789 - Close button and gear button are misaligned in notifications. r=MattN

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/909241ae2daf

Comment 13

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

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

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74dc63cabfbf

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/74dc63cabfbf