Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

AX_FOCUS_01 Shouldn't anchor elements visible onFocus be excluded from this warning. #65

Closed
hexalys opened this issue Oct 20, 2013 · 29 comments

Comments

@hexalys
Copy link

hexalys commented Oct 20, 2013

It's me again with another link focus related conundrum:

Sites get the warning "These elements are focusable but either invisible or obscured by another element" on <a> elements such as skip links or menubar links, even if those focusable elements are actually shown via specific CSS a:focus rules.

Shouldn't you check the onFocus visibility of such elements dynamically first, before considering it invisible or obscured?

If the element is purposely shown with a:focus, it is technically visible by a keyboard user using the tab key, as well as a mouse user. Properly designed navigation bars with pull down submenus made visible onFocus is a good use case here.

It would also help developers determining which links are not visible by sightseeing users vs those which are purposely shown to both via a:focus rules, and assess whether the ones receiving a warning shall be tagged as aria-hidden or not.

PS: As added enforcement to verify that those links are in fact shown, you could perhaps check the contrast ratio while onFocus. If the contrast ratio fail in both states, report it as insufficiently visible or something like that?

@ckundo
Copy link
Collaborator

ckundo commented Oct 20, 2013

Thanks @hexalys, I'll work on a test case to surface this issue.

@ckundo
Copy link
Collaborator

ckundo commented Oct 21, 2013

@alice, there is logic you've implemented around aria-hidden that suggests to me that you've accounted for the scenario @hexalys is describing. If that's true, is there some strategy around using aria-hidden that we can recommend to get this test passing, or is this in fact a gap in the test logic?

@alice
Copy link
Contributor

alice commented Oct 21, 2013

@ckundo That code (did you link to the thing you meant to link to?) is for a subtly different scenario - that's to exclude elements which can never take focus from being considered in the first place - in that case, even though the element has a tabIndex, it can never take focus because the tabIndex is < 0 and the parent has a widget role. This occurs in some Closure widgets to work around some issue to do with calling focus() - focus is set programmatically to the child of a widget and then immediately transferred to the parent. I tried to forget the details as soon as possible.

What we'd need to do here is, presumably, focus the element or otherwise simulate the style which applies when the :focus selector is active, and then check visibility. This would be in the test method, rather than the relevantElementMatcher method. I don't think there's any util code currently which simulates a :focus style, we'd need to add that.

@alice
Copy link
Contributor

alice commented Oct 21, 2013

@hexalys I assume this is the type of thing you're talking about: http://webaim.org/techniques/skipnav/#focus

@hexalys
Copy link
Author

hexalys commented Oct 22, 2013

@alice Correct. You may also want to wait 500ms after simulating focus on the element, to allow for a subtle css transition or animation to proceed.

@ckundo
Copy link
Collaborator

ckundo commented Oct 22, 2013

@alice thanks for the clarification, I was struggling to sort through the widget code and thought maybe it was relevant. A visibility with focus assertion sounds like a good idea then.

@alice
Copy link
Contributor

alice commented Oct 22, 2013

@hexalys Good point. However, I don't think waiting for an arbitrary timeout is a good idea: for one thing it will make the audit incredibly slow (waiting 500ms for each element times the number focusable elements on the page), and it'll be flaky as well if some transitions take longer than the timeout. We unfortunately might need to punt on checking for CSS transitions until the issues around transitionEnd events are resolved (see http://lists.w3.org/Archives/Public/www-style/2012Dec/0191.html - we might be waiting a while, that thread started late last year).

@hexalys
Copy link
Author

hexalys commented Oct 22, 2013

@alice Nods, you are right on the timing. You can also disable all transition rules. i.e. Remove all transition properties or kill them off globally (http://stackoverflow.com/a/11132887/1647538), to avoid dealing with that burden all together.

@alice
Copy link
Contributor

alice commented Oct 22, 2013

@hexalys Good idea! I think we can do something along those lines but on a per-element basis to minimise the potential for disrupting the page.

Design sketch: when testing element, set element.style.transition to none - that will override all other styles based on CSS precedence - and then set it back to a saved value after testing. We could do this in AuditRule so that we don't need to do it in each place that checks any type of style.

@alice
Copy link
Contributor

alice commented Oct 22, 2013

(Oh, I should note that we will obviously need to account for cross-browser differences when we do this - I believe WebKit/Blink still use the webkitTransition terminology.)

@hexalys
Copy link
Author

hexalys commented Oct 22, 2013

I like it. And yes, but you need to account for both the prefixed version and/or the un-prefixed one as needed. Chrome 26+ takes either, as per http://caniuse.com/#search=transition.

And sites can be designed to feed the browser a dynamic stylesheet with just the css terminology it needs, and not always all 4-5 versions. At least that's what I personally do when possible, because I don't like bloat :)

@alice
Copy link
Contributor

alice commented Oct 24, 2013

Ok, I've started a branch with a test here: https://github.com/alice/accessibility-developer-tools/compare/skip-link

Haven't started on implementation yet - @ckundo do you want to take this one, or shall I?

@ckundo
Copy link
Collaborator

ckundo commented Oct 24, 2013

@alice I'll take it.

@ckundo
Copy link
Collaborator

ckundo commented Oct 26, 2013

I've added this to the top of our Tracker backlog: https://www.pivotaltracker.com/s/projects/883176

Hoping to pick it up this weekend.

@ckundo
Copy link
Collaborator

ckundo commented Oct 26, 2013

This issue seems very specific to skip nav links, since we're not checking for display: none or visibility: hidden when checking if something is hidden. (see https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/AccessibilityUtils.js#L239). @hexalys, are you seeing false positives on dropdowns etc?

ckundo pushed a commit to ckundo/accessibility-developer-tools that referenced this issue Oct 26, 2013
@hexalys
Copy link
Author

hexalys commented Oct 26, 2013

@ckundo It's not specific only to skip links. I do have false positive on drop-downs. See example of the CSS3 menu at http://www.dinahperezlaw.com. I have deliberately made the 'Terminology' drop-down visually focusable (via js, for lack of css parent selector). Though if tabbed to, the submenu links and its parent are made visible.

PS: I have used the 1x1px method for the first skip link to bypass the warning for now, since you are not currently checking for that.

@jeremyboggs
Copy link

Also have this warning for skip links, and for links where I use icons with a text fallback, using techniques in Bulletproof Accessible Icon Fonts. I'm also using the .visuallyhidden rules used by HTML5Boilerplate.

@ckundo
Copy link
Collaborator

ckundo commented Oct 17, 2014

@alice @hexalys it looks like this was addressed in 21176ae. Can I close this issue?

@hexalys
Copy link
Author

hexalys commented Nov 7, 2014

@ckundo Not addressed yet in the current 2.9.2 extension. I even see visible normal links being flagged as: "These elements are focusable but either invisible or obscured by another element".

e.g. See sample tests it on webaim.org. You'll see the bottom links are flagged.
Not sure why visible links are not being detected as elementIsVisible().

@ckundo
Copy link
Collaborator

ckundo commented Nov 7, 2014

@hexalys alright thanks for the update.

@alice
Copy link
Contributor

alice commented Nov 13, 2014

Just released a new version of the extension (2.10.0) - please let me know if you're still seeing this issue with the new version.

@hexalys
Copy link
Author

hexalys commented Nov 13, 2014

Ignore my previous deleted comment. It seems resolved now. My menubar now passes all tests. I had 'overlappingElements' on some of them which I addressed in production and they now pass the test.

@ckundo You can close the issue. Thanks

@ckundo ckundo closed this as completed Nov 13, 2014
@jeremyboggs
Copy link

@alice yes I'm still seeing this issue with 2.10.0. Also encounter the same issue on webaim.org that @hexalys mentions.

@hexalys
Copy link
Author

hexalys commented Nov 22, 2014

@clioweb Yes sorry, the problem is back on my now live site, and I had forgotten to check the webaim.org site again. There are quite a few issues with the tests, as I can observe while running console.logs() in the `axs.AuditRule.specs.focusableElementNotVisibleAndNotAriaHidden' rule set.

  1. The overlappingElements() rule goes quite too far in checking element outside the main ancestors.
    In my menubar case, it detects the <header> above the <nav> as overlapping, when it's really not. (it's flex and has no given height but it does compute a height). I don't think overlappingElements() should check beyond the primary ancestor like it currently does, or the rule is failing somewhere...
  2. In the case of links on webaim.org, the visible links report as elementIsOutsideScrollArea(); just like skip links. I am not sure if the focus is being triggered, or something else is going on...

@alice
Copy link
Contributor

alice commented Nov 22, 2014

@hexalys Thanks for following up; it does sound like something odd is going on... digging into it now.

@alice
Copy link
Contributor

alice commented Nov 22, 2014

I can't reproduce the issue with visible links on webaim.org - could you give a concrete example?

It looks like the issue with the skip link is that it's still not actually getting the :focus style - logging element.matches(':focus'); after element.focus() is returning false (whereas it returns true if you check it during a focus event on the element). It could be that when running the audit using the extension, the developer tools are keeping focus so we can't focus the element. Frustrating...

And, could you link to the menubar example so that I can check that out?

@jeremyboggs
Copy link

I can't reproduce the issue with visible links on webaim.org - could you give a concrete example?

@alice On webaim.org homepage, I get 20 warnings for AX_FOCUS_01. One is for the skip link they have, and the rest are for visible links in their footer. Not sure if it's any help, but I've attached a screenshot of my audit on the page:

webaim-axfocus

@hexalys
Copy link
Author

hexalys commented Nov 22, 2014

And, could you link to the menubar example so that I can check that out?

@alice Here is the menubar. The sub-menuitems are the ones that should pass when focused.

the developer tools are keeping focus so we can't focus the element. Frustrating...

I hear ya! There needs to be a way to apply focus.
Perhaps try a blur on whatever is focused before each run? Just a thought.

@GaryJones
Copy link

This issue is marked as closed, but skip links are still getting flagged:

screenshot 2017-02-10 03 18 53

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants
@jeremyboggs @GaryJones @alice @ckundo @hexalys and others