Bug 195510 - [ContentChangeObserver] Start observing for content change between touchEnd and mouseMoved start
Summary: [ContentChangeObserver] Start observing for content change between touchEnd a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 20:57 PST by zalan
Modified: 2019-03-09 16:09 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.50 KB, patch)
2019-03-08 21:15 PST, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.56 MB, application/zip)
2019-03-09 05:54 PST, EWS Watchlist
no flags Details
Patch (10.56 KB, patch)
2019-03-09 14:41 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (10.56 KB, patch)
2019-03-09 15:21 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-03-08 20:57:59 PST
Interesting things could happen in there.
Comment 1 Radar WebKit Bug Importer 2019-03-08 20:58:34 PST
<rdar://problem/48735695>
Comment 2 zalan 2019-03-08 21:15:45 PST
Created attachment 364113 [details]
Patch
Comment 3 EWS Watchlist 2019-03-09 05:54:34 PST
Comment on attachment 364113 [details]
Patch

Attachment 364113 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11438456

New failing tests:
accessibility/mac/selection-notification-focus-change.html
Comment 4 EWS Watchlist 2019-03-09 05:54:36 PST
Created attachment 364127 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 5 Simon Fraser (smfr) 2019-03-09 11:35:47 PST
Comment on attachment 364113 [details]
Patch

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

> Source/WebCore/page/ios/ContentChangeObserver.h:140
> +    bool isInBetweenTouchEndAndMouseMoved() const { return m_isInBetweenTouchEndAndMouseMoved; }

Maybe just "isBetweenTouchEndAndMouseMoved" everywhere?

> Source/WebCore/page/ios/ContentChangeObserver.h:198
> +        || isInBetweenTouchEndAndMouseMoved()

Why use the getter here but not for the others?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:868
> +        m_potentialTapNode->document().contentChangeObserver().didCancelTouchEvent();

We're in cancelPotentialTapInFrame() but you're calling didCancelTouchEvent(), suggesting that didCancelTouchEvent has the wrong name. Aren't we really canceling the pending synthetic "click"?
Comment 6 zalan 2019-03-09 14:41:01 PST
Created attachment 364142 [details]
Patch
Comment 7 zalan 2019-03-09 14:44:49 PST
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 364113 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364113&action=review
> 
> > Source/WebCore/page/ios/ContentChangeObserver.h:140
> > +    bool isInBetweenTouchEndAndMouseMoved() const { return m_isInBetweenTouchEndAndMouseMoved; }
> 
> Maybe just "isBetweenTouchEndAndMouseMoved" everywhere?
Good idea.

> 
> > Source/WebCore/page/ios/ContentChangeObserver.h:198
> > +        || isInBetweenTouchEndAndMouseMoved()
> 
> Why use the getter here but not for the others?
I haven't made up my mind whether I want getters for all those flags. Let's just use the member directly for now as you suggested.

> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:868
> > +        m_potentialTapNode->document().contentChangeObserver().didCancelTouchEvent();
> 
> We're in cancelPotentialTapInFrame() but you're calling
> didCancelTouchEvent(), suggesting that didCancelTouchEvent has the wrong
> name. Aren't we really canceling the pending synthetic "click"?
You are right, we are not canceling the touch. However we are not canceling the synthetic click either since at this point we haven't even started it yet. I'll just use "willNotProceedWithClick" (unless you have a better name)
Comment 8 zalan 2019-03-09 15:21:32 PST
Created attachment 364144 [details]
Patch
Comment 9 WebKit Commit Bot 2019-03-09 16:09:11 PST
Comment on attachment 364144 [details]
Patch

Clearing flags on attachment: 364144

Committed r242675: <https://trac.webkit.org/changeset/242675>
Comment 10 WebKit Commit Bot 2019-03-09 16:09:12 PST
All reviewed patches have been landed.  Closing bug.