Bug 144451

Summary: [iOS] DOM click event may not be dispatched when page has :active style and <input type="search">
Product: WebKit Reporter: Michael "Spell" Spellacy <michael.spellacy>
Component: CSSAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dbates, dino, eric.carlson, esprehn+autocc, glenn, jer.noble, kondapallykalyan, michael.spellacy, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 525.x (Safari 3.2)   
Hardware: iPhone / iPad   
OS: iOS 8.2   
URL: https://tmpworldwide.github.io/bugs/ios-tappy-bug.html
Bug Depends on:    
Bug Blocks: 152449    
Attachments:
Description Flags
[Web Archive] YouTube embed not working in iOS 8 Safari and Chrome
none
Patch and layout test
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Patch and layout test
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch and layout tests
none
[Alternative] Patch and layout tests
none
Patch and layout tests
none
Patch and layout tests none

Description Michael "Spell" Spellacy 2015-04-30 08:49:42 PDT
iPhone 5s Model: ME341LL/A iOS Version: 8.3 (12F0)

Under certain conditions, YouTube videos embedded within iframes cannot be viewed when a user clicks on the video.

In our particular instance, the conditions had to be just right. A "perfect storm" of issues, if you will. :)

The Bug

Applying the :active pseudo-class to a universal selector (*) and including a property of -webkit-tap-highlight-color seems to be the culprit.

*:active {
-webkit-tap-highlight-color: tomato;
}

Now, here is the strange part. The bug is only triggered when the above CSS block is present and there is an input element present on the page with a type attribute value of "search". I know, crazy, right?

<input type="search" placeholder="Search"/>

Another oddity here is that when you apply focus to the search input, type something in, and then attempt to play video, it will then work.

The Solution

Either renaming the input type to "text" or removing -webkit-tap-highlight-color (likely preferred solution) should alleviate the issue, but this obviously might be a rendering bug.

You can the same overview here, and see the bug in action for yourself on any current iOS device. 

https://tmpworldwide.github.io/bugs/ios-tappy-bug.html
Comment 1 Daniel Bates 2015-05-15 20:31:30 PDT
Created attachment 253260 [details]
[Web Archive] YouTube embed not working in iOS 8 Safari and Chrome

For historical preservation, a web archive of <https://tmpworldwide.github.io/bugs/ios-tappy-bug.html> as it appeared on 05/15/2015.
Comment 2 Daniel Bates 2015-10-13 16:25:35 PDT
This issue isn't specific to using the CSS property -webkit-tap-highlight-color. This issue can be reproduced on the example site (attachment #253260 [details]) with an arbitrary CSS property in the definition of *:active.
Comment 3 Daniel Bates 2015-10-13 16:25:40 PDT
On iOS we only dispatch a DOM click event if the content does not change as part of dispatching a DOM mousemove event at the tapped element. In particular, we do not dispatch a DOM click event if there is a visibility change to some element on the page as part of dispatching a mousemove event at the tapped element. Notice that we update the active state of elements as part of dispatching a mousemove event, forcing a full a style recalculation of affected elements, including setting its CSS property visibility to visible. And we update the visibility of the cancel button whenever its CSS property visibility disagrees with the CSS property visibility of its associated search field or the value of the search field changes. So, we always update the visibility of the cancel button since its CSS property visibility changes when updating the active state of the HTML body; => the content of the page changed => we do not dispatch a DOM click event.
Comment 4 Radar WebKit Bug Importer 2015-10-13 16:27:00 PDT
<rdar://problem/23099482>
Comment 5 Daniel Bates 2015-10-13 16:29:23 PDT
Created attachment 263037 [details]
Patch and layout test
Comment 6 Daniel Bates 2015-10-13 16:48:56 PDT
Comment on attachment 263037 [details]
Patch and layout test

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

> Source/WebCore/ChangeLog:3
> +        [iOS] DOM click event may not be dispatched when page has :active style and <input type="search">rch">

We remove trailing 'rch">' from title before landing.

> LayoutTests/ChangeLog:3
> +        [iOS] DOM click event may not be dispatched when page has :active style and <input type="search">rch">

Ditto.
Comment 7 Simon Fraser (smfr) 2015-10-13 16:57:42 PDT
Comment on attachment 263037 [details]
Patch and layout test

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

> Source/WebCore/ChangeLog:9
> +        Without loss of generality, fixes an issue where a DOM click event is not dispatched to an

"Without loss of generality" makes this sound like a maths thesis, and adds nothing.

> Source/WebCore/ChangeLog:22
> +        Test: platform/ios-simulator/ios/fast/events/can-click-element-on-page-with-active-pseudo-class-and-search-field.html

We should not be adding new tests under platform.

> Source/WebCore/ChangeLog:32
> +        (WebCore::SearchInputType::updateCancelButtonVisibility): Added. Updates the CSS property display
> +        of the cancel button element such that it is visible when its associated search field is non-empty
> +        or the argument forceUpdate is true (defaults to false). We update the CSS property display instead
> +        of the CSS property visibility as a means to show and hide the cancel button because it is consistent
> +        with how we show and hide other buttons (e.g. the AutoFill button) and the former destroys the render
> +        tree associated with the cancel button when the cancel button is not shown, allowing its memory to be

I would like this to justify why you moved the code from rendering to DOM.
Comment 8 Build Bot 2015-10-13 17:02:36 PDT
Comment on attachment 263037 [details]
Patch and layout test

Attachment 263037 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/280895

New failing tests:
fast/forms/search-disabled-readonly.html
fast/forms/searchfield-heights.html
fast/forms/search/search-padding-cancel-results-buttons.html
fast/forms/placeholder-pseudo-style.html
fast/forms/search-vertical-alignment.html
fast/forms/search-zoomed.html
fast/replaced/width100percent-searchfield.html
fast/forms/search-cancel-button-style-sharing.html
fast/css/input-search-padding.html
fast/forms/control-restrict-line-height.html
fast/forms/search-abs-pos-cancel-button.html
fast/forms/search-cancel-button-events.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/placeholder-position.html
fast/forms/box-shadow-override.html
accessibility/mac/search-field-cancel-button.html
fast/forms/search-cancel-button-mouseup.html
fast/forms/search-transformed.html
fast/css/focus-ring-exists-for-search-field.html
fast/forms/search-rtl.html
fast/forms/search-styled.html
fast/css/text-overflow-input.html
fast/repaint/search-field-cancel.html
fast/forms/input-appearance-height.html
fast/forms/search/search-size-with-decorations.html
Comment 9 Build Bot 2015-10-13 17:02:41 PDT
Created attachment 263039 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 10 Build Bot 2015-10-13 17:06:27 PDT
Comment on attachment 263037 [details]
Patch and layout test

Attachment 263037 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/280899

New failing tests:
fast/forms/search-disabled-readonly.html
fast/forms/searchfield-heights.html
fast/forms/search/search-padding-cancel-results-buttons.html
fast/forms/placeholder-pseudo-style.html
fast/forms/search-vertical-alignment.html
fast/forms/search-zoomed.html
fast/replaced/width100percent-searchfield.html
fast/forms/search-cancel-button-style-sharing.html
fast/css/input-search-padding.html
fast/forms/control-restrict-line-height.html
fast/forms/search-abs-pos-cancel-button.html
fast/forms/search-cancel-button-events.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/placeholder-position.html
fast/forms/box-shadow-override.html
accessibility/mac/search-field-cancel-button.html
fast/forms/search-cancel-button-mouseup.html
fast/forms/search-transformed.html
fast/css/focus-ring-exists-for-search-field.html
fast/forms/search-rtl.html
fast/forms/search-styled.html
fast/css/text-overflow-input.html
fast/repaint/search-field-cancel.html
fast/forms/input-appearance-height.html
fast/forms/search/search-size-with-decorations.html
Comment 11 Build Bot 2015-10-13 17:06:31 PDT
Created attachment 263040 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 12 Daniel Bates 2015-10-13 17:23:25 PDT
(In reply to comment #7)
> Comment on attachment 263037 [details]
> Patch and layout test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263037&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Without loss of generality, fixes an issue where a DOM click event is not dispatched to an
> 
> "Without loss of generality" makes this sound like a maths thesis, and adds
> nothing.
> 

Will remove.

For completeness, I used this phrase to convey that defining a CSS :active pseudo-class explicitly on the HTML body is not necessary.

> > Source/WebCore/ChangeLog:22
> > +        Test: platform/ios-simulator/ios/fast/events/can-click-element-on-page-with-active-pseudo-class-and-search-field.html
> 
> We should not be adding new tests under platform.
> 

Will move tests to LayoutTests/fast/events.

> > Source/WebCore/ChangeLog:32
> > +        (WebCore::SearchInputType::updateCancelButtonVisibility): Added. Updates the CSS property display
> > +        of the cancel button element such that it is visible when its associated search field is non-empty
> > +        or the argument forceUpdate is true (defaults to false). We update the CSS property display instead
> > +        of the CSS property visibility as a means to show and hide the cancel button because it is consistent
> > +        with how we show and hide other buttons (e.g. the AutoFill button) and the former destroys the render
> > +        tree associated with the cancel button when the cancel button is not shown, allowing its memory to be
> 
> I would like this to justify why you moved the code from rendering to DOM.

Will justify movement of code.
Comment 13 Daniel Bates 2015-10-13 20:39:10 PDT
Comment on attachment 263037 [details]
Patch and layout test

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

> Source/WebCore/ChangeLog:31
> +        (WebCore::SearchInputType::updateCancelButtonVisibility): Added. Updates the CSS property display
> +        of the cancel button element such that it is visible when its associated search field is non-empty
> +        or the argument forceUpdate is true (defaults to false). We update the CSS property display instead
> +        of the CSS property visibility as a means to show and hide the cancel button because it is consistent
> +        with how we show and hide other buttons (e.g. the AutoFill button) and the former destroys the render

Actually, we need to update the CSS visibility property instead of using the CSS display property since we want the cancel button to take up space even when not visible so as to be consistent with the appearance of NSSearchField.
Comment 14 Daniel Bates 2015-10-13 20:40:53 PDT
Created attachment 263052 [details]
Patch and layout test
Comment 15 Build Bot 2015-10-13 21:06:15 PDT
Comment on attachment 263052 [details]
Patch and layout test

Attachment 263052 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/281696

New failing tests:
fast/forms/search-cancel-in-invisible-elements.html
fast/events/can-click-element-on-page-with-active-pseudo-class-and-search-field.html
Comment 16 Build Bot 2015-10-13 21:06:20 PDT
Created attachment 263054 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 17 Build Bot 2015-10-13 21:13:09 PDT
Comment on attachment 263052 [details]
Patch and layout test

Attachment 263052 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/281726

New failing tests:
fast/forms/search-cancel-in-invisible-elements.html
Comment 18 Build Bot 2015-10-13 21:13:13 PDT
Created attachment 263055 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 19 Daniel Bates 2015-10-14 18:24:12 PDT
Created attachment 263128 [details]
Patch and layout tests

Updated patch to update the visibility of the cancel button when the visibility of the search field is changed programmatically. Added tests for such changes as well as to ensure we update the cancel button (if needed) when the search field is disabled or made read-only.
Comment 20 Daniel Bates 2015-10-14 18:25:50 PDT
Created attachment 263129 [details]
[Alternative] Patch and layout tests

An alternative patch to fix this issue that keeps the cancel button visibility logic in RenderSearchField.
Comment 21 Daniel Bates 2015-10-14 18:28:00 PDT
Created attachment 263130 [details]
Patch and layout tests

Rebased patch.
Comment 22 Daniel Bates 2015-10-15 09:09:02 PDT
Committed r191113: <http://trac.webkit.org/changeset/191113>
Comment 23 Daniel Bates 2015-10-22 15:11:05 PDT
(In reply to comment #22)
> Committed r191113: <http://trac.webkit.org/changeset/191113>

This regressed the visibility of the search cancel button when the search field is empty or showing placeholder text.
Comment 24 Daniel Bates 2015-10-22 15:15:13 PDT
Reverted r191113 for reason:

Rollout r144451 since it regressed the visibility of the search cancel button when a search field is empty or showing placeholder text. Further investigation is needed.

Committed r191480: <http://trac.webkit.org/changeset/191480>
Comment 25 Csaba Osztrogonác 2015-11-17 02:50:46 PST
Comment on attachment 263052 [details]
Patch and layout test

Cleared Simon Fraser's review+ from obsolete attachment 263052 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 26 Daniel Bates 2015-12-12 16:14:36 PST
Created attachment 267247 [details]
Patch and layout tests

Updated patch to ignore visibility changes to user agent shadow DOM elements, such as the search field cancel button.
Comment 27 WebKit Commit Bot 2015-12-14 10:07:45 PST
Comment on attachment 267247 [details]
Patch and layout tests

Clearing flags on attachment: 267247

Committed r194038: <http://trac.webkit.org/changeset/194038>
Comment 28 WebKit Commit Bot 2015-12-14 10:07:51 PST
All reviewed patches have been landed.  Closing bug.