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.
<rdar://problem/28927508>
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
Created attachment 308089 [details] testcase
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.
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?
<rdar://problem/26322565>
(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)
(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.
(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")
(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?
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]
Created attachment 312006 [details] More complex testcase
(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
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.
Created attachment 324671 [details] Patch (WIP) New attempt. This seems to give better result.
Created attachment 324810 [details] More complex testcase (with a button to call window.find)
Created attachment 324811 [details] More complex testcase (with a button to call window.find)
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.
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.
The way to test this would be with new functions on UIScriptController.
(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.