[iOS] Find UI: Wrong position of text highlight inside unflattened subframes

• Component: Frames
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Created attachment 337156 [details]
Testcase

Attached testcase is similar to attachment 308089 [details] but using a subframe instead of an "overflow: auto" node.

Searching text inside works when the subframe is flattened (currently the default). However, if you disable frame flattening (e.g. if you enable AsyncFrameScrolling) then the text highlight is wrongly positioned outside the frame.

It seems the issue is similar to scrollToVisible (bug 176451) or hit testing (bug 173833), where scroll offset from subframes should really be ignored.

[Radar WebKit Bug Importer]

<rdar://problem/39172466>

[Frédéric Wang (:fredw)]

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)

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

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

Rebasing...

[Frédéric Wang (:fredw)]

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

Rebasing...

[Frédéric Wang (:fredw)]

Created attachment 349304 [details]
Patch

New version, following bug 184302 comment 8

[Frédéric Wang (:fredw)]

Comment on attachment 349304 [details]
Patch

@smfr: Can you please take a look at that version (no longer introducing a deprecated version).

[Frédéric Wang (:fredw)]

Created attachment 353085 [details]
Patch

[Frédéric Wang (:fredw)]

@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.

[Simon Fraser (smfr)]

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?

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

@smfr: review ping.

[Frédéric Wang (:fredw)]

Created attachment 359740 [details]
Patch

Just extracting the tests... Let's see if this is fixed by https://trac.webkit.org/changeset/240232/webkit

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Simon Fraser (smfr)]

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?

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

(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?

[Frédéric Wang (:fredw)]

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?

[Simon Fraser (smfr)]

Please do.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #26)
> Please do.

Done in https://bugs.webkit.org/show_bug.cgi?id=213647