Bug 200531 – REGRESSION (iOS 13): united.com web forms do not respond to taps

• Component: UI Events
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Tapping on form controls on united.com, to signup for milage plus for example, doesn't bring up the software keyboard because form controls are never focused.

<rdar://problem/53581411>

[Ryosuke Niwa]

Created attachment 375786 [details]
Fixes the bug

[Antti Koivisto]

Comment on attachment 375786 [details]
Fixes the bug

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

> Source/WebCore/page/Quirks.cpp:417
> +    DOMTokenList& classList = parentElement->classList();
> +    if (!classList.contains("feedback") || !classList.contains("feedback-mid"))
> +        return false;
> +
> +    if (!equalLettersIgnoringASCIICase(topPrivatelyControlledDomain(m_document->url().host().toString()), "united.com"))
> +        return false;

It is bit weird to test for class names before the host. It would be better to write this so that adding another case doesn't require reorganizing the code.

[EWS Watchlist]

Comment on attachment 375786 [details]
Fixes the bug

Attachment 375786 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12878219

New failing tests:
http/tests/css/filters-on-iframes-transform.html

[EWS Watchlist]

Created attachment 375794 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[zalan]

Comment on attachment 375786 [details]
Fixes the bug

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

> Source/WebCore/page/ios/ContentChangeObserver.cpp:619
> +        if (!m_element.document().quirks().shouldIgnoreContentChange(m_element))

This check should go to ContentChangeObserver::shouldObserveVisibilityChangeForElement

[Ryosuke Niwa]

(In reply to zalan from comment #5)
> Comment on attachment 375786 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375786&action=review
> 
> > Source/WebCore/page/ios/ContentChangeObserver.cpp:619
> > +        if (!m_element.document().quirks().shouldIgnoreContentChange(m_element))
> 
> This check should go to
> ContentChangeObserver::shouldObserveVisibilityChangeForElement

Okay, will move.

[Ryosuke Niwa]

Created attachment 375851 [details]
Patch for landing

[Ryosuke Niwa]

Ping reviewers

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #8)
> Ping reviewers

It looks like Antti already r+'d your patch earlier (with the suggestion of moving the host name check before the class name check).

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #9)
> (In reply to Ryosuke Niwa from comment #8)
> > Ping reviewers
> 
> It looks like Antti already r+'d your patch earlier (with the suggestion of
> moving the host name check before the class name check).

He did but due to addressing Zalan's comment above, the patch has substantially changed .

[Ryosuke Niwa]

Committed r248463: <https://trac.webkit.org/changeset/248463>

[Darin Adler]

Comment on attachment 375851 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:17
> +        The bug is caused by the content change observer detecting âSite Feedbackâ link at the bottom of
> +        the page (https://www.united.com/ual/en/US/account/enroll/default) constantly getting re-generated
> +        in every frame via requestAnimationFrame when the page is opened with iPhone UA string.
> +        Note that the content re-generation can be reproduced even in Chrome if iPhone UA string is used.
> +
> +        Ignore this constant content change in ContentChangeObserver as a site specific quirk.
> +
> +        In the future, we should make ContentChangeObserver observe the final location of each element
> +        being observed so that we can ignore content that like this which is placed outside the viewport,
> +        and/or far away from where the user tapped.

Ideally comments inside Quirks::shouldIgnoreContentChange would make this plan for the future a little more clear.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #12)
> Comment on attachment 375851 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375851&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        The bug is caused by the content change observer detecting âSite Feedbackâ link at the bottom of
> > +        the page (https://www.united.com/ual/en/US/account/enroll/default) constantly getting re-generated
> > +        in every frame via requestAnimationFrame when the page is opened with iPhone UA string.
> > +        Note that the content re-generation can be reproduced even in Chrome if iPhone UA string is used.
> > +
> > +        Ignore this constant content change in ContentChangeObserver as a site specific quirk.
> > +
> > +        In the future, we should make ContentChangeObserver observe the final location of each element
> > +        being observed so that we can ignore content that like this which is placed outside the viewport,
> > +        and/or far away from where the user tapped.
> 
> Ideally comments inside Quirks::shouldIgnoreContentChange would make this
> plan for the future a little more clear.

I think Zalan is already tracking it elsewhere.