[iOS] Text highlighted by the Find UI overlaps with NBC news header on google.com

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

Created attachment 335845 [details]
Screenshot

This was reported by Ali Ghassemi (Google AMP) who tested the patch from bug 178789. See also his video: https://photos.google.com/share/AF1QipP8FzbWiprlR7RLgzsNhrJeVTzmo34pWwT3pg_IvsKC_SRxNf6EUvUkgTsDAZNVrA/photo/AF1QipNUqsRFPMJl3DWl7xGu-BQUPHTnqKPJXLo0GXou?key=Y1dNcWtTMmFweGhwbUF3aWRsRDJIcmhIaG1oZm13

With the iOS simulator, go to

https://www.google.com/amp/s/www.nbcnews.com/news/us-news/amp/michael-cohen-used-trump-org-email-stormy-daniels-arrangements-n855021

Use Ctrl+F to open the find UI and search forward for the word "cohen". Continue to search forward until the page scrolls and the title of the article is no longer visible. Then search backward until the word "cohen" in the title of the article. The nbcnews.com header may appear and overlap with the result (see attached screenshot).

Note that the highlighted text match appears over the header, which is a behavior different from Gecko or Chromium but is the same as Safari Desktop. Quoting Ali:

"Displaying highlighted text over seems actually better than under, I guess the issue is - given many sites have overlay headers that hide/show - it might make sense to scroll more so the highlighted text comes to the middle of the viewport (or at least not in the top 25% of viewport) rather than just scrolling enough to make it visible on very top."

What do people think? Does that proposal make sense?

Here is a trivial patch to make the highlighted text always come to the middle of the viewport:

diff --git a/Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm
index f493d6ce092..05528de202b 100644
--- a/Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm
+++ b/Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm
@@ -160,7 +160,7 @@ void FindController::didFindString()
 
     // Scrolling the main frame is handled by the SmartMagnificationController class but we still
     // need to consider overflow nodes and subframes here.
-    frame.selection().revealSelection(SelectionRevealMode::RevealUpToMainFrame, ScrollAlignment::alignToEdgeIfNeeded, WebCore::DoNotRevealExtent);
+    frame.selection().revealSelection(SelectionRevealMode::RevealUpToMainFrame, ScrollAlignment::AlignCenter, WebCore::DoNotRevealExtent);
 }
 void FindController::didFailToFindString()

[Radar WebKit Bug Importer]

<rdar://problem/38500973>

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

(In reply to Frédéric Wang (:fredw) from comment #0)
> +   
> frame.selection().revealSelection(SelectionRevealMode::RevealUpToMainFrame,
> ScrollAlignment::AlignCenter, WebCore::DoNotRevealExtent);

Oops, sorry it's ScrollAlignment::alignCenterAlways

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

Created attachment 335937 [details]
Patch

[Tim Horton]

Comment on attachment 335937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335937&action=review

> Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm:163
> +    // On Mobile, many sites have overlay headers or footers that may overlap with the highlighted

I don’t love this comment, since fixed bars are not unique to “mobile”.

The ideal fix here would of course be to scroll as little as possible while avoiding fixedpos things, but I imagine that is quite hard. I guess this is a reasonable change in the meantime.

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

Committed r229801: <https://trac.webkit.org/changeset/229801>

(In reply to Tim Horton from comment #4)
> Comment on attachment 335937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335937&action=review
> 
> > Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm:163
> > +    // On Mobile, many sites have overlay headers or footers that may overlap with the highlighted
> 
> I don’t love this comment, since fixed bars are not unique to “mobile”.
> 
> The ideal fix here would of course be to scroll as little as possible while
> avoiding fixedpos things, but I imagine that is quite hard. I guess this is
> a reasonable change in the meantime.

Isn’t there an existing mechanism for estimating the obscured area, used for scroll by page on Mac?

(In reply to mitz from comment #6)
> Isn’t there an existing mechanism for estimating the obscured area, used for
> scroll by page on Mac?

FrameView::adjustScrollStepForFixedContent

[zalan]

Comment on attachment 335937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335937&action=review

> Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm:164
> +    // text, so we reveal the text at the center of the viewport. See https://webkit.org/b/183658

Please rollout the "On Mobile" part, it's misleading. Also, not sure why you need to refer back to the original bugzilla#. According to Tim, "The ideal fix here would of course be to scroll as little as possible while avoiding fixedpos things" so I'd rather see a reference to that here instead.

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

Thanks I'll open a new bug to investigate the cleaner option and update the comment to point to that new bug.

[zalan]

(In reply to Frédéric Wang (:fredw) from comment #9)
> Thanks I'll open a new bug to investigate the cleaner option and update the
> comment to point to that new bug.
Awesome! Thanks!

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

Committed r229846: https://trac.webkit.org/changeset/229846

And I also opened https://bugs.webkit.org/show_bug.cgi?id=183889