WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.5k stars 4.19k forks source link

Autocomplete: Must move inside iFrame for slash block insert #47767

Closed alexstine closed 1 year ago

alexstine commented 1 year ago

Description

When trying to insert a block with the /block name syntax, the list box is no longer read to screen readers. This is because the aria-owns and aria-activedescendent attributes contain IDs out of the current iFrame scope causing a null error.

Step-by-step reproduction instructions

  1. Open a post or page.
  2. Go to insert a block by typing in the empty paragraph field. Something like /ima should do.
  3. Arrow up and down.
  4. Check to see the field now has the 2 ARIA attributes as mentioned in the description above.
  5. Try to do a document.getElementByID() or document.querySelector() using the IDs from the ARIA attributes. The commands will return null. 6.I believe this is because the Autocomplete component renders outside the iFrame. Needs a fix as this feature is completely broken for screen reader users.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress: 6.2-alpha-54678 Browser: Firefox Screen reader: NVDA OS: Windows 10 Professional

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

ellatrix commented 1 year ago

Thanks a lot @alexstine! Investigating this now.

ellatrix commented 1 year ago

So yes, it looks like aria-owns has no way to reference an element outside a frame which is a bit annoying. We can't really move the list as it is now inside the iframe, because the list has admin/UI styling and not front-end content styling. So there's only two solutions it seems:

alexstine commented 1 year ago

@ellatrix Figures, silly browser issues...

Move the list inside the iframe, but but it in shadow DOM so it can be styled differently from the rest of the content. This is pretty much what native browsers do I think.

Sounds like this is the way to go for sure.

ellatrix commented 1 year ago

I'll spin up a PR. The shadow DOM approach is also what I want to do block placeholders in the future because we also have styling issues with that.

ellatrix commented 1 year ago

@alexstine I found this article on the subject: https://nolanlawson.com/2022/11/28/shadow-dom-and-accessibility-the-trouble-with-aria/.

Looks like it will soon be possible to set these aria- attributes in JavaScript by passing a reference instead of an ID, but it's so far only in Chrome Canary and Webkit Nightly. Edit: looks like this API wouldn't work with shadow DOM and iframe anyway.

The problem with Shadow DOM is that we'll pretty much have the same issue with aria-owns and aria-activedescendant. If the ID is in the Shadow DOM it won't be found.

Do you have any ideas an how to solve autocomplete without the use of aria-owns and aria-activedescendant?

ellatrix commented 1 year ago

The above article mentions: "Use an ARIA live region instead of IDREFs." How would that work?

alexstine commented 1 year ago

@ellatrix Oh, yeah, using aria-live here will make things too noisy. Currently, we're announcing keyboard instructions via aria-live, there is not really anymore room to be announcing even more information. Probably need to find another solution but I do not know what that solution is as I always avoid implementing iFrames. Never worked on this area of web dev before.

@joedolson Do you know what to do here?

joedolson commented 1 year ago

Ugh. That's kind of ugly. This feels like a super hacky suggestion, but can we duplicate the data inside the iframe in a visibly hidden container?

alexstine commented 1 year ago

That might actually be a good option here as long as that will not throw off visual focus.

ellatrix commented 1 year ago

I will try it now.

ellatrix commented 1 year ago

@alexstine @joedolson I created #47907. Does this work? For autocomplete we never move focus away from the input field (rich text), so focus shouldn't be a problem. I hid the list box with display: none, I think this should work because this is how the native datalist element works as well. I guess theoretically we could use the datalist element, but for now I simply cloned the currently list box.

alexstine commented 1 year ago

@ellatrix Left a comment on the PR. I think display:none; is causing it to not work. If you inspect the current list box in the DOM, I do not see that style attribute.

<div class="popover-slot"><div class="components-popover components-autocomplete__popover is-positioned" tabindex="-1" style="position: absolute; top: 0px; left: 0px; transform: translateX(2115px) translateY(194px) translateZ(0px);"><div class="components-popover__content" style="max-height: 2174.47px; overflow: auto;"><div id="components-autocomplete-listbox-1" role="listbox" class="components-autocomplete__results">
afercia commented 1 year ago

Reopening, as the implemented fix doesn't work with Safari and VoiceOver. See https://github.com/WordPress/gutenberg/pull/47907#issuecomment-1440414694

I'd like to remind that this kind of potentially very impactful changes need to be tested with all the major browsers / assistive technology combination. Also, some tests wouldn't harm.

I haven't checked yet, but I hope all the autocomplete-comboboxes work as expected in the Gutenberg version included in the next WP 6.2 release, otherwise WP is going to ship an UI that is basically unusable for many screen reader users.

alexstine commented 1 year ago

@afercia Yeah, it really is a trash shoot at best. The whole iFrame A11Y compatibility issue was really highlighted from this problem alone. No telling what else is hiding that none of us have found yet. Voiceover is stupid complicated to make work so I take fault for not testing it. It is not a good reason but this is why most people I have talked to do not use Mac for the web. Ouch, that sounded better in my head.

Not sure where we go from here because the iFrame was a solution for visuals but not great for accessibility. Andrea, do you remember if classic editor was wrapped in an iFrame? I know it had a role="application" for accessibility support but cannot remember if it was an actual iFrame. If it was, maybe we just never had list box autocompletes.

Thanks.

afercia commented 1 year ago

@alexstine Yeah, based on my experience, I do agree the iframe is very likely causing more issues that none of us have found yet.

Re: the Classic editor: it does use an iframe, which is generated by TinyMCE. The ifame contains only the contenteditable area though. There is an ancestor element with role="application" that wraps the two main parts of the editor:

Other parts of the UI are rendered outside of the application. For example: when inserting a link, a visual 'popup' appears. That is a combobox with autocomplete but it is appended to the body of the main document. As such, both the input field and the Listbox live in the same document.

afercia commented 1 year ago

One more problem I observed here is that now the Listbox fully re-renders hen navigating through the options with the arrow keys. Each time a new option is highlighted, the whole Listbox gets removed from the DOM and immediately injected again. It appears some screen readers don't like that. A full re-render is also not ideal for other reasons. There must be something in the new implementation that triggers a full re-render.

Also, worth noting that https://github.com/WordPress/gutenberg/pull/47907 needs to be tested with a block-based theme and with a classic theme as well. For example:

This is relevant when testing and may easily confuse testers. Cc @alexstine

alexstine commented 1 year ago

@afercia That makes sense. Sounds like there is no way around simply rendering the elements inside the iFrame or the other solution, not using the iFrame at all. Cannot have it both ways from my understanding of your comment. It is kind of amazing how limited iFrames can be.

Thanks.

alexstine commented 1 year ago

@jeryj This one may be of interest to you? Need to find something that works across all screen readers.

bph commented 1 year ago

Would it be ok to close this issue with the merged PR?

@ellatrix @alexstine

alexstine commented 1 year ago

@bph No, this issue is still present on Mac. 😞

alexstine commented 1 year ago

@afercia I came up with a fix but it isn't great. I have done a lot of research around React portals and there is no easy way to make this work on Mac because Apple doesn't have support for aria-activedescendant. Apple has also failed to support aria-owns.

At this point, there is no real way to approach this any other way besides the visually hidden list box due to the fact the React portal and popover position it outside of the iFrame. There is ongoing research on how this might change later, screen readers interacting with the shadow DOM, but I have a feeling we're still years off from any promising movement.

I took some inspiration from this article: https://react-spectrum.adobe.com/blog/building-a-combobox.html

Please give it a test and let me know what you think. I tested it on both Mac and Windows and the experience is improved.

Still need to figure out why the list box keeps re-rendering though, that is super annoying.

annezazu commented 1 year ago

Hey @alexstine -- I wanted to follow up as this was previously punted by the Core Editor Tech and Core Editor triage leads from the release for 6.5. This was done via our async triage that happens in the Core Editor channel and happened twice (first in session in September and later in the Oct 13th session). Because this issue wasn't discovered during the release and we are past the RC1 date, a fix can't be merged just yet. I've added it for consideration to 6.4.1 for now though. Thank you for all your work here.

Edited to add it's unlikely for 6.4.1 considering the issue was introduced in the cycle either.

cc @SiobhyB @karmatosed @bph @mikachan can you all please check my thinking here? Especially with https://github.com/WordPress/gutenberg/pull/54902 marked for backport yet not merged.

alexstine commented 1 year ago

@annezazu We should be able to merge the above PR if we can land reviews in time. It is a regression so really hoping we're not going to ship another release with something that doesn't function. I would have appreciated a heads up vs. finding out now a total accessibility issue was punted. For Mac users, the "/" inserter is totally inaccessible. Cannot be used by non-sighted.

SiobhyB commented 1 year ago

I see the release guide is clear that RCs should focus solely on regressions introduced during the current cycle, so I do agree that this change should technically be punted. That said, I also acknowledge that this is a high-priority accessibility fix for a core piece of functionality. I'd personally be willing to make an exception to the official policy in this instance, but only if we all agree.

For awareness, the deadlines for merging editor changes for the remaining two RCs in this cycle are as follows:

annezazu commented 1 year ago

I would have appreciated a heads up vs. finding out now a total accessibility issue was punted. For Mac users, the "/" inserter is totally inaccessible. Cannot be used by non-sighted.

The issue getting moved in the board is usually the heads up but I agree! We can do a better job of sharing context and why as it aligns with where we are in the release cycle.

My only concern is in ensuring this fix doesn't likely break something else in some way and is more low risk. I don't have the ability to review to that degree but perhaps @jeryj could help out here? I worry about how often it's referenced as "hacky" solution but I know sometimes that's what gets merged to fix the problem today.

jeryj commented 1 year ago

Going to take a look at it today!

annezazu commented 1 year ago

Fantastic, thank you!

alexstine commented 1 year ago

@annezazu It would be a huge help in the future if you could add a comment when moving issues around on the board. Could be nice and short.

Moving this to future release due to X reason.

I'm not even sure email notifications are sent out for board moves.

Re: hacky. I should have cleared that up. I used that term because there is no official accessibility spec to deal with this at the moment. The iFrame introduced a breaking change with the Autocomplete component and a fix was made. I approved that fix during a couple release cycles ago assuming it was fine. Turns out, it was fine for Windows but not on Mac. Doing additional reading helped me discover that Mac Voiceover does not support the accessibility attributes being used. This change is mainly limited to Mac only, how Voiceover reads the suggestions. This should not effect anything for the sighted. The technical details are kind of hard to understand but essentially the TLDR is the accessibility attributes we use to associate a block with the Popover cannot find the ID because the ID is outside of the iFrame. Sadly, visuals started this whole cascading mess. The solution was to clone the Autocomplete component so there is a hidden version for screen readers and the visible one that still appears outside of the iFrame.

Hope this adds some context. I really did want to land this earlier but once release starts, it's very difficult to get reviews. I'll try much harder to avoid this in the future.

annezazu commented 1 year ago

It would be a huge help in the future if you could add a comment when moving issues around on the board. Could be nice and short.

Will loop back with the wider core editor triage crew to make sure we do this more.

Thank you so much for the added context. That is super helpful and helps assuage my concerns. The issue remains around the intent of the RC period but I think we can discuss that in the wider 6.4 group ultimately if needed.

afercia commented 1 year ago

@afercia I came up with a fix but it isn't great. I have done a lot of research around React portals and there is no easy way to make this work on Mac because Apple doesn't have support for aria-activedescendant. Apple has also failed to support aria-owns.

Thanks @alexsting for all your work on this issue.

Just to add some more context: Yeah, that's alays been the case. Based on my experience aria-activedescendant never worked with Safari and VoiceOver. They also needed aria-selected="true" on the highlighted combobox item to work, while other browsers didn't. However, seems to me this is likely a regression in Safari and VoiceOver. A while ago, we already fixed the url input combobox suggestions in https://github.com/WordPress/gutenberg/issues/47147 That combobox used to work with Safari and VoiceOver. To make one mor example, the Tags suggesions in the Classic Editor used to work. Now they don't work any longer. Not even the W3C example in the ARIA authoring practices works any longer with Safari and VoiceOver. You can test it here: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-autocomplete-list/

Surprisingly, it works with Chrome and VoiceOver so I'd tend to think it's a regression in Safari. Thanks Apple 😞

Tested on October 23rd, 2023: macOS Sonoma 14.0 (23A344) Safari 17.0 (19616.1.27.211.1) VoiceOver 10 (913.3)

None of the combobox examples at https://www.w3.org/WAI/ARIA/apg/patterns/combobox/ work as expected. Not even with Safari Technology Preview Release 181 (Safari 17.4, WebKit 19618.1.3.1)

The only example where VoiceOver announces something is the combobox 'with both list and inline autocomplete' but that's only because it announces the value in the input field that gests updated. It doesn'c actually announces the options.

Looking at the sheer amount of combobox related open issues on the WebKit Bugzilla is, honetly, disappointing.

annezazu commented 1 year ago

Following up here after a public discussion on this issue in #core-editor (that was flagged for #6-4-release-leads): this will not be included in 6.4 but is slated for 6.4.1. Including it in 6.4.1 is a bit unusual and against the usual norm of point release but will be done in recognition of the fact that this is a high priority issue to resolve for users.

Let me know if there's any further communication that we can provide!