[PEPC] Disable focusing via script [chromium/src : main]
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45766.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
Related details
- Thomas Nguyen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
if (params.focus_trigger == FocusTrigger::kScript) {
return;
}A bit overlap as I am also looking at this, but thanks for the CL.
From what I saw, the `kScript` is called even not for focus() from JS, for example focus restoring. What will be expect in this case, we might need to verify it?
Related details
- Andy Paicu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
if (params.focus_trigger == FocusTrigger::kScript) {
return;
}A bit overlap as I am also looking at this, but thanks for the CL.
From what I saw, the `kScript` is called even not for focus() from JS, for example focus restoring. What will be expect in this case, we might need to verify it?
Thanks for bringing this to my attention, I had a look through usages:
[1] `Element::focusForBindings` this is for the `focus()` function so we for sure want to avoid that.
[2] `WebElement::SelectText` this seems to be for focusing text controls that have had their entire contents selected. Should be irrelevant to us.
[3] `TextControlElement::select()` seems to complement the one above
[4], [5] `HTMLDialogElement::close`, `HTMLElement::HidePopoverInternal` this is related to modal elements being closed and restoring focus to whatever was focused before they opened. If the permission element was focused before the dialog/popup opened, there is a good chance that the user has forgotten already about it and any enter/space key presses are probably unintentional. I think in this case ignoring the focus event is the correct thing to do.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/exported/web_element.cc;l=189
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/text_control_element.cc;l=277
[4] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=187
[5] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=1861
Related details
- Thomas Nguyen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
if (params.focus_trigger == FocusTrigger::kScript) {
return;
}Andy PaicuA bit overlap as I am also looking at this, but thanks for the CL.
From what I saw, the `kScript` is called even not for focus() from JS, for example focus restoring. What will be expect in this case, we might need to verify it?
Thanks for bringing this to my attention, I had a look through usages:
[1] `Element::focusForBindings` this is for the `focus()` function so we for sure want to avoid that.
[2] `WebElement::SelectText` this seems to be for focusing text controls that have had their entire contents selected. Should be irrelevant to us.
[3] `TextControlElement::select()` seems to complement the one above
[4], [5] `HTMLDialogElement::close`, `HTMLElement::HidePopoverInternal` this is related to modal elements being closed and restoring focus to whatever was focused before they opened. If the permission element was focused before the dialog/popup opened, there is a good chance that the user has forgotten already about it and any enter/space key presses are probably unintentional. I think in this case ignoring the focus event is the correct thing to do.[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/exported/web_element.cc;l=189
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/text_control_element.cc;l=277
[4] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=187
[5] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=1861
Thank you. And I think, it would be narrowed down to use mojom::blink::FocusType::kScript instead of FocusTrigger::kScript.
FocusTrigger::kScript is set by default and there would be more callees that we are note aware of.
Related details
- Andy Paicu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
if (params.focus_trigger == FocusTrigger::kScript) {
return;
}Andy PaicuA bit overlap as I am also looking at this, but thanks for the CL.
From what I saw, the `kScript` is called even not for focus() from JS, for example focus restoring. What will be expect in this case, we might need to verify it?
Thomas NguyenThanks for bringing this to my attention, I had a look through usages:
[1] `Element::focusForBindings` this is for the `focus()` function so we for sure want to avoid that.
[2] `WebElement::SelectText` this seems to be for focusing text controls that have had their entire contents selected. Should be irrelevant to us.
[3] `TextControlElement::select()` seems to complement the one above
[4], [5] `HTMLDialogElement::close`, `HTMLElement::HidePopoverInternal` this is related to modal elements being closed and restoring focus to whatever was focused before they opened. If the permission element was focused before the dialog/popup opened, there is a good chance that the user has forgotten already about it and any enter/space key presses are probably unintentional. I think in this case ignoring the focus event is the correct thing to do.[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/exported/web_element.cc;l=189
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/text_control_element.cc;l=277
[4] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=187
[5] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=1861
Thank you. And I think, it would be narrowed down to use mojom::blink::FocusType::kScript instead of FocusTrigger::kScript.
FocusTrigger::kScript is set by default and there would be more callees that we are note aware of.
- Thomas Nguyen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted Code-Review+1![]( "Open in Gerrit")
- Andy Paicu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
Hi Mason, PTAL html_permission_element.*
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +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 4 comments![]( "Open in Gerrit")
The PEPC element should not be able to focused via the `focus()`
function since it can be abused for obtaining unintentional clicks by
focusing the element at unexpected times.So this is quite unique/odd behavior that I don't believe has a precedent. I'm worried about accessibility and other things getting broken. Is there a discussion somewhere about this behavior?
console.log(event.target.id);Probably don't want a console.log in here?
// The exact number of "tab" presses needed to pass through all elementsWhy is that? You have `tabindex=0` on the important ones, so I would think the sequential focus behavior would be the same.
if (params.type == mojom::blink::FocusType::kScript) {
return;
}So you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
Related details
- Andy Paicu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 4 comments![]( "Open in Gerrit")
The PEPC element should not be able to focused via the `focus()`
function since it can be abused for obtaining unintentional clicks by
focusing the element at unexpected times.So this is quite unique/odd behavior that I don't believe has a precedent. I'm worried about accessibility and other things getting broken. Is there a discussion somewhere about this behavior?
The desire is to prevent programmatic focus on the permission element as it could easily be used by bad developers to obtain unintended activations (by getting the user to hit enter/space when the developers has sneakily focused the permission element). So we want to prevent APIs like `focus()` (I believe right now the only APIs to do this are `focus()` and `blur()`).
The reasoning is similar to why we don't want to allow developers to use `click()` and we filter out those events.
console.log(event.target.id);Probably don't want a console.log in here?
Oops.
// The exact number of "tab" presses needed to pass through all elementsWhy is that? You have `tabindex=0` on the important ones, so I would think the sequential focus behavior would be the same.
The variations seem to be on how many tabs it takes to actually reach the web contents elements, as it will first tab through the browser UI elements. I can re-work
if (params.type == mojom::blink::FocusType::kScript) {
return;
}So you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
`Element::IsFocusable()` doesn't seem to provide the information we need as the `UpdateBehavior` enum doesn't have a value that maps to "focus events coming from a script". I don't know how I would override `Element::IsFocusable()` so that it keeps the same behavior.
I'm confused why this would negatively impact A11y. The focus type enum specifically mentions accessibility events will be `kNone` and we're only filtering out `kScript`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/focus_type.mojom;l=7
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. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
// The exact number of "tab" presses needed to pass through all elementsAndy PaicuWhy is that? You have `tabindex=0` on the important ones, so I would think the sequential focus behavior would be the same.
The variations seem to be on how many tabs it takes to actually reach the web contents elements, as it will first tab through the browser UI elements. I can re-work
Somehow my previous comments got cut out:
I can re-work it to something like:
- tab until the first element gets focus
- tab 4 more times
- check all elements had focus
if you think this approach is too shoddy.
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 5 comments![]( "Open in Gerrit")
+aleventhal - please see the question below
The PEPC element should not be able to focused via the `focus()`
function since it can be abused for obtaining unintentional clicks by
focusing the element at unexpected times.Andy PaicuSo this is quite unique/odd behavior that I don't believe has a precedent. I'm worried about accessibility and other things getting broken. Is there a discussion somewhere about this behavior?
The desire is to prevent programmatic focus on the permission element as it could easily be used by bad developers to obtain unintended activations (by getting the user to hit enter/space when the developers has sneakily focused the permission element). So we want to prevent APIs like `focus()` (I believe right now the only APIs to do this are `focus()` and `blur()`).
The reasoning is similar to why we don't want to allow developers to use `click()` and we filter out those events.
This will all be interesting to get into standards. But I understand the desire here at least.
void Focus(const FocusParams& params) override;One thing I do think is absolutely required is to override `SupportsFocus()` here to be `true`, since you are explicitly allowing the element to be focused. Right?
if (params.type == mojom::blink::FocusType::kScript) {
return;
}Andy PaicuSo you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
`Element::IsFocusable()` doesn't seem to provide the information we need as the `UpdateBehavior` enum doesn't have a value that maps to "focus events coming from a script". I don't know how I would override `Element::IsFocusable()` so that it keeps the same behavior.
I'm confused why this would negatively impact A11y. The focus type enum specifically mentions accessibility events will be `kNone` and we're only filtering out `kScript`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/focus_type.mojom;l=7
Right - `IsFocusable()` is a property of the node, so it can't change depending on "how" the node tries to become focused. I suppose this might work?
+aleventhal to comment on potential issues around not allowing script focus. Perhaps there aren't any issues!
let el = document.getElementById("id1");
el.focus();
assert_not_equals(document.activeElement, el,
"Permission element should not be focused");Perhaps add a test that uses `test_driver`'s `pointerDown()` to manually click the element and make sure it *does* get focus in that case?
Related details
- Aaron Leventhal
- Andy Paicu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andy Paicu
| Auto-Submit | +0 |
| Commit-Queue | +1 |
2 comments
One thing I do think is absolutely required is to override `SupportsFocus()` here to be `true`, since you are explicitly allowing the element to be focused. Right?
Done
let el = document.getElementById("id1");
el.focus();
assert_not_equals(document.activeElement, el,
"Permission element should not be focused");Perhaps add a test that uses `test_driver`'s `pointerDown()` to manually click the element and make sure it *does* get focus in that case?
- Aaron Leventhal
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
if (params.type == mojom::blink::FocusType::kScript) {
return;
}Andy PaicuSo you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
Mason Freed`Element::IsFocusable()` doesn't seem to provide the information we need as the `UpdateBehavior` enum doesn't have a value that maps to "focus events coming from a script". I don't know how I would override `Element::IsFocusable()` so that it keeps the same behavior.
I'm confused why this would negatively impact A11y. The focus type enum specifically mentions accessibility events will be `kNone` and we're only filtering out `kScript`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/focus_type.mojom;l=7
Right - `IsFocusable()` is a property of the node, so it can't change depending on "how" the node tries to become focused. I suppose this might work?
+aleventhal to comment on potential issues around not allowing script focus. Perhaps there aren't any issues!
Can you tab to the element?
If so, I would think screen reader users will need to be able to navigate to the element in their virtual buffer, and set focus to it.
If that's true, it needs to support the focusable state.
Either way, we should have a content_browsertest for this element that asserts the correct focusable state, and correct behavior if the Action::kFocus action is used on it. Probably would go into content/browser/accessibility/accessibility_action_browsertest.cc.
Related details
- Andy Paicu
- 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 1 comment![]( "Open in Gerrit")
if (params.type == mojom::blink::FocusType::kScript) {
return;
}Andy PaicuSo you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
Mason Freed`Element::IsFocusable()` doesn't seem to provide the information we need as the `UpdateBehavior` enum doesn't have a value that maps to "focus events coming from a script". I don't know how I would override `Element::IsFocusable()` so that it keeps the same behavior.
I'm confused why this would negatively impact A11y. The focus type enum specifically mentions accessibility events will be `kNone` and we're only filtering out `kScript`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/focus_type.mojom;l=7
Aaron LeventhalRight - `IsFocusable()` is a property of the node, so it can't change depending on "how" the node tries to become focused. I suppose this might work?
+aleventhal to comment on potential issues around not allowing script focus. Perhaps there aren't any issues!
Can you tab to the element?
If so, I would think screen reader users will need to be able to navigate to the element in their virtual buffer, and set focus to it.
If that's true, it needs to support the focusable state.Either way, we should have a content_browsertest for this element that asserts the correct focusable state, and correct behavior if the Action::kFocus action is used on it. Probably would go into content/browser/accessibility/accessibility_action_browsertest.cc.
Aaron, when an AT using a virtual buffer this way tries to focus an element, what value of `mojom::blink::FocusType` does it use? I don't see one that is like `kAccessibility`, so I'm concerned that perhaps those calls use `kScript` which will get blocked here.
I presume this is why you're asking for an a11y test for this behavior, so perhaps that'll answer my question.
Related details
- Aaron Leventhal
- Andy Paicu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
if (params.type == mojom::blink::FocusType::kScript) {
return;
}Andy PaicuSo you don't want to do this, since it'll break accessibility. (The a11y code will think the element is focusable, and it won't be.)
You should instead override `Element::IsFocusable()` appropriately.
Mason Freed`Element::IsFocusable()` doesn't seem to provide the information we need as the `UpdateBehavior` enum doesn't have a value that maps to "focus events coming from a script". I don't know how I would override `Element::IsFocusable()` so that it keeps the same behavior.
I'm confused why this would negatively impact A11y. The focus type enum specifically mentions accessibility events will be `kNone` and we're only filtering out `kScript`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/focus_type.mojom;l=7
Aaron LeventhalRight - `IsFocusable()` is a property of the node, so it can't change depending on "how" the node tries to become focused. I suppose this might work?
+aleventhal to comment on potential issues around not allowing script focus. Perhaps there aren't any issues!
Mason FreedCan you tab to the element?
If so, I would think screen reader users will need to be able to navigate to the element in their virtual buffer, and set focus to it.
If that's true, it needs to support the focusable state.Either way, we should have a content_browsertest for this element that asserts the correct focusable state, and correct behavior if the Action::kFocus action is used on it. Probably would go into content/browser/accessibility/accessibility_action_browsertest.cc.
Aaron, when an AT using a virtual buffer this way tries to focus an element, what value of `mojom::blink::FocusType` does it use? I don't see one that is like `kAccessibility`, so I'm concerned that perhaps those calls use `kScript` which will get blocked here.
I presume this is why you're asking for an a11y test for this behavior, so perhaps that'll answer my question.
As per the comment in [1], this shouldn't apply to accessibility events. I've added a test and `SupportsFocus` implementation. It's also brought to attention the fact that the permission element doesn't have its default role set to "button" so it seemed fitting to do it here. PTAL.
Related details
- Aaron Leventhal
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Aaron Leventhal
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |