Closed Bug 1296128 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- unaffected
firefox50 - fixed
firefox51 --- fixed

People

(Reporter: MattN, Assigned: rickychien)

References

Details

(Keywords: access, regression, Whiteboard: [fxprivacy] )

Attachments

(6 files)

The remove icon is in the tab order but I can't tell when the button is focused.
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
[Tracking Requested - why for this release]: accessiblity / usability regression

Ricky, can you take this?
Component: General → Theme
Flags: needinfo?(rchien)
Keywords: regression
OS: Mac OS X → All
Priority: P3 → P2
Assignee: nobody → rchien
Flags: needinfo?(rchien)
Status: NEW → ASSIGNED
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
Flags: needinfo?(yzenevich)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(agrigas)
(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.
(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.
Flags: needinfo?(paolo.mozmail) → needinfo?(shorlander)
(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?
(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.
(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
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
Flags: needinfo?(yzenevich)
Flags: needinfo?(shorlander)
Flags: needinfo?(agrigas)
According to comment 3, style of focus rings depend on platforms. How do we apply a css rule for different platform?
Flags: needinfo?(paolo.mozmail)
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.
(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.
(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?
Flags: needinfo?(ntim.bugs)
(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.
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.
Yeah, I can understand but how they affect with each other? I saw that -moz-appearance:none will hide these focus ring style.
Attached patch patchSplinter Review
this works for me on Linux
Attached image screenshot
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.
Flags: needinfo?(paolo.mozmail) → needinfo?(shorlander)
(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.
Flags: needinfo?(ntim.bugs)
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.
Flags: needinfo?(dao+bmo)
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@;.
Flags: needinfo?(dao+bmo)
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.
(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.
Flags: needinfo?(shorlander)
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 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?
Attachment #8786585 - Flags: review?(paolo.mozmail) → review-
(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.
Flags: needinfo?(dao+bmo)
(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).
Flags: needinfo?(dao+bmo)
> (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?
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Attachment #8786585 - Flags: review?(paolo.mozmail) → review?(dao+bmo)
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?
Attachment #8786585 - Flags: review?(dao+bmo) → review-
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 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.
Attachment #8786585 - Flags: review?(paolo.mozmail)
This is with my earlier patch applied on Windows. Works just fine.
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.
Attachment #8786585 - Flags: review?(dao+bmo) → review-
I was wrong on comment 31, it seems that :-moz-focusring works great on Windows too. Patch has been updated. Thank you!
Attachment #8786585 - Flags: review?(dao+bmo) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a1c000291e34
Indicate permission panel's 'X' button while focus. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1c000291e34
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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
Attachment #8786585 - Flags: approval-mozilla-aurora?
Comment on attachment 8786585 [details]
Bug 1296128 - Indicate permission panel's 'X' button while focus

Fixes a recent regression in 50, Aurora+
Attachment #8786585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Now that it's fixed I don't feel the need to track it.
You need to log in before you can comment on or make changes to this bug.