Skip to content

Commit

Permalink
[iOS][Omnibox] Fix RTL bug in treatment 1 new popup
Browse files Browse the repository at this point in the history
This cherry-pick has a merge conflict because this refactoring CL:
https://chromium-review.googlesource.com/c/chromium/src/+/3645048
is not present on the branch. Manually resolved.

Original CL description:

Fix RTL behavior in the new popup:
1. Use the existing setSemanticContentAttribute method to get omnibox's
text direction, and push it down as layoutDirection @Environment var
2. Update the layout-guide-to-swiftui-offset calculation to take RTL
into account
3. Stop aligning images by center, align by left edge instead. It's less
future-proof but makes code a bit more readable
4. Minor changes: previews configuration and debug description added
to PopupUIConfiguration


(cherry picked from commit 43d9d4d)

Fixed: 1327329
Change-Id: If00bc82b228b4c4056017bea6bc641000e619fa8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3657317
Auto-Submit: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Robbie Gibson <rkgibson@google.com>
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1007341}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3679078
Reviewed-by: Louis Romero <lpromero@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#406}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Stepan Khapugin authored and Chromium LUCI CQ committed May 31, 2022
1 parent 4e4edfa commit 732563f
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -72,21 +72,32 @@ where Content: View {
// gets accurate spacing.
let omniboxFrameInView = omniboxGuide.constrainedView.convert(
omniboxGuide.constrainedView.bounds, to: view)
uiConfiguration.omniboxLeadingSpace = omniboxFrameInView.minX
uiConfiguration.omniboxTrailingSpace = view.bounds.width - omniboxFrameInView.maxX

let isLTR =
UIApplication.shared.userInterfaceLayoutDirection
== UIUserInterfaceLayoutDirection.leftToRight

uiConfiguration.omniboxLeadingSpace =
isLTR ? omniboxFrameInView.minX : view.frame.size.width - omniboxFrameInView.maxX
uiConfiguration.omniboxTrailingSpace =
isLTR ? view.bounds.width - omniboxFrameInView.maxX : omniboxFrameInView.minX

let safeAreaFrame = safeAreaGuide.layoutFrame
uiConfiguration.safeAreaTrailingSpace = view.bounds.width - safeAreaFrame.maxX

let omniboxLeadingImageFrameInView = omniboxLeadingImageGuide.constrainedView.convert(
omniboxLeadingImageGuide.constrainedView.bounds, to: view)
uiConfiguration.omniboxLeadingImageLeadingSpace =
omniboxLeadingImageFrameInView.midX - omniboxFrameInView.minX
isLTR
? omniboxLeadingImageFrameInView.minX - omniboxFrameInView.minX
: omniboxFrameInView.maxX - omniboxLeadingImageFrameInView.maxX

let omniboxTextFieldFrameInView = omniboxTextFieldGuide.constrainedView.convert(
omniboxTextFieldGuide.constrainedView.bounds, to: view)
uiConfiguration.omniboxTextFieldLeadingSpace =
omniboxTextFieldFrameInView.minX - omniboxFrameInView.minX
isLTR
? omniboxTextFieldFrameInView.minX - omniboxFrameInView.minX
: omniboxFrameInView.maxX - omniboxTextFieldFrameInView.maxX
}

var hasContent: Bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ struct GradientTextView: View {

}

@Environment(\.popupUIVariation) var uiVariation: PopupUIVariation
@Environment(\.layoutDirection) var layoutDirection: LayoutDirection

var body: some View {
// This is equivalent to left/right since the locale is ignored below.
let variationTwoAlignment: Alignment = isTextLTR ? .leading : .trailing
let alignment: Alignment = layoutDirection == .leftToRight ? .leading : .trailing

ZStack {
// Render text at full size inside a fixed-size container.
Expand Down Expand Up @@ -103,7 +103,7 @@ struct GradientTextView: View {
let contents = VStack { text }
.frame(
width: geometry.size.width,
alignment: uiVariation == .one ? .leading : variationTwoAlignment
alignment: alignment
)
// Force LTR layout direction to prevent incorrect behavior in RTL locales.
// The goal is to deal with the text language, not the user's locale.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,17 @@ struct PopupMatchRowView: View {
Spacer().frame(
width: leadingMarginForRowContent + spaceBetweenRowContentLeadingEdgeAndSuggestionText)
customSeparatorColor.frame(height: 0.5)
}.environment(\.layoutDirection, .leftToRight)
}
}

@Environment(\.layoutDirection) var layoutDirection: LayoutDirection

var leadingMarginForRowContent: CGFloat {
switch uiVariation {
case .one:
return uiConfiguration.omniboxLeadingSpace
return uiConfiguration.omniboxLeadingSpace + 7
case .two:
return 0
return 8
}
}

Expand All @@ -114,6 +114,15 @@ struct PopupMatchRowView: View {
}
}

var spaceBetweenTextAndImage: CGFloat {
switch uiVariation {
case .one:
return 14
case .two:
return 15
}
}

var spaceBetweenRowContentLeadingEdgeAndSuggestionText: CGFloat {
switch uiVariation {
case .one:
Expand Down Expand Up @@ -148,20 +157,18 @@ struct PopupMatchRowView: View {
// The content is in front of the button, for proper hit testing.
HStack(alignment: .center, spacing: 0) {
Color.clear.frame(width: leadingMarginForRowContent)
HStack(alignment: .center, spacing: 0) {
Color.clear.frame(
width: spaceBetweenRowContentLeadingEdgeAndCenterOfSuggestionImage
- PopupMatchImageView.Dimension.image / 2)
match.image
.map { image in
PopupMatchImageView(
image: image, highlightColor: highlightColor
)
.accessibilityHidden(true)
.clipShape(RoundedRectangle(cornerRadius: 7, style: .continuous))
}
Spacer()
}.frame(width: spaceBetweenRowContentLeadingEdgeAndSuggestionText)

match.image
.map { image in
PopupMatchImageView(
image: image, highlightColor: highlightColor
)
.accessibilityHidden(true)
.clipShape(RoundedRectangle(cornerRadius: 7, style: .continuous))
}

Color.clear.frame(width: spaceBetweenTextAndImage)

VStack(alignment: .leading, spacing: 0) {
VStack(alignment: .leading, spacing: 0) {
GradientTextView(match.text, highlightColor: highlightColor)
Expand Down Expand Up @@ -190,14 +197,13 @@ struct PopupMatchRowView: View {
if match.isAppendable || match.isTabMatch {
PopupMatchTrailingButton(match: match, action: trailingButtonHandler)
.foregroundColor(isHighlighted ? highlightColor : .chromeBlue)
.environment(\.layoutDirection, layoutDirection)
}
Color.clear.frame(width: trailingMarginForRowContent)
}
.padding(
uiVariation == .one ? Dimensions.VariationOne.padding : Dimensions.VariationTwo.padding
)
.environment(\.layoutDirection, .leftToRight)
.environment(\.layoutDirection, layoutDirection)
}
.frame(maxWidth: .infinity, minHeight: Dimensions.minHeight)
}
Expand Down
5 changes: 4 additions & 1 deletion ios/chrome/browser/ui/omnibox/popup/shared/popup_model.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import UIKit
@Published var sections: [PopupMatchSection]

@Published var highlightedMatchIndexPath: IndexPath?
@Published var rtlContentAttribute: UISemanticContentAttribute = .forceLeftToRight

/// Index of the preselected section when no row is highlighted.
var preselectedSectionIndex: Int
Expand Down Expand Up @@ -56,7 +57,9 @@ import UIKit
}

public func setTextAlignment(_ alignment: NSTextAlignment) {}
public func setSemanticContentAttribute(_ semanticContentAttribute: UISemanticContentAttribute) {}
public func setSemanticContentAttribute(_ semanticContentAttribute: UISemanticContentAttribute) {
rtlContentAttribute = semanticContentAttribute
}
}

// MARK: OmniboxSuggestionCommands
Expand Down
17 changes: 15 additions & 2 deletions ios/chrome/browser/ui/omnibox/popup/shared/popup_view.swift
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,15 @@ struct PopupView: View {
}

func listContent(geometry: GeometryProxy) -> some View {
ForEach(Array(zip(model.sections.indices, model.sections)), id: \.0) {
let layoutDirection =
(model.rtlContentAttribute == .unspecified)
? (UIApplication.shared.userInterfaceLayoutDirection
== UIUserInterfaceLayoutDirection.leftToRight
? LayoutDirection.leftToRight : LayoutDirection.rightToLeft)
: ((model.rtlContentAttribute == .forceLeftToRight)
? LayoutDirection.leftToRight : LayoutDirection.rightToLeft)

return ForEach(Array(zip(model.sections.indices, model.sections)), id: \.0) {
sectionIndex, section in

let sectionContents =
Expand Down Expand Up @@ -287,7 +295,6 @@ struct PopupView: View {
var body: some View {
listView
.onAppear(perform: onAppear)
.environment(\.layoutDirection, .leftToRight)
}

@ViewBuilder
Expand Down Expand Up @@ -438,6 +445,12 @@ struct PopupView_Previews: PreviewProvider {
toolbarConfiguration: ToolbarConfiguration(style: .NORMAL))
)

sample.environment(\.popupUIVariation, .one)
.environment(\.locale, .init(identifier: "ar"))

sample.environment(\.popupUIVariation, .two)
.environment(\.locale, .init(identifier: "ar"))

sample.environment(\.popupUIVariation, .one)
sample.environment(\.popupUIVariation, .two)
sample.environment(\.locale, .init(identifier: "ar"))
Expand Down

0 comments on commit 732563f

Please sign in to comment.