[PM-12345] Add cipher type settings for inline autofill menu #11260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ll settings service
…tings view component
…ill settings view component
…ne-menu-positioning-improvements feature flag is off
…ser setting disallows it
|
New Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11260 +/- ##
=======================================
Coverage 33.16% 33.16%
=======================================
Files 2779 2779
Lines 86228 86291 +63
Branches 16421 16438 +17
=======================================
+ Hits 28597 28619 +22
- Misses 55365 55402 +37
- Partials 2266 2270 +4 ☔ View full report in Codecov by Sentry. |
jprusik
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
danielleflinn
previously approved these changes
Sep 27, 2024
cagonzalezcs
suggested changes
Sep 27, 2024
Comment on lines
+188
to
+198
| if (!this.inlineMenuVisibility) { | ||
| await this.getInlineMenuVisibility(); | ||
| } | ||
|
|
||
| if (this.showInlineMenuCards == null) { | ||
| await this.getInlineMenuCardsVisibility(); | ||
| } | ||
|
|
||
| if (this.showInlineMenuIdentities == null) { | ||
| await this.getInlineMenuIdentitiesVisibility(); | ||
| } |
There was a problem hiding this comment.
❗ - We have a bit of an issue with how these settings are being referenced/stored.
If we cache these values, and a user changes the setting for showing inline menu cards/identities within their extension, we still retain the presentation of identities/cards on all fields until they refresh the page.
As a result of how we handle binding event listeners on each field, I think we're kind of forced to do a rebuild of the content script logic for each opened tab when the settings change. We do this for changes that happen to the inline menu visibility setting... and likely we need to do the same for these other settings.
Look at the implementation of AutofillService.handleInlineMenuVisibilityChange to see what we're doing when that setting is modified. Something similar needs to exist for the two now settings... either that, or we need to incorporate some extended logic to unset the listeners of identity or card fields separately (and reset the showAccountCreation element for login fields)
There was a problem hiding this comment.
addressed in 6463191
…isibility setting value changes
cagonzalezcs
reviewed
Oct 3, 2024
Comment on lines
3087
to
3092
| tabs.forEach((tab) => | ||
| BrowserApi.tabSendMessageData(tab, "updateAutofillInlineMenuVisibility", { | ||
| settingType: cipherType, | ||
| newSettingValue, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
this.reloadAutofillScripts call.
Since we're triggering a reload of the content scripts after this, we are in effect trigger a re-init of the entirety of the autofill content script behavior. Updating the setting with a message is a throwaway action as a result of that re-init.
The reason why I originally set up the updateAutofillInlineMenuVisibility message was to avoid doing this expensive re-load of the scripts. In the current implementation, the inline menu visibility setting is referenced when the menu is opened... the update to that reference should only occur when we need to identify if we want to open the whole menu or just the button on focus.
If the setting is going from and disabled to enabled state (or vice versa), we are forced to trigger a re-injection of the scripts since we load different scripts depending on the visibility setting... otherwise we can just update that content script ref.
Now that said, I think the right call here is to remove the updateAutofillInlineMenuVisibility message when changing the identity or credit card inline menu settings and just trigger a reload of the script in either regard. The reason we'd want to do that is to ensure we reset the listeners present on all fields. We could implement the reset ourselves in a more optimal manner... but given that these settings aren't likely to be changed often, I think it's fine for us to reload the scripts entirely.
There was a problem hiding this comment.
Suggested change
| tabs.forEach((tab) => | |
| BrowserApi.tabSendMessageData(tab, "updateAutofillInlineMenuVisibility", { | |
| settingType: cipherType, | |
| newSettingValue, | |
| }), | |
| ); |
Suggestion, remove this entirely
Comment on lines
3072
to
3084
| // If the setting change is for the behavior of overall inline menu display | ||
| if (cipherType == null) { | ||
| const inlineMenuPreviouslyDisabled = oldSettingValue === AutofillOverlayVisibility.Off; | ||
| const inlineMenuCurrentlyDisabled = newSettingValue === AutofillOverlayVisibility.Off; | ||
|
|
||
| if (!inlineMenuPreviouslyDisabled && !inlineMenuCurrentlyDisabled) { | ||
| tabs.forEach((tab) => | ||
| BrowserApi.tabSendMessageData(tab, "updateAutofillInlineMenuVisibility", { | ||
| newSettingValue, | ||
| }), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggested change
| // If the setting change is for the behavior of overall inline menu display | |
| if (cipherType == null) { | |
| const inlineMenuPreviouslyDisabled = oldSettingValue === AutofillOverlayVisibility.Off; | |
| const inlineMenuCurrentlyDisabled = newSettingValue === AutofillOverlayVisibility.Off; | |
| if (!inlineMenuPreviouslyDisabled && !inlineMenuCurrentlyDisabled) { | |
| tabs.forEach((tab) => | |
| BrowserApi.tabSendMessageData(tab, "updateAutofillInlineMenuVisibility", { | |
| newSettingValue, | |
| }), | |
| ); | |
| return; | |
| } | |
| const inlineMenuPreviouslyDisabled = previousSetting === AutofillOverlayVisibility.Off; | |
| const inlineMenuCurrentlyDisabled = currentSetting === AutofillOverlayVisibility.Off; | |
| if (!inlineMenuPreviouslyDisabled && !inlineMenuCurrentlyDisabled) { | |
| const tabs = await BrowserApi.tabsQuery({}); | |
| tabs.forEach((tab) => | |
| BrowserApi.tabSendMessageData(tab, "updateAutofillInlineMenuVisibility", { | |
| inlineMenuVisibility: currentSetting, | |
| }), | |
| ); | |
| return; |
Suggestion - revert this logic, only triggering an update when the actual inline menu visibility is modified.
| private async handleInlineMenuVisibilitySettingsChange( | ||
| oldSettingValue: InlineMenuVisibilitySetting | boolean, | ||
| newSettingValue: InlineMenuVisibilitySetting | boolean, | ||
| cipherType?: CipherType, |
There was a problem hiding this comment.
Suggested change
| cipherType?: CipherType, |
Suggestion - Remove this, it likely isn't necessary
| this.handleInlineMenuVisibilitySettingsChange( | ||
| previousSetting, | ||
| currentSetting, | ||
| CipherType.Identity, |
There was a problem hiding this comment.
Suggested change
| CipherType.Identity, |
Suggestion - Remove this
| this.handleInlineMenuVisibilitySettingsChange( | ||
| previousSetting, | ||
| currentSetting, | ||
| CipherType.Card, |
There was a problem hiding this comment.
Suggested change
| CipherType.Card, |
Suggestion - Remove this
Comment on lines
1366
to
1388
| const settingNewValue = data?.newSettingValue; | ||
|
|
||
| if (settingNewValue == null) { | ||
| return; | ||
| } | ||
|
|
||
| const settingType = data?.settingType; | ||
|
|
||
| // Setting value update is for overall inline menu visibility | ||
| if (settingType == null && !isNaN(settingNewValue as InlineMenuVisibilitySetting)) { | ||
| this.inlineMenuVisibility = settingNewValue as InlineMenuVisibilitySetting; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (typeof settingType === "boolean") { | ||
| if (settingType === CipherType.Card) { | ||
| this.showInlineMenuCards = settingNewValue as boolean; | ||
| } | ||
|
|
||
| if (settingType === CipherType.Identity) { | ||
| this.showInlineMenuIdentities = settingNewValue as boolean; | ||
| } |
There was a problem hiding this comment.
Suggested change
| const settingNewValue = data?.newSettingValue; | |
| if (settingNewValue == null) { | |
| return; | |
| } | |
| const settingType = data?.settingType; | |
| // Setting value update is for overall inline menu visibility | |
| if (settingType == null && !isNaN(settingNewValue as InlineMenuVisibilitySetting)) { | |
| this.inlineMenuVisibility = settingNewValue as InlineMenuVisibilitySetting; | |
| return; | |
| } | |
| if (typeof settingType === "boolean") { | |
| if (settingType === CipherType.Card) { | |
| this.showInlineMenuCards = settingNewValue as boolean; | |
| } | |
| if (settingType === CipherType.Identity) { | |
| this.showInlineMenuIdentities = settingNewValue as boolean; | |
| } | |
| if (!isNaN(data?.inlineMenuVisibility)) { | |
| this.inlineMenuVisibility = data.inlineMenuVisibility; |
Suggestion - Revert this, in effect the script reload that is happening when the inline menu cards and inline menu identities setting is changed will handle the expected change to happen on the content script side.
Comment on lines
27
to
28
| settingType?: CipherType; | ||
| newSettingValue?: InlineMenuVisibilitySetting | boolean; |
There was a problem hiding this comment.
Suggested change
| settingType?: CipherType; | |
| newSettingValue?: InlineMenuVisibilitySetting | boolean; | |
| inlineMenuVisibility?: InlineMenuVisibilitySetting; |
Suggestion - Revert this
cagonzalezcs
reviewed
Oct 3, 2024
cagonzalezcs
previously approved these changes
Oct 3, 2024
cagonzalezcs
approved these changes
Oct 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-12345
📔 Objective
Provide users a way to turn off the inline autofill menu for specific cipher types
Notes
"inline-menu-positioning-improvements"feature flag📸 Screenshots
Kapture.2024-09-26.at.15.57.18.mp4
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes