Bug 167486

Summary: Don't flash a tap highlight for the entirety of an editable WKWebView
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, enrica, esprehn+autocc, kangil.han, mitz
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=167488
Attachments:
Description Flags
Patch
none
Patch mitz: review+

Tim Horton
Reported 2017-01-26 20:35:35 PST
Don't flash a tap highlight for the entirety of an editable WKWebView
Attachments
Patch (1.83 KB, patch)
2017-01-26 20:35 PST, Tim Horton 		no flags
DetailsFormatted DiffDiff
Patch (3.15 KB, patch)
2017-01-27 16:51 PST, Tim Horton 		mitz: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tim Horton
Comment 1 2017-01-26 20:35:52 PST
Created attachment 299903 [details] Patch
mitz
Comment 2 2017-01-26 21:02:32 PST
Comment on attachment 299903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299903&action=review > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:639 > + if (is<HTMLBodyElement>(node) && m_page->isEditable()) > + return; > + Does this exclude the body of subframes as well? Is that expected?
Tim Horton
Comment 3 2017-01-26 21:05:06 PST
That is a fair question; this will certainly stop flashing in that case too, but I don't know that that is necessarily a bad thing. To me, the tap highlight is much more about small buttons/links/input fields, and less about (usually) large things like frames. But we could limit it to just the main frame body. What do you think?
mitz
Comment 4 2017-01-26 21:13:35 PST
(In reply to comment #3) > That is a fair question; this will certainly stop flashing in that case too, > but I don't know that that is necessarily a bad thing. To me, the tap > highlight is much more about small buttons/links/input fields, and less > about (usually) large things like frames. But we could limit it to just the > main frame body. What do you think? If the goal is to make editable views behave more like text views, or to “not flash a tap highlight for the entirety of an editable WKWebView”, then applying this to subframes’ bodies doesn’t seem to support the goal. I believe that there is already a size-based filter to prevent large highlights, so if a subframe’s body or an image are sufficiently large, they won’t highlight. There is no reason to infer size from element type.
Tim Horton
Comment 5 2017-01-26 21:17:54 PST
(In reply to comment #4) > (In reply to comment #3) > > That is a fair question; this will certainly stop flashing in that case too, > > but I don't know that that is necessarily a bad thing. To me, the tap > > highlight is much more about small buttons/links/input fields, and less > > about (usually) large things like frames. But we could limit it to just the > > main frame body. What do you think? > > If the goal is to make editable views behave more like text views, I think that is the goal. > or to “not flash a tap highlight for the entirety of an editable WKWebView”, then > applying this to subframes’ bodies doesn’t seem to support the goal. I > believe that there is already a size-based filter to prevent large > highlights, Does not seem like a very good filter if it lets a iPad-sized <body> get highlighted, but OK. > so if a subframe’s body or an image are sufficiently large, they > won’t highlight. There is no reason to infer size from element type. Fair! I will adjust the patch.
mitz
Comment 6 2017-01-26 21:33:46 PST
(In reply to comment #5) > Does not seem like a very good filter if it lets a iPad-sized <body> get > highlighted, but OK. Filed bug 167488 for that.
Tim Horton
Comment 7 2017-01-27 16:51:50 PST
Created attachment 299982 [details] Patch
mitz
Comment 8 2017-01-28 16:14:56 PST
Comment on attachment 299982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299982&action=review > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:637 > + if (node == m_page->mainFrame().document()->body() && m_page->isEditable()) Is it better to evaluate these in the reverse order?
Tim Horton
Comment 9 2017-01-28 16:18:25 PST
https://trac.webkit.org/changeset/211343
Note You need to log in before you can comment on or make changes to this bug.