Bug 209888

Summary:

Track editable elements on screen

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

WebCore Misc.

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, ap, cdumez, esprehn+autocc, ews-watchlist, fred.wang, glenn, kangil.han, keith_miller, koivisto, kondapallykalyan, mark.lam, msaboff, pdr, saam, simon.fraser, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 13

See Also:

https://bugs.webkit.org/show_bug.cgi?id=209896
https://bugs.webkit.org/show_bug.cgi?id=210010
https://bugs.webkit.org/show_bug.cgi?id=210305
https://bugs.webkit.org/show_bug.cgi?id=210311

Bug Depends on:

210041

Bug Blocks:

210305

Attachments:

Description	Flags
Rough patch - just for bots	none
Patch and tests	none
Patch and tests	none
Patch and tests	none
To Land	none
To Land Once Bug #210041 is fixed	none
To land before bug #210041 is fixed (if desired)	none
To land after bug #210041	none

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.