Bug 203292

<rdar://problem/56544829>

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

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.

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

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

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

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

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

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.

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.

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

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.

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.

I'll try to write a test for this. Ryosuke, are there any others to need to work?

Created attachment 410867 [details] Patch I added tests on the top of Ryosuke's original fix and format the patch.

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.

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

Created attachment 410953 [details] Patch I addressed reviewed points

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

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.

Created attachment 411063 [details] Patch

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.

Created attachment 411122 [details] Patch

Comment on attachment 411122 [details] Patch Waiting to see successful test results on iOS-wk2 EWS.

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.

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

Created attachment 412272 [details] Patch

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.

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

Created attachment 412339 [details] Patch I addressed the comment in the comment #28

Created attachment 412433 [details] Patch I rebased the patch on bug 218162.

I presume Ryosuke wants to review the revised tests.

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

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.

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

Created attachment 412441 [details] Patch

Did you also meant to set cq?

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

Comment on attachment 412441 [details] Patch Great. Thanks for writing a test & refining it many times!

Committed r269059: <https://trac.webkit.org/changeset/269059> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412441 [details].

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

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.