| Summary: | Double tap to select does not work if the page clears selections on tap, like grammarly.com does | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||
| Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | thorton, wenson_hsieh | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Megan Gardner
2021-01-07 20:05:41 PST
Created attachment 417239 [details]
Patch
Created attachment 417313 [details]
Patch
Comment on attachment 417313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417313&action=review The general approach (deferring the selection change when performing a synthetic click) seems reasonable. Should we also invoke the selection change completion handler in the case where we don't end up completing the synthetic click? > Source/WebKit/ChangeLog:15 > + Reviewed by NOBODY (OOPS!). Nit - reviewed by line should move up before the description. > Source/WebKit/WebProcess/WebPage/WebPage.h:1830 > + WTF::Function<void()> _selectionChangedBlock; This should probably be something like `m_selectionChangedHandler` or `m_selectionChangedFunction`, since it's not an Objective-C block (and it's also a member of a C++ class). > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2137 > + Nit - extra newline here. > LayoutTests/fast/events/touch/ios/double-tap-on-editable-content-for-selection-with-clear-on-touch.html:52 > + var editableElement =document.getElementById('editable'); Nit - space after the = Created attachment 417319 [details]
Patch
Created attachment 417393 [details]
Patch
Created attachment 417394 [details]
Patch
Comment on attachment 417394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417394&action=review > Source/WebKit/ChangeLog:9 > + Double tap to select content did not work on grammerly.com on iPad because this is a desktop website run on a touch-based device, Nit - grammerly.com => grammarly.com > Source/WebKit/ChangeLog:14 > + these two events. This makes for a more expected change of events, and does not let grammerly.com clear a valid selection based off of synthetic Ditto. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:856 > + if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil)) Nit - nil => { }. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1172 > + if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil)) Nit - nil => { }. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1191 > + if (auto selectionChangedHandler = std::exchange(m_selectionChangedHandler, nil)) Nit - nil => { }. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2141 > + We should assert that `m_selectionChangedHandler` is not set here, and/or invoke the previous handler if it is set. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2142 > + m_selectionChangedHandler = [point, granularity, isInteractingWithFocusedElement, completionHandler = WTFMove(completionHandler), webPage = makeWeakPtr(*this), this]()mutable { Nit - space before the `mutable`. Created attachment 417420 [details]
Patch for landing
Committed r271386: <https://trac.webkit.org/changeset/271386> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417420 [details]. |