1296128 - (X) icon to remove permissions in control center doesn't have any indication its focused

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Matthew N. [:MattN]]

The remove icon is in the tab order but I can't tell when the button is focused.

Comment 1

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]: accessiblity / usability regression

Ricky, can you take this?

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Aisling, I suspect that tab focus effect of remove icon in permission panel would be same as hover style. If I'm wrong, please correct me.

Paolo & Yura, AFAIK, Download Panel is able to use "tab" to focus downloadHistory. However, I cannot figure out how to use "tab" to change focus easily in XUL through HTML ARIA (I suspect that ARIA doesn't support in XUL).

As a result, we probably implement this behavior manually like as we did in download panel [1] to define each key event such as tab, up and down. Does it make sense?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#393-524

Comment 3

[Dão Gottwald [:dao]]

(In reply to Ricky Chien [:rickychien] from comment #2)
> Aisling, I suspect that tab focus effect of remove icon in permission panel
> would be same as hover style. If I'm wrong, please correct me.

Focus rings are supposed to be dotted borders on Linux and Windows and a blue shadow on Mac. We should stick to these conventions.

Comment 4

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #3)
> Focus rings are supposed to be dotted borders on Linux and Windows and a
> blue shadow on Mac. We should stick to these conventions.

I agree that following the platform conventions for the focus ring looks reasonable, although the style guide currently shows a blue outline on all platforms:

https://firefoxux.github.io/StyleGuide/#/controls

Stephen, can you comment on how keyboard focused controls would look like? It's useful to define it for square buttons as well, but in particular here we're working on the round "X" buttons from this mockup:

http://people.mozilla.org/~shorlander/mockups-interactive/panel-mockups/panel-control-center.html

A visual mockup would help, for example we might see the distance between the round button and the outline.

Note that we can start implementing something without waiting on the exact definition in the style guide, but eventually I'd like to see the style guide and the implementation in sync to avoid confusion.

Comment 5

[:Paolo Amadini]

(In reply to Ricky Chien [:rickychien] from comment #2)
> Paolo & Yura, AFAIK, Download Panel is able to use "tab" to focus
> downloadHistory. However, I cannot figure out how to use "tab" to change
> focus easily in XUL through HTML ARIA (I suspect that ARIA doesn't support
> in XUL).
> 
> As a result, we probably implement this behavior manually like as we did in
> download panel [1] to define each key event such as tab, up and down. Does
> it make sense?

Ricky, I can use the TAB key to focus controls in the Control Center without any issues - we surely don't need special code in this panel to do that. The Downloads Panel is special as we need to exclude mouse interaction during keyboard interaction. Maybe I'm misunderstanding what you're asking?

Comment 6

[Ricky Chien [:rickychien] (inactive)]

(In reply to :Paolo Amadini from comment #5)
> (In reply to Ricky Chien [:rickychien] from comment #2)
> Ricky, I can use the TAB key to focus controls in the Control Center without
> any issues - we surely don't need special code in this panel to do that. The
> Downloads Panel is special as we need to exclude mouse interaction during
> keyboard interaction. Maybe I'm misunderstanding what you're asking?

No, I reproduced it on Windows and see top right button on Control Center is able to be focused properly. I also saw blue focus highlight around cancel button after removing -moz-appearance on Windows. There is no any effect on Mac and I don't know why.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Ricky Chien [:rickychien] from comment #6)
> No, I reproduced it on Windows and see top right button on Control Center is
> able to be focused properly. I also saw blue focus highlight around cancel
> button after removing -moz-appearance on Windows. There is no any effect on
> Mac and I don't know why.

You probably need to enable full keyboard access. https://support.apple.com/kb/PH18413?locale=en_US

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Paolo came up with a solution that has posted on IRC:

As comment 7 mentioned, Mac user needs to enable full keyboard access first.

After then, we can saw focus effect (blue highlight border around element) works as expected, but focus effect still not works for cancel button until we remove -moz-appearance attribute from cancel button.

Solution would be (IRC log):

<paolo> rickychien: we need to use either the "border" or the "outline" - the square buttons in the Downloads Panel currently use the outline
<paolo> you may take a look at those styles as a start

Comment 9

[Ricky Chien [:rickychien] (inactive)]

According to comment 3, style of focus rings depend on platforms. How do we apply a css rule for different platform?

Comment 10

[Dão Gottwald [:dao]]

button.css already sets the focus ring on Windows and Linux. It looks like you explicitly removed it for this button. You should just undo this.

Comment 11

[Ash Grigas]

(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Ricky Chien [:rickychien] from comment #2)
> > Aisling, I suspect that tab focus effect of remove icon in permission panel
> > would be same as hover style. If I'm wrong, please correct me.
> 
> Focus rings are supposed to be dotted borders on Linux and Windows and a
> blue shadow on Mac. We should stick to these conventions.

correct.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #10)
> button.css already sets the focus ring on Windows and Linux. It looks like
> you explicitly removed it for this button. You should just undo this.

Yes. In order to customize a cancel button, we remove native button style which includes native focus border as well. Only way to render focus border is to add new focus border around our customize circle button but it would be trouble for supporting multi-platform.

Tim, I'm curious about where we define the focus style around XUL button? How do we apply different style on a customize XUL button for all platform?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Ricky Chien [:rickychien] from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > button.css already sets the focus ring on Windows and Linux. It looks like
> > you explicitly removed it for this button. You should just undo this.
> 
> Yes. In order to customize a cancel button, we remove native button style
> which includes native focus border as well.

Nope. That native focus ring is a CSS border set in button.css.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

In order to get rid of native button style, we set "-moz-appearance:none" and focus ring disappear as well. As a result, we cannot use focus ring which defines in button.css at all.

Comment 15

[Dão Gottwald [:dao]]

No, the focus ring does not come from -moz-appearance. It's implemented here:

https://hg.mozilla.org/mozilla-central/annotate/052656fc513c05da969590ac5934abd67271a897/toolkit/themes/linux/global/button.css#l54

https://hg.mozilla.org/mozilla-central/annotate/052656fc513c05da969590ac5934abd67271a897/toolkit/themes/windows/global/button.css#l49

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Yeah, I can understand but how they affect with each other? I saw that -moz-appearance:none will hide these focus ring style.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch

this works for me on Linux

Comment 18

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 19

[:Paolo Amadini]

Dão, I see that this solution keeps a square focus ring, which could be fine as far as I can tell.

Since the button is round, we need to suppress the button hover effect while the button is keyboard focused, which is what your patch does, otherwise the square and round shapes wouldn't work together. We normally don't do this for other square buttons.

This hover suppression may look weird when more than one button is present in the same panel and one of them has the focus ring. As I said, this may not be an issue because it only happens on keyboard interaction.

The needinfo for Stephen was removed accidentally. I still think we should verify that the visual design for the focus ring of round buttons is right, and incorporate decisions in the style guide. Stephen, see comment 4 as well.

Comment 20

[Tim Nguyen :ntim]

(In reply to Ricky Chien [:rickychien] from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > button.css already sets the focus ring on Windows and Linux. It looks like
> > you explicitly removed it for this button. You should just undo this.
> 
> Yes. In order to customize a cancel button, we remove native button style
> which includes native focus border as well. Only way to render focus border
> is to add new focus border around our customize circle button but it would
> be trouble for supporting multi-platform.
> 
> Tim, I'm curious about where we define the focus style around XUL button?
> How do we apply different style on a customize XUL button for all platform?

I think Dão has provided the answer for the first question in comment 15.
As for the second one, you can use the :-moz-focusring selector to target focused XUL buttons as shown in comment 17.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Dao, if you'd like to take this bug, feel free to assign yourself :)

And we still want to hear the confirm from visual design as Paolo mentioned on comment 19.

Comment 22

[Dão Gottwald [:dao]]

My patch was just a proof of concept. It needs at least testing on Windows and probably additional code for OS X using box-shadow: @focusRingShadow@;.

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Attachment: Screen Shot 2016-08-30 at 9.03.22 PM.png

This `box-shadow: @focusRingShadow@;` magical property works great on OS X!
Focusing effect is uploaded as attachment.

And I'll confirm that effect works on Windows and Linux tomorrow.

Comment 24

[Stephen Horlander [:shorlander]]

(In reply to :Paolo Amadini from comment #19)
> The needinfo for Stephen was removed accidentally. I still think we should
> verify that the visual design for the focus ring of round buttons is right,
> and incorporate decisions in the style guide. Stephen, see comment 4 as well.

I think we need to look into improving the focus styles and making them more consistent. But that feels like a bigger project. The square dotted outline is fine for now.

Comment 25

[Ricky Chien [:rickychien] (inactive)]

After confirmation, the `box-shadow: @focusRingShadow@;` doesn't apply any effect on Windows and Linux. 

For Windows and Linux user, they will see the "square dotted outline" focus ring as attachment 8784341 [details].
For OS X user, they will see the "round shadow" focus ring as attachment 8786334 [details].

Comment 26

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1296128 - Indicate permission panel's 'X' button while focus

Review commit: https://reviewboard.mozilla.org/r/75516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75516/

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

@focusRingShadow@ only exists on Mac. You need to put that rule in browser/themes/osx/controlcenter/panel.css.

Why did you drop my change that prevented the focus ring and hover feedback from showing simultaneously?

Comment 28

[Ricky Chien [:rickychien] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #27)
> Comment on attachment 8786585 [details]
> Bug 1296128 - Indicate permission panel's 'X' button while focus
> 
> @focusRingShadow@ only exists on Mac. You need to put that rule in
> browser/themes/osx/controlcenter/panel.css.
> 
> Why did you drop my change that prevented the focus ring and hover feedback
> from showing simultaneously?

I noticed that keyboard focusing does not show hover effect simultaneously with my patch. If I add :not(-moz-focursing) as previous patch, and that would cause background-color disappear while mouse hover.

Comment 29

[Dão Gottwald [:dao]]

Attachment: Screenshot from 2016-08-31 12-45-55.png

(In reply to Ricky Chien [:rickychien] from comment #28)
> > Why did you drop my change that prevented the focus ring and hover feedback
> > from showing simultaneously?
> 
> I noticed that keyboard focusing does not show hover effect simultaneously
> with my patch.

It does (screenshot attached).

> If I add :not(-moz-focursing) as previous patch, and that
> would cause background-color disappear while mouse hover.

Only if the button is focused, right? Btw it's :not(:-moz-focusring) rather than :not(-moz-focusring).

Comment 30

[Ricky Chien [:rickychien] (inactive)]

> (In reply to Ricky Chien [:rickychien] from comment #28)
> > > Why did you drop my change that prevented the focus ring and hover feedback
> > > from showing simultaneously?
> > 
> > I noticed that keyboard focusing does not show hover effect simultaneously
> > with my patch.
> 
> It does (screenshot attached).

Do you mean that use keyboard focusing and mouse hovering at the button simultaneously? You're right if you did this at the same time.

> 
> > If I add :not(-moz-focursing) as previous patch, and that
> > would cause background-color disappear while mouse hover.
> 
> Only if the button is focused, right? Btw it's :not(:-moz-focusring) rather
> than :not(-moz-focusring).

Sorry for typo, I used :not(:-moz-focusring) actually.

Mouse hover effect on button would not work after applying :not(:-moz-focusring) but it works by using :not(:focus) on Windows.

Does :not(:focus) work fine on your Linux environment?

Comment 31

[Ricky Chien [:rickychien] (inactive)]

I made a confirmation on Linux. Indeed, both :not(:focus) and :not(:-moz-focusring) work fine on Linux nightly but :not(:-moz-focusring) doesn't work on Windows.

I'm going to use :not(:focus) to address the issue and cover both platforms.

Comment 32

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75516/diff/1-2/

Comment 33

[:Paolo Amadini]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Dão is reviewing this patch, remember to update the commit message so that MozReview sets the flags correctly.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

(In reply to Dão Gottwald [:dao] from comment #27)
> @focusRingShadow@ only exists on Mac. You need to put that rule in
> browser/themes/osx/controlcenter/panel.css.

Still need to address this.

I don't understand why :not(:-moz-focusring) wouldn't work on Windows. Can you attach that patch for me to try it out?

Comment 35

[Ricky Chien [:rickychien] (inactive)]

I just apply your patch at attachment 8784301 [details] [diff] [review] on Windows. (You can just modify panel.css directly in Devtools's Style Editor and then see new effect on permission panel)

Comment 36

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75516/diff/2-3/

Comment 37

[:Paolo Amadini]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

If you've configured your environment correctly, you can use "hg commit --amend -e" to change the reviewer in the commit message.

Comment 38

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75516/diff/3-4/

Comment 39

[Dão Gottwald [:dao]]

Attachment: screenshot using :-moz-focusring on Windows

This is with my earlier patch applied on Windows. Works just fine.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

We should use :-moz-focusring. It has different semantics than :focus, because we don't always want to draw a focus ring (e.g. for mouse interactions on Windows). See bug 418521 for details.

Comment 41

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75516/diff/4-5/

Comment 42

[Ricky Chien [:rickychien] (inactive)]

I was wrong on comment 31, it seems that :-moz-focusring works great on Windows too. Patch has been updated. Thank you!

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a1c000291e34
Indicate permission panel's 'X' button while focus. r=dao

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1c000291e34

Comment 45

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Approval Request Comment
[Feature/regressing bug #]: bug 1203292
[User impact if declined]: minor, user is unable to notice focus ring around cancel button in permission panel when using keyboard navigation.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: risk is low, only css styling changes
[String/UUID change made/needed]: none

Comment 46

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

Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Fixes a recent regression in 50, Aurora+

Comment 47

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

Now that it's fixed I don't feel the need to track it.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddd6e3e541f1