Bug 209888 - Track editable elements on screen
Summary: Track editable elements on screen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on: 210041
Blocks: 210305
  Show dependency treegraph
 
Reported: 2020-04-01 17:20 PDT by Daniel Bates
Modified: 2020-04-09 16:48 PDT (History)
19 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-04-01 17:20:19 PDT
Track editable elements on screen.
Comment 1 Daniel Bates 2020-04-01 17:22:20 PDT
Created attachment 395228 [details]
Rough patch - just for bots
Comment 2 Daniel Bates 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!
Comment 3 Daniel Bates 2020-04-02 01:09:36 PDT
Need to fix up floats....see bug #209896
Comment 4 Daniel Bates 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.
Comment 5 Radar WebKit Bug Importer 2020-04-02 01:22:41 PDT
<rdar://problem/61196886>
Comment 6 Daniel Bates 2020-04-02 15:01:24 PDT
Created attachment 395306 [details]
Patch and tests
Comment 7 Daniel Bates 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.
Comment 8 Daniel Bates 2020-04-02 15:18:01 PDT
Created attachment 395310 [details]
Patch and tests
Comment 9 Simon Fraser (smfr) 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?
Comment 10 Daniel Bates 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 })
Comment 11 Daniel Bates 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 🧠.
Comment 12 Daniel Bates 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.
Comment 13 Daniel Bates 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
Comment 14 Simon Fraser (smfr) 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?
Comment 15 Daniel Bates 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.
Comment 16 Daniel Bates 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().
Comment 17 Antti Koivisto 2020-04-03 21:53:01 PDT
Maybe we should rename EventRegion to InteractionRegion or similar.
Comment 18 Daniel Bates 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....
Comment 19 Daniel Bates 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.
Comment 20 Daniel Bates 2020-04-04 15:51:46 PDT
iOS API failure is due to bug #210010.
Comment 21 Daniel Bates 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.
Comment 22 Daniel Bates 2020-04-05 11:26:40 PDT
Created attachment 395518 [details]
To Land
Comment 23 Daniel Bates 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.
Comment 24 Daniel Bates 2020-04-05 17:22:20 PDT
Created attachment 395535 [details]
To Land Once Bug #210041 is fixed
Comment 25 Daniel Bates 2020-04-05 17:26:48 PDT
Created attachment 395536 [details]
To land before bug #210041 is fixed (if desired)
Comment 26 Daniel Bates 2020-04-08 13:05:02 PDT
Created attachment 395858 [details]
To land after bug #210041
Comment 27 Daniel Bates 2020-04-08 15:41:53 PDT
Committed r259762: <https://trac.webkit.org/changeset/259762>
Comment 28 Aakash Jain 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)
Comment 29 Daniel Bates 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
Comment 30 Daniel Bates 2020-04-09 06:57:19 PDT
Committed layout test fix in <https://trac.webkit.org/changeset/259796>.
Comment 31 Aakash Jain 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
Comment 32 Ryan Haddad 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.