[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. |
Mason Freed (Gerrit)
Mason Freed added 8 comments![]( "Open in Gerrit")
Ok, given the comments and additional tests, I'm ok with this change. Just some nits and bug fixes now. Thanks!
invalid_pepc->AccessibilityPerformAction(action_data);
valid_pepc->AccessibilityPerformAction(action_data);Even if `invalid_pepc` was focusable, this test would pass, since you focus them both right after each other. I think you would need to add more `ASSERT`s between these two calls to make sure.
return !permission_descriptors_.empty();Comment would be nice here also: "The permission element is only focusable if it has a valid type" or something similar.
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.
Andy PaicuAaron, 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.
Ok, thanks for the additional context. I think it's worth adding a comment above this line explaining what this does, and that a11y is not affected.
return ax::mojom::blink::Role::kButton;Yes, nice, thanks!
<permission tabindex="0" id="id1" type="geolocation">Perhaps also add a case for `type=invalid`?
test_driver.set_test_context(window.top);I'm not sure this is needed...?
assert_equals(document.activeElement, el,This is clicking on the `<span>`. I think you might want to click on the `<permission>` element?
Related details
- Aaron Leventhal
- Andy Paicu
- Thomas Nguyen
Aaron Leventhal (Gerrit)
Aaron Leventhal added 3 comments![]( "Open in Gerrit")
Thanks for the a11y changes so far. I'm fine with what we have here, although I do have a question about the accessible name for these elements. You can either do the following in a new CL or in here:
We should have a DumpAccessibilityTreeTest. It should have a corresponding pair of files:
content/test/data/accessibility/html/permission.html
content/test/data/accessibility/html/permission-expected-blink.txt
There's a readme in content/test/data/accessibility
The name of the test would be something like:
All/DumpAccessibilityTreeTest.AccessibilityPermission/blink
You can copy an example from the AccessibilityDiv test
Here's the file it goes into:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/dump_accessibility_tree_browsertest.cc
<permission type="invalid" aria-label="invalid-pepc">Is there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
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.
Andy PaicuAaron, 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.
Mason FreedAs 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.
Ok, thanks for the additional context. I think it's worth adding a comment above this line explaining what this does, and that a11y is not affected.
Andy/Mason, can you take a look at AXNodeObject::OnNativeFocusAction() and make sure everything looks right? That's the function where we handle the incoming focus request from an AT.
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Andy Paicu (Gerrit)
Andy Paicu added 8 comments![]( "Open in Gerrit")
Thanks for the a11y changes so far. I'm fine with what we have here, although I do have a question about the accessible name for these elements. You can either do the following in a new CL or in here:
We should have a DumpAccessibilityTreeTest. It should have a corresponding pair of files:
content/test/data/accessibility/html/permission.html
content/test/data/accessibility/html/permission-expected-blink.txt
There's a readme in content/test/data/accessibility
The name of the test would be something like:
All/DumpAccessibilityTreeTest.AccessibilityPermission/blink
You can copy an example from the AccessibilityDiv testHere's the file it goes into:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/dump_accessibility_tree_browsertest.cc
Ack, will do in a follow-up.
<permission type="invalid" aria-label="invalid-pepc">Is there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
They do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
invalid_pepc->AccessibilityPerformAction(action_data);
valid_pepc->AccessibilityPerformAction(action_data);Even if `invalid_pepc` was focusable, this test would pass, since you focus them both right after each other. I think you would need to add more `ASSERT`s between these two calls to make sure.
I tested this with `HtmlPermissionElement::SupportsFocus` hardcoded to `return true` and the test failed because the waiter immediately returned after the first focus event for the invalid element. So the test does not currently pass if both elements are actually focusable.
I agree that it's not the best but I don't really see how to improve it. There aren't any productive `ASSERT`s to put in between the 2 calls, because the focus event requires an async step in order to take effect. Before making the `WaitForNotification` call there won't be time for any element to become focused, so there's no point in checking the focus state. And there is no real way to wait for the focus event to *not* do anything on the `invalid_pepc` element, other than the somewhat brittle wait-for-a-fixed-amount-of-time approach.
I'm happy to add any checks you suggest but I think that any such checks will be useless before a `WaitForNotification` call and they will pass regardless of the focus behavior of the `permission` element.
Comment would be nice here also: "The permission element is only focusable if it has a valid type" or something similar.
Done
Comment added.
The code seems to work as expected to me. It sets a `FocusTrigger` but not a `FocusType` so it will use the `FocusType::kNone` type which therefore passes the `HTMLPermissionElement::Focus` check (and `kNone` is the correct value according to the comment above).
Perhaps also add a case for `type=invalid`?
Done
I'm not sure this is needed...?
Done
This is clicking on the `<span>`. I think you might want to click on the `<permission>` element?
`el` got re-initialized to element with id="id1" on line 28. For clarity I've moved this part a bit up.
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
They do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
An accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
Aaron LeventhalThey do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
An accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
I imagine this feature will need to go through an a11y review -- put something you think is reasonable for the name/description for now.
Mason Freed (Gerrit)
Mason Freed voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
6 comments
invalid_pepc->AccessibilityPerformAction(action_data);
valid_pepc->AccessibilityPerformAction(action_data);Andy PaicuEven if `invalid_pepc` was focusable, this test would pass, since you focus them both right after each other. I think you would need to add more `ASSERT`s between these two calls to make sure.
I tested this with `HtmlPermissionElement::SupportsFocus` hardcoded to `return true` and the test failed because the waiter immediately returned after the first focus event for the invalid element. So the test does not currently pass if both elements are actually focusable.
I agree that it's not the best but I don't really see how to improve it. There aren't any productive `ASSERT`s to put in between the 2 calls, because the focus event requires an async step in order to take effect. Before making the `WaitForNotification` call there won't be time for any element to become focused, so there's no point in checking the focus state. And there is no real way to wait for the focus event to *not* do anything on the `invalid_pepc` element, other than the somewhat brittle wait-for-a-fixed-amount-of-time approach.
I'm happy to add any checks you suggest but I think that any such checks will be useless before a `WaitForNotification` call and they will pass regardless of the focus behavior of the `permission` element.
I'm glad the test fails without the code change - thanks for verifying.
Wouldn't this do it then?
```
invalid_pepc->AccessibilityPerformAction(action_data);
ASSERT_FALSE(waiter.WaitForNotification());
ASSERT_FALSE(invalid_pepc->IsFocused());
valid_pepc->AccessibilityPerformAction(action_data);
```
I.e. make sure no notifications are returned after one spin of the run loop, and then verify that the element isn't focused. I think your point is that a single spin of the run loop might not be enough to catch the invalid element being focused, but at least you check one spin.
Thanks! LGTM.
el = document.getElementById("id2");nit: sorry that I missed the reassignment of `el` in the old patchset. But a suggestion to make this more readable and avoid some boilerplate:
```
<permission tabindex="0" id="valid_permission_element" type="geolocation">
<span tabindex="0" id="focusable_span">This is some text</span>
<permission tabindex="0" id="invalid_permission_element" type="invalid">
```
and then in code, you don't even need the `getElementById()` calls at all, since that's provided by the browser. You can just use the ids directly in code:
```
focusable_span.focus();
assert_equals(document.activeElement, focusable_span,
"Other element should be focused");
```
assert_not_equals(document.activeElement, el,So I'm guessing the behavior is that the `<body>` element is focused here, right? I think it'd be better to explicitly check the expectation, e.g. `assert_equals(document.activeElement, document.body);`.
assert_not_equals(document.activeElement, el,ditto
Related details
- Aaron Leventhal
- Andy Paicu
- 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 added 5 comments![]( "Open in Gerrit")
invalid_pepc->AccessibilityPerformAction(action_data);
valid_pepc->AccessibilityPerformAction(action_data);Andy PaicuEven if `invalid_pepc` was focusable, this test would pass, since you focus them both right after each other. I think you would need to add more `ASSERT`s between these two calls to make sure.
Mason FreedI tested this with `HtmlPermissionElement::SupportsFocus` hardcoded to `return true` and the test failed because the waiter immediately returned after the first focus event for the invalid element. So the test does not currently pass if both elements are actually focusable.
I agree that it's not the best but I don't really see how to improve it. There aren't any productive `ASSERT`s to put in between the 2 calls, because the focus event requires an async step in order to take effect. Before making the `WaitForNotification` call there won't be time for any element to become focused, so there's no point in checking the focus state. And there is no real way to wait for the focus event to *not* do anything on the `invalid_pepc` element, other than the somewhat brittle wait-for-a-fixed-amount-of-time approach.
I'm happy to add any checks you suggest but I think that any such checks will be useless before a `WaitForNotification` call and they will pass regardless of the focus behavior of the `permission` element.
I'm glad the test fails without the code change - thanks for verifying.
Wouldn't this do it then?
```
invalid_pepc->AccessibilityPerformAction(action_data);
ASSERT_FALSE(waiter.WaitForNotification());
ASSERT_FALSE(invalid_pepc->IsFocused());
valid_pepc->AccessibilityPerformAction(action_data);
```I.e. make sure no notifications are returned after one spin of the run loop, and then verify that the element isn't focused. I think your point is that a single spin of the run loop might not be enough to catch the invalid element being focused, but at least you check one spin.
`WaitForNotification` will block until a notification is received (or the test times out). So that doesn't work. I've used `WaitForNotificationWithTimeout` instead to limit the time it needs to wait but this will increase the run-time of this test significantly every time.
if (params.type == mojom::blink::FocusType::kScript) {
return;
}Done
nit: sorry that I missed the reassignment of `el` in the old patchset. But a suggestion to make this more readable and avoid some boilerplate:
```
<permission tabindex="0" id="valid_permission_element" type="geolocation">
<span tabindex="0" id="focusable_span">This is some text</span>
<permission tabindex="0" id="invalid_permission_element" type="invalid">
```and then in code, you don't even need the `getElementById()` calls at all, since that's provided by the browser. You can just use the ids directly in code:
```
focusable_span.focus();
assert_equals(document.activeElement, focusable_span,
"Other element should be focused");
```
Excellent suggestion, thank you.
So I'm guessing the behavior is that the `<body>` element is focused here, right? I think it'd be better to explicitly check the expectation, e.g. `assert_equals(document.activeElement, document.body);`.
Done
assert_not_equals(document.activeElement, el,Andy Paicuditto
Done
Related details
- Aaron Leventhal
- 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 added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
Aaron LeventhalThey do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
Aaron LeventhalAn accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
I imagine this feature will need to go through an a11y review -- put something you think is reasonable for the name/description for now.
Ah I see, the <permission> element has an internal span with text which is hard-set by the renderer [1]. So on focus the screen reader reads the text which is the action the button will trigger. Example text depending on permission status and type: "Use Camera" or "Share location". This is what is read out when the user focuses the permission element.
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
Aaron LeventhalThey do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
Aaron LeventhalAn accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
Andy PaicuI imagine this feature will need to go through an a11y review -- put something you think is reasonable for the name/description for now.
Ah I see, the <permission> element has an internal span with text which is hard-set by the renderer [1]. So on focus the screen reader reads the text which is the action the button will trigger. Example text depending on permission status and type: "Use Camera" or "Share location". This is what is read out when the user focuses the permission element.
Does that show up as the accessible name of the button? A dump tree test is easy to do. Feel free to ping me if you want any hints.
Related details
- Andy Paicu
- Thomas Nguyen
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
Aaron LeventhalThey do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
Aaron LeventhalAn accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
Andy PaicuI imagine this feature will need to go through an a11y review -- put something you think is reasonable for the name/description for now.
Aaron LeventhalAh I see, the <permission> element has an internal span with text which is hard-set by the renderer [1]. So on focus the screen reader reads the text which is the action the button will trigger. Example text depending on permission status and type: "Use Camera" or "Share location". This is what is read out when the user focuses the permission element.
Does that show up as the accessible name of the button? A dump tree test is easy to do. Feel free to ping me if you want any hints.
I've added the dump tree test, though it's not clear to me what should show up, and whether this answers the question or not. 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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Andy PaicuIs there a default accessible name for these?
It would be very unusual if we expected the author to use ARIA for it.
Aaron LeventhalThey do not have a default accessible name, can you give me an example of how this works for other elements (if possible, "button" since that is the closest to the permission element)? What should I implement/override and what is usually the convention when it comes to default accessible names?
Aaron LeventhalAn accessible name is a localized label, kind of like alt text if you had an image button. Imagine you cannot see the screen, but tab to the button. It would not be enough to hear "button". You would need to know what kind of button it is. An example might be "Camera permissions". You can also add a description field that provides more information, such as "Press to change whether this website has permission to use your camera.".
A <button> does not need this, because it gets the name from the contents inside, such as <button>Feed the cat</button>.
Andy PaicuI imagine this feature will need to go through an a11y review -- put something you think is reasonable for the name/description for now.
Aaron LeventhalAh I see, the <permission> element has an internal span with text which is hard-set by the renderer [1]. So on focus the screen reader reads the text which is the action the button will trigger. Example text depending on permission status and type: "Use Camera" or "Share location". This is what is read out when the user focuses the permission element.
Andy PaicuDoes that show up as the accessible name of the button? A dump tree test is easy to do. Feel free to ping me if you want any hints.
I've added the dump tree test, though it's not clear to me what should show up, and whether this answers the question or not. PTAL?
For the dump tree test, you need to add two more files:
1. HTMK source with a bunch of examples of permission elements in a minimal html file:
content/test/data/accessibility/html/permission.html
2. The expectations. At first, just create an empty text file:
content/test/data/accessibility/html/permission-expected-blink.txt
Locally, you will run the content_browsertest called All/DumpAccessibilityEventsTest.AccessibilityPermission/blink
and there will be a section with expected results -- paste that into the permission-expected-blink.txt.
Finally upload them to your change and I'll take a look
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
Oops, I somehow did not include the new files into the upload. PTAL now.
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
++++genericContainerThe button is missing from this output. That probably means it was marked ignored by AXNodeObject::ComputeAccessibilityIsIgnored().
It should have been included because if this line in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_node_object.cc;l=753
```
// All focusable elements except the <body> and <html> are included.
if (!IsA<HTMLBodyElement>(node) && CanSetFocusAttribute())
return kIncludeObject;
```
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
<permission type="camera">May as well put all the different permission types in this test.
Andy Paicu (Gerrit)
Andy Paicu added 3 comments![]( "Open in Gerrit")
<permission type="invalid" aria-label="invalid-pepc">Done
The button is missing from this output. That probably means it was marked ignored by AXNodeObject::ComputeAccessibilityIsIgnored().
It should have been included because if this line in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_node_object.cc;l=753
```
// All focusable elements except the <body> and <html> are included.
if (!IsA<HTMLBodyElement>(node) && CanSetFocusAttribute())
return kIncludeObject;
```
D'oh... I just forgot to enable the feature in this test.
May as well put all the different permission types in this test.
- Aaron Leventhal
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 2 comments![]( "Open in Gerrit")
++++genericContainerThis looks good. However, let's put this at the top so that we can ensure these are focusable. You'll have to update the expectations after.
<!--
@BLINK-ALLOW:focusable*
-->
Related details
- Andy Paicu
- 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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andy Paicu
- Mason Freed
- Thomas Nguyen
Code-Coverage
Code-Review
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
++++genericContainerThis looks good. However, let's put this at the top so that we can ensure these are focusable. You'll have to update the expectations after.
<!--
@BLINK-ALLOW:focusable*
-->
(Note: it goes at the top of the .html).
Andy Paicu (Gerrit)
Andy Paicu added 2 comments![]( "Open in Gerrit")
Thank you Aaron for helping me with the a11y part of it.
Mason, Thomas PTAL (the +1s got lost).
++++genericContainerThis looks good. However, let's put this at the top so that we can ensure these are focusable. You'll have to update the expectations after.
<!--
@BLINK-ALLOW:focusable*
-->
(Note: it goes at the top of the .html).
Done. Assuming I'm not reading it incorrectly the element is focusable as expected.
Related details
- Mason Freed
- Thomas Nguyen
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 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
invalid_pepc->AccessibilityPerformAction(action_data);
valid_pepc->AccessibilityPerformAction(action_data);Andy PaicuEven if `invalid_pepc` was focusable, this test would pass, since you focus them both right after each other. I think you would need to add more `ASSERT`s between these two calls to make sure.
Mason FreedI tested this with `HtmlPermissionElement::SupportsFocus` hardcoded to `return true` and the test failed because the waiter immediately returned after the first focus event for the invalid element. So the test does not currently pass if both elements are actually focusable.
I agree that it's not the best but I don't really see how to improve it. There aren't any productive `ASSERT`s to put in between the 2 calls, because the focus event requires an async step in order to take effect. Before making the `WaitForNotification` call there won't be time for any element to become focused, so there's no point in checking the focus state. And there is no real way to wait for the focus event to *not* do anything on the `invalid_pepc` element, other than the somewhat brittle wait-for-a-fixed-amount-of-time approach.
I'm happy to add any checks you suggest but I think that any such checks will be useless before a `WaitForNotification` call and they will pass regardless of the focus behavior of the `permission` element.
Andy PaicuI'm glad the test fails without the code change - thanks for verifying.
Wouldn't this do it then?
```
invalid_pepc->AccessibilityPerformAction(action_data);
ASSERT_FALSE(waiter.WaitForNotification());
ASSERT_FALSE(invalid_pepc->IsFocused());
valid_pepc->AccessibilityPerformAction(action_data);
```I.e. make sure no notifications are returned after one spin of the run loop, and then verify that the element isn't focused. I think your point is that a single spin of the run loop might not be enough to catch the invalid element being focused, but at least you check one spin.
`WaitForNotification` will block until a notification is received (or the test times out). So that doesn't work. I've used `WaitForNotificationWithTimeout` instead to limit the time it needs to wait but this will increase the run-time of this test significantly every time.
Thanks for adding this with the timeout.
assert_not_equals(document.activeElement, invalid_permission_element,Can't this one also be `assert_equals(..,body)`?
Related details
- Andy Paicu
- 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 voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andy Paicu
Andy Paicu (Gerrit)
Andy Paicu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andy Paicu
| Commit-Queue | +2 |
2 comments
assert_not_equals(document.activeElement, invalid_permission_element,Can't this one also be `assert_equals(..,body)`?
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Paicu (Gerrit)
Andy Paicu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/html/semantics/permission-element/no-focus.tentative.html
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
Insertions: 139, Deletions: 15.
The diff is too large to show. Please review the diff.
```
Change information
[PEPC] Disable focusing via script
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.
- M chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
- M chrome/test/data/permissions/permission_element.html
- M content/browser/accessibility/accessibility_action_browsertest.cc
- M content/browser/accessibility/dump_accessibility_tree_browsertest.cc
- M content/test/content_test_bundle_data.filelist
- A content/test/data/accessibility/html/permission-expected-blink.txt
- A content/test/data/accessibility/html/permission.html
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/modules/accessibility/ax_node_object.cc
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/no-focus.tentative.html
Code-Review: +1 by Aaron Leventhal, +1 by Mason Freed, +1 by Thomas Nguyen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45766
Related details
Code-Coverage
Code-Review
| 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")
++++genericContainerAaron LeventhalThis looks good. However, let's put this at the top so that we can ensure these are focusable. You'll have to update the expectations after.
<!--
@BLINK-ALLOW:focusable*
-->
Andy Paicu(Note: it goes at the top of the .html).
Done. Assuming I'm not reading it incorrectly the element is focusable as expected.
Looks great!
I'll be interested to see what comes up with the button is activated. Were you asked to go through any kind of a11y review process? Maybe I reviewed an early version of the plans, it sounds familiar.
As I understand it, this will be a very good thing for low vision users who are zoomed in, because the UI for activating the permission won't be far away from the page feature/activity that needed it. But we should do a great job on the UI for all the disability types. LMK if we need to hook you up with our a11y analyst team for a review whenever you're ready.