Bug 203292 – Accessory bar next/previous buttons do not work on inputs in shadow roots

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Liam DeBeasi]

Inputs that are slotted in shadow roots are not accessible via the next/previous buttons in the iOS Safari Accessory Bar.

Link to CodePen reproduction: https://codepen.io/liamdebeasi/pen/bGGqMpO

Steps to reproduce:

1. Open CodePen link on an iOS device (tested on iOS 12 and iOS 13).
2. Tap "Light DOM Input 2". Notice that the next/previous buttons are active.
3. Tap "Shadow DOM Input 2". Notice that the next/previous buttons are disabled.
4. Tap "Shadow DOM Input 3". Notice that the next button is disabled, even though the next input is in the light DOM and the same form.


Expected Result:

I would expect to be able to use the next/previous buttons to jump between inputs regardless of whether or not the input is in the Shadow DOM.


Other Info:

Next/Previous buttons on inputs in the Shadow DOM behave as expected on Chrome for Android.

[Radar WebKit Bug Importer]

<rdar://problem/56544829>

[cyptus]

can we get any attention here? this is quite a big issue.

[Niklas Merz]

Can we provide anything to get this resolved? This is a pretty annoying usability bug for Ionic apps which typically use shadow DOM for all inputs.

[Ryosuke Niwa]

I can reproduce the issue. It's just a matter of taking the time to fix the bug now.

[Luan Freitas]

I know it sucks to be asking. But do we have any predictions? Even how far? Middle 2021, maybe?

[Ryosuke Niwa]

If someone can point out a major website which gets affected by this bug, that would help us prioritize it.

[cyptus]

The whole ionic framework (https://ionicframework.com/) and every app that is build for iOS or visited by safari is affected by this bug.

Here is an offical list of major ionic customers who build their apps with this framework:
https://ionicframework.com/customers

[Ryosuke Niwa]

(In reply to cyptus from comment #7)
> The whole ionic framework (https://ionicframework.com/) and every app that
> is build for iOS or visited by safari is affected by this bug.
> 
> Here is an offical list of major ionic customers who build their apps with
> this framework:
> https://ionicframework.com/customers

Rather than saying every app is affected, saying X doesn't work in app Y would be useful. Apple's way of prioritizing things rely heavily on data & exact user impact.

[Niklas Merz]

I can give an example how this affects our app. We are working on an enterprise app with many different forms with lots of inputs item. Imagine a for where you enter contact information like: name, company, street, city etc. Most of our forms have can have dozens intputs as a list. So the user enters the first input an enters information. No they click on the up and down arrows of the accessory bar of the keyboard and nothing happens. Users would expect the focus cursor to jump to the next input just below.

Apps that use Ionic just like ours all will have this problem for their forms, because Ionic uses shadow DOM for almost all their components. An Ionic input element is using shadow DOM and may be one of the most used components because it styles an HTML input just like a native input. That's why Ionic is popular for app devs and probably many enterprise CRUD apps that work like ours with many input fields in forms.

I hope that gives a perspective. Fixing this would really improve the UX of apps built with Ionic.

[cyptus]

Referred to the ionic customers link i provided, please download the app "UNTAPPD" on any iOS device (https://apps.apple.com/de/app/untappd-discover-beer/id449141888).

- Open the app and click on the bottom link "No Account? Register".
- A form will be shown to register your new account.
- iOS keyboard does not provide any option to switch between the input fields, the user needs to click manually on them for each input.

Screenshot: https://abload.de/img/ios-shadowdom-forms-bl1kor.jpg

See the same behavior in the example initially provided by Liam.

[Simanchal Sahu]

Amtrak is currently in the process of upgrading our mobile application that is built with the Ionic Framework. With the latest upgrade including support for the Shadow Dom, this has interfered with the functionality of the accessory bar buttons to navigate forms effectively on iOS. The Amtrak app powers our entire customer facing native experience and with users filling out forms constantly for ticket purchases, the disruption from a typical iOS app flow can cause a significant impact in terms of usability as well as customer satisfaction and retention.

This is a key issue for the ADA community which is one of the top priority for Amtrak in addition to millions of other users.

We would love to see this issue resolved quickly to reduce the impact that this bug has on our app, as well as improve the overall iOS experience for our customers.

We would highly appreciate if this can be resolved at the earliest.


Thanks,
Simanchal Sahu, Sr Principal Technologist
Customer, Architecture and Platform Services - CAPS

[Ryosuke Niwa]

Created attachment 409657 [details]
WIP

I think this will do the trick but need to find a time to write a test & verify that it actually fixes the bug.

[Ryosuke Niwa]

Wensons says:
On UIScriptController, there’s:
undefined keyboardAccessoryBarNext();
undefined keyboardAccessoryBarPrevious();
These call WKWebView SPI that directly call into -accessoryTab: (edited) 

So that's how I'd write a test.

[Tetsuharu Ohzeki [UTC+9]]

I'll try to write a test for this.

Ryosuke, are there any others to need to work?

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 410867 [details]
Patch

I added tests on the top of Ryosuke's original fix and format the patch.

[Ryosuke Niwa]

Comment on attachment 410867 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410867&action=review

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:61
> +    this
> +        .attachShadow({ mode: 'open' })
> +        .innerHTML = '<p><slot></slot></p>';  

Can we just fit this in a single line?

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:72
> +    this
> +        .attachShadow({ mode: 'closed' })
> +        .innerHTML = '<p><slot></slot></p>';  

Ditto.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:78
> +function moveToNextElement() {
> +    return new Promise((resolve) => {

Hm... can we add this function to UIHelper in ui-helper.js?
Maybe call it moveToNextByKeyboardAccessoryBar.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:86
> +function moveToPrevElement() {

Ditto.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:96
> +if (window.testRunner) {
> +    description('Test that Accessory bar next/previous buttons work on inputs in shadow roots');

Please add an instruction on how to test this manually in a browser.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:98
> +    const runTest = async () => {

Can we define this outside if?

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:112
> +                shouldBe(() => document.activeElement.getAttribute('placeholder'), () => element.getAttribute('placeholder'));

shouldBe doesn't take functions. They take strings to be eval'ed.

Can we mirror tests like negative-tabindex-on-shadow-host and just log the element which got focus?
This will allow easier manual testing.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 410941 [details]
Patch

I moved testcases to under LayoutTests/platform/ios/ios/. I'll address points reviewed by Ryosuke in the next patch

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 410953 [details]
Patch

I addressed reviewed points

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #16)
> > LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:112
> > +                shouldBe(() => document.activeElement.getAttribute('placeholder'), () => element.getAttribute('placeholder'));
> 
> shouldBe doesn't take functions. They take strings to be eval'ed.

If we don't pass a function, `shouldBe()` will fail by `ReferrenceError`. It cannot capture a block scupe variables...

[Ryosuke Niwa]

Comment on attachment 410953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410953&action=review

> Source/WebCore/ChangeLog:9
> +        Tests: platform/ios/ios/fast/shadow-dom/accessory-bar-work-on-input-in-shadow-roots.html
> +               platform/ios/ios/fast/shadow-dom/accessory-bar-work-on-input-in-shadow-tree.html

This isn't right. We need to have this test under fast/shadow-dom/ios as you did in your original patch.

platform directories are supposed to only contain -expected.txt files going forward.

> LayoutTests/platform/ios/ios/fast/shadow-dom/accessory-bar-work-on-input-in-shadow-roots.html:14
> +        <input type="text" onfocus="logOnFocus(this)" placeholder="0. Shadow DOM Input 0">

Just do: Array.from(document.querySelectorAll('input')).forEach((input) => input.addEventListener(logOnFocus)).

We also need to test input elements inside a shadow tree, input elements hosts not assigned to any slot,
as well as non-input elements with negative, positive, and 0 tabIndex.

This is why I was suggesting earlier to model it over other existing tabindex tests.

> LayoutTests/platform/ios/ios/fast/shadow-dom/accessory-bar-work-on-input-in-shadow-roots.html:59
> +<pre style="display: none" id="console-for-manually"></pre>

It's okay for this element to exist even inside WebKitTestRunner. Just forget about display: none.
"manually" is an adverb, we need a noun there instead. We usually call this just "log" so: id="log".

> LayoutTests/platform/ios/ios/fast/shadow-dom/accessory-bar-work-on-input-in-shadow-roots.html:130
> +    consoleForManually.style.display = '';

consoleForManually.style.display = null would be better.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 411063 [details]
Patch

[Darin Adler]

Comment on attachment 411063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411063&action=review

> LayoutTests/platform/gtk/TestExpectations:650
> +# These tests are for iOS' accessory bar. Not for other ports.

The preferred pattern here is to skip this directory in the top level TestExpectations file and then set expectations of [Pass] in the iOS TestExpectations file.

Also part of the pattern is to do the expectation for the whole directory, not a file at a time.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 411122 [details]
Patch

[Darin Adler]

Comment on attachment 411122 [details]
Patch

Waiting to see successful test results on iOS-wk2 EWS.

[Ryosuke Niwa]

Comment on attachment 411122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411122&action=review

It's not great to split these tests into multiple files. Now not all the combinations are tested.
Also, it seems like once accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html becomes complete,
all other test files are sort of redundant.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-roots.html:130
> +} else {
> +    document.getElementById('console').style.display = 'none';
> +}

Nit: not curly bracket around a single line statement:
https://webkit.org/code-style-guidelines/#braces-one-line

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-tree.html:33
> +    this
> +        .attachShadow({ mode: 'open' })
> +        .innerHTML = '<p><input onfocus="logOnFocus(this)" placeholder="1. input hosted by input-holder"/></p>';

Can we place attachShadow & this in the same line?

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-tree.html:43
> +    this
> +        .attachShadow({ mode: 'open' })

Ditto.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-in-shadow-tree.html:44
> +        .innerHTML = '<input-holder></input-holder>';

We should add another input before & after this inner shadow host.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:22
> +    <input id="input-host-with-positive-tabindex" tabindex="0" onfocus="logOnFocus(this)" placeholder="1. Shadow host with a positive tabindex" />

Seems like we're missing setting tabIndex to positive & negative values in the slotted content.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:45
> +    <slot></slot>
> +    <input id="inside-shadow-host-with-positive-tabindex" tabindex="0" onfocus="logOnFocus(this)" placeholder="2. Focusable element inside a shadow host with a positive tabindex"/>

It seems like we're missing a test case where set a positive & negative tabIndex values inside a shadow tree.

[Tetsuharu Ohzeki [UTC+9]]

I'm sorry to no reaction for a while.
I'll reply in this weekend.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 412272 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 412272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412272&action=review

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:130
> +class Testcase {
> +    constructor(documentOrShadowRoot, inputGetter) {

It seems that we can just have a helper function that creates a simple dictionary instead of a class with a getter.
For tests, we should strive to make things as simple as possible because we don't want to be debugging test code.

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:145
> +        new Testcase(document, (_) => document.getElementById('first')),

It seems this class is totally unnecessary.
Given we're not dynamically mutating the tree, we can simply compute the input element right here:
e.g. testCase(document, document.getElementById('first'))

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:181
> +    for (const [i, { documentOrShadowRoot, inputElement, }] of stack.entries()) {

Why not just start at i=1 instead?

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:196
> +    for (let i = stack.length - 1; -1 < i; --i) {
> +        if (stack.length - 1 === i) {

Why not just start i = stack.length - 2?

> LayoutTests/resources/ui-helper.js:1383
> +    static moveToNextByKeyboardAccessoryBar() {

Nit: { should be on the next line.

> LayoutTests/resources/ui-helper.js:1392
> +    static moveToPrevByKeyboardAccessoryBar() {

Ditto.

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #28)
> Comment on attachment 412272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412272&action=review
> 
> > LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:130
> > +class Testcase {
> > +    constructor(documentOrShadowRoot, inputGetter) {
> 
> It seems that we can just have a helper function that creates a simple
> dictionary instead of a class with a getter.
> For tests, we should strive to make things as simple as possible because we
> don't want to be debugging test code.


As a viewpoint to simplify the tests, how do you think about change `runTest()` in this patch to following?:

```
async function runTest() {
    const NUMBER_OF_INPUT_ELEMENTS_SHOULD_BE_FOCUSED = 10;
    for (let i = 0; i < NUMBER_OF_INPUT_ELEMENTS_SHOULD_BE_FOCUED; ++i) {
        await UIHelper.moveToNextByKeyboardAccessoryBar();        
    }

    for (let i = NUMBER_OF_INPUT_ELEMENTS_SHOULD_BE_FOCUED; -1 < i; --i) {
        await UIHelper.moveToPrevByKeyboardAccessoryBar();
    }
}
```

I had thought this pattern is at first, but I also thought that 

1. this might be hard to debug failures in CI job.
2. this might be a oversimplified.


So I chose the current patch's approach.
What do you think about this?



> > LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:181
> > +    for (const [i, { documentOrShadowRoot, inputElement, }] of stack.entries()) {
> 
> Why not just start at i=1 instead?

This is because my refactoring was not enough....


> > LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:196
> > +    for (let i = stack.length - 1; -1 < i; --i) {
> > +        if (stack.length - 1 === i) {
> 


Ditto.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 412339 [details]
Patch

I addressed the comment in the  comment #28

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 412433 [details]
Patch

I rebased the patch on bug 218162.

[Darin Adler]

I presume Ryosuke wants to review the revised tests.

[Tetsuharu Ohzeki [UTC+9]]

I also think so.
I'd like to hear Ryosuke's feedback about comment #29

[Ryosuke Niwa]

Comment on attachment 412433 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412433&action=review

New test looks good!

> LayoutTests/fast/shadow-dom/ios/accessory-bar-work-on-input-with-tabindex-in-shadow-tree.html:206
> +} else {
> +    document.getElementById('console').style.display = 'none';
> +}

Nit: no curly braces around a single line statement.

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #34)
> Comment on attachment 412433 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412433&action=review
> 
> New test looks good!


Thank you!
Is "New test" the attachment 412433 [details], or the proposal that I commented in comment #29?



> Nit: no curly braces around a single line statement.

Oops. I'll address...

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 412441 [details]
Patch

[Ryosuke Niwa]

Did you also meant to set cq?

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #37)
> Did you also meant to set cq?

Ah, not yet.
I thought your feedback about #comment 35 will come and I was going to set cq? after that.

[Ryosuke Niwa]

Comment on attachment 412441 [details]
Patch

Great. Thanks for writing a test & refining it many times!

[EWS]

Committed r269059: <https://trac.webkit.org/changeset/269059>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412441 [details].

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #39)

> Great. Thanks for writing a test & refining it many times!

You're welcome!
I also say thank you for your review.

[Niklas Merz]

I just tested our app with iOS 14.5 Developer Beta and this issue is fixed.

Thank you all very much for this patch. This is a huge UX improvement for our users.