Closed Bug 1164067 Opened 5 years ago Closed 4 years ago

Find-in-page

Categories

(Firefox for iOS :: Browser, enhancement)

All
iOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fxios 2.0+ ---

People

(Reporter: aaronmt, Assigned: bnicholson)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [parity=safari][parity=chrome])

Attachments

(3 files, 1 obsolete file)

We should support Find in Page.
Our Chromium friends filed this one https://bugs.webkit.org/show_bug.cgi?id=140210

I don't think we are blocking on that missing feature. We can probably do a decent enough impl in JavaScript until there is native support.
Requested in ios-feedback
Component: General → Browser
Hardware: ARM → All
Summary: Find in Page → Find-in-page
Assignee: nobody → dhenein
Status: NEW → ASSIGNED
Karen says 2.0.
Whiteboard: [parity=safari][parity=chrome] → [parity=safari][parity=chrome][nicetohave]
I'll take a look to see if I can throw together a quick prototype.
Assignee: dhenein → bnicholson
Early Find In Page prototype using window.find. It mostly works OK, though there are weird issues whenever the page contains absolutely positioned elements that match. Using this on Google, for example, causes the search input field to gain focus, which shows a search suggestions popup, and then there's some wonky behavior with random rects that aren't near the search term match.

A few notes:
* The find highlight must be translucent since we overlay the highlight onto the page.
* We can make case sensitive searches if wanted.
* We can limit searches to whole word matches if wanted.
I think the base implementation is complete enough, so onto UX!
Flags: needinfo?(randersen)
Flags: needinfo?(dhenein)
Whiteboard: [parity=safari][parity=chrome][nicetohave] → [parity=safari][parity=chrome][nicetohave][needs strings]
Duplicate of this bug: 1206475
Bumping to 2.0 since this needs some UX love.
Whiteboard: [parity=safari][parity=chrome][nicetohave][needs strings] → [parity=safari][parity=chrome]
Summary: Find-in-page → [meta] Find-in-page
tracking-fxios: ? → ---
Please reset the 2.0+ flag.
Bulk changes to Aha cards. Filter on 'mpopova-aha-20151008' to find all matching messages.
Blocks: 1213016
To enable, tap the menu icon on the bottom nav bar and choose Find in Page or long-press any text and an option for Find in Page will be included in the context menu. 

The search input loads with the keyboard enabled and a custom-color cursor blinking (230/96/0/100). The text entered is default typeface, regular weight, 18pt, 230/96/0/100. The search input bar includes a counter and previous/next navigation carets. The counter text is regular weight, 15pt, 216/216/216/100 when inactive, 155/155/155/100 when active.

If the user taps the webview, the keyboard will close but the search input bar will remain. An 'X' button allows dismissal.

Once a character has been entered, the text search has begun. Case does not matter. The first search result will highlight in 230/96/0/66 (if possible, behind the text), with subsequent results (if any) will highlight in 255/203/0/66 (if possible, behind the text). Tapping the previous/next carets will advance to the previous/next result if available.
Flags: needinfo?(randersen)
Attached file Find-in-Page.zip
Flags: needinfo?(dhenein)
For those who want to see the screens but hate downloading zips: https://invis.io/JK51NBM69
Summary: [meta] Find-in-page → Find-in-page
This PR replaces the bookmark button with a menu (which currently has just Find in Page and Bookmark). I think this is about as far as I can go for now, so back to UX for the next iteration.

Robin, can you take a look? Note that this PR only adds the toolbar -- I'll probably do the selection menu as a follow-up.

One thing I noticed is that when there are multiple results and I try clicking Next, I end up tapping the page/clicking random links because the keyboard slides down (and the Find in Page bar goes with it). Not sure if/how we want to handle that...Chrome avoids that issue by putting the bar at the top; Safari avoids it by keeping the keyboard on the screen.

When you get a chance to upload the assets, I'll go ahead and include them in the PR. I think that's just the menu button image, and the previous/next/close buttons for the Find in Page bar.
Attachment #8658977 - Attachment is obsolete: true
Flags: needinfo?(randersen)
:bnicholson, it's looking good! 2 nits:
Flags: needinfo?(randersen)
1) there's a slight delay in the keyboard catching up to the search bar when it animates up.
2) persist the search between tabs, the user has the option to X out.
(In reply to Robin Andersen [:tecgirl] from comment #17)
> 1) there's a slight delay in the keyboard catching up to the search bar when
> it animates up.

This is tricky since we're using the keyboard's animation curve, but the start locations of the find bar and keyboard are different (the keyboard starts at the bottom of the screen, but the find bar is above the toolbar). They both animate together, so the keyboard has to "catch up" to the find bar since it starts below it.

A few workarounds we could try:
1) Replace the bottom toolbar with the Find in Page bar so that the bar will always be at the very bottom of the screen when the keyboard isn't visible. This is what Safari does.
2) Don't show the Find in Page bar until after the keyboard animation has finished. This wouldn't fix the janky animation when the keyboard goes back down (after hitting Previous/Next), so we might also want to consider keeping the keyboard on the screen if we did this. Keeping the keyboard on the screen would also fix the issue I mentioned in comment 15.
3) Put the Find in Page bar at the top so that it doesn't need to animate with the keyboard. This is what Chrome does. This would also fix the issue I mentioned in comment 15.

> 2) persist the search between tabs, the user has the option to X out.

I'll probably end up doing this in a follow-up since this will require extra work. We'll have to manually re-show the bar after the tab change animation happens, and we'll also have to do a find in the newly selected tab to update the highlights/highlight count. Might be a bit janky since the find will also cause the tab to scroll to the first match.


A couple more questions (both can be addressed in a follow-up, just thinking out loud):
* What do we want the overflow menu to look like? I remember seeing a few different designs in your mockups. The PR is currently just using a standard UIAlertController.
* Does this mean we'll lose the jumpy bookmark star animation when we replace it with the menu button, or do we still want some effect somewhere?
Flags: needinfo?(randersen)
Depends on: 1229593
Depends on: 1229595
(In reply to Brian Nicholson (:bnicholson) from comment #18)
> (In reply to Robin Andersen [:tecgirl] from comment #17)
> > 1) there's a slight delay in the keyboard catching up to the search bar when
> > it animates up.
> 
> This is tricky since we're using the keyboard's animation curve, but the
> start locations of the find bar and keyboard are different (the keyboard
> starts at the bottom of the screen, but the find bar is above the toolbar).
> They both animate together, so the keyboard has to "catch up" to the find
> bar since it starts below it.
> 
> A few workarounds we could try:
> 1) Replace the bottom toolbar with the Find in Page bar so that the bar will
> always be at the very bottom of the screen when the keyboard isn't visible.
> This is what Safari does.

We should keep the bottom navigation so the user can use the same query across multiple pages.


> 2) Don't show the Find in Page bar until after the keyboard animation has
> finished. This wouldn't fix the janky animation when the keyboard goes back
> down (after hitting Previous/Next), so we might also want to consider
> keeping the keyboard on the screen if we did this. Keeping the keyboard on
> the screen would also fix the issue I mentioned in comment 15.

I would rather see the little bit of jank than have it pop in at the end of the load, and still prefer dropping the keyboard when the input isn't engaged to allow more visible viewport. 

> 3) Put the Find in Page bar at the top so that it doesn't need to animate
> with the keyboard. This is what Chrome does. This would also fix the issue I
> mentioned in comment 15.

I think it's cleaner at the bottom and won't clash with site nav (as seen on IRC).

> 
> > 2) persist the search between tabs, the user has the option to X out.
> 
> I'll probably end up doing this in a follow-up since this will require extra
> work. We'll have to manually re-show the bar after the tab change animation
> happens, and we'll also have to do a find in the newly selected tab to
> update the highlights/highlight count. Might be a bit janky since the find
> will also cause the tab to scroll to the first match.
> 

Great. I'll file. 


> 
> A couple more questions (both can be addressed in a follow-up, just thinking
> out loud):
> * What do we want the overflow menu to look like? I remember seeing a few
> different designs in your mockups. The PR is currently just using a standard
> UIAlertController.
> * Does this mean we'll lose the jumpy bookmark star animation when we
> replace it with the menu button, or do we still want some effect somewhere?
Flags: needinfo?(randersen)
With regards to the offset animation of the toolbar and keyboard, there might be a trick we can do to have it match.

UITextFields have an inputAccessoryView property that allows you to add a custom toolbar on top of the keyboard when the text field is selected. I wonder if we can use the exact same toolbar we want to display on top of the keyboard as the inputAccessoryView for the text field. The illusion is that the toolbar animates up when the keyboard is shown but in reality the text field is just a trigger and the real input happens in a clone that is the accessory view. There some more documentation here about this: https://developer.apple.com/library/ios/documentation/StringsTextFonts/Conceptual/TextAndWebiPhoneOS/InputViews/InputViews.html
Depends on: 1233284
Depends on: 1235446
Comment on attachment 8693732 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1296

Giving r? to you two since it looks like you're the only ones not on PTO :) Steph, I filed bug 1235446 as a follow-up to comment 20 to keep this PR as small as possible.
Attachment #8693732 - Flags: review?(sleroux)
Attachment #8693732 - Flags: review?(sarentz)
Keywords: uiwanted
Comment on attachment 8693732 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1296

Code looks good - mostly just nits. I also left a couple of UX comments on the PR thread.
Attachment #8693732 - Flags: review?(sleroux) → review+
Attachment #8693732 - Flags: review?(sarentz)
Depends on: 1235860
Merged with nits addressed: https://github.com/mozilla/firefox-ios/commit/93c5e71041e399bfe5250522bfb5b08ab45bcf1b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1235882
Depends on: 1241215
No longer blocks: 1239796
Depends on: 1239796
Depends on: 1245360
You need to log in before you can comment on or make changes to this bug.