209888 – Track editable elements on screen

RESOLVED FIXED 209888

Track editable elements on screen

https://bugs.webkit.org/show_bug.cgi?id=209888

Summary Track editable elements on screen

Daniel Bates

Reported 2020-04-01 17:20:19 PDT

Track editable elements on screen.

Attachments
Rough patch - just for bots (37.91 KB, patch) 2020-04-01 17:22 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Patch and tests (47.74 KB, patch) 2020-04-02 15:01 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Patch and tests (48.04 KB, patch) 2020-04-02 15:18 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Patch and tests (77.86 KB, patch) 2020-04-03 17:04 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
To Land (124.09 KB, patch) 2020-04-05 11:26 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
To Land Once Bug #210041 is fixed (124.10 KB, patch) 2020-04-05 17:22 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
To land before bug #210041 is fixed (if desired) (126.56 KB, patch) 2020-04-05 17:26 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
To land after bug #210041 (140.74 KB, patch) 2020-04-08 13:05 PDT, Daniel Bates	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Daniel Bates

Comment 1 2020-04-01 17:22:20 PDT

Created attachment 395228 [details] Rough patch - just for bots

Daniel Bates

Comment 2 2020-04-01 21:02:05 PDT

Comment on attachment 395228 [details] Rough patch - just for bots View in context: https://bugs.webkit.org/attachment.cgi?id=395228&action=review > LayoutTests/editing/editability/editable-region/input-basic.html:1 > +<!DOCTYPE html> Oops! This file shouldn't have been included. Rename kerfuffle....Ignore this and this patch passed all the bots!! Yay!

Daniel Bates

Comment 3 2020-04-02 01:09:36 PDT

Need to fix up floats....see bug #209896

Daniel Bates

Comment 4 2020-04-02 01:20:46 PDT

Comment on attachment 395228 [details] Rough patch - just for bots View in context: https://bugs.webkit.org/attachment.cgi?id=395228&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:3100 > + context.setFillColor({ 128, 0, 128, 50 }); Debating whether to conditioning this on a new debug overlay type that could be exposed in the Debug menu.... I'm debating whether to do this now or in a future patch... I'll sleep on it.

Radar WebKit Bug Importer

Comment 5 2020-04-02 01:22:41 PDT

<rdar://problem/61196886>

Daniel Bates

Comment 6 2020-04-02 15:01:24 PDT

Created attachment 395306 [details] Patch and tests

Daniel Bates

Comment 7 2020-04-02 15:08:34 PDT

Comment on attachment 395306 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395306&action=review > LayoutTests/ChangeLog:9 > + Add some tests. Some of these tests were derived from a torture test page written by Simon Fraser. Thanks Simon! I will amend this ChangeLog to say so much before landing.

Daniel Bates

Comment 8 2020-04-02 15:18:01 PDT

Created attachment 395310 [details] Patch and tests

Simon Fraser (smfr)

Comment 9 2020-04-02 15:22:08 PDT

Comment on attachment 395306 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395306&action=review > Source/WebCore/rendering/RenderBlock.cpp:1261 > + needsTraverseDescendants = needsTraverseDescendants || document().mayHaveElementsWithNonAutoTouchAction() || document().mayHaveEditableElements(); Shame that this is #ifdeffed and has to know which things contribute to event regions. This needs cleaning up. > Source/WebCore/rendering/RenderLayerBacking.cpp:3070 > + // Paint rects for touch action. 'eventRegion' is just the interactive part tho. Comment was accurate, but we lacked a comment for the touch-action part. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2106 > +#if PLATFORM(IOS_FAMILY) > + if (!WebKit::containsEditableElementsInRect(_contentView, rectInRootViewCoordinates)) { > + completionHandler(@[]); > + return; > + } > +#endif We have iOS-specific files. Can this be moved into a function to avoid the #ifdeffing? > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:79 > +static void collectDescendantViewsInRect(Vector<UIView *, 16>& viewsInRect, UIView *parent, CGRect rect) The name of this method conflicts with the fact that it is testing 'containsEditableElementsInRect'. It's not a general-purpose method. It also gets things wrong, because it doesn't test that a node with an eventRegion() for a given point overlaps an underlying node with an editable region for that point. It should be possible to make a testcase that shows this flaw. > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:180 > + return node->eventRegion().containsEditableElementsInRect(WebCore::IntRect { [hitView convertRect:rect fromView:rootView] }); collectDescendantViewsInRect() did the containsEditableElementsInRect() check already. Why do you do it again here?

Daniel Bates

Comment 10 2020-04-02 15:43:17 PDT

Comment on attachment 395310 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395310&action=review > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:102 > + return node->eventRegion().containsEditableElementsInRect(WebCore::IntRect { subviewRect }); So, Simon has pointed out to me that I can make this method more optimal by changing this to: node->eventRegion().contains(WebCore::IntRect { subviewRect })

Daniel Bates

Comment 11 2020-04-02 16:30:43 PDT

Comment on attachment 395306 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395306&action=review Thanks Simon for the feedback...going to address. >> Source/WebCore/rendering/RenderLayerBacking.cpp:3070 >> + // Paint rects for touch action. > > 'eventRegion' is just the interactive part tho. Comment was accurate, but we lacked a comment for the touch-action part. I'll put back the original comment + add my comment. >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2106 >> +#endif > > We have iOS-specific files. Can this be moved into a function to avoid the #ifdeffing? This was a last minute addition....my plan is to move the implementation of this function into WKContentViewInteraction.mm, but I decided to do this in a **follow** up patch to keep this patch focused on the event region stuff. I hope you're okay with this. >> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:79 >> +static void collectDescendantViewsInRect(Vector<UIView *, 16>& viewsInRect, UIView *parent, CGRect rect) > > The name of this method conflicts with the fact that it is testing 'containsEditableElementsInRect'. It's not a general-purpose method. > > It also gets things wrong, because it doesn't test that a node with an eventRegion() for a given point overlaps an underlying node with an editable region for that point. It should be possible to make a testcase that shows this flaw. I'll change containsEditableElementsInRect() to contains() that was a slip up and I'll take a look at the edge cases you privately shared with me. >> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:180 >> + return node->eventRegion().containsEditableElementsInRect(WebCore::IntRect { [hitView convertRect:rect fromView:rootView] }); > > collectDescendantViewsInRect() did the containsEditableElementsInRect() check already. Why do you do it again here? Because I messed up and had containsEditableElementsInRect() on 🧠.

Daniel Bates

Comment 12 2020-04-03 17:04:31 PDT

Created attachment 395419 [details] Patch and tests The hit test tests are not exhaustive. When I have a moment, I will look to add more.

Daniel Bates

Comment 13 2020-04-03 17:10:41 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review > LayoutTests/editing/ios/editable-region/hit-test-fixed.html:44 > + extra empty line

Simon Fraser (smfr)

Comment 14 2020-04-03 18:00:16 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review > Source/WebCore/dom/Document.h:1249 > #if PLATFORM(IOS_FAMILY) This should be wrapped in a TOUCH_ACTION #ifdef > Source/WebCore/dom/Document.h:1254 > + bool mayHaveEditableElements() const { return m_mayHaveEditableElements; } > + void setMayHaveEditableElements() { m_mayHaveEditableElements = true; } And this should be wrapped in an EDITABLE_REGIONS #ifdef > Source/WebCore/rendering/RenderLayerBacking.cpp:1626 > #if PLATFORM(IOS_FAMILY) > hasTouchActionElements = renderer().document().mayHaveElementsWithNonAutoTouchAction(); > + hasEditableElements = renderer().document().mayHaveEditableElements(); > #endif Would be nice not to use a platform #ifdef. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2093 > +- (BOOL)_mightContainEditableElementsInRect:(CGRect)rect _mayContain? > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.h:84 > +bool mightContainEditableElementsInRect(UIView *rootView, const WebCore::FloatRect&); mayContain > Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:122 > + // Editable region > + > + virtual bool mightContainEditableElementsInRect(unsigned, unsigned, unsigned, unsigned) { notImplemented(); return false; } Blank line. mayContain?

Daniel Bates

Comment 15 2020-04-03 21:19:52 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:102 > + return node->eventRegion().contains(WebCore::IntRect { subviewRect }); This needs to be intersect(). That's why the hit-test-fixed.html test fails: the specified rect may not be wholly contained.

Daniel Bates

Comment 16 2020-04-03 21:46:42 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review >> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:102 >> + return node->eventRegion().contains(WebCore::IntRect { subviewRect }); > > This needs to be intersect(). That's why the hit-test-fixed.html test fails: the specified rect may not be wholly contained. Actually, the original patch, which called containsEditableElementInRect() was correct (that's why the test RequestTextInputContext.CompositedOverlap fails: it expects when there is overlap -requestTextInputContext will return all intersecting editable elements). Having said that, using containsEditableElementInRect() disagreed with the name of this function. Moreover, to implement mightContainEditableElementsInRect() we just need to find *one* view that contains an editable element in the rect. Keep in mind that if there is a composited overlapping element that does *not* any editable element then its editable region will be empty. To summarize, I think I can simplify this function and inline its implementation into mightContainEditableElementsInRect().

Antti Koivisto

Comment 17 2020-04-03 21:53:01 PDT

Maybe we should rename EventRegion to InteractionRegion or similar.

Daniel Bates

Comment 18 2020-04-03 21:54:11 PDT

(In reply to Antti Koivisto from comment #17) > Maybe we should rename EventRegion to InteractionRegion or similar. Yes, but I hope you don't mind I do this in a follow up....

Daniel Bates

Comment 19 2020-04-03 21:59:07 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:188 > + contexts = [webView synchronouslyRequestTextInputContextsInRect:CGRectMake(170, 150, 10, 10)]; No need to load this again. > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:192 > + [webView synchronouslyLoadTestPageNamed:@"editable-region-composited-and-non-composited-overlap"]; No need to load this again. > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:209 > + [webView synchronouslyLoadTestPageNamed:@"editable-region-composited-and-non-composited-overlap"]; No need to load this again. > Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:214 > + [webView synchronouslyLoadTestPageNamed:@"editable-region-composited-and-non-composited-overlap"]; No need to load this again.

Daniel Bates

Comment 20 2020-04-04 15:51:46 PDT

iOS API failure is due to bug #210010.

Daniel Bates

Comment 21 2020-04-05 10:52:38 PDT

Comment on attachment 395419 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=395419&action=review >> Source/WebCore/dom/Document.h:1249 >> #if PLATFORM(IOS_FAMILY) > > This should be wrapped in a TOUCH_ACTION #ifdef Not going to do this because there is no such define, but more importantly we **don't** compile-out any other touch-action code. Whether we should compile out the touch-action code I haven't thought about that... >> Source/WebCore/dom/Document.h:1254 >> + void setMayHaveEditableElements() { m_mayHaveEditableElements = true; } > > And this should be wrapped in an EDITABLE_REGIONS #ifdef Will do. >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2093 >> +- (BOOL)_mightContainEditableElementsInRect:(CGRect)rect > > _mayContain? Will rename. >> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.h:84 >> +bool mightContainEditableElementsInRect(UIView *rootView, const WebCore::FloatRect&); > > mayContain Will rename. >>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:102 >>> + return node->eventRegion().contains(WebCore::IntRect { subviewRect }); >> >> This needs to be intersect(). That's why the hit-test-fixed.html test fails: the specified rect may not be wholly contained. > > Actually, the original patch, which called containsEditableElementInRect() was correct (that's why the test RequestTextInputContext.CompositedOverlap fails: it expects when there is overlap -requestTextInputContext will return all intersecting editable elements). Having said that, using containsEditableElementInRect() disagreed with the name of this function. Moreover, to implement mightContainEditableElementsInRect() we just need to find *one* view that contains an editable element in the rect. Keep in mind that if there is a composited overlapping element that does *not* any editable element then its editable region will be empty. To summarize, I think I can simplify this function and inline its implementation into mightContainEditableElementsInRect(). I decided to just change this to intersects() <-- which I will add to EventRegion. Going to keep this helper function for now and simplify in a follow up so as to not accidentally introduce a bug and not request a re-review. > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:147 > +static UIView *frontmostHitView(const Vector<UIView *, 16>& views) > +{ > + for (auto *view : WTF::makeReversedRange(views)) { > + // We only hit WKChildScrollView directly if its content layer doesn't have an event region. > + // We don't generate the region if there is nothing interesting in it (e.g. no touch-action elements). > + if ([view isKindOfClass:WKChildScrollView.class]) > + return nil; > + > + if ([view isKindOfClass:WKCompositingView.class]) > + return view; > + } > + return nil; > +} Going to revert this because I don't need it. > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:157 > - UIView *hitView = nil; > - for (auto *view : WTF::makeReversedRange(viewsAtPoint)) { > - // We only hit WKChildScrollView directly if its content layer doesn't have an event region. > - // We don't generate the region if there is nothing interesting in it, meaning the touch-action is auto. > - if ([view isKindOfClass:[WKChildScrollView class]]) > - return WebCore::TouchAction::Auto; > - > - if ([view isKindOfClass:[WKCompositingView class]]) { > - hitView = view; > - break; > - } > - } > - > + UIView *hitView = frontmostHitView(viewsAtPoint); Going to revert this because I don't need it. > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:181 > + Vector<UIView *, 16> viewsInRect; > + collectDescendantViewsInRect(viewsInRect, rootView, rect); > + if (viewsInRect.isEmpty()) > + return false; > + UIView *hitView = frontmostHitView(viewsInRect); > + if (!hitView) > + return false; > + if (auto* node = RemoteLayerTreeNode::forCALayer(hitView.layer)) > + return node->eventRegion().containsEditableElementsInRect(WebCore::IntRect { [hitView convertRect:rect fromView:rootView] }); > + return false; This is the only substantial change I need to make + the contains() => intersects() above to fix the test hit-test-fixed.html: [[ Vector<UIView *, 16> viewsInRect; collectDescendantViewsInRect(viewsInRect, rootView, rect); if (viewsInRect.isEmpty()) return false; for (auto *view : WTF::makeReversedRange(viewsInRect)) { if (![view isKindOfClass:WKCompositingView.class]) continue; WebCore::IntRect rectToTest { [view convertRect:rect fromView:rootView] }; if (auto* node = RemoteLayerTreeNode::forCALayer(view.layer); node && node->eventRegion().containsEditableElementsInRect(rectToTest)) return true; } return false; ]] ^^^ iterate over all the hit views instead of just the frontmost one to find the first one >> Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:122 >> + virtual bool mightContainEditableElementsInRect(unsigned, unsigned, unsigned, unsigned) { notImplemented(); return false; } > > Blank line. mayContain? Keeping blank line to match the style in this file. Will rename.

Daniel Bates

Comment 22 2020-04-05 11:26:40 PDT

Created attachment 395518 [details] To Land

Daniel Bates

Comment 23 2020-04-05 17:06:47 PDT

(In reply to Daniel Bates from comment #20) > iOS API failure is due to bug #210010. This fix is not enough with the new infra I added. This patch requires that the iframe tests wait until all the iframes to are loaded, layed out and painted <-- which is a must stronger condition that just being loaded (^^^ fixed that). To ensure the stronger condition two things need to be done: 1. The iframe tests in RequestTextInputContext MUST wait for the next presentation update after ensuring all the immediate child frames have loaded (only 1 per test). 2. (1) is only meaningful once bug #210041 is fixed. Because of that bug editable elements (and also elements with non-auto touch-action) will not have regions if the element is inside a non-composited frame.

Daniel Bates

Comment 24 2020-04-05 17:22:20 PDT

Created attachment 395535 [details] To Land Once Bug #210041 is fixed

Daniel Bates

Comment 25 2020-04-05 17:26:48 PDT

Created attachment 395536 [details] To land before bug #210041 is fixed (if desired)

Daniel Bates

Comment 26 2020-04-08 13:05:02 PDT

Created attachment 395858 [details] To land after bug #210041

Daniel Bates

Comment 27 2020-04-08 15:41:53 PDT

Committed r259762: <https://trac.webkit.org/changeset/259762>

Aakash Jain

Comment 28 2020-04-09 03:37:53 PDT

(In reply to Daniel Bates from comment #27) > Committed r259762: <https://trac.webkit.org/changeset/259762> Following newly added tests seems to be failing consistently on iOS (and slowing down EWS). This was also indicated by red EWS bubble in previous version of this patch (e.g.: https://ews-build.webkit.org/#/builders/24/builds/14687) - editing/editable-region/hit-test-basic.html - editing/editable-region/hit-test-fixed.html - editing/editable-region/hit-test-overlap.html History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=editing%2Feditable-region%2Fhit-test-basic.html&test=editing%2Feditable-region%2Fhit-test-fixed.html&test=editing%2Feditable-region%2Fhit-test-overlap.html editing/editable-region/hit-test-basic-actual.txt: +CONSOLE MESSAGE: line 43: ReferenceError: Can't find variable: description editing/editable-region/hit-test-fixed-expected.txt: +CONSOLE MESSAGE: line 44: TypeError: description is not a function. (In 'description("Hit test fixed positioned editable element.")', 'description' is an instance of HTMLParagraphElement) editing/editable-region/hit-test-overlap-actual.txt +CONSOLE MESSAGE: line 102: TypeError: description is not a function. (In 'description("Hit test editable elements that are overlapped by another element.")', 'description' is an instance of HTMLParagraphElement)

Daniel Bates

Comment 29 2020-04-09 06:51:23 PDT

(In reply to Aakash Jain from comment #28) > (In reply to Daniel Bates from comment #27) > > Committed r259762: <https://trac.webkit.org/changeset/259762> > Following newly added tests seems to be failing consistently on iOS (and > slowing down EWS). This was also indicated by red EWS bubble in previous > version of this patch (e.g.: > https://ews-build.webkit.org/#/builders/24/builds/14687) > > - editing/editable-region/hit-test-basic.html > - editing/editable-region/hit-test-fixed.html > - editing/editable-region/hit-test-overlap.html > > > History: > https://results.webkit.org/?suite=layout-tests&suite=layout- > tests&suite=layout-tests&test=editing%2Feditable-region%2Fhit-test-basic. > html&test=editing%2Feditable-region%2Fhit-test-fixed. > html&test=editing%2Feditable-region%2Fhit-test-overlap.html > > > editing/editable-region/hit-test-basic-actual.txt: > +CONSOLE MESSAGE: line 43: ReferenceError: Can't find variable: description > > editing/editable-region/hit-test-fixed-expected.txt: > +CONSOLE MESSAGE: line 44: TypeError: description is not a function. (In > 'description("Hit test fixed positioned editable element.")', 'description' > is an instance of HTMLParagraphElement) > > editing/editable-region/hit-test-overlap-actual.txt > +CONSOLE MESSAGE: line 102: TypeError: description is not a function. (In > 'description("Hit test editable elements that are overlapped by another > element.")', 'description' is an instance of HTMLParagraphElement) I will fix this up

Daniel Bates

Comment 30 2020-04-09 06:57:19 PDT

Committed layout test fix in <https://trac.webkit.org/changeset/259796>.

Aakash Jain

Comment 31 2020-04-09 13:48:51 PDT

editing/editable-region/hit-test-overlap.html is still failing. e.g.: https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Release%20WK2%20(Tests)/r259803%20(3610)/results.html

Ryan Haddad

Comment 32 2020-04-09 14:24:14 PDT

(In reply to Aakash Jain from comment #31) > editing/editable-region/hit-test-overlap.html is still failing. > > e.g.: > https://build.webkit.org/results/ > Apple%20iOS%2013%20Simulator%20Release%20WK2%20(Tests)/r259803%20(3610)/ > results.html Filed https://bugs.webkit.org/show_bug.cgi?id=210305 to track this.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Local Build

Hardware iPhone / iPad

OS iOS 13

Product WebKit

Component WebCore Misc.

Assignee

Daniel Bates

Reported

2020-04-01 17:20 PDT

Modified

2020-04-09 16:48 PDT History

CC List

19 users Show

URL

Keywords InRadar

Depends on

210041

Blocks

210305

Dependencies

tree graph