Bug 208352

<rdar://problem/59867815>

Created attachment 391940 [details] Patch

Created attachment 392075 [details] Patch Update test results for Mac.

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.

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.

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

Created attachment 392078 [details] Patch + iOS guess

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

Created attachment 392106 [details] Patch

Created attachment 392107 [details] Patch

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!

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.

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

Created attachment 392154 [details] To land

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

Comment on attachment 392155 [details] To land - removed unused Deque header Clearing flags on attachment: 392155 Committed r257722: <https://trac.webkit.org/changeset/257722>

All reviewed patches have been landed. Closing bug.