PrismJS / prism

Lightweight, robust, elegant syntax highlighting.
https://prismjs.com
MIT License
12.24k stars 1.29k forks source link

Safari doesn't tab to Prism code block #2695

Closed dsenneff closed 3 years ago

dsenneff commented 3 years ago

When using the Safari desktop browser (v14) with a keyboard, Prism blocks don't appear in the tab order. So a keyboard-only user won't be able to tab to the block of code and scroll to view any overflowed content. Chrome, FireFox, and Edge work fine, I'm only seeing this in Safari.

I noticed this while testing a local site, but then checked the examples on Prism's site and I'm having the same issue, so it should be easy to replicate. No plugins or anything of that nature.

To replicate:

  1. Go to https://prismjs.com/index.html in Safari
  2. Navigate the page by using the tab key
  3. The focus indicator moves right past any Prism blocks of code. In this case, I'm able to tab to the "Examples" heading, then the next item in the focus order is the "view more example" link below the first scrollable block of code.
mAAdhaTTah commented 3 years ago

Safari weirdly decided it doesn't tab-focus everything by default. You can enable it to do so if you go into Settings -> Advanced -> check "Press tab to highlight each item on a webpage". Screen Shot 2021-01-05 at 7 42 24 AM

Beyond that, we're not doing anything specific to make it tabbable in those browsers, so I'm not sure this is something we can solve.

dsenneff commented 3 years ago

@mAAdhaTTah I'm aware of that setting and that's not the issue here, as I have that enabled already. And any regular keyboard user would have that enabled as well.

While you're not adding anything to make scrollable areas tabbable by default in Chrome and FireFox, by default areas do not overflow and instead grow to the size of their contents. If Prism is creating a structure and applying styles that create scrollable areas, then I feel it should take on a responsibility to make a good faith effort to make that content accessible. I don't think the fact that scrollable areas are tabbable by default in other browsers lets us off the hook.

I haven't tested fully, but I think a quick solution might be to just add a tabindex="0" to all of the pre elements created by Prism.

mAAdhaTTah commented 3 years ago

That's at least a reasonable principle, so let me look into it.

RunDevelopment commented 3 years ago

Some additional info:

Chrome, Firefox, and Edge work fine.

I tested it on my machine and Firefox lets me tab to scrollable code blocks. Chrome and (Chromium) Edge have the problem you described. IE11 also has this problem.

dsenneff commented 3 years ago

Interesting, @RunDevelopment. Are you on Windows? I haven't tested on Windows, but on my Mac I don't experience the issue in Chrome or Chromium Edge. Might be the OSs handling key events different for UI elements, not sure.

RunDevelopment commented 3 years ago

Yes, I am using Windows.

Veyfeyken commented 3 years ago

I'm on macOS. Codeblock with scrollable content don't get focused in Chrome or Safari. They do get focus in Firefox.

This issue also get's flagged by Axe-core: https://dequeuniversity.com/rules/axe/4.1/scrollable-region-focusable?application=AxeChrome

rschristian commented 2 years ago

I don't think this is correct for accessibility. Focusable elements should have interactive semantics, but there's nothing about a block of code that inherently is interactive.

I've had to modify prism as I started to fail accessibility tests because of this incorrect behaviour.

If someone wants to be able to navigate code blocks with the keyboard then this is something they should add in themselves, as this breaks WCAG compliance.

RunDevelopment commented 2 years ago

there's nothing about a block of code that inherently is interactive.

I disagree. Code blocks often have scroll bars, be it horizontal or vertical. These scroll bars must be interacted with in order to see the full content of a code block. However, in some browsers, this interaction is only possible with the tabindex attribute. As such, code blocks must be focusable for cross-browser accessibility.

If we made code blocks non-focusable again, we would make it impossible for keyword-users to see the full contents of some code blocks by default.

Veyfeyken commented 2 years ago

Related: https://www.tpgi.com/short-note-on-improving-usability-of-scrollable-regions/

If someone wants to be able to navigate code blocks with the keyboard then this is something they should add in themselves, as this breaks WCAG compliance.

I assume you're getting a failure on Success Criterion 4.1.2 Name, Role, Value (A). This doesn't fail Success Criterion 2.1.1 Keyboard, it actually fixes it. Adding role="region" in combination with the tabindex="0" will handle the Name, Role, Value error.

RunDevelopment commented 2 years ago

According to MDN, landmark roles should be used sparingly. Wouldn't role="generic" be more fitting?

Veyfeyken commented 2 years ago

Yes but role="generic" still gets flagged as an error on 4.1.2 Name, Role, Value unfortunately. Role="region" does not.

Veyfeyken commented 2 years ago

As role="region" creates an unnamed landmark, it would be good to identify it with an aria-label. Fox example: <pre tabindex="0" role="region" aria-label="codeblock"> The above will receive focus so it can be scrolled with the keyboard. Screenreaders will announce a "region codeblock".

rschristian commented 2 years ago

I disagree. Code blocks often have scroll bars, be it horizontal or vertical. These scroll bars must be interacted with in order to see the full content of a code block. However, in some browsers, this interaction is only possible with the tabindex attribute.

You're confusing the fact that they can have scroll bars with an assumption that they will. This is not always the case.

Adding tabindex blindly breaks accessibility as, again, non-interactive areas should not be focusable. There are no actions a user can take.

This should be something users add for themselves unless you want to start detecting overflows and adding the attribute only when scroll bars actually appear.

If we made code blocks non-focusable again, we would make it impossible for keyword-users to see the full contents of some code blocks by default.

Right now you're making it more difficult for every keyboard user to navigate a site in an incorrect assumption.

At the end of the day, blocks of code are not inherently interactive. You cannot be add tabindex to them blindly.

I assume you're getting a failure on Success Criterion 4.1.2 Name, Role, Value (A)

No.

This doesn't fail Success Criterion 2.1.1 Keyboard, it actually fixes it.

If the area is non-interactive, adding tabindex fails this test.

dsenneff commented 2 years ago

I don't think it's fair to say that allowing all code blocks to be focusable "blindy breaks" the functionality for a keyboard user. Yes, in cases where the content doesn't overflow it will cause an extra tab stop, but that doesn't mean the page is broken for those users.

The spirit of WCAG SC 2.1.1 is that all content CAN be accessed with a keyboard, that doesn't mean that a user necessarily will interact with it. By being consistent and predictable, users will learn that all code blocks are accessible even though a particular one might not require it. To my knowledge, a code block that is focusable but doesn't require scrolling to view the content is not a scenario that fails this SC.

Content can change: authors can make edits that now cause a code block to overflow. And users can change their viewing specifics: they can revisit the page on a different device or different size browser window, they might enlarge their text to view something better, or other actions that create overflow where it didn't exist before. So a consistent experience is key.

Developers can also mitigate confusion with proper headings and other information paired with the code block, such as "CSS Example 3 (content may overflow)".

extra808 commented 2 years ago

There are pros and cons but I think what PrismJS is now doing, adding tabindex=0 with no role or name, is best for users. Including tabindex removes a barrier for keyboard (and keyboard-equivalent) users to enable access to scrollable codeblocks. Since the tabindex is always present, it does add an unnecessary tab stop to non-scrollable codeblocks for keyboard users but that only makes page navigation a little worse, the trade-off is worth it. If there was a whole set of codeblocks that might not be of interest to all readers, the author could precede the set with a "skip link" or put them all within a disclosure widget (like details-summary elements). In theory, you could have JavaScript checking whether each codeblock is scrollable to add/remove the tabindex attribute (or toggle the value between 0 and -1) but that doesn't sound like a worthwhile trade-off (to develop or to have continually running on every page).

Regarding MDN's WCAG Keyboard page, it's not an authoritative source but it is useful. It says "focusable elements should have interactive semantics." A scrollable codeblock is interactive, you can scroll it, but there are no interactive semantics that convey "scrollability." role=region and an accessible name, like aria-label="code block" could be added but they would be no benefit for those who need the tab stop and would make the user experience worse for others. Screen reader users don't need the tab stop, the screen reader gives them access to all the content without it. The role and accessible name will only be conveyed to the screen reader user but they don't need it; they're unlikely to use the regions for landmark navigation and if all the codeblocks have the same accessible name, there's nothing to differentiate them.

The MDN page says "focusable elements should have interactive semantics" but for other subjects, it uses the word "must," like "interactive elements must be focusable." "Must" is a requirement, "should" is a recommendation or a description of typical situations; this case is an exception, for the reasons I've given.

Additionally, in the browsers that are automatically adding a tab stop for scrollable elements, they are not adding any semantics with it; two wrongs don't make a right but they do serve as real-world examples of focusable elements without interactive semantics.

If tests are failing on elements solely because they have tabindex=0, the tests should be improved. Adding roles and names to satisfy the tests would benefit only developers and make the user experience worse for some people.

oldrup commented 2 years ago

I was confused about the new tabindex of the pre element to be honest, as when testing keyboard navigation on my site "something" was getting focused, but I didn't know what. So I'm experimentally adding a focus outline to pre elements with tabindex like

/** Improve general outline visibility, especially in firefox **/ :is(a, button, pre[tabindex="0"]):focus { outline-width: var(--outline-width); outline-style: var(--outline-style); outline-color: var(--linkHoverColor); outline-offset: var(--outline-offset); }

I think adding an aria-label to the element would benefit users of screen readers, but that should probably be done manually? Has anyone been researching this, maybe done some accessibility testing?

RunDevelopment commented 2 years ago

"something" was getting focused, but I didn't know what

Firefox does have a subtle focus outline, so unless you overwrite that, it should be clear which element is focused.

image

I think adding an aria-label to the element

I don't think that we can do that on a library level. The main problem is that screen readers seem to just read out the value of the label. So the screen reader saying "code block" won't help non-English speaking people.

oldrup commented 2 years ago

Thank you @RunDevelopment for your insights. Those are valid points. I fixed my firefox styling to make the focus outline properly, and you're right, an aria label should be descriptive and thus probably added manually - a generic "code block" label is not helping much.