Bug 195510 – [ContentChangeObserver] Start observing for content change between touchEnd and mouseMoved start

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

Interesting things could happen in there.

[Radar WebKit Bug Importer]

<rdar://problem/48735695>

[zalan]

Created attachment 364113 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Simon Fraser (smfr)]

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"?

[zalan]

Created attachment 364142 [details]
Patch

[zalan]

(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)

[zalan]

Created attachment 364144 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364144 [details]
Patch

Clearing flags on attachment: 364144

Committed r242675: <https://trac.webkit.org/changeset/242675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.