Bug 183658

Summary: [iOS] Text highlighted by the Find UI overlaps with NBC news header on google.com
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: FramesAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: dvoytenko, malteubl, mitz, simon.fraser, thorton, tonikitoo, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 178789    
Bug Blocks: 183889    
Attachments:
Description Flags
Screenshot
none
Patch thorton: review+

Description Frédéric Wang (:fredw) 2018-03-15 08:06:18 PDT
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()
Comment 1 Radar WebKit Bug Importer 2018-03-15 08:06:43 PDT
<rdar://problem/38500973>
Comment 2 Frédéric Wang (:fredw) 2018-03-15 08:07:19 PDT
(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
Comment 3 Frédéric Wang (:fredw) 2018-03-16 08:10:46 PDT
Created attachment 335937 [details]
Patch
Comment 4 Tim Horton 2018-03-21 01:13:55 PDT
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.
Comment 5 Frédéric Wang (:fredw) 2018-03-21 02:46:18 PDT
Committed r229801: <https://trac.webkit.org/changeset/229801>
Comment 6 mitz 2018-03-21 07:21:04 PDT
(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?
Comment 7 mitz 2018-03-21 07:22:07 PDT
(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
Comment 8 zalan 2018-03-21 08:24:28 PDT
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.
Comment 9 Frédéric Wang (:fredw) 2018-03-21 08:30:05 PDT
Thanks I'll open a new bug to investigate the cleaner option and update the comment to point to that new bug.
Comment 10 zalan 2018-03-21 08:31:53 PDT
(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!
Comment 11 Frédéric Wang (:fredw) 2018-03-21 23:49:40 PDT
Committed r229846: https://trac.webkit.org/changeset/229846

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