Bug 163911 – window.find does not scroll with overflow:auto content on mobile Safari

• Component: WebKit Misc.
• Status: ASSIGNED

[Dima Voytenko]

See http://output.jsbin.com/kufici. 

This page has main content wrapped into `overflow: auto; -webkit-overflow-scrolling: touch;` element. Executing `window.find('find')` on Desktop Safari finds the text and scrolls it into view as expected. Doing the same on mobile Safari (iPhone 6+) finds the text, but does NOT scroll it into view.

Please advise.

[Radar WebKit Bug Importer]

<rdar://problem/28927508>

[Malte Ubl]

Note that "Find in page" via UI in mobile Safari has the same problem.

Here is a slightly simplified test case http://output.jsbin.com/piwuvu/quiet

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

Created attachment 308089 [details]
testcase

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

So I debugged a bit what is happening here. Basically, in WebKit2/WebProcess/WebPage/FindController.cpp, FindController::findString sets a special DoNotRevealSelection flag under IOS:

    // iOS will reveal the selection through a different mechanism, and
    // we need to avoid sending the non-painted selection change to the UI process
    // so that it does not clear the selection out from under us.
#if PLATFORM(IOS)
    coreOptions = static_cast<FindOptions>(coreOptions | DoNotRevealSelection);
#endif

As a consequence, Editor::findString will skip the call to FrameSelection::revealSelection where the actual scrolling request is performed:

#if PLATFORM(IOS)
        if (RenderLayer* layer = start.deprecatedNode()->renderer()->enclosingLayer()) {
            if (!m_scrollingSuppressCount) {
                layer->setAdjustForIOSCaretWhenScrolling(true);
                layer->scrollRectToVisible(revealMode, rect, insideFixed, alignment, alignment);
                layer->setAdjustForIOSCaretWhenScrolling(false);
                updateAppearance();
                if (m_frame->page())
                    m_frame->page()->chrome().client().notifyRevealedSelectionByScrollingFrame(*m_frame);
            }
        }
#else

If we force the call to FrameSelection::revealSelection then the scrolling happens as expected.

Note that this behavior was added in bug 131510.

[Simon Fraser (smfr)]

On the internal bug I said:

The problem seems to be that FindController::updateFindIndicator() and UI-side code (oddly, SmartMagnificationController code) only know about scrolling the main page, and not about scrolling overflow.

Why doesn't this use the same code path as scrollIntoView and friends?

[Simon Fraser (smfr)]

<rdar://problem/26322565>

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

(In reply to Simon Fraser (smfr) from comment #5)
> On the internal bug I said:
> 
> The problem seems to be that FindController::updateFindIndicator() and
> UI-side code (oddly, SmartMagnificationController code) only know about
> scrolling the main page, and not about scrolling overflow.
> 
> Why doesn't this use the same code path as scrollIntoView and friends?

Thank you for sharing this, Simon. I think it is useful piece of information. So my understanding is that:

1) iOS requires some specific code path to do the highlight of matched text and also this special "smart magnification" feature.
2) BUT, this currently only works for the main page and is completely broken for overflow:auto (or other situations e.g. iframes when bug 149264 is fixed)

I am not exactly sure how important 1) is (maybe Timothy can provide more details here) but 2) definitely sounds serious to me. It is certainly worth discussing a bit more to see how we can make progress on it...

Do you think we should fix the iOS code path to handle the non-main-frame-view case? How much effort would that require? What about the (maybe easier) alternative of falling back to the Mac code path in the non-main-frame-view case? I guess the "smart magnification" is not too important, but maybe that would cause issue for highlighting?

(I also wonder if we should refactor a bit the code so that the two paths share as much code as possible)

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #7)
> (In reply to Simon Fraser (smfr) from comment #5)
> > On the internal bug I said:
> > 
> > The problem seems to be that FindController::updateFindIndicator() and
> > UI-side code (oddly, SmartMagnificationController code) only know about
> > scrolling the main page, and not about scrolling overflow.
> > 
> > Why doesn't this use the same code path as scrollIntoView and friends?
> 
> Thank you for sharing this, Simon. I think it is useful piece of
> information. So my understanding is that:
> 
> 1) iOS requires some specific code path to do the highlight of matched text
> and also this special "smart magnification" feature.
> 2) BUT, this currently only works for the main page and is completely broken
> for overflow:auto (or other situations e.g. iframes when bug 149264 is fixed)
> 
> I am not exactly sure how important 1) is (maybe Timothy can provide more
> details here) but 2) definitely sounds serious to me. It is certainly worth
> discussing a bit more to see how we can make progress on it...
> 
> Do you think we should fix the iOS code path to handle the
> non-main-frame-view case? How much effort would that require?

We should, and we can re-use existing "scroll into view" code for this, although I think we do
some zooming for Find results (but we also do this on focus, so should be similar to existing code).

> What about the
> (maybe easier) alternative of falling back to the Mac code path in the
> non-main-frame-view case? I guess the "smart magnification" is not too
> important, but maybe that would cause issue for highlighting?

The "scroll into view" code is largely shared with Mac.

> (I also wonder if we should refactor a bit the code so that the two paths
> share as much code as possible)

We should. Obviously Find needs to show the find indicator, but that should be done independently from scrolling into view.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Frédéric Wang (:fredw) from comment #7)
> > (In reply to Simon Fraser (smfr) from comment #5)
> > 
> > Do you think we should fix the iOS code path to handle the
> > non-main-frame-view case? How much effort would that require?
> 
> We should, and we can re-use existing "scroll into view" code for this,
> although I think we do
> some zooming for Find results (but we also do this on focus, so should be
> similar to existing code).

If the generic "good" scroll-into-view code supports zooming too, maybe we should use it for SmartMagnificationController too? (@frederic, "smart magnification" is "double tap to zoom")

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

(In reply to Tim Horton from comment #9)
> > We should, and we can re-use existing "scroll into view" code for this,
> > although I think we do
> > some zooming for Find results (but we also do this on focus, so should be
> > similar to existing code).
> 
> If the generic "good" scroll-into-view code supports zooming too, maybe we
> should use it for SmartMagnificationController too? (@frederic, "smart
> magnification" is "double tap to zoom")

@Simon: So you are suggesting that zooming for focus is supported in the existing scroll-into-view code? Can you indicate when it is done?

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

For the record, here are relevant backtraces for Focus (tab to an input field) and Find features. For the former feature, scrollRectToVisible is used for scrolling and _zoomToFocusRect is used for zooming. For the latter feature, scrollRectVisible is not called (as explained in comment 4). Instead, FindController::updateFindIndicator will rely on SmartMagnificationController to perform the scrolling and zooming... But this only works for the main frame :-(

== Focus ==

WebProcess:

WebCore::RenderLayer::scrollRectToVisible
WebCore::FrameSelection::revealSelection
WebCore::HTMLInputElement::updateFocusAppearance
WebCore::Element::focus

UIProcess:

-[WKContentView _zoomToFocusRect]
-[WKContentView _displayFormNodeInputView]

== Find ==

WebProcess:

WebCore::Editor::findString (FrameSelection::revealSelection and hence RenderLayer::scrollRectToVisible not called)
WebCore::Page::findString
WebKit::FindController::findString (DoNotRevealSelection is set)
WebKit::WebPage::findString

WebKit::FindController::updateFindIndicator (send magnify IPC message)
WebKit::FindController::updateFindUIAfterPageScroll
WebKit::DrawingArea::dispatchAfterEnsuringUpdatedScrollPosition
WebKit::FindController::findString
WebKit::WebPage::findString

UIProcess:

-[WKWebView _zoomToRect] (either _scrollToRect or _zoomToRect is called)
-[WKContentView _zoomToRect]
-[SmartMagnificationController::magnify]

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

Created attachment 312006 [details]
More complex testcase

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

(In reply to Frédéric Wang (:fredw) from comment #11)
> For the record, here are relevant backtraces for Focus (tab to an input
> field) and Find features.

Here are slightly more detailed traces:

== Focus ==

WebProcess:

WebKit::WebPage::elementDidFocus (sends StartAssistingNode IPC message)
WebKit::WebChromeClient::elementDidFocus
WebCore::Element::dispatchFocusEvent
...
WebCore::Document::setFocusedElement
WebCore::FocusControler::setFocusedElement
WebCore::Element::focus

followed by

WebCore::RenderLayer::scrollRectToVisible
WebCore::FrameSelection::revealSelection
WebCore::HTMLInputElement::updateFocusAppearance
WebCore::Element::focus

UIProcess:

-[WKContentView _zoomToFocusRect]
-[WKContentView _displayFormNodeInputView]
-[WKContentView _startAssistingNode]
WebKit::PageClientImplIOS::startAssistingNode
WebKit::WebPageProxy::startAssistingNode

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

Created attachment 316866 [details]
Patch

So for the record, I'm uploading the experimental patch I had to try and do the same operations as "Focus" i.e.

1) IPC call to perform WKContentView _zoomTo*Rect (smart magnification + scrolling)
2) Followed by a call to WebCore::RenderLayer::scrollRectToVisible for the overflow nodes

The following video demonstrates some bugs with attachment 312006 [details]: https://people.igalia.com/fwang/ios/2017-07-28/find.mov ; I had variants of the patch with the same issues...

1) Rendering may become wrong when the main frame is scrolled/zoomed.
2) Highlight is missing when the inner overflow node is scrolled.
3) When some text of the inner overflow node is highlighted, scrolling that node does not update the rendering of the highlighted text

I am not sure how to handle all the cases but maybe we could agree on a first step to address the most important issue (no scrolling of inner frames) and use cases (only a single overflow:auto body)? For example, I'm moving (3) to bug 175032.

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

Created attachment 324671 [details]
Patch (WIP)

New attempt. This seems to give better result.

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

Created attachment 324810 [details]
More complex testcase (with a button to call window.find)

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

Created attachment 324811 [details]
More complex testcase (with a button to call window.find)

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

Created attachment 324813 [details]
testcase without overflow nodes (with a button to call window.find)

Here is a testcase that shows that window.find does not scroll for the main frame either.

The scrolling works correctly for the main frame when the "Find UI" is used (Cmd+F on the simulator, see comment 2). Attachment 324671 [details] fixes the cases of overflow nodes.

Regarding window.find, the relevant stack trace is

Editor::findString
DOMWindow::find

So indeed, the existing code in FindController::findString to call updateFindUIAfterPageScroll is not executed, nor is the changes made by attachment 324671 [details]. Actually, DoNotRevealSelection is not set and indeed the code in FrameSelection::revealSelection that I mentioned in comment 4 *is* executed. However, for some reason it does not seem to scroll the page (this would need to be debugged further).

To summarize, window.find uses a different code path, won't use the FindController at all and does not set DoNotRevealSelection, so probably it should be handled separately.

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

For the record, the trace for document.execCommand("FindString", ...) is

WebCore::Editor::findString
WebCore::executeFindString
WebCore::Editor::Command::execute
WebCore::Document::execCommand

So we arrive at the same point as or window.find and we have the same bug. Passing showUI = true does not seem to have any effect.

[Simon Fraser (smfr)]

The way to test this would be with new functions on UIScriptController.

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

(In reply to Simon Fraser (smfr) from comment #20)
> The way to test this would be with new functions on UIScriptController.

@Simon: Thanks. I added a test to the patch for bug 178789 (iOS's Find UI). For the remaining cases of window.find and document.execCommand("FindString", ...), there is already a JS API to test them so UIScriptController should not be necessary.