Enable KeyboardFocusableScrollersEnabled code in ax_object.cc [chromium/src : main]
Di Zhang (Gerrit)
Attention is currently required from: Aaron Leventhal.
Di Zhang would like Aaron Leventhal to review this change.
Enable KeyboardFocusableScrollersEnabled code in ax_object.cc
As we prepare to ship KeyboardFocusableScrollers, we need to address all
TODOs related to this feature for accessibility concerns.
Change-Id: I6d9ff6e3bc8f9493c7d95aede3c2af033d04f3c1
Fixed: 1444450
---
M third_party/blink/renderer/modules/accessibility/ax_object.cc
1 file changed, 8 insertions(+), 20 deletions(-)
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Aaron Leventhal.
1 comment:
Patchset:
PTAL. I am not very familiar with A11Y trees. Do I just need to rebaseline the failing tests or are these unexpected changes?
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Di Zhang.
3 comments:
Patchset:
A problem I see is that when a focusable descendant is added or removed, we don't invalidate the focusable state on keyboard focusable scroll containers.
Another question: I think they are always focusable with the mouse, regardless of whether the descendant is focusable, right? The only dependency on descendants is whether they are keyboard focusable or not?
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #1, Line 3875:
IsUserScrollable() && elem->HasNoFocusableChildren()) {Will the above elem->SupportsFocus() not already return true for this case?
Patch Set #1, Line 3892:
if (RuntimeEnabledFeatures::KeyboardFocusableScrollersEnabled() &&elem->HasNoFocusableChildren() seems expensive. Do we not cache the value of it for purposes of tabbing?
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
Di Zhang would like Mason Freed to review this change.
Enable KeyboardFocusableScrollersEnabled code in ax_object.cc
As we prepare to ship KeyboardFocusableScrollers, we need to address all
TODOs related to this feature for accessibility concerns.
Change-Id: I6d9ff6e3bc8f9493c7d95aede3c2af033d04f3c1
Fixed: 1444450
---
M third_party/blink/renderer/modules/accessibility/ax_object.cc
1 file changed, 8 insertions(+), 20 deletions(-)
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
2 comments:
Patchset:
A problem I see is that when a focusable descendant is added or removed, we don't invalidate the focusable state on keyboard focusable scroll containers.
Do you think we should invalidate the focusable state? We actually have a case where the scroller first has a disabled button so it is focusable. But mid-scroll, the button becomes enabled/focusable through an event listener. In this case, our understanding is that the focus should remain on the scroller. Since we don't want to break user experience by suddenly changing focus.
https://codepen.io/TPG/full/RwegqJJ/fefdd4f18fb0e79480943effed37e733
Another question: I think they are always focusable with the mouse, regardless of whether the descendant is focusable, right? The only dependency on descendants is whether they are keyboard focusable or not?
Yes, that is correct.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #1, Line 3875:
IsUserScrollable() && elem->HasNoFocusableChildren()) {Will the above elem->SupportsFocus() not already return true for this case?
No, I believe not. The SupportsFocus() check that the element can be focused. Element::IsKeyboardFocusable() also looks for this condition.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Di Zhang, Mason Freed.
2 comments:
Patchset:
> A problem I see is that when a focusable descendant is added or removed, we don't invalidate the f […]
I just mean we need to invalidate the accessibility focusable state. This only affects what we communicate to ATs, and not not the focus state, just whether it can receive focus.
When it goes from being focusable to not focusable (which I think should be represented by SupportsFocus()), then we should call AXObjectCache::MarkElementDirty(node). That will take care of the invalidation of the a11y focusable state.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #1, Line 3875:
IsUserScrollable() && elem->HasNoFocusableChildren()) {
No, I believe not. The SupportsFocus() check that the element can be focused. […]
I think SupportsFocus() should return true for this case. IsKeyboardFocusable() should be a subset of things that SupportFocus().
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang, Mason Freed.
Di Zhang uploaded patch set #2 to this change.
Enable KeyboardFocusableScrollersEnabled code in ax_object.cc
As we prepare to ship KeyboardFocusableScrollers, we need to address all
TODOs related to this feature for accessibility concerns.
Change-Id: I6d9ff6e3bc8f9493c7d95aede3c2af033d04f3c1
Bug: 1444450
---
M third_party/blink/renderer/modules/accessibility/ax_object.cc
1 file changed, 8 insertions(+), 20 deletions(-)
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Aaron Leventhal.
Patch set 3:Commit-Queue +1
2 comments:
Patchset:
I just mean we need to invalidate the accessibility focusable state. […]
I have been thinking and I am unsure I can make the change inside Element::SupportsFocus() as this function is called by both IsKeyboardFocusable and IsMouseFocusable. I believe a logic specific to keyboard focusing shouldn't affect mouse focusable elements.
As a sanity check, what do you think should be the values after a scroller becomes non-focusable (by adding a keyboard focusable child):
- SupportsFocus() should return ?
- IsMouseFocusable() should return ?
- IsKeyboardFocusable() should return false.
And the reverse logic be true as well (when removing the keyboard focusable child)?
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #1, Line 3875:
IsUserScrollable() && elem->HasNoFocusableChildren()) {
I think SupportsFocus() should return true for this case. […]
I understand your question now and you are right, the above check will return true already so this if condition isn't really used. I have removed it.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Di Zhang.
1 comment:
Patchset:
I have been thinking and I am unsure I can make the change inside Element::SupportsFocus() as this f […]
Here's what I'm thinking -- let's prove or disprove these:
- The scroller can become keyboard focusable or become not keyboard focusable, but it's always mouse focusable.
- If something is focusable it's always mouse focusable too, and vice-versa. I don't think there should be any difference. I think IsMouseFocusable() should just be { return SupportFocus(); } -- so I'd like to understand why that isn't the case now.
- Keyboard focusable should be a superset of the other 2. If something returns true for IsKeyboardFocusable(), then it should also return return true for SupportsFocus() and IsMouseFocusable().
- The way the current code is, where there are 3 different boolean statements related to focusable, seems incorrect according to my experience in accessibility. There are only really 2 boolean statements. 1) Is something focusable at all, and 2) If #1 is true, is it also keyboard focusable aka tabbable.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Di Zhang.
1 comment:
Patchset:
Here's what I'm thinking -- let's prove or disprove these: […]
So I've put up a fresh CL with these comments addressed. See https://chromium-review.googlesource.com/c/chromium/src/+/4795287. We can/should abandon this CL, and I'll follow up with comments in the new one.
Some quick replies to your comments, though:
>> The scroller can become keyboard focusable or become not keyboard
>> focusable, but it's always mouse focusable.
==> implemented.
>> If something is focusable it's always mouse focusable too, and vice-versa.
>> I don't think there should be any difference. I think IsMouseFocusable()
>> should just be { return SupportFocus(); } -- so I'd like to understand why
>> that isn't the case now.
==> not true. See explanation below.
>> Keyboard focusable should be a superset of the other 2. If something
>> returns true for IsKeyboardFocusable(), then it should also return
>> true for SupportsFocus() and IsMouseFocusable().
==> agreed, and this is true by construction in the new CL.
>> The way the current code is, where there are 3 different boolean statements
>> related to focusable, seems incorrect according to my experience in
>> accessibility. There are only really 2 boolean statements. 1) Is something
>> focusable at all, and 2) If #1 is true, is it also keyboard focusable aka
>> tabbable.
==> No, there really are three states here:
1. SupportsFocus: this element *can* be made focusable. A `<div>` is not
focusable and cannot support focus. A *disconnected* `<div tabindex=0>`
supports focus but isn't focusable. A *connected* `<div tabindex=0>`
supports focus and is focusable.
2. IsMouseFocusable: this element SupportsFocus() *and* is currently
focusable.
3. IsKeyboardFocusable: this element IsMouseFocusable() *and* is currently
keyboard focusable.I've added these as comments next to the functions in the .h file.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Di Zhang, Mason Freed.
1 comment:
Patchset:
So I've put up a fresh CL with these comments addressed. See https://chromium-review.googlesource. […]
I'm confused about what SupportsFocus() does and where it's useful. Does it need to be a public method?
The only ones I think a11y would ever care about are IsMouseFocusable() and IsKeyboardFocusable(). Also, do we think it might be confusing naming? E.g. might some devs that are not as familiar with focus be confused that IsKeyboardFocusable() implies IsMouseFocusable()?
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Di Zhang.
2 comments:
Patchset:
I'm confused about what SupportsFocus() does and where it's useful. […]
Let's pick up the discussion on:
https://chromium-review.googlesource.com/c/chromium/src/+/4795287
Patchset:
I think this CL should be Abandoned at this point.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Mason Freed abandoned this change.
To view, visit change 4518938. To unsubscribe, or for help writing mail filters, visit settings.