Add an enterprise policy to disable KeyboardFocusableScrollers [chromium/src : main]
Di Zhang (Gerrit)
Di Zhang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
The code looks good, except for the case when the feature is disabled. Also the test has a few bugs.
If unset, the feature should remain enabled.nit: `the feature will be controlled by the state of the underlying runtime enabled feature.`
if (prefs->GetBoolean(
policy::policy_prefs::kKeyboardFocusableScrollersEnabled)) {
command_line->AppendSwitch(
blink::switches::kKeyboardFocusableScrollersEnabled);
}Does this work? It seems like setting the policy to `false` (which should turn off the feature) won't do anything in this case.
feature_list_.InitAndDisableFeature(
blink::features::kKeyboardFocusableScrollers);I think it'd be a good idea for this test to check both cases: when the base feature is enabled and disabled. I think (with the code as-is) it'll fail when the feature is enabled.
EXPECT_TRUE(message_queue.WaitForMessage(&message));no such thing
// an nterprise policy override.nit: `enterprise`
Related details
- Di Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 5 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
5 comments
nit: `the feature will be controlled by the state of the underlying runtime enabled feature.`
Done
if (prefs->GetBoolean(
policy::policy_prefs::kKeyboardFocusableScrollersEnabled)) {
command_line->AppendSwitch(
blink::switches::kKeyboardFocusableScrollersEnabled);
}Does this work? It seems like setting the policy to `false` (which should turn off the feature) won't do anything in this case.
Fixed by adding an else case.
feature_list_.InitAndDisableFeature(
blink::features::kKeyboardFocusableScrollers);I think it'd be a good idea for this test to check both cases: when the base feature is enabled and disabled. I think (with the code as-is) it'll fail when the feature is enabled.
I agree this should be
```
feature_list_.InitAndEnableFeature(
blink::features::kKeyboardFocusableScrollers);
```
However, why should we also test for disabled case? Wouldn't all cases return false since the feature is off?
EXPECT_TRUE(message_queue.WaitForMessage(&message));Di Zhangno such thing
Done
// an nterprise policy override.Di Zhangnit: `enterprise`
Done
Related details
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
if (prefs->GetBoolean(
policy::policy_prefs::kKeyboardFocusableScrollersEnabled)) {
command_line->AppendSwitch(
blink::switches::kKeyboardFocusableScrollersEnabled);But I don't think you want to do that. The point of the policy is to allow opting out. As the description of the policy says, if you set it to "true" or "unset", the feature is controlled by the underlying state of the feature. So I think you just want the else here, right?
feature_list_.InitAndDisableFeature(
blink::features::kKeyboardFocusableScrollers);Di ZhangI think it'd be a good idea for this test to check both cases: when the base feature is enabled and disabled. I think (with the code as-is) it'll fail when the feature is enabled.
I agree this should be
```
feature_list_.InitAndEnableFeature(
blink::features::kKeyboardFocusableScrollers);
```
However, why should we also test for disabled case? Wouldn't all cases return false since the feature is off?
You want to check:
1. Feature enabled, policy disabled (KFS should be disabled)
2. Feature enabled, policy enabled or default (KFS should be enabled)
3. Feature disabled, policy enabled or disabled or default (KFS should be disabled)
Related details
- Di Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
2 comments
if (prefs->GetBoolean(
policy::policy_prefs::kKeyboardFocusableScrollersEnabled)) {
command_line->AppendSwitch(
blink::switches::kKeyboardFocusableScrollersEnabled);But I don't think you want to do that. The point of the policy is to allow opting out. As the description of the policy says, if you set it to "true" or "unset", the feature is controlled by the underlying state of the feature. So I think you just want the else here, right?
Acknowledged
feature_list_.InitAndDisableFeature(
blink::features::kKeyboardFocusableScrollers);Di ZhangI think it'd be a good idea for this test to check both cases: when the base feature is enabled and disabled. I think (with the code as-is) it'll fail when the feature is enabled.
Mason FreedI agree this should be
```
feature_list_.InitAndEnableFeature(
blink::features::kKeyboardFocusableScrollers);
```
However, why should we also test for disabled case? Wouldn't all cases return false since the feature is off?
You want to check:
1. Feature enabled, policy disabled (KFS should be disabled)
2. Feature enabled, policy enabled or default (KFS should be enabled)
3. Feature disabled, policy enabled or disabled or default (KFS should be disabled)
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang added 1 comment![]( "Open in Gerrit")
Adding more enterprise policy reviewers, PTAL thanks.
Related details
- Colin Blundell
- Enterprise Policy Reviews
- Mason Freed
- Owen Min
- Pavel Feldman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
zm...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Colin Blundell
- Mason Freed
- Owen Min
- Pavel Feldman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! Please ping once the other reviewers have LGTM'd as they have the expertise for the technical changes here, and then I can stamp for whatever is still needed.
Related details
- Di Zhang
Owen Min (Gerrit)
Owen Min added 4 comments![]( "Open in Gerrit")
This policy provides a temporary opt-out for the new keyboard focusable scrollers behavior.
When this policy is Enabled or unset, scrollers without focusable children are keyboard-focusable by default. Further, scrollers are click-focusable and programmatically-focusable by default.
When this policy is Disabled, scrollers will not be focusable by default.
This policy is a temporary workaround, and will be removed in M135.Please add empty lines between each section.
```
This policy provides ...
When this policy is ...
When this policy is ...
This policy is a temporary ...
```
- caption: "Enabled: Scrollers without focusable children are keyboard-focusable by default. Scrollers are click-focusable and programmatically-focusable by default."You don't have to repeat yourself in the caption here for all details. Keep it short and say something like
```
Scrollers are focusable
Scrollers are not focusable
```
- chrome.*:126-We have passed the M126 branch point. Please merge it back to M126. In case we can't merge, please update the milestone to M127.
"policy_pref_mapping_tests": [Simple test can use `simple_policy_pref_mapping_test`
You can learn more from docs/enterprise/policy_pref_mapping_test.md.
Related details
- Di Zhang
- Mason Freed
- Pavel Feldman
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
- Di Zhang
- Pavel Feldman
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang added 4 comments![]( "Open in Gerrit")
This policy provides a temporary opt-out for the new keyboard focusable scrollers behavior.
When this policy is Enabled or unset, scrollers without focusable children are keyboard-focusable by default. Further, scrollers are click-focusable and programmatically-focusable by default.
When this policy is Disabled, scrollers will not be focusable by default.
This policy is a temporary workaround, and will be removed in M135.Please add empty lines between each section.
```
This policy provides ...When this policy is ...
When this policy is ...
This policy is a temporary ...
```
Done
- caption: "Enabled: Scrollers without focusable children are keyboard-focusable by default. Scrollers are click-focusable and programmatically-focusable by default."You don't have to repeat yourself in the caption here for all details. Keep it short and say something like
```
Scrollers are focusableScrollers are not focusable
```
Done
We have passed the M126 branch point. Please merge it back to M126. In case we can't merge, please update the milestone to M127.
Updating the milestone to M127 instead.
Simple test can use `simple_policy_pref_mapping_test`
You can learn more from docs/enterprise/policy_pref_mapping_test.md.
- Owen Min
- Pavel Feldman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Owen Min (Gerrit)
Owen Min voted Code-Review+1![]( "Open in Gerrit")
- Di Zhang
- Pavel Feldman
Di Zhang (Gerrit)
Di Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
2 comments
Updated the reviewers given current LGTM, PTAL thanks!
Related details
- Jeremy Roman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
2 comments
Bug: 40113891
Bug: 336490037
Bug: 337158524I think these are normally written on one line, separated by commas.
```suggestion
Bug: 40113891,336490037,337158524
```
Not sure whether tools process this form correctly or not.
"keyboard-focusable-scrollers-enabled";nit: usually these are named `enable-foo` and `disable-foo`; is there a reason to deviate here?
Related details
- Di Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +2 |
2 comments
I think these are normally written on one line, separated by commas.
```suggestion
Bug: 40113891,336490037,337158524
```Not sure whether tools process this form correctly or not.
Changed. (It does parse correctly e.g. https://chromium-review.googlesource.com/c/chromium/src/+/5519624)
nit: usually these are named `enable-foo` and `disable-foo`; is there a reason to deviate here?
We have a few Blink Features that use the format `kFeatureEnabled`, for example `kMutationEventsEnabled`. For this case, we chose to use `opt-out` instead so it can match origin trial feature KeyboardFocusableScrollersOptOut.
Related details
Code-Coverage
Code-Review
| 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
9 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Add an enterprise policy to disable KeyboardFocusableScrollers
This policy, when disabled, will revert to previous behavior when
scrollers are not focusable by default. If unset, the feature will be
controlled by the state of the underlying runtime enabled feature.
This feature broke a website's focus logic during Finch experiment.
Out of caution, I would like to add an enterprise policy to help
existing users to adapt to the new changes.
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/policy/BUILD.gn
- M chrome/browser/policy/configuration_policy_handler_list_factory.cc
- A chrome/browser/policy/test/keyboard_focusable_scollers_policy_browsertest.cc
- M components/policy/core/common/policy_pref_names.cc
- M components/policy/core/common/policy_pref_names.h
- M components/policy/resources/templates/policies.yaml
- A components/policy/resources/templates/policy_definitions/Miscellaneous/KeyboardFocusableScrollersEnabled.yaml
- A components/policy/test/data/pref_mapping/KeyboardFocusableScrollersEnabled.json
- M content/child/runtime_features.cc
- M testing/variations/fieldtrial_testing_config.json
- M third_party/blink/common/switches.cc
- M third_party/blink/public/common/switches.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M tools/metrics/histograms/metadata/enterprise/enums.xml
Code-Review: +1 by Mason Freed, +1 by Owen Min, +1 by Jeremy Roman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |