Bug 184297 - [iOS] Find UI: Wrong position of text highlight inside unflattened subframes
Summary: [iOS] Find UI: Wrong position of text highlight inside unflattened subframes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified iOS 11
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 184302 190823
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-04 05:38 PDT by Frédéric Wang (:fredw)
Modified: 2019-01-30 03:07 PST (History)
7 users (show)

See Also:


Attachments
Testcase (492 bytes, text/html)
2018-04-04 05:38 PDT, Frédéric Wang (:fredw)
no flags Details
WIP Patch (5.65 KB, patch)
2018-04-04 07:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 184302) (471.38 KB, patch)
2018-04-05 01:53 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (includes 184302) (493.95 KB, patch)
2018-04-05 02:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 184302) (473.52 KB, patch)
2018-05-10 01:56 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 184302) (473.50 KB, patch)
2018-09-05 07:15 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (471.59 KB, patch)
2018-09-10 02:24 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (235.89 KB, patch)
2018-10-25 08:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (231.88 KB, patch)
2019-01-22 09:29 PST, Frédéric Wang (:fredw)
fred.wang: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-04-04 05:38:13 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-04-04 05:38:33 PDT
<rdar://problem/39172466>
Comment 2 Frédéric Wang (:fredw) 2018-04-04 07:51:16 PDT
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)
Comment 3 Frédéric Wang (:fredw) 2018-04-05 01:53:41 PDT
Created attachment 337259 [details]
Patch (applies on top of 184302)
Comment 4 Frédéric Wang (:fredw) 2018-04-05 02:02:38 PDT
Created attachment 337260 [details]
Patch for EWS (includes 184302)
Comment 5 Simon Fraser (smfr) 2018-04-09 06:52:54 PDT
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?
Comment 6 Simon Fraser (smfr) 2018-04-09 07:00:44 PDT
I would also expect this to use the fixed windowToContents() (if the underlying problem is indeed the same).
Comment 7 Frédéric Wang (:fredw) 2018-04-09 08:10:12 PDT
(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.
Comment 8 Frédéric Wang (:fredw) 2018-05-10 01:56:10 PDT
Created attachment 340077 [details]
Patch (applies on top of 184302)

Rebasing...
Comment 9 Frédéric Wang (:fredw) 2018-09-05 07:15:12 PDT
Created attachment 348917 [details]
Patch (applies on top of 184302)

Rebasing...
Comment 10 Frédéric Wang (:fredw) 2018-09-10 02:24:21 PDT
Created attachment 349304 [details]
Patch

New version, following bug 184302 comment 8
Comment 11 Frédéric Wang (:fredw) 2018-09-10 05:07:35 PDT
Comment on attachment 349304 [details]
Patch

@smfr: Can you please take a look at that version (no longer introducing a deprecated version).
Comment 12 Frédéric Wang (:fredw) 2018-10-25 08:11:10 PDT
Created attachment 353085 [details]
Patch
Comment 13 Frédéric Wang (:fredw) 2018-10-25 08:16:47 PDT
@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 14 Simon Fraser (smfr) 2018-10-25 08:25:09 PDT
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 15 Frédéric Wang (:fredw) 2018-10-25 08:50:12 PDT
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 16 Frédéric Wang (:fredw) 2018-10-25 21:53:55 PDT
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.
Comment 17 Frédéric Wang (:fredw) 2018-11-22 09:10:44 PST
@smfr: review ping.
Comment 18 Frédéric Wang (:fredw) 2019-01-22 09:29:24 PST
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 19 Frédéric Wang (:fredw) 2019-01-22 11:04:47 PST
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.
Comment 20 Frédéric Wang (:fredw) 2019-01-22 11:04:54 PST
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.
Comment 21 Frédéric Wang (:fredw) 2019-01-22 11:04:59 PST
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.
Comment 22 Simon Fraser (smfr) 2019-01-22 12:59:16 PST
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?
Comment 23 Frédéric Wang (:fredw) 2019-01-22 13:08:56 PST
(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.
Comment 24 Frédéric Wang (:fredw) 2019-01-30 03:07:25 PST
(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?