[iOS] Fix Post-FRE IPH Positioning By Preventing Contextual Panel Chips [chromium/src : main]
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 2 comments![]( "Open in Gerrit")
thanks Joemer.
My overall question is why is this promo non-FET? or, why don't we make the associated IPH respect FET conditions? this is the exact use case we'd want to be able to block the IPH appearing, because otherwise this logic feels very ad-hoc, and I'm wondering about any codepaths we may be missing which would not re-activate the CP chips.
if it's a product question maybe we should ask in Chat if it's okay that *rarely* we might not show the Gemini IPH since the reader mode one is showing?
// Prevents entrypoint IPH from showing while the non-FET dependent AI Hub IPHis it the entrypoint IPH we're targeting to block or the entrypoint itself?
Related details
- Adam Arcaro
- Joemer Ramos
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. |
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Commit-Queue | +1 |
2 comments
thanks Joemer.
My overall question is why is this promo non-FET? or, why don't we make the associated IPH respect FET conditions? this is the exact use case we'd want to be able to block the IPH appearing, because otherwise this logic feels very ad-hoc, and I'm wondering about any codepaths we may be missing which would not re-activate the CP chips.
if it's a product question maybe we should ask in Chat if it's okay that *rarely* we might not show the Gemini IPH since the reader mode one is showing?
Initially, the change to make the IPH not respect FET conditions was to avoid things like impression counts from other bubble presenters. We found that the Gemini IPH had a hard time showing due to [session rate impact](https://chromium-review.googlesource.com/c/chromium/src/+/6812435/10/components/feature_engagement/public/ios_promo_feature_configuration.cc) because other IPHs were showing. Considering that we only trigger this IPH once per user and as a result of FRE Gemini Promo dismissal, we felt like not relying on FET conditions would be okay.
I tried my best to thoroughly go through logic missing for "not re-activating" contextual panel chips, and I believe that every BWGCoordinator stop method will go through `[stopWithCompletion]` thus ensuring that we always reactive CP chips once the Gemini Promo is dismissed. The only event a promo isn't dismissed would be a tab being closed whether for background termination or force closing the app. In either case, the CP chips would re-activate with a new tab being created (default value for preventing is false).
As for the product question, I think the issue is above as stated and also fun fact, the reader mode chip expansion isn't an IPH which can be seen by the naming convention and logic in [these functions(https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm;l=501-512;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f). Not sure why though, I feel like it should be
// Prevents entrypoint IPH from showing while the non-FET dependent AI Hub IPHis it the entrypoint IPH we're targeting to block or the entrypoint itself?
We're targeting to block the entrypoint since the entrypoint is what handles the chip expansion. The entrypoint IPH is just a bubble that shows as we discussed. I inquired about the entrypoint IPH to validate that the IPH and the chip expansion were actually two different UIs.
Related details
- Adam Arcaro
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
| Commit-Queue | +1 |
3 comments
Joemer Ramosthanks Joemer.
My overall question is why is this promo non-FET? or, why don't we make the associated IPH respect FET conditions? this is the exact use case we'd want to be able to block the IPH appearing, because otherwise this logic feels very ad-hoc, and I'm wondering about any codepaths we may be missing which would not re-activate the CP chips.
if it's a product question maybe we should ask in Chat if it's okay that *rarely* we might not show the Gemini IPH since the reader mode one is showing?
Initially, the change to make the IPH not respect FET conditions was to avoid things like impression counts from other bubble presenters. We found that the Gemini IPH had a hard time showing due to [session rate impact](https://chromium-review.googlesource.com/c/chromium/src/+/6812435/10/components/feature_engagement/public/ios_promo_feature_configuration.cc) because other IPHs were showing. Considering that we only trigger this IPH once per user and as a result of FRE Gemini Promo dismissal, we felt like not relying on FET conditions would be okay.
I tried my best to thoroughly go through logic missing for "not re-activating" contextual panel chips, and I believe that every BWGCoordinator stop method will go through `[stopWithCompletion]` thus ensuring that we always reactive CP chips once the Gemini Promo is dismissed. The only event a promo isn't dismissed would be a tab being closed whether for background termination or force closing the app. In either case, the CP chips would re-activate with a new tab being created (default value for preventing is false).
As for the product question, I think the issue is above as stated and also fun fact, the reader mode chip expansion isn't an IPH which can be seen by the naming convention and logic in [these functions(https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm;l=501-512;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f). Not sure why though, I feel like it should be
Thanks for the context. It still feels like we should be hooked up in the FET (aren't there controls to be unaffected by session rate?), but I guess it can be left as future work. I also feel like we shouldn't be blocking the entire CP entrypoint (and thus the entire feature itself) from appearing simply because we want our IPH to show (which probably actually tracks back to Reader Mode should not be in the contextual panel in the first place, but I digress), and that this solution feels ad-hoc-y, but if it's what we want from a product perspective I don't see a short term solution much better than this. thanks!
// Prevents entrypoint IPH from showing while the non-FET dependent AI Hub IPHJoemer Ramosis it the entrypoint IPH we're targeting to block or the entrypoint itself?
We're targeting to block the entrypoint since the entrypoint is what handles the chip expansion. The entrypoint IPH is just a bubble that shows as we discussed. I inquired about the entrypoint IPH to validate that the IPH and the chip expansion were actually two different UIs.
Thanks. Right, so then should we update this comment to be "entrypoint" instead of "entrypoint IPH"?
Related details
- Adam Arcaro
- Joemer Ramos
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. |
Joemer Ramos (Gerrit)
Joemer Ramos added 2 comments![]( "Open in Gerrit")
My overall question is why is this promo non-FET? or, why don't we make the associated IPH respect FET conditions? this is the exact use case we'd want to be able to block the IPH appearing, because otherwise this logic feels very ad-hoc, and I'm wondering about any codepaths we may be missing which would not re-activate the CP chips.
if it's a product question maybe we should ask in Chat if it's okay that *rarely* we might not show the Gemini IPH since the reader mode one is showing?
Nicolas MacBethInitially, the change to make the IPH not respect FET conditions was to avoid things like impression counts from other bubble presenters. We found that the Gemini IPH had a hard time showing due to [session rate impact](https://chromium-review.googlesource.com/c/chromium/src/+/6812435/10/components/feature_engagement/public/ios_promo_feature_configuration.cc) because other IPHs were showing. Considering that we only trigger this IPH once per user and as a result of FRE Gemini Promo dismissal, we felt like not relying on FET conditions would be okay.
I tried my best to thoroughly go through logic missing for "not re-activating" contextual panel chips, and I believe that every BWGCoordinator stop method will go through `[stopWithCompletion]` thus ensuring that we always reactive CP chips once the Gemini Promo is dismissed. The only event a promo isn't dismissed would be a tab being closed whether for background termination or force closing the app. In either case, the CP chips would re-activate with a new tab being created (default value for preventing is false).
As for the product question, I think the issue is above as stated and also fun fact, the reader mode chip expansion isn't an IPH which can be seen by the naming convention and logic in [these functions(https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm;l=501-512;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f). Not sure why though, I feel like it should be
Thanks for the context. It still feels like we should be hooked up in the FET (aren't there controls to be unaffected by session rate?), but I guess it can be left as future work. I also feel like we shouldn't be blocking the entire CP entrypoint (and thus the entire feature itself) from appearing simply because we want our IPH to show (which probably actually tracks back to Reader Mode should not be in the contextual panel in the first place, but I digress), and that this solution feels ad-hoc-y, but if it's what we want from a product perspective I don't see a short term solution much better than this. thanks!
Sorry I don't think I was articulating well, to be clear, our IPH **is** hooked up in the FET ([here](https://chromium-review.googlesource.com/c/chromium/src/+/6812435/10/components/feature_engagement/public/ios_promo_feature_configuration.cc), but our FET configs make it "non-dependent" on FET criterias. I've updated the code comments to reflect a better explanation.
As for blocking the entire CP feature, I think this won't be for long. When we migrate to a better badge system, we'll be able to toggle the chip expansion (and the feature) a lot more freely. As of now, it's hard to "re-enable" the feature after our IPH goes away as explained in [point 1](https://docs.google.com/document/d/1V8M8U1V-GfpUJ_uuAj_rEE_7o12d-PNBNBFCR3ubkkc/edit?resourcekey=0-OPz22RHxp6qfwEETth2PYg&tab=t.pojatxyvh3q1) in this doc
// Prevents entrypoint IPH from showing while the non-FET dependent AI Hub IPHJoemer Ramosis it the entrypoint IPH we're targeting to block or the entrypoint itself?
Nicolas MacBethWe're targeting to block the entrypoint since the entrypoint is what handles the chip expansion. The entrypoint IPH is just a bubble that shows as we discussed. I inquired about the entrypoint IPH to validate that the IPH and the chip expansion were actually two different UIs.
Thanks. Right, so then should we update this comment to be "entrypoint" instead of "entrypoint IPH"?
Yes, thank you, done.
Related details
- Adam Arcaro
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Arcaro (Gerrit)
Adam Arcaro voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Arcaro
| Code-Review | +1 |
2 comments
Still lgtm with a style comment, thanks
void SetPreventContextualPanelEntryPoint(bool shouldPrevent);nit: `should_prevent` for C++
Related details
- Joemer Ramos
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. |
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Commit-Queue | +2 |
2 comments
void SetPreventContextualPanelEntryPoint(bool shouldPrevent);Joemer Ramosnit: `should_prevent` for C++
Fix applied.
void SetPreventContextualPanelEntryPoint(bool shouldPrevent);Joemer Ramosnit: `should_prevent` for C++
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper.h
Insertions: 1, Deletions: 1.
@@ -80,7 +80,7 @@
bool ShouldPreventContextualPanelEntryPoint();
// Setter for `prevent_contextual_panel_entry_point_`.
- void SetPreventContextualPanelEntryPoint(bool shouldPrevent);
+ void SetPreventContextualPanelEntryPoint(bool should_prevent);
// WebStateObserver:
void WasShown(web::WebState* web_state) override;
```
Change information
[iOS] Fix Post-FRE IPH Positioning By Preventing Contextual Panel Chips
Adds logic to prevent contextual panel chips from displaying when Gemini
promo is shown and plans to display the post FRE IPH.
UI Videos: crbug.com/447399963#comment6
- M ios/chrome/browser/contextual_panel/entrypoint/coordinator/BUILD.gn
- M ios/chrome/browser/contextual_panel/entrypoint/coordinator/DEPS
- M ios/chrome/browser/contextual_panel/entrypoint/coordinator/contextual_panel_entrypoint_mediator.mm
- M ios/chrome/browser/intelligence/bwg/coordinator/bwg_coordinator.mm
- M ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper.h
- M ios/chrome/browser/intelligence/bwg/model/bwg_tab_helper.mm
Code-Review: +1 by Nicolas MacBeth, +1 by Adam Arcaro
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |