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.

Sign up for GitHub

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

Jump to bottom

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding #26107

Merged

Brandon-T merged 8 commits into master from bugfix/URLBar-Eliding-Interstitial-iOS

Oct 22, 2024

Contributor

Brandon-T commented Oct 19, 2024 •

edited by kjozwiak

Loading

Resolves brave/brave-browser#41803
Resolves https://github.com/brave/internal/issues/1210

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Brandon-T added CI/skip-android CI/skip-macos-x64 CI/skip-windows-x64 CI/skip-macos-arm64 CI/skip-teamcity labels

Brandon-T self-assigned this

Brandon-T requested a review from a team as a code owner

October 19, 2024 03:43

Brandon-T requested a review from stoletheminerals

October 19, 2024 03:44

Brandon-T force-pushed the bugfix/URLBar-Eliding-Interstitial-iOS branch 3 times, most recently from 19268ec to 1664850 Compare

October 19, 2024 14:31

Brandon-T added 3 commits

October 21, 2024 10:26


          Fix interstitial not showing deceptive site warning. Fix URLBar Eliding

Copy code from #26099 and handle data, about, file, wss, etc schemes.


          Omit HTTP

245c1ba


          Fix left-alignment and URL parsing for URL-Bar display

f6b7742

Brandon-T force-pushed the bugfix/URLBar-Eliding-Interstitial-iOS branch from 1664850 to f6b7742 Compare

October 21, 2024 14:26


          Use the same logic in the collapsed url bar view.

1d6b26b

stoletheminerals reviewed

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/TabManager.swift Outdated Show resolved Hide resolved

kylehickinson requested changes

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift Outdated Show resolved Hide resolved

ios/brave-ios/Sources/Brave/Frontend/Browser/TabManager.swift Outdated Show resolved Hide resolved

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/CollapsedURLBarView.swift Outdated Show resolved Hide resolved

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/CollapsedURLBarView.swift Outdated

-                        URLFormatter.formatURLOrigin(
-                          forDisplayOmitSchemePathAndTrivialSubdomains: $0.absoluteString
-                        )
+                        if URLOrigin(url: $0).url == nil && URIFixup.getURL($0.absoluteString) == nil {

Collaborator

kylehickinson Oct 21, 2024

What are trying to check here? URLOrigin.url being nil could mean the origin is opaque or it could mean the resulting GURL is not valid (the origin itself doesn't have a valid scheme/host/port tuple), or even the GURL to NSURL conversion failed. Why are we then checking URIFixup?

Contributor Author

Brandon-T Oct 21, 2024

What do you mean by opaque? Like about:home?

The reason for the URIFixup is because if it returns null, it's not a valid URL at all. It also checks about: scheme URLs via the AutoCompleteClassifier which is what Chrome does. So it'll return nil for all about: URLs :)

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TabLocationView.swift Outdated Show resolved Hide resolved

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TabLocationView.swift Outdated

Comment on lines 570 to 588

+                    let attributedString = NSMutableAttributedString(string: text)
+                    let paragraphStyle = NSMutableParagraphStyle()
+                    paragraphStyle.lineBreakMode = .byClipping
+                    paragraphStyle.baseWritingDirection = .leftToRight
+                    if let font = font {
+                      attributedString.addAttribute(
+                        .font,
+                        value: font,
+                        range: NSRange(location: 0, length: attributedString.length)
+                      )
+                    }
+                    attributedString.addAttribute(
+                      .paragraphStyle,
+                      value: paragraphStyle,
+                      range: NSRange(location: 0, length: attributedString.length)
+                    )
+                    self.attributedText = attributedString

Collaborator

kylehickinson Oct 21, 2024

Why is this all needed when we're already overriding the underlying drawText method to adjust the visible rect?

Contributor Author

Brandon-T Oct 21, 2024

The underlying draw text does not properly handle right alignment in Arabic. I tested a mixed URL consisting of English and Arabic and the underlying code will always right align which is wrong if the domain itself is English (left aligned).

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TabLocationView.swift Outdated

@@ @@ -565,7 +603,7 @@ private class DisplayURLLabel: UILabel { @@
                   case .arabic, .hebrew, .persian, .urdu:
                     isRightToLeft = true
                   default:
-                    isRightToLeft = false
+                    isRightToLeft = ["http", "https"].contains(URL(string: text)?.scheme ?? "")  // Only left-align if the scheme is not http/https.

Collaborator

kylehickinson Oct 21, 2024 •

edited

Loading

This logic shouldn't go here, which is primarily for determining RTL language modes. Instead you should introduce a new variable that is set when text is set that will store whether or not the underlying URL is a web scheme one. You're already doing that when you do URLOrigin(...).url, you could store the origin and check the scheme directly instead in the drawText override. This also may mean adjusting where the // Update clipping fade direction logic goes which is usually set right below this

Contributor Author

Brandon-T Oct 21, 2024

Done.


          Address feedback

Brandon-T requested review from kylehickinson and stoletheminerals

October 21, 2024 21:19

kylehickinson reviewed

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TabLocationView.swift Outdated

Comment on lines 564 to 568

+                      if let text = text {
+                        isWebScheme = ["http", "https"].contains(URL(string: text)?.scheme ?? "")
+                      } else {
+                        isWebScheme = false
+                      }

Collaborator

kylehickinson Oct 21, 2024

You set this after calling detectLanguageForNaturalDirectionClipping which relies on it when updating the gradientLayer

Collaborator

kylehickinson Oct 21, 2024

You probably need to set this before updateText

Contributor Author

Brandon-T Oct 21, 2024 •

edited

Loading

EDIT: Fixed.

kylehickinson reviewed

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/TabLocationView.swift Outdated

@@ @@ -628,7 +634,7 @@ private class DisplayURLLabel: UILabel { @@
                   super.layoutSubviews()
                   clippingFade.frame = .init(
-                    x: isRightToLeft ? bounds.width - 20 : 0,
+                    x: isRightToLeft && isWebScheme ? bounds.width - 20 : 0,

Collaborator

kylehickinson Oct 21, 2024

This should be isRightToLeft || !isWebScheme since you want the clipping on the trailing edge if its not a web scheme

Contributor Author

Brandon-T Oct 22, 2024 •

edited

Loading

Fixed everything: https://github.com/brave/internal/issues/1210#issuecomment-2427950790
and verified everything is working properly again.

So Arabic is displaying the URLs correctly, even if they contain RTL characters.

Brandon-T added 2 commits

October 21, 2024 18:03


          Left align URLs if they are NOT http/https. If the URL-Bar is RTL, th…

9c28b46

…en right align them.


          Addressed Feedback.

Brandon-T requested a review from kylehickinson

October 22, 2024 00:08

Brandon-T enabled auto-merge (squash)

October 22, 2024 14:32


          Cleanup & add location view invalidation when isWebScheme changes

fcfc189

kylehickinson approved these changes

View reviewed changes

Brandon-T merged commit 126b504 into master

17 checks passed

Brandon-T deleted the bugfix/URLBar-Eliding-Interstitial-iOS branch

October 22, 2024 17:26

github-actions bot added this to the 1.73.x - Nightly milestone

Brandon-T added a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

9cc22fd

…ng (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

Brandon-T added a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

de6f8a2

…ng (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

Brandon-T added a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

e3766dd

…ng (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

This was referenced Oct 22, 2024

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (1.70.x) #26148

Merged

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (1.71.x) #26149

Merged

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (1.72.x) #26150

Merged

Collaborator

brave-builds commented Oct 22, 2024

Released in v1.73.37

Member

kjozwiak commented Oct 23, 2024

Verification PASSED on iPhone 11 running iOS 18 using the following build(s):

Brave | 1.73.37 Chromium: 130.0.6723.58 (Official Build) nightly (64-bit) 
--- | ---
Revision | 0056fe288a70...
OS | iOS

Verification notes can be found via https://github.com/brave/internal/issues/1210#issuecomment-2430624953.

kjozwiak pushed a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

a246b0c

…ng (1.72.x) (#26150)

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

kjozwiak pushed a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

73e29cc

…ng (1.71.x) (#26149)

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

kjozwiak pushed a commit that referenced this pull request


          Fix interstitial not showing deceptive site warning. Fix URLBar Elidi…

0e8a62f

…ng (1.70.x) (#26148)

Fix interstitial not showing deceptive site warning. Fix URLBar Eliding (#26107)

* Fix interstitial not showing deceptive site warning. Fix URLBar Eliding
* Fix left-alignment and URL parsing for URL-Bar display
* Left align URLs if they are NOT http/https. If the URL-Bar is RTL, then right align them.

kylehickinson mentioned this pull request

http scheme is shown in the URL bar when visiting insecure sites brave/brave-browser#42070

Closed

5 tasks

bsclifton modified the milestones: 1.73.x - Nightly, 1.74.x - Nightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/skip-android CI/skip-macos-arm64 CI/skip-macos-x64 CI/skip-teamcity CI/skip-windows-x64