Bug 184297

<rdar://problem/39172466>

Created attachment 337168 [details] WIP Patch This is a WIP patch that makes the testcase behaves as expected. It applies on top of bug 184302. As I said, the issue is similar to scrollToVisible (bug 176451) or hit testing (bug 173833). There is however a subtle edge case: When the highlighted text is near the limit of the frame's document (e.g. the text at the bottom of in attachment 337156 [details]) then the highlighted may be wrongly positioned at the center. This is because scroll position is no longer clamped in the WebProcess (bug 179735) so TextIndicator is not aware of the final position. Here is the relevant backtrace: #0 WebCore::ScrollView::setScrollPosition(WebCore::IntPoint const&) #1 WebCore::FrameView::setScrollPosition(WebCore::IntPoint const&) #2 WebCore::RenderLayer::scrollRectToVisible(WebCore::SelectionRevealMode, WebCore::LayoutRect const&, bool, WebCore::ScrollAlignment const&, WebCore::ScrollAlignment const&) #3 WebCore::RenderLayer::scrollRectToVisible(WebCore::SelectionRevealMode, WebCore::LayoutRect const&, bool, WebCore::ScrollAlignment const&, WebCore::ScrollAlignment const&) #4 WebCore::FrameSelection::revealSelection(WebCore::SelectionRevealMode, WebCore::ScrollAlignment const&, WebCore::RevealExtentOption) #5 WebKit::FindController::didFindString() #6 WebKit::FindController::findString(WTF::String const&, WebKit::FindOptions, unsigned int) Note that RenderLayer::scrollRectToVisible has a comment "Adjust offsets if they're outside the allowable range" but that does not seem to work. For the record, this is the backtrace to TextIndicator that is called after the frame content is revealed: #0 WebCore::initializeIndicator(WebCore::TextIndicatorData&, WebCore::Frame&, WebCore::Range const&, WebCore::FloatSize, bool) #1 WebCore::TextIndicator::createWithSelectionInFrame(WebCore::Frame&, unsigned short, WebCore::TextIndicatorPresentationTransition, WebCore::FloatSize) #2 WebKit::FindController::updateFindIndicator(WebCore::Frame&, bool, bool) #3 WebKit::FindController::updateFindUIAfterPageScroll(bool, WTF::String const&, WebKit::FindOptions, unsigned int, WebCore::DidWrap) #4 WebKit::FindController::findString(WTF::String const&, WebKit::FindOptions, unsigned int)

Created attachment 337259 [details] Patch (applies on top of 184302)

Created attachment 337260 [details] Patch for EWS (includes 184302)

Comment on attachment 337259 [details] Patch (applies on top of 184302) How is this related to bug 184302? Is it really the same underlying issue?

I would also expect this to use the fixed windowToContents() (if the underlying problem is indeed the same).

(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 337259 [details] > Patch (applies on top of 184302) > > How is this related to bug 184302? Is it really the same underlying issue? (In reply to Simon Fraser (smfr) from comment #6) > I would also expect this to use the fixed windowToContents() (if the > underlying problem is indeed the same). Note that this depends on bug 184302 which is about *contentsToRootView()* not bug 182785 which is about *windowToContents()*. But the issue is similar, several functions do not properly take into account scroll offset of subframes.

Created attachment 340077 [details] Patch (applies on top of 184302) Rebasing...

Created attachment 348917 [details] Patch (applies on top of 184302) Rebasing...

Created attachment 349304 [details] Patch New version, following bug 184302 comment 8

Comment on attachment 349304 [details] Patch @smfr: Can you please take a look at that version (no longer introducing a deprecated version).

Created attachment 353085 [details] Patch

@smfr: I rebased the patch after r237411. As I explained on webkit-dev and bug 178789 comment 15, I'm not really able to test the fix on WK1. However, note that this is only replacing delegatesScrolling() with "delegatesScrolling() && (platformWidget() || frame().isMainFrame())" for FrameView so in the case of WK1 (platformWidget() == true) this actually does not change the behavior. The only possible issue is whether this might break something on WK2, but all the existing tests pass.

Comment on attachment 353085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353085&action=review > Source/WebCore/page/FrameView.cpp:1401 > + return delegatesScrolling() && (platformWidget() || frame().isMainFrame()); It's correct that for WK2 with delegated scrolling, all callers want to ignore the main frame scroll position? This function is really about whether we run the viewToContents()/contentsToView() part of the code, so maybe its name should reflect that. > LayoutTests/platform/ios/fast/scrolling/find-text-in-subframe-indicator-position-expected.txt:1 > +layer at (0,0) size 800x600 I don't think this result contains the scroll offset anywhere, so without pixel testing, how does it work?

Comment on attachment 353085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353085&action=review >> Source/WebCore/page/FrameView.cpp:1401 >> + return delegatesScrolling() && (platformWidget() || frame().isMainFrame()); > > It's correct that for WK2 with delegated scrolling, all callers want to ignore the main frame scroll position? > > This function is really about whether we run the viewToContents()/contentsToView() part of the code, so maybe its name should reflect that. This patch tries to avoid changes I'm not sure about, I can try to check but there are probably too many places as we previously discussed. I don't know if all callers want to ignore the main frame scroll position, but that's what they all do at the moment (and I remember it was definitely needed for some of them). I'm not sure if all callers want to take the offset into account for subframes with delegated scrolling, but again I think it is safe until async frame scrolling is implemented (and frame flattening disabled on iOS). I plan to use FrameView::coordinateChangeShouldIgnoreScrollPosition() in ScrollView::windowToContents (bug 173833) and it could have been safer to use for ScrollView::contentsToContainingViewContents (bug 186956 and bug 176615). I suspect it might be used for other functions. What name do you suggest? >> LayoutTests/platform/ios/fast/scrolling/find-text-in-subframe-indicator-position-expected.txt:1 >> +layer at (0,0) size 800x600 > > I don't think this result contains the scroll offset anywhere, so without pixel testing, how does it work? Right, it only works via pixel comparison unfortunately. I could have done something similar to find-text-in-subframe.html but I don't remember why I chose pixel rendering instead. I could also dump the scroll offset but that would make the value arbitrary and the test less easy to understand.

Comment on attachment 353085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353085&action=review >>> LayoutTests/platform/ios/fast/scrolling/find-text-in-subframe-indicator-position-expected.txt:1 >>> +layer at (0,0) size 800x600 >> >> I don't think this result contains the scroll offset anywhere, so without pixel testing, how does it work? > > Right, it only works via pixel comparison unfortunately. I could have done something similar to find-text-in-subframe.html but I don't remember why I chose pixel rendering instead. I could also dump the scroll offset but that would make the value arbitrary and the test less easy to understand. @smfr: OK, I remember now :-) - find-text-in-subframe/overflow-node.html are for bug 178789: We want to check whether the element is actually scrolled (to reveal the match) and hence we can just check the scroll offset. - find-text-in-subframe-node-indicator-position.html and find-text-in-subframe-node-indicator-position-limit.html are for this bug (184297): We want to verify that the yellow highlight is actually rendered at the position of the matched text. We cannot use scroll offset for that and we really need to test the visual rendering. It's unfortunate that the actual PNG expectation might not actually be verified when running the test... Maybe I can try to convert them to reftests, although it might be a bit difficult.

@smfr: review ping.

Created attachment 359740 [details] Patch Just extracting the tests... Let's see if this is fixed by https://trac.webkit.org/changeset/240232/webkit

Comment on attachment 359740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359740&action=review > LayoutTests/ChangeLog:12 > + I need to add that this was fixed by https://trac.webkit.org/changeset/240232/webkit but it seems we can now take these tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=359740&action=review > LayoutTests/ChangeLog:12 > + I need to add that this was fixed by https://trac.webkit.org/changeset/240232/webkit but it seems we can now take these tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=359740&action=review > LayoutTests/ChangeLog:12 > + I need to add that this was fixed by https://trac.webkit.org/changeset/240232/webkit but it seems we can now take these tests.

We don't do pixel testing by default, so I'm not sure how useful these tests are. Can you convert the to ref tests?

(In reply to Simon Fraser (smfr) from comment #22) > We don't do pixel testing by default, so I'm not sure how useful these tests > are. Can you convert the to ref tests? OK, indeed that's what I wrote in comment 16. Will try to do it.

(In reply to Frédéric Wang (:fredw) from comment #23) > (In reply to Simon Fraser (smfr) from comment #22) > > We don't do pixel testing by default, so I'm not sure how useful these tests > > are. Can you convert the to ref tests? > > OK, indeed that's what I wrote in comment 16. Will try to do it. @Simon: I just checked now with the existing similar test LayoutTests/platform/ios/fast/scrolling/find-text-in-overflow-node-indicator-position.html Checking the screenshot, the "smart magnification" effects does not happen when running the test, so that removes one complication. However, the position/size (overflowing a bit the scroller and taller than its div container) and shape (rounded corners) of the yellow highlight seems a bit difficult to reproduce precisely with pure CSS. Also the yellow highlight is over the content, so it does not seem possible to add some overlay to hide the differences. If I remove the -expected.txt and only keep the -expected.png is pixel testing executed by default?

Comment on attachment 359740 [details] Patch This seems fixed in the iOS 14 beta, however there is a different bug (feature) happening in the testcase: the text is still visible even when the highlighted text is outside the visible area of the iframe. @Simon: Should I close this one and open a separate bug?

Please do.

*** Bug 191437 has been marked as a duplicate of this bug. ***

(In reply to Simon Fraser (smfr) from comment #26) > Please do. Done in https://bugs.webkit.org/show_bug.cgi?id=213647