Bug 185777 - Click events only fire once when editing
Summary: Click events only fire once when editing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-18 11:55 PDT by Aditya Keerthi
Modified: 2018-05-21 17:19 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.76 KB, patch)
2018-05-18 12:24 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2018-05-18 14:37 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2018-05-18 11:55:19 PDT
Click events are only firing once on editable regions of text - this includes elements that specify contenteditable, as well as <input> elements.
Comment 1 Aditya Keerthi 2018-05-18 11:56:05 PDT
<rdar://problem/9724138>
Comment 2 Aditya Keerthi 2018-05-18 12:24:36 PDT
Created attachment 340726 [details]
Patch
Comment 3 Wenson Hsieh 2018-05-18 13:17:46 PDT
Comment on attachment 340726 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:714
> +    if (node->document().focusedElement() == node)

We probably only want to bail here if we're also editing...

Otherwise, this means that if an element that's not in editable content already has focus, we will no longer flash the tap highlight over that node.
For instance, if you have a <button id='button'> and call button.focus(), we should still see the tap highlight over the button if you tap on it. Perhaps we could check if m_assistedNode == node instead?
Comment 4 Wenson Hsieh 2018-05-18 13:36:43 PDT
Comment on attachment 340726 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:714
>> +    if (node->document().focusedElement() == node)
> 
> We probably only want to bail here if we're also editing...
> 
> Otherwise, this means that if an element that's not in editable content already has focus, we will no longer flash the tap highlight over that node.
> For instance, if you have a <button id='button'> and call button.focus(), we should still see the tap highlight over the button if you tap on it. Perhaps we could check if m_assistedNode == node instead?

Actually, checking against m_assistedNode is probably still won't work in the case of editable elements that have been programmatically focused (select, input, textarea, etc.) prior to the tap...

We really only want to suppress the tap highlight if we've tapped inside a node that is assisted in the UI process (i.e. the keyboard is up and the selection assistant is active).
Comment 5 Aditya Keerthi 2018-05-18 13:39:02 PDT
We also have m_isAssistingNodeDueToUserInteraction in the WebProcess. I'm going to see if I can use that.
Comment 6 Aditya Keerthi 2018-05-18 14:37:00 PDT
Created attachment 340738 [details]
Patch
Comment 7 Aditya Keerthi 2018-05-18 14:41:05 PDT
Wenson pointed out that we can't use m_isAssistingNodeDueToUserInteraction in order to bail out of highlighting, due to a corner case.

Clients that implement _webView:focusShouldStartInputSession: can programmatically focus editable elements and show the keyboard. This would result in m_isAssistingNodeDueToUserInteraction remaining false, and a tap highlight showing if a user were to tap on the element while editing.

For this reason, I have moved the tap highlight check to the UIProcess.
Comment 8 Tim Horton 2018-05-21 16:48:28 PDT
Comment on attachment 340738 [details]
Patch

Megan and I looked at this together.
Comment 9 WebKit Commit Bot 2018-05-21 17:19:34 PDT
Comment on attachment 340738 [details]
Patch

Clearing flags on attachment: 340738

Committed r232040: <https://trac.webkit.org/changeset/232040>
Comment 10 WebKit Commit Bot 2018-05-21 17:19:35 PDT
All reviewed patches have been landed.  Closing bug.