Bug 200531 - REGRESSION (iOS 13): united.com web forms do not respond to taps
Summary: REGRESSION (iOS 13): united.com web forms do not respond to taps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL: https://www.united.com/ual/en/US/acco...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-08 00:27 PDT by Ryosuke Niwa
Modified: 2019-08-09 14:39 PDT (History)
6 users (show)

See Also:


Attachments
Fixes the bug (4.45 KB, patch)
2019-08-08 00:34 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.34 MB, application/zip)
2019-08-08 02:39 PDT, Build Bot
no flags Details
Patch for landing (4.35 KB, patch)
2019-08-08 16:13 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-08-08 00:27:57 PDT
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>
Comment 1 Ryosuke Niwa 2019-08-08 00:34:46 PDT
Created attachment 375786 [details]
Fixes the bug
Comment 2 Antti Koivisto 2019-08-08 01:19:56 PDT
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.
Comment 3 Build Bot 2019-08-08 02:39:21 PDT
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
Comment 4 Build Bot 2019-08-08 02:39:24 PDT
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
Comment 5 zalan 2019-08-08 06:37:34 PDT
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
Comment 6 Ryosuke Niwa 2019-08-08 16:13:21 PDT
(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.
Comment 7 Ryosuke Niwa 2019-08-08 16:13:30 PDT
Created attachment 375851 [details]
Patch for landing
Comment 8 Ryosuke Niwa 2019-08-08 23:15:25 PDT
Ping reviewers
Comment 9 Wenson Hsieh 2019-08-08 23:22:27 PDT
(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).
Comment 10 Ryosuke Niwa 2019-08-08 23:33:50 PDT
(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 .
Comment 11 Ryosuke Niwa 2019-08-09 00:17:46 PDT
Committed r248463: <https://trac.webkit.org/changeset/248463>
Comment 12 Darin Adler 2019-08-09 08:06:16 PDT
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.
Comment 13 Ryosuke Niwa 2019-08-09 14:39:52 PDT
(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.