Add feature and about_flags entry to disable KeyboardFocusableScrollers [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed added 10 comments![]( "Open in Gerrit")
flag_descriptions::kKeyboardFocusableScrollersOptOutName,So it's a lot easier to understand this if you link the `about:flags` flag to the main `KeyboardFocusableScrollers` feature. Then "enabled" means enabled and "disabled" means disabled.
"expiry_milestone": 140That's a long time. I'd maybe shoot for something shorter, like 134 or 135?
"When disabled, scrollers are not focusable by default. This should be "e.g. "disabled" here happens when this feature is "enabled", which is confusing to users.
kKeyboardFocusableScrollersOptOut = 4968,Thanks for going to the trouble to add a use counter, but I'm not sure it's necessary. Was there a reason you wanted to add one?
if (!RuntimeEnabledFeatures::KeyboardFocusableScrollersEnabled()) {
return false;
}
if (ExecutionContext* context = GetExecutionContext()) {
if (RuntimeEnabledFeatures::KeyboardFocusableScrollersOptOutEnabled(
context)) {
UseCounter::Count(context, WebFeature::kKeyboardFocusableScrollersOptOut);
return false;
}
}See my comment below, but this whole chunk would be `{insert logic here}`.
GetDocument()->GetExecutionContext())) &&Mason FreedThis change feels awkward. I am not sure how necessary it is to guard this A11Y code behind the opt out flag.
I think you definitely need to guard this code with the flag. This isn't terrible as-is, but perhaps it would be better to pull this logic out into something like
```
Document::KeyboardFocusableScrollersEnabled() {
return {insert logic here};
}
```
and then use that here and above?
// TODO(crbug.com/40113891): Disables KeyboardFocusableScrollers.nit: please add another sentence that this feature takes precedence over the one above, etc. etc.
origin_trial_type: "deprecation",Since this isn't a "deprecation", but I do agree you need this to allow the next two flags, you should add a comment about why you're adding this type. This flag is behaving kind of like a deprecation trial, to allow sites to have time to adjust to a breaking change, but the logic is inverted from a normal "deprecation".
histograms_xml_files = [Mason FreedThis file got regenerated by the tools/metrics/histograms/histogram_paths.py script.
Weird. It looks like it's just alphabetized, right? Did you have to run that script because of some presubmit warning or something?
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 9 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
9 comments
flag_descriptions::kKeyboardFocusableScrollersOptOutName,So it's a lot easier to understand this if you link the `about:flags` flag to the main `KeyboardFocusableScrollers` feature. Then "enabled" means enabled and "disabled" means disabled.
I am not sure I know how to link the flag? Do I rename these to:
kKeyboardFocusableScrollersName and kKeyboardFocusableScrollersDescription, but keep blink::features::kKeyboardFocusableScrollersOptOut?
That's a long time. I'd maybe shoot for something shorter, like 134 or 135?
Updated to 135.
"When disabled, scrollers are not focusable by default. This should be "e.g. "disabled" here happens when this feature is "enabled", which is confusing to users.
Done
Thanks for going to the trouble to add a use counter, but I'm not sure it's necessary. Was there a reason you wanted to add one?
As discussed offline, this feature does not need a use counter.
if (!RuntimeEnabledFeatures::KeyboardFocusableScrollersEnabled()) {
return false;
}
if (ExecutionContext* context = GetExecutionContext()) {
if (RuntimeEnabledFeatures::KeyboardFocusableScrollersOptOutEnabled(
context)) {
UseCounter::Count(context, WebFeature::kKeyboardFocusableScrollersOptOut);
return false;
}
}See my comment below, but this whole chunk would be `{insert logic here}`.
Done
GetDocument()->GetExecutionContext())) &&Mason FreedThis change feels awkward. I am not sure how necessary it is to guard this A11Y code behind the opt out flag.
I think you definitely need to guard this code with the flag. This isn't terrible as-is, but perhaps it would be better to pull this logic out into something like
```
Document::KeyboardFocusableScrollersEnabled() {
return {insert logic here};
}
```
and then use that here and above?
Done
// TODO(crbug.com/40113891): Disables KeyboardFocusableScrollers.nit: please add another sentence that this feature takes precedence over the one above, etc. etc.
Done
Since this isn't a "deprecation", but I do agree you need this to allow the next two flags, you should add a comment about why you're adding this type. This flag is behaving kind of like a deprecation trial, to allow sites to have time to adjust to a breaking change, but the logic is inverted from a normal "deprecation".
Done
histograms_xml_files = [Mason FreedThis file got regenerated by the tools/metrics/histograms/histogram_paths.py script.
Weird. It looks like it's just alphabetized, right? Did you have to run that script because of some presubmit warning or something?
- 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 3 comments![]( "Open in Gerrit")
flag_descriptions::kKeyboardFocusableScrollersOptOutName,Di ZhangSo it's a lot easier to understand this if you link the `about:flags` flag to the main `KeyboardFocusableScrollers` feature. Then "enabled" means enabled and "disabled" means disabled.
I am not sure I know how to link the flag? Do I rename these to:
kKeyboardFocusableScrollersName and kKeyboardFocusableScrollersDescription, but keep blink::features::kKeyboardFocusableScrollersOptOut?
These couple lines right here link them. You've typed this:
```
FEATURE_VALUE_TYPE(blink::features::kKeyboardFocusableScrollersOptOut)},
```
which connects this about:flags entry to the `kKeyboardFocusableScrollersOptOut` feature. But I think you should instead do this:
```
{"keyboard-focusable-scrollers",
flag_descriptions::kKeyboardFocusableScrollersName,
flag_descriptions::kKeyboardFocusableScrollersDescription, kOsAll,
FEATURE_VALUE_TYPE(blink::features::kKeyboardFocusableScrollers)},
```
"Disable keyboard focusable scrollers";
const char kKeyboardFocusableScrollersOptOutDescription[] =
"By choosing to opt out, we are disabling the feature "
"KeyboardFocusableScrollers. This means scrollers are not focusable by "
"default. This option should be used to diagnose any issue with the "
"feature.";These will need to change to match the change in feature above.
// TODO(accessibility) Scrollables are currently allowed here in orderPlease move this comment down to 7787.
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 3 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
3 comments
flag_descriptions::kKeyboardFocusableScrollersOptOutName,Di ZhangSo it's a lot easier to understand this if you link the `about:flags` flag to the main `KeyboardFocusableScrollers` feature. Then "enabled" means enabled and "disabled" means disabled.
Mason FreedI am not sure I know how to link the flag? Do I rename these to:
kKeyboardFocusableScrollersName and kKeyboardFocusableScrollersDescription, but keep blink::features::kKeyboardFocusableScrollersOptOut?
These couple lines right here link them. You've typed this:
```
FEATURE_VALUE_TYPE(blink::features::kKeyboardFocusableScrollersOptOut)},
```
which connects this about:flags entry to the `kKeyboardFocusableScrollersOptOut` feature. But I think you should instead do this:```
{"keyboard-focusable-scrollers",
flag_descriptions::kKeyboardFocusableScrollersName,
flag_descriptions::kKeyboardFocusableScrollersDescription, kOsAll,
FEATURE_VALUE_TYPE(blink::features::kKeyboardFocusableScrollers)},
```
Done
"Disable keyboard focusable scrollers";
const char kKeyboardFocusableScrollersOptOutDescription[] =
"By choosing to opt out, we are disabling the feature "
"KeyboardFocusableScrollers. This means scrollers are not focusable by "
"default. This option should be used to diagnose any issue with the "
"feature.";These will need to change to match the change in feature above.
Done
// TODO(accessibility) Scrollables are currently allowed here in orderPlease move this comment down to 7787.
- 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 voted and added 1 comment![]( "Open in Gerrit")
- Di Zhang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted Commit-Queue+2![]( "Open in Gerrit")
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
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/enums.xml
Insertions: 2, Deletions: 2.
@@ -17948,6 +17948,7 @@
<int value="-1469555275"
label="VmCameraMicIndicatorsAndNotifications:enabled"/>
<int value="-1469536698" label="ChromeHomeDoodle:enabled"/>
+ <int value="-1469234511" label="KeyboardFocusableScrollers:disabled"/>
<int value="-1469228683" label="QuickUnlockPinSignin:disabled"/>
<int value="-1469116522" label="Floss:disabled"/>
<int value="-1468126425" label="ResourceLoadScheduler:disabled"/>
@@ -19726,6 +19727,7 @@
<int value="-686788480" label="OsSettingsAppNotificationsPage:enabled"/>
<int value="-686761381" label="UseHDRTransferFunction:enabled"/>
<int value="-686617279" label="WebAppEnableLaunchHandler:enabled"/>
+ <int value="-685128769" label="KeyboardFocusableScrollers:enabled"/>
<int value="-685107150" label="FilesConflictDialog:enabled"/>
<int value="-684900739" label="disable-merge-key-char-events"/>
<int value="-684503292"
@@ -23008,7 +23010,6 @@
<int value="752939691" label="disable-tab-for-desktop-share"/>
<int value="756018202" label="CCTMinimized:enabled"/>
<int value="756897997" label="PaintHoldingCrossOrigin:disabled"/>
- <int value="756944411" label="KeyboardFocusableScrollers:enabled"/>
<int value="757645375" label="AndroidDarkSearch:disabled"/>
<int value="757912345" label="SnapGroup:disabled"/>
<int value="758299873" label="OmniboxSuggestionHoverFillShape:enabled"/>
@@ -25485,7 +25486,6 @@
<int value="1840892133" label="StylusWritingToInput:enabled"/>
<int value="1841051176" label="WindowsScrollingPersonality:disabled"/>
<int value="1841793150" label="TwoPanesStartSurfaceAndroid:enabled"/>
- <int value="1841920885" label="KeyboardFocusableScrollers:disabled"/>
<int value="1841976850" label="FeedLoadingPlaceholder:enabled"/>
<int value="1841993231"
label="AutofillRemoveCardExpirationAndTypeTitles:enabled"/>
```
Change information
Add feature and about_flags entry to disable KeyboardFocusableScrollers
We are starting an Origin Trial for KeyboardFocusableScrollers. We add
the feature KeyboardFocusableScrollersOptOut to allow individual sites
to opt-out of the feature.
An enterprise policy will be added in a follow-up patch.
- M chrome/browser/about_flags.cc
- M chrome/browser/flag-metadata.json
- M chrome/browser/flag_descriptions.cc
- M chrome/browser/flag_descriptions.h
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/document.h
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/modules/accessibility/ax_object.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/virtual/keyboard-focusable-scrollers-opt-out/README.md
- A third_party/blink/web_tests/virtual/keyboard-focusable-scrollers-opt-out/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus-expected.txt
- A third_party/blink/web_tests/virtual/keyboard-focusable-scrollers-opt-out/shadow-dom/focus-navigation/focus-navigation-scroller-interactive-child-expected.txt
- A third_party/blink/web_tests/virtual/keyboard-focusable-scrollers-opt-out/shadow-dom/focus-navigation/focus-navigation-scroller-tabindex-expected.txt
- A third_party/blink/web_tests/virtual/keyboard-focusable-scrollers-opt-out/shadow-dom/focus-navigation/focus-scroller-layout-update-expected.txt
- M tools/metrics/histograms/enums.xml
Code-Review: +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |