-
-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Find in Page #947
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent!
There is some more work to be done though so that we could eventually use it also with the Chromium backend though. We cannot break the engine agnostic approach of wolvic's architecture.
app/src/common/shared/com/igalia/wolvic/findinpage/FindInPageInteractor.kt
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/views/NavigationURLBar.java
Outdated
Show resolved
Hide resolved
ea765d9
to
6c048b7
Compare
6c048b7
to
f658605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, just some minor comments
app/src/common/shared/com/igalia/wolvic/findinpage/FindInPageInteractor.kt
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/browser/api/WSession.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I forgot to mention, please add some placeholders or empty implementations for the chromium backend, otherwise this patch would break the chromium build
Another comment, this is kind of a request that you could ignore if it involves a lot of work. I thought it'd be nice to show the prev/next buttons as inactive/hidden whenever either there is no result or there is no text to search yet. I guess this can be done in the xml by adding a condition on the number of results... |
Signed-off-by: Songlin Jiang <sjiang@igalia.com>
f658605
to
927e8e3
Compare
Add the getSessionFinder method override in chromium SessionImpl, hopefully, that will make the chromium backend build. But I don't have a test environment for that so I'm not 100% sure.
This should be done on the Mozilla Android component side (UI-related), maybe we can open an issue for feature requests there. https://github.com/mozilla-mobile/firefox-android/tree/main/android-components/components/feature/findinpage |
Submitted a feature request at: https://connect.mozilla.org/t5/ideas/find-in-page-for-firefox-android-hide-inactivate-previous-amp/idi-p/38778 |
Resolve #929
Translations are copied from Chromium. https://github.com/chromium/chromium/blob/main/components/strings/components_strings_en-GB.xtb
translation id: 3495081129428749620