Bug 163911 - window.find does not scroll with overflow:auto content on mobile Safari
Summary: window.find does not scroll with overflow:auto content on mobile Safari
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-24 14:27 PDT by Dima Voytenko
Modified: 2017-08-01 09:52 PDT (History)
6 users (show)

See Also:


Attachments
testcase (707 bytes, text/html)
2017-04-25 04:02 PDT, Frédéric Wang (:fredw)
no flags Details
More complex testcase (1.43 KB, text/html)
2017-06-05 05:02 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (894 bytes, patch)
2017-08-01 09:52 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2016-10-24 14:27:14 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-10-24 17:58:12 PDT
<rdar://problem/28927508>
Comment 2 Malte Ubl 2017-01-05 08:25:31 PST
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
Comment 3 Frédéric Wang (:fredw) 2017-04-25 04:02:48 PDT
Created attachment 308089 [details]
testcase
Comment 4 Frédéric Wang (:fredw) 2017-04-27 03:36:19 PDT
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.
Comment 5 Simon Fraser (smfr) 2017-04-27 10:35:29 PDT
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?
Comment 6 Simon Fraser (smfr) 2017-05-22 11:07:10 PDT
<rdar://problem/26322565>
Comment 7 Frédéric Wang (:fredw) 2017-05-24 02:52:11 PDT
(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)
Comment 8 Simon Fraser (smfr) 2017-05-24 09:54:52 PDT
(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.
Comment 9 Tim Horton 2017-05-24 10:16:56 PDT
(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")
Comment 10 Frédéric Wang (:fredw) 2017-05-31 09:29:37 PDT
(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?
Comment 11 Frédéric Wang (:fredw) 2017-06-01 03:32:30 PDT
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]
Comment 12 Frédéric Wang (:fredw) 2017-06-05 05:02:26 PDT
Created attachment 312006 [details]
More complex testcase
Comment 13 Frédéric Wang (:fredw) 2017-07-31 04:04:33 PDT
(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
Comment 14 Frédéric Wang (:fredw) 2017-08-01 09:52:06 PDT
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.