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

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

Closed
alexstine opened this issue Feb 6, 2023 · 31 comments · Fixed by #47907 or #54902
Closed

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

alexstine opened this issue Feb 6, 2023 · 31 comments · Fixed by #47907 or #54902
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Dev Ready for, and needs developer efforts [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@alexstine
Copy link
Contributor

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

@alexstine alexstine added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Dev Ready for, and needs developer efforts [Type] Regression Related to a regression in the latest release labels Feb 6, 2023
@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2023

Thanks a lot @alexstine! Investigating this now.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2023

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:

  • Moves focus outside the frame and let the autocomplete happen there. Doesn't seem viable.
  • 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.

@alexstine
Copy link
Contributor Author

@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
Copy link
Member

ellatrix commented Feb 6, 2023

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
Copy link
Member

ellatrix commented Feb 7, 2023

@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
Copy link
Member

ellatrix commented Feb 7, 2023

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

@alexstine
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

@ellatrix
Copy link
Member

ellatrix commented Feb 9, 2023

I will try it now.

@ellatrix
Copy link
Member

ellatrix commented Feb 9, 2023

@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
Copy link
Contributor Author

@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
Copy link
Contributor

afercia commented Feb 22, 2023

Reopening, as the implemented fix doesn't work with Safari and VoiceOver. See #47907 (comment)

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.

@afercia afercia reopened this Feb 22, 2023
@alexstine
Copy link
Contributor Author

@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
Copy link
Contributor

afercia commented Feb 23, 2023

@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:

  • the toolbar (the one with Bold, Italic, etc.)
  • the iframe

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
Copy link
Contributor

afercia commented Feb 23, 2023

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 #47907 needs to be tested with a block-based theme and with a classic theme as well. For example:

  • With Twenty Twenty-Three activated, the Listbox is duplicated.
  • With Twenty Twenty-One there's only one Listbox instead, because there's no iframe in the first place.

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

@alexstine
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@bph
Copy link
Contributor

bph commented Sep 18, 2023

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

@ellatrix @alexstine

@alexstine
Copy link
Contributor Author

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

@bph bph moved this from Needs Dev / Todo to Punted to 6.5 in WordPress 6.4 Editor Tasks Sep 23, 2023
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 28, 2023
@alexstine
Copy link
Contributor Author

@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.

@alexstine alexstine moved this from Punted to 6.5 to Needs Review in WordPress 6.4 Editor Tasks Sep 28, 2023
@annezazu annezazu moved this from Needs Review to Punted to 6.5 in WordPress 6.4 Editor Tasks Oct 19, 2023
@annezazu annezazu moved this from Punted to 6.5 to Punted to 6.4.1 in WordPress 6.4 Editor Tasks Oct 19, 2023
@annezazu
Copy link
Contributor

annezazu commented Oct 19, 2023

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 #54902 marked for backport yet not merged.

@alexstine
Copy link
Contributor Author

@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
Copy link
Contributor

SiobhyB commented Oct 19, 2023

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:

  • RC2: Monday, October 23rd at 3pm UTC
  • RC3: Monday, October 30th at 3pm UTC

@annezazu
Copy link
Contributor

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
Copy link
Contributor

jeryj commented Oct 19, 2023

Going to take a look at it today!

@annezazu
Copy link
Contributor

Fantastic, thank you!

@alexstine
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

afercia commented Oct 23, 2023

@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 #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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Dev Ready for, and needs developer efforts [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
Status: Done 🎉
Status: Done
8 participants