-
Notifications
You must be signed in to change notification settings - Fork 893
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
[iOS] - Add Page Translation #25391
[iOS] - Add Page Translation #25391
Conversation
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password, secure, insecure" and so security team members have been added as reviewers to take a look. |
@Brandon-T where the |
It comes from combining: https://source.chromium.org/chromium/chromium/src/+/main:components/translate/core/browser/resources/translate.js;l=5;drc=84091f477eff8e67261d6cdd9471519e8fee6185?q=translate&sq=&ss=chromium%2Fchromium%2Fsrc which downloads: https://translate.brave.com/static/v1/js/element/main.js We can't currently pull it from Brave-Core yet. |
886c0bc
to
e740285
Compare
ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/File.swift
Outdated
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Outdated
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,8438 @@ | |||
window.__firefox__ = {}; |
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.
It's impossible to review 8kb of minified/obfuscated JS. If it comes from Chromium - why is it obfuscated?
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.
I went ahead and found each script + dependency manually in Chromium. Then I got the de-obfuscated version of those scripts and replaces the necessary code with it + added links to where the script was found.
This should drastically help as there's no more minified/obfuscated JS: 885cb0e
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.
The current approach looks extremely hard to support and review.
It's not clear why we have to bundle all the stuff into a one script. Why we can't use the desktop approach?
Also, the script from translate.brave.com is designed to be updatable from the backend side.
Option 1 (preferable): enable translate code from Chromium for iOS (or at least partially): https://source.chromium.org/chromium/chromium/src/+/main:components/translate/ios/browser/
It probably needs some chromium primitives like WebState, not sure about the current status wherever we support them or not.
Option 2: bundle all the supplementary scripts from b-c using webpack and put them to resources. Load the main script from https://translate.brave.com/, combine with the new script from resources and inject
@kylehickinson @StephenHeaps maybe you have ideas how to simplify things here?
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.
- We don't support WebState yet, so for this PR it isn't possible to use the Desktop approach.
- If we load it from translate.brave.com, it doesn't help that it's still minified and unreadable and still impossible to review.
- Loading the scripts from translate.brave.com doesn't let us modify it to add Apple Translate support, which this PR does.
- How do we web-pack all the Chromium translate scripts and dependencies including all the ones specific to iOS that are in typescript, with Webpack?
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.
thanks for the aswers, @Brandon-T.
We don't support WebState yet, so for this PR it isn't possible to use the Desktop approach.
Ok, we can't reuse it as-is, but I mean to reuse the approach to build the script on the fly.
If we load it from translate.brave.com, it doesn't help that it's still minified and unreadable and still impossible to review.
It makes sense for the translate script itself, but not for all the script from /src.
The point is to have the same script for desktop & iOS, but different platform adapters.
If you want to improve the current translate script, it's stored here: https://github.com/brave/go-translate/tree/master/assets/static/v1
Loading the scripts from translate.brave.com doesn't let us modify it to add Apple Translate support
it's not clear to me.. If we download the script in advance and store locally, what is the difference with hardcoded script?
- How do we web-pack all the Chromium translate scripts and dependencies including all the ones specific to iOS that are in typescript, with Webpack?
Why we can't import them in a .ts file to transpile. That way we avoid duplicating the files.
Otherwise it's not clear how they will be synchronized in future.
Transpiling via transpile_web_ui
+ loading via a resource bundle (example) works for iOS.
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.
- I download it on the fly in the latest commit. So now it downloads the translate script instead of hard-coding.
transpile_web_ui
only works with WebUI. It doesn't work with just plain .ts files without any html.
https://github.com/brave/reviews/issues/1650#issuecomment-2223144564
It requires us to build resource files for iOS to export strings and other resources explicitly, when WebUI is not involved. We've had this discussion with Brian J. and a few others. Also it currently breaks unit tests. It's on the roadmap, but it's not ready yet, so for now it's not possible.
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.
Also it currently breaks unit tests.
Not sure specifics of what unit tests would break / how in this PR, but this sounds similar to unit tests in #25694. I will be updating that PR soon to allow loading the needed resources from BraveCore (we need to load procedural_filters.ts
to allow our ScriptExecutionTests
to use it & run again, some more info here). Not sure if that approach helps with unit test concerns here too.
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.
Deleted all the copied JS, and pulled it from Brave-Core: https://github.com/brave/brave-core/blob/0a452b206220fb6f5e629dc76c58a30ed4075a13/ios/browser/api/translate/translate_script.mm
via Features. We can't use WebState, and we can't use what CosmeticFilters are using as these scripts are already compiled.
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Show resolved
Hide resolved
ab61b03
to
8278a9e
Compare
16f359e
to
7b39433
Compare
885cb0e
to
e88edba
Compare
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift
Outdated
Show resolved
Hide resolved
6d8f27c
to
4417172
Compare
4417172
to
897ceb5
Compare
897ceb5
to
6bc61a9
Compare
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
6bc61a9
to
4c24b5c
Compare
Implement Translation UI Add Translate Toast and Settings Add language selection view Improve language detection. Allow translation of ReaderMode pages! Download Brave Translate scripts on the fly, lazily.
…s when disabled so URL-Bar doesn't show the button when disabled
4c24b5c
to
f5ed2af
Compare
[puLL-Merge] - brave/brave-core@25391 DescriptionThis PR adds a new Brave Translate feature to the iOS app. It includes functionality for detecting page language, offering translation options, and performing translations using a custom translation service. The feature is integrated into the browser UI with new buttons, settings, and user onboarding. ChangesChanges
sequenceDiagram
participant User
participant BrowserViewController
participant TabLocationView
participant BraveTranslateTabHelper
participant BraveTranslateSession
participant TranslationAPI
User->>BrowserViewController: Loads webpage
BrowserViewController->>BraveTranslateTabHelper: Detect page language
BraveTranslateTabHelper->>TabLocationView: Update translate button state
User->>TabLocationView: Taps translate button
TabLocationView->>BraveTranslateTabHelper: Start translation
BraveTranslateTabHelper->>BraveTranslateSession: Initiate translation
BraveTranslateSession->>TranslationAPI: Send translation request
TranslationAPI->>BraveTranslateSession: Return translated content
BraveTranslateSession->>BraveTranslateTabHelper: Process translated content
BraveTranslateTabHelper->>BrowserViewController: Update page with translation
BrowserViewController->>User: Display translated page
Possible Issues
Security Hotspots
|
Released in v1.76.4 |
Security Review
Summary
The tweaks are:
This allows us to intercept translate requests, and pass it to the Apple On Device translation. Everything else is a direct copy from Brave-Core (UNTIL we get Chromium WebViews finished. After that, there will be no need for such a giant script copy).
Resolves brave/brave-browser#40782
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Please note that Brave-Translate might not always work (iOS 17). This is entirely due to our backend!
Apple Translate should always work (iOS 18+).
Screen.Recording.2024-08-30.at.10.00.17.AM.mov