[iOS] Add footer section in page action menu [chromium/src : main]
Francis Beauchamp (Gerrit)
Francis Beauchamp added 2 comments![]( "Open in Gerrit")
// Paddings and spacingNote to the reviewer: consolidated the ui spacing values here. I believe these kind of values should be in a much higher level of abstraction than here, but I wasn't able to find a centralized place responsible for consistent UI spacing.
- (UILabel*)primaryLabel {Note to the reviewer: while these are not directly linked to my CL, I believe it is better to centralize the creation of identical elements and have moved forward with this.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 3 comments![]( "Open in Gerrit")
Sending this your way Ibrahim so you can do a quick first pass of the review. More particularly for the question I've sent your way. Can also pitch in with the comment about symbols.
UIImage* icon =Here, an alternative would be to give the raw symbol name (i.e. `kEnterpriseSymbol`) instead of the UIImage and let the view controller configure with the proper sizing. Wondering if that is something we'd want? Not a big deal, but I think it can be a little more idiomatic (typically, a class like this handles the ordering and what gets presented, not how it is presented).
return IsPageToolsFeatureUnavailabilityEnabled()@kano...@google.com I think you were changing this, I'm not sure if your changes got in yet?
Related details
- Ibrahim Kanouche
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 1 comment![]( "Open in Gerrit")
UIImage* icon =Here, an alternative would be to give the raw symbol name (i.e. `kEnterpriseSymbol`) instead of the UIImage and let the view controller configure with the proper sizing. Wondering if that is something we'd want? Not a big deal, but I think it can be a little more idiomatic (typically, a class like this handles the ordering and what gets presented, not how it is presented).
I will move all of these into the page_action_menu_content_entry_point file which lives in the UI. I think this will be cleaner and remove some of the noise from this file which should really only have "business" logic.
Related details
- Ibrahim Kanouche
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche added 8 comments![]( "Open in Gerrit")
UIImage* icon =Here, an alternative would be to give the raw symbol name (i.e. `kEnterpriseSymbol`) instead of the UIImage and let the view controller configure with the proper sizing. Wondering if that is something we'd want? Not a big deal, but I think it can be a little more idiomatic (typically, a class like this handles the ordering and what gets presented, not how it is presented).
I agree. It would be more idiomatic for the Mediator to pass the symbol name (e.g., kEnterpriseSymbol) rather than a pre-configured UIImage.
By moving CustomSymbolWithPointSize(...) into the VC (or the view's configuration logic), we can keep layout-specific constants like kFeatureRowIconSize out of the Coordinator/Mediator layer. And this will makes the data item more reusable and the Mediator easier to test.
// Paddings and spacingNote to the reviewer: consolidated the ui spacing values here. I believe these kind of values should be in a much higher level of abstraction than here, but I wasn't able to find a centralized place responsible for consistent UI spacing.
I'd prefer we keep the original semantically named constants (kMenuSidePadding, kFeatureRowHorizontalPadding, etc.). The values being equal today is coincidental but they describe different visual elements and could diverge independently if UX updates the spec. Collapsing them into generic kSpacingSmall/Medium/Large tokens couples unrelated layout decisions together, and that makes future per-element adjustments harder.
}I think the calculation here is missing the `kSpacingMedium` (12pt) gap between `_contentStackView` and `_footerStackView`.
Currently: `navBar + contentHeight + 8 (top) + bottomPadding + footerHeight`.
Should be: `navBar + 8 (top) + contentHeight + 12 (gap) + footerHeight + bottomPadding`.
Without this, the sheet might be 12pt too short, potentially clipping the bottom of the footer.
_geminiButton = [self createBWGButton];To match the rename of `_BWGButton` to `_geminiButton`, you might want to rename this method to `createGeminiButton` and the handler to `handleGeminiTapped:`.
constant:-kSpacingMedium],This constraint is redundant with the one defined on lines 884-886. Since `contentBottomAnchor` is `_footerStackView.topAnchor`, this line effectively sets the same 12pt spacing. It's cleaner to keep the logic in the `if (IsPageToolsFeatureUnavailabilityEnabled())` block below.
[attributedText addAttributes:linkAttributes range:parsedString.ranges[0]];Accessing `ranges[0]` without checking `parsedString.ranges.count` is unsafe. If `action` is provided but the string doesn't contain the `<ph>` tags, this will crash.
Even though current callers pass `nil` for the action when no tags are present, it's safer to check the count here.
- (UILabel*)primaryLabel {Note to the reviewer: while these are not directly linked to my CL, I believe it is better to centralize the creation of identical elements and have moved forward with this.
can we do this on a separate CL? so we are not mixing up refactoring with feature work.
return IsPageToolsFeatureUnavailabilityEnabled()@kano...@google.com I think you were changing this, I'm not sure if your changes got in yet?
Yes this already landed: https://chromium-review.googlesource.com/c/chromium/src/+/7697138
Related details
- Francis Beauchamp
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 8 comments![]( "Open in Gerrit")
Thanks for the review Ibrahim! I've laid out some comments that could lead to interesting conversations.
// Paddings and spacingIbrahim KanoucheNote to the reviewer: consolidated the ui spacing values here. I believe these kind of values should be in a much higher level of abstraction than here, but I wasn't able to find a centralized place responsible for consistent UI spacing.
I'd prefer we keep the original semantically named constants (kMenuSidePadding, kFeatureRowHorizontalPadding, etc.). The values being equal today is coincidental but they describe different visual elements and could diverge independently if UX updates the spec. Collapsing them into generic kSpacingSmall/Medium/Large tokens couples unrelated layout decisions together, and that makes future per-element adjustments harder.
I'll try to explain why I think we should aim to unify spacing constants (and not just here; everywhere).
Without unified values, we end up with situations like the co-presence bubble not being aligned with the Gemini overlay. I expect an app of this size to use standardized spacing values and for the UX updates to align on those values; not the other way around. Defining your own custom value should be an exception, not the norm!
When we decide to specify custom values for everything like we have here, features easily become inconsistent. Flows start feeling disconnected. The polished look & feel that we would expect from the best iOS browser starts showing some cracks through this inconsistency.
I'd be curious to hear your thoughts on this and see if there are rooms for improvements in aligning and streamlining our UI work.
I think the calculation here is missing the `kSpacingMedium` (12pt) gap between `_contentStackView` and `_footerStackView`.
Currently: `navBar + contentHeight + 8 (top) + bottomPadding + footerHeight`.
Should be: `navBar + 8 (top) + contentHeight + 12 (gap) + footerHeight + bottomPadding`.Without this, the sheet might be 12pt too short, potentially clipping the bottom of the footer.
I think you speak the truth. I'll add the missing padding (gap in your example?) to the `IsPageToolsFeatureUnavailabilityEnabled` branch.
_geminiButton = [self createBWGButton];To match the rename of `_BWGButton` to `_geminiButton`, you might want to rename this method to `createGeminiButton` and the handler to `handleGeminiTapped:`.
Agreed 🎉
constant:-kSpacingMedium],This constraint is redundant with the one defined on lines 884-886. Since `contentBottomAnchor` is `_footerStackView.topAnchor`, this line effectively sets the same 12pt spacing. It's cleaner to keep the logic in the `if (IsPageToolsFeatureUnavailabilityEnabled())` block below.
I agree and I'll double check; I meant to be very explicit with the constraints but chances are that just the one constraint won't create any ambiguity for the layout engine.
[attributedText addAttributes:linkAttributes range:parsedString.ranges[0]];Accessing `ranges[0]` without checking `parsedString.ranges.count` is unsafe. If `action` is provided but the string doesn't contain the `<ph>` tags, this will crash.
Even though current callers pass `nil` for the action when no tags are present, it's safer to check the count here.
I guess you're right. I'm quite displeased with how verbose and tedious creating a label with links currently is and I have a proposal in the making to transform all of this into low level plumbing a one liner.
For our case here, I'll add the condition as `if (action && parsedString.ranges.count > 0)`.
- (UILabel*)primaryLabel {Ibrahim KanoucheNote to the reviewer: while these are not directly linked to my CL, I believe it is better to centralize the creation of identical elements and have moved forward with this.
can we do this on a separate CL? so we are not mixing up refactoring with feature work.
I get why you're suggesting this and I'm not entirely opposed to splitting the CL. This being said, I believe that improving the code we touch within our CLs has several tangible benefits.
First, it ensures that landing CLs consistently lowers tech debt over time. I think this is something that benefits not just us, but all of Chrome.
Second, it creates a culture of continuously delivered improvement; the alternative of pushing this "in the next cl" which often never comes is detrimental to quality.
I think this can be an interesting conversation to have with the rest of the team. I'm particularly interested in developing healthy habits and finding ways to improve the way we work together!
return IsPageToolsFeatureUnavailabilityEnabled()Ibrahim Kanouche@kano...@google.com I think you were changing this, I'm not sure if your changes got in yet?
Yes this already landed: https://chromium-review.googlesource.com/c/chromium/src/+/7697138
Sounds good; then it's fair to assume I can remove this change and revert to just returning `return geminiService->IsProfileEligibleForGemini()`? I'm wondering if I should add `IsPageToolsFeatureUnavailabilityEnabled` somewhere around your changes ? It's a bit unclear how the two of them are working together and whether they will be rolled out at the same time.
Related details
- Ibrahim Kanouche
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 8 comments![]( "Open in Gerrit")
Addressed main comments from the review.
UIImage* icon =Ibrahim KanoucheHere, an alternative would be to give the raw symbol name (i.e. `kEnterpriseSymbol`) instead of the UIImage and let the view controller configure with the proper sizing. Wondering if that is something we'd want? Not a big deal, but I think it can be a little more idiomatic (typically, a class like this handles the ordering and what gets presented, not how it is presented).
I agree. It would be more idiomatic for the Mediator to pass the symbol name (e.g., kEnterpriseSymbol) rather than a pre-configured UIImage.
By moving CustomSymbolWithPointSize(...) into the VC (or the view's configuration logic), we can keep layout-specific constants like kFeatureRowIconSize out of the Coordinator/Mediator layer. And this will makes the data item more reusable and the Mediator easier to test.
Done
Francis BeauchampI think the calculation here is missing the `kSpacingMedium` (12pt) gap between `_contentStackView` and `_footerStackView`.
Currently: `navBar + contentHeight + 8 (top) + bottomPadding + footerHeight`.
Should be: `navBar + 8 (top) + contentHeight + 12 (gap) + footerHeight + bottomPadding`.Without this, the sheet might be 12pt too short, potentially clipping the bottom of the footer.
I think you speak the truth. I'll add the missing padding (gap in your example?) to the `IsPageToolsFeatureUnavailabilityEnabled` branch.
Done
_geminiButton = [self createBWGButton];Francis BeauchampTo match the rename of `_BWGButton` to `_geminiButton`, you might want to rename this method to `createGeminiButton` and the handler to `handleGeminiTapped:`.
Agreed 🎉
Done
_geminiButton = [self createBWGButton];Francis BeauchampTo match the rename of `_BWGButton` to `_geminiButton`, you might want to rename this method to `createGeminiButton` and the handler to `handleGeminiTapped:`.
Agreed 🎉
Done
constant:-kSpacingMedium],Francis BeauchampThis constraint is redundant with the one defined on lines 884-886. Since `contentBottomAnchor` is `_footerStackView.topAnchor`, this line effectively sets the same 12pt spacing. It's cleaner to keep the logic in the `if (IsPageToolsFeatureUnavailabilityEnabled())` block below.
I agree and I'll double check; I meant to be very explicit with the constraints but chances are that just the one constraint won't create any ambiguity for the layout engine.
I've double tested and the conditional anchor is necessary. The fact that the content stack view bottom anchor attaches to the scrollview bottom anchor means that there is an ambiguity between the anchoring of the content and the footer. The result is a cropped footer, regardless of the footer view top anchoring.
What I can do though, is remove this part below:
```
[_footerStackView.topAnchor
constraintEqualToAnchor:_contentStackView.bottomAnchor
constant:kSpacingMedium],
```
And keep the conditional anchoring.
[attributedText addAttributes:linkAttributes range:parsedString.ranges[0]];Francis BeauchampAccessing `ranges[0]` without checking `parsedString.ranges.count` is unsafe. If `action` is provided but the string doesn't contain the `<ph>` tags, this will crash.
Even though current callers pass `nil` for the action when no tags are present, it's safer to check the count here.
I guess you're right. I'm quite displeased with how verbose and tedious creating a label with links currently is and I have a proposal in the making to transform all of this into low level plumbing a one liner.
For our case here, I'll add the condition as `if (action && parsedString.ranges.count > 0)`.
Done
return IsPageToolsFeatureUnavailabilityEnabled()Ibrahim Kanouche@kano...@google.com I think you were changing this, I'm not sure if your changes got in yet?
Francis BeauchampYes this already landed: https://chromium-review.googlesource.com/c/chromium/src/+/7697138
Sounds good; then it's fair to assume I can remove this change and revert to just returning `return geminiService->IsProfileEligibleForGemini()`? I'm wondering if I should add `IsPageToolsFeatureUnavailabilityEnabled` somewhere around your changes ? It's a bit unclear how the two of them are working together and whether they will be rolled out at the same time.
Discussed offline; I will rely on `IsPageActionMenuAuthFlowEnabled` for my tests.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francis Beauchamp (Gerrit)
Francis Beauchamp added 1 comment![]( "Open in Gerrit")
Looping you in to double check your thoughts on the entry point approach.
Related details
- Adam Arcaro
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro added 1 comment![]( "Open in Gerrit")
// Paddings and spacingIbrahim KanoucheNote to the reviewer: consolidated the ui spacing values here. I believe these kind of values should be in a much higher level of abstraction than here, but I wasn't able to find a centralized place responsible for consistent UI spacing.
Francis BeauchampI'd prefer we keep the original semantically named constants (kMenuSidePadding, kFeatureRowHorizontalPadding, etc.). The values being equal today is coincidental but they describe different visual elements and could diverge independently if UX updates the spec. Collapsing them into generic kSpacingSmall/Medium/Large tokens couples unrelated layout decisions together, and that makes future per-element adjustments harder.
I'll try to explain why I think we should aim to unify spacing constants (and not just here; everywhere).
Without unified values, we end up with situations like the co-presence bubble not being aligned with the Gemini overlay. I expect an app of this size to use standardized spacing values and for the UX updates to align on those values; not the other way around. Defining your own custom value should be an exception, not the norm!
When we decide to specify custom values for everything like we have here, features easily become inconsistent. Flows start feeling disconnected. The polished look & feel that we would expect from the best iOS browser starts showing some cracks through this inconsistency.
I'd be curious to hear your thoughts on this and see if there are rooms for improvements in aligning and streamlining our UI work.
I think this change is good, and we can always extract new constants if we need to adjust a specific element. This view controller has grown significantly and it's best to ensure consistency with spacing
Ideally we'd have more consistency at an app level, but that's a far bigger conversation!
Related details
- Francis Beauchamp
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ibrahim Kanouche (Gerrit)
Ibrahim Kanouche added 5 comments![]( "Open in Gerrit")
action:@selector(handleBWGTapped:)Since `_BWGButton` was renamed to `_geminiButton`, consider renaming `handleBWGTapped:` to `handleGeminiTapped:` for consistency.
[self updateFooterContent];Since `updateFooterContent` can add or remove items from the footer and change its total height, you should call `[self.sheetPresentationController invalidateDetents]` after updating to ensure the sheet resizes to fit the new content. (The same applies to `updateGeminiAvailability` below).
[_contentStackView.bottomAnchor constraintEqualToAnchor:contentBottomAnchorIf `IsPageToolsFeatureUnavailabilityEnabled()` is disabled, this constraint pins the bottom of the content stack to `_scrollView.bottomAnchor - kSpacingMedium`. However, this extra 12pt of content size is not accounted for in `resolveDetentValueForSheetPresentation:`, which will make the sheet slightly scrollable.
Also, if the feature is enabled but there are no unavailability items, this constraint unconditionally adds 12 points of empty space at the bottom of the menu.
To simplify the layout and fix both issues, we should make `_footerStackView` an arranged subview of `_contentStackView`. If you hide the footer when it's empty (`_footerStackView.hidden = items.count == 0;`), Auto Layout and `systemLayoutSizeFittingSize:` will automatically handle the heights and spacing perfectly without needing custom detent calculation logic.
stackView.alignment = UIStackViewAlignmentFill;With `UIStackViewAlignmentFill` and `UIViewContentModeScaleAspectFit`, the icon will be vertically centered relative to the multiline text. If the UX guidance expects the icon to be top-aligned with the first line of text, you should use `UIStackViewAlignmentTop`.
- (UILabel*)primaryLabel {Ibrahim KanoucheNote to the reviewer: while these are not directly linked to my CL, I believe it is better to centralize the creation of identical elements and have moved forward with this.
Francis Beauchampcan we do this on a separate CL? so we are not mixing up refactoring with feature work.
I get why you're suggesting this and I'm not entirely opposed to splitting the CL. This being said, I believe that improving the code we touch within our CLs has several tangible benefits.
First, it ensures that landing CLs consistently lowers tech debt over time. I think this is something that benefits not just us, but all of Chrome.
Second, it creates a culture of continuously delivered improvement; the alternative of pushing this "in the next cl" which often never comes is detrimental to quality.
I think this can be an interesting conversation to have with the rest of the team. I'm particularly interested in developing healthy habits and finding ways to improve the way we work together!
Acknowledged