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

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Theme
P2
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: MattN, Assigned: rickychien)

Tracking

({access, regression})

Trunk
Firefox 51
access, regression
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox50- fixed, firefox51 fixed)

Details

(Whiteboard: [fxprivacy] )

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

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

Updated

11 months ago
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]

Comment 1

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

Ricky, can you take this?
status-firefox49: --- → unaffected
status-firefox50: --- → affected
tracking-firefox50: --- → ?
Component: General → Theme
Flags: needinfo?(rchien)
Keywords: regression
OS: Mac OS X → All

Updated

11 months ago
Priority: P3 → P2
(Assignee)

Updated

11 months ago
Assignee: nobody → rchien
Flags: needinfo?(rchien)
(Assignee)

Updated

11 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 months ago
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)

Comment 3

11 months ago
(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

11 months ago
(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)

Comment 5

11 months ago
(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?
(Assignee)

Comment 6

11 months ago
(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

11 months ago
(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
(Assignee)

Comment 8

11 months ago
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)
(Assignee)

Comment 9

11 months ago
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)

Comment 10

11 months ago
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

11 months ago
(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.
(Assignee)

Comment 12

11 months ago
(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)

Comment 13

11 months ago
(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.
(Assignee)

Comment 14

11 months ago
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

11 months ago
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
(Assignee)

Comment 16

11 months ago
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

11 months ago
Created attachment 8784301 [details] [diff] [review]
patch

this works for me on Linux

Comment 18

11 months ago
Created attachment 8784341 [details]
screenshot

Comment 19

11 months ago
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)

Comment 20

11 months ago
(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)
(Assignee)

Comment 21

11 months ago
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)

Comment 22

11 months ago
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)
(Assignee)

Comment 23

11 months ago
Created attachment 8786334 [details]
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.
(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)
(Assignee)

Comment 25

11 months ago
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 hidden (mozreview-request)

Comment 27

11 months ago
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-
(Assignee)

Comment 28

11 months ago
(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)

Comment 29

11 months ago
Created attachment 8786688 [details]
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).
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 30

11 months ago
> (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)
(Assignee)

Comment 31

11 months ago
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 hidden (mozreview-request)

Comment 33

11 months ago
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 34

11 months ago
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-
(Assignee)

Comment 35

11 months ago
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 hidden (mozreview-request)

Comment 37

11 months ago
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)
Comment hidden (mozreview-request)

Comment 39

11 months ago
Created attachment 8786778 [details]
screenshot using :-moz-focusring on Windows

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

Comment 40

11 months ago
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-
Comment hidden (mozreview-request)
(Assignee)

Comment 42

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

Updated

11 months ago
Attachment #8786585 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 43

11 months ago
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

Comment 44

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1c000291e34
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Comment 45

11 months ago
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 46

11 months ago
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+

Comment 47

11 months ago
Now that it's fixed I don't feel the need to track it.
tracking-firefox50: ? → -

Comment 48

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddd6e3e541f1
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.