Bug 208352 – Page::editableElementsInRect() should find nested editable elements and return found elements in paint order

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Page::editableElementsInRect() cannot find nested editable elements and the order of the returned elements are in DOM order, not paint order. The former means that a client can never get a hold of those elements. The latter means that a client cannot tell which element is closest to a person's face.

[Radar WebKit Bug Importer]

<rdar://problem/59867815>

[Daniel Bates]

Created attachment 391940 [details]
Patch

[Daniel Bates]

Created attachment 392075 [details]
Patch

Update test results for Mac.

[Daniel Bates]

Comment on attachment 392075 [details]
Patch

Patch does not address the iOS test failure and more likely the updated results will cause more iOS test failures. I am very suspicious of "[webView scrollView].contentOffset = ..." in the test code. I would have expected window.scrollTo() to also work on iOS. In fact, as I'm typing this now and thinking about how iOS is currently failing I think setting .contentOffset will never produce good results because the engine is never told about the scroll position change so it cannot update its visible content rect and hit testing will fail.

The kind of failure seen on the bots in the earlier patch: the test is hit testing at the top of the document (scrolled offscreen). As to why the test passes before my change. I can answer that: the original code was ASYMMETRIC! Walking the DOM will find things **outside the visible viewport** whereas hit testing will NOT, by default.

[Daniel Bates]

Just to be clear, the asymmetry with the current code that the visible content rect in the UI process does NOT match the visible content rect in the web process. In the UI process, the visible content rect reflects the set contentOffset, but the web process never is told about this offset.

[Daniel Bates]

^^^ both of those comments about iOS are guesses....I'll find out soon...

[Daniel Bates]

Created attachment 392078 [details]
Patch + iOS guess

[Daniel Bates]

Comment on attachment 392078 [details]
Patch + iOS guess

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:147
> +    [webView _scheduleVisibleContentRectUpdate];

schedule is not enough.... need to wait...-_doAfterNextVisibleContentRectUpdate

[Daniel Bates]

Created attachment 392106 [details]
Patch

[Daniel Bates]

Created attachment 392107 [details]
Patch

[Daniel Bates]

Comment on attachment 392107 [details]
Patch

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

> Tools/ChangeLog:13
> +        to a y position of 50000. Otherwise, it's impossible to do because the page height is < 5000px.

5000 not 50000!

[Wenson Hsieh]

Comment on attachment 392107 [details]
Patch

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

> Source/WebCore/page/Page.cpp:930
> +    auto* frameView = mainFrame().view();

auto frameView = makeRefPtr(mainFrame().view());

We generally ref local variables like this in new code (especially in this case, since the hit-testing code below will trigger layout).

> Source/WebCore/page/Page.cpp:934
> +    auto* document = mainFrame().document();

Ditto.

[Daniel Bates]

Comment on attachment 392107 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/page/Page.cpp:930
>> +    auto* frameView = mainFrame().view();
> 
> auto frameView = makeRefPtr(mainFrame().view());
> 
> We generally ref local variables like this in new code (especially in this case, since the hit-testing code below will trigger layout).

OK

>> Source/WebCore/page/Page.cpp:934
>> +    auto* document = mainFrame().document();
> 
> Ditto.

OK

[Daniel Bates]

Created attachment 392154 [details]
To land

[Daniel Bates]

Created attachment 392155 [details]
To land - removed unused Deque header

[Daniel Bates]

Comment on attachment 392155 [details]
To land - removed unused Deque header

Clearing flags on attachment: 392155

Committed r257722: <https://trac.webkit.org/changeset/257722>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.