Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button example's sample buttons lack visual focus indication #2650

Closed
charmarkk opened this issue Mar 28, 2023 · 9 comments · Fixed by #2673
Closed

Button example's sample buttons lack visual focus indication #2650

charmarkk opened this issue Mar 28, 2023 · 9 comments · Fixed by #2673
Assignees
Labels
bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern

Comments

@charmarkk
Copy link
Contributor

charmarkk commented Mar 28, 2023

The example "Print" and "Mute" buttons in the Button example do not have visual tab focus indicators, not even browser defaults. Should they?

Example page: https://www.w3.org/WAI/ARIA/apg/patterns/button/examples/button/

@charmarkk charmarkk added the bug Code defects; not for inaccurate prose label Mar 28, 2023
@mcking65 mcking65 added the Example Page Related to a page containing an example implementation of a pattern label Mar 28, 2023
@charmarkk charmarkk assigned charmarkk and unassigned charmarkk Mar 28, 2023
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Button example's sample buttons lack visual focus indication.

The full IRC log of that discussion <jugglinmike> Subtopic: Button example's sample buttons lack visual focus indication
<jugglinmike> github: https://github.com//issues/2650
<jugglinmike> Matt_King: This seems like a stunning find. Stunning in the sense that I can't believe we would have overlooked it
<jugglinmike> MarkMcCarthy: If I tab to the buttons, then I see nothing
<jugglinmike> Matt_King: How is that possible? We put "outline" to "none" and then did nothing...?
<jugglinmike> Matt_King: This seems like a pretty major oversight
<jugglinmike> Matt_King: I'm assuming we're overriding the default browser styling for focus
<jugglinmike> Alex_Flenniken: the issue seems to be happening between the APG and the WAI ARIA Practices repository
<jugglinmike> Matt_King: I expect there may be some styling in the template that is somewhat generic
<jugglinmike> Matt_King: I'll assign the issue to Carmen Cañas at Bocoup, then she can take of delegating this to someone at Bocoup
<jugglinmike> Matt_King: Looks like I can't do that right now. After this meeting, I will add Carmen's GitHub handle to the "aria contributors" GitHub Team and then assign this issue to her
<CurtBellew> It looks like there's no outline color set on those particular buttons. So I suspect outline color is inherited from somewhere else.

@daniel-montalvo
Copy link
Contributor

@ccanash @mcking65

Any chance we can get this addressed before the AT support tables announcement?

@ccanash
Copy link

ccanash commented Apr 11, 2023

Hi @daniel-montalvo, unfortunately it won't be released with the AT support tables today.

@daniel-montalvo
Copy link
Contributor

Hi @ccanash and @mcking65

Thanks for your reply.

I understand.

w3c/wai-aria-practices#210 was not published yesterday. Maybe that gives us an opportunity to include the fix in there?

I would expect these pages to get more traction given the imminent blog announcement. Even if it is not related to AT support tables, it seems to me that having an example that lacks visual focus indication is problematic and can raise some eyebrows. If I am not mistaking, the button examples are one of the four that will have the new tables, so that is yet another reason for people to look specifically at this pages. I would suggest we prioritize this.

@howard-e
Copy link
Contributor

howard-e commented Apr 12, 2023

A clue to why this is happening, when it previously did show, may be in the example's usage of z-index: -1 being applied to the :focus:before state of the Print and Mute buttons.

It seems like the surrounding body + wrappers of the transform in the APG generated web pages might have some layering rules that would not have been present on the ReSpec version so I'm also curious about other areas of the APG site that could be impacted, so looking to the transform first would maybe be best for a long-term solution. Otherwise, it just may mean special attention needs to be awarded to the use of z-index rules during the examples' code reviews.

Alternatively, I'm curious about the motivation behind modifying the z-index here, as you'll typically want to do this so the content of the element doesn't get blocked out by the pseudo-element content of the :before. But in this instance, the :before's content is just a border which should(?) never interact with the parent element's content and removing the z-index or setting it to 0+ doesn't seem to impact the example negatively.

However, I may be missing an example-specific nuance here and don't mean to trivialize that motivation, but removing the z-index rule may be a potential quick-fix for whoever is assigned.

@alflennik
Copy link
Contributor

I opened a PR for this issue: #2673

mcking65 pushed a commit that referenced this issue Apr 13, 2023
… (pull #2673)

Fixes issue #2650: Button example's sample buttons lack visual focus indication.

There was a conflict between the site template and example code. Fixed by:

In `patterns/button/examples/css/button.css`,
Removed: `z-index: -1;` from button selector.
@mcking65
Copy link
Contributor

mcking65 commented Apr 13, 2023

A clue to why this is happening, when it previously did show, may be in the example's usage of z-index: -1 being applied to the :focus:before state of the Print and Mute buttons.

It seems like the surrounding body + wrappers of the transform in the APG generated web pages might have some layering rules that would not have been present on the ReSpec version so I'm also curious about other areas of the APG site that could be impacted, so looking to the transform first would maybe be best for a long-term solution. Otherwise, it just may mean special attention needs to be awarded to the use of z-index rules during the examples' code reviews.

Isn't it the case that as long as PR reviewers are doing their accessibility and visual design reviews in the netlify preview with the site templates in effect, they will easily spot this kind of problem?

Alternatively, I'm curious about the motivation behind modifying the z-index here, as you'll typically want to do this so the content of the element doesn't get blocked out by the pseudo-element content of the :before. But in this instance, the :before's content is just a border which should(?) never interact with the parent element's content and removing the z-index or setting it to 0+ doesn't seem to impact the example negatively.

However, I may be missing an example-specific nuance here and don't mean to trivialize that motivation, but removing the z-index rule may be a potential quick-fix for whoever is assigned.

The person who would know the motivation is @ZoeBijl. They added the focus::before with the z-index -1 in commit 18d9133 on Feb 18, 2017. You can see the change in this diff in PR 298.

@mcking65
Copy link
Contributor

@howard-e and @alflennik, thank you, thank you for the speedy response to @daniel-montalvo's concern. I agree with the concern. Thank you @charmarkk for spotting the issue and for the speedy review, and thank you @daniel-montalvo for your attention to this detail!

@ZoeBijl
Copy link
Contributor

ZoeBijl commented Sep 6, 2023

Thank you @charmarkk for bringing this up and @alflennik for fixing it! I don’t remember what the reason for the z-index: -1 was but I’m happy it’s fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants