Add argument to IsFocusable() for a11y use only [chromium/src : main]
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
Mason Freed would like Chris Harrelson and Aaron Leventhal to review this change.
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059
Change-Id: I7f8b73a74341e6dad04102c327b1b8ce99a2ef1d
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/forms/clear_button_element.h
M third_party/blink/renderer/core/html/forms/spin_button_element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/html/html_anchor_element.h
M third_party/blink/renderer/core/html/html_area_element.cc
M third_party/blink/renderer/core/html/html_area_element.h
M third_party/blink/renderer/core/html/html_dialog_element.h
M third_party/blink/renderer/core/html/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/modules/accessibility/ax_object.cc
12 files changed, 52 insertions(+), 40 deletions(-)
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
1 comment:
Patchset:
Not sure what this will fail, test-wise, but LMK what you think.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Chris Harrelson, Mason Freed.
1 comment:
Patchset:
If IsFocusable() only ever needs updated style, and not updated layout, then I think we should only need one method, and it should only update style if necessary.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Chris Harrelson, Mason Freed.
Mason Freed uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Mason Freed
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059
Change-Id: I7f8b73a74341e6dad04102c327b1b8ce99a2ef1d
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/forms/clear_button_element.h
M third_party/blink/renderer/core/html/forms/spin_button_element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/html/html_anchor_element.h
M third_party/blink/renderer/core/html/html_area_element.cc
M third_party/blink/renderer/core/html/html_area_element.h
M third_party/blink/renderer/core/html/html_dialog_element.h
M third_party/blink/renderer/core/html/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/modules/accessibility/ax_object.cc
12 files changed, 63 insertions(+), 40 deletions(-)
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Chris Harrelson, Mason Freed.
1 comment:
Patchset:
If IsFocusable() only ever needs updated style, and not updated layout, then I think we should only […]
Maybe we need to update IsFocusableStyleAfterUpdate()
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;drc=3bbea6fd3874c6ebee3c803064b4fb8ec8b6109a;l=5992```
-snip-
// Also note that if this node is ignored due to a display lock for focus
// activation reason, we simply return false to avoid updating style & layout
// tree for this node.
if (DisplayLockUtilities::ShouldIgnoreNodeDueToDisplayLock(
*this, DisplayLockActivationReason::kUserFocus)) {
return false;
}
GetDocument().UpdateStyleAndLayoutTreeForNode(this,
DocumentUpdateReason::kFocus);
return IsFocusableStyle();
}
```Change UpdateStyleAndLayoutTreeForNode() to something that only updates the style.
Also, I don't think we want to return early for display locking, otherwise why bother updating style when a11y if we're just going to do that.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Mason Freed.
1 comment:
Patchset:
The check was not for clean layout, but having built the layout tree. The latter happens during the style recalc lifecycle phase.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Mason Freed.
Mason Freed uploaded patch set #3 to this change.
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059
Change-Id: I7f8b73a74341e6dad04102c327b1b8ce99a2ef1d
---
M content/test/data/accessibility/html/dialog-expected-fuchsia.txt
M content/test/data/accessibility/html/dialog-expected-uia-win.txt
M content/test/data/accessibility/html/dialog-expected-win.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-android.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-fuchsia.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-uia-win.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-win.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-android.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-fuchsia.txt
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/forms/clear_button_element.h
M third_party/blink/renderer/core/html/forms/spin_button_element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/html/html_anchor_element.h
M third_party/blink/renderer/core/html/html_area_element.cc
M third_party/blink/renderer/core/html/html_area_element.h
M third_party/blink/renderer/core/html/html_dialog_element.h
M third_party/blink/renderer/core/html/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-ignoredNodesModal-expected.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
23 files changed, 89 insertions(+), 52 deletions(-)
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal.
1 comment:
Patchset:
If IsFocusable() only ever needs updated style, and not updated layout, then I think we should only need one method, and it should only update style if necessary.
IsFocusable uses *layout* information, in the form of the existence of the layout tree and layout objects:
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal.
2 comments:
File third_party/blink/renderer/core/html/html_dialog_element.h:
bool IsFocusable() const override {
return isConnected() && IsFocusableStyleAfterUpdate();
}I think the dialog changes seem correct, and were an oversight in the prior implementation. This implementation (left side) of `IsFocusable()` implicitly says that `SupportsFocus()` is true always, since it never checks it. The right side does this correctly, in my opinion.
This is why the new expectation files show open dialogs as *both* focusable *and* focused. Previously they were just focused, but *not* focusable, which obviously shouldn't happen.
File third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-ignoredNodesModal-expected.txt:
Patch Set #3, Line 60:
focusedNote this dialog is `focused` but *not* `focusable`. That shouldn't be possible.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal.
Patch set 3:Auto-Submit +1
1 comment:
Patchset:
Alright, hard to be sure, but I think this patch should make it so that neither `AXObject::IsKeyboardFocusable()` nor `AXObject::ComputeCanSetFocusAttribute` should be able to try to run a layout/style lifecycle. At worst, if layout information is not up to date, stale layout information will be used to determine focusability.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal.
Mason Freed uploaded patch set #4 to this change.
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059, 1489580, 1492703
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
Mason Freed uploaded patch set #5 to this change.
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059, 1489580, 1492703
Change-Id: I7f8b73a74341e6dad04102c327b1b8ce99a2ef1d
---
M content/test/data/accessibility/html/dialog-expected-android-external.txt
M content/test/data/accessibility/html/dialog-expected-android.txt
M content/test/data/accessibility/html/dialog-expected-fuchsia.txt
M content/test/data/accessibility/html/dialog-expected-uia-win.txt
M content/test/data/accessibility/html/dialog-expected-win.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-android.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-fuchsia.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-uia-win.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-win.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-android.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-fuchsia.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-uia-win.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-win.txt
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/html/forms/clear_button_element.h
M third_party/blink/renderer/core/html/forms/spin_button_element.h
M third_party/blink/renderer/core/html/html_anchor_element.cc
M third_party/blink/renderer/core/html/html_anchor_element.h
M third_party/blink/renderer/core/html/html_area_element.cc
M third_party/blink/renderer/core/html/html_area_element.h
M third_party/blink/renderer/core/html/html_dialog_element.h
M third_party/blink/renderer/core/html/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-ignoredNodesModal-expected.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
27 files changed, 93 insertions(+), 56 deletions(-)
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal.
Patch set 5:Auto-Submit +1
1 comment:
Patchset:
Ok, this should pass all tests. Ready for review.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Mason Freed.
1 comment:
Patchset:
> If IsFocusable() only ever needs updated style, and not updated layout, then I think we should onl […]
Ok. Here's what Chris Harrelson told me. Updating style will lead to the creation of a layout object that contains the correct style. The rest of the layout object may not be up to date, but you can use all of the code you just linked to even if you only update style, and don't update layout. AIUI.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
1 comment:
Patchset:
Ok. Here's what Chris Harrelson told me. […]
There are three things: style, layout tree building, and performing layout. In general layout tree building always happens along with style. Rune confirmed to me today that it's a bug if we ever recalc style for a node but don't generate the layout objects for it.
However, IsFocusable does require layout, because IsFocusable calls SupportsFocus() which calls IsScrollableContainerThatShouldBeKeyboardFocusable() which calls IsScrollableNode() which calls UpdateStyleAndLayout(). The reason UpdateStyleAndLayout() is called is because the method needs to call IsUserScrollable(), which depends not just on whether the element has overflow:scroll style, but also whether it actually has any scrollable overflow.
Also, IsScrollableNode unconditionally calls UpdateStyleAndLayout which is a problem because we should not be doing that during the lifecycle or in post-lifecycle steps.
Therefore the KeyboardFocusableScrollers feature causes a dependency on layout and also this code cannot safely be called directly from the a11y update. Possible fixes:
- Always compute layout when a11y is on, so that IsScrollableContainerThatShouldBeKeyboardFocusable can be computed without doing layout at the wrong time. We would also need to make a version of IsScrollableContainerThatShouldBeKeyboardFocusable that does not call UpdateStyleAndLayout.
- Don't depend on layout in IsScrollableContainerThatShouldBeKeyboardFocusable, by just looking for overflow:scroll and not whether there is any scrollable overflow.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Chris Harrelson, Mason Freed.
1 comment:
Patchset:
Always compute layout when a11y is on
Seems expensive. A11y is on a lot of the time.
Don't depend on layout in IsScrollableContainerThatShouldBeKeyboardFocusable, by just looking for overflow:scroll and not whether there is any scrollable overflow.
+1 -- I like this.
Another option would be to just return false for the scrollable overflow case if the object doesn't have layout yet. I do not believe this will affect the user experience in any noticeable way.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
1 comment:
Patchset:
+1 -- I like this.
I'm working on a patch for this now.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
1 comment:
Patchset:
Also, IsScrollableNode unconditionally calls UpdateStyleAndLayout which is a problem because we should not be doing that during the lifecycle or in post-lifecycle steps.
It doesn't call it *unconditionally*, does it?
```
bool IsScrollableNode(const Node* node) {
...
if (box->NeedsLayout()) {
node->GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kFocus);
}
```
>> Don't depend on layout in IsScrollableContainerThatShouldBeKeyboardFocusable, by just looking for overflow:scroll and not whether there is any scrollable overflow.
+1 -- I like this.
I don't think we should do this - it's bad for users. The scroller should get focused only if it actually has content that needs to be scrolled. E.g. if a page has `div{overflow:scroll}`, then every <div> gets focused, even if none of them have overflowing content.
Another option would be to just return false for the scrollable overflow case if the object doesn't have layout yet. I do not believe this will affect the user experience in any noticeable way.
I like this approach the best. It's basically what this patch will do, effectively. If the layout is stale (or nonexistent) then return false. There are likely corner cases that aren't 100% "correct" but this will be so much less noticeable to users than any of the options above. #1 slows everyone down, and #2 adds tab stops for too many boxes.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
1 comment:
Patchset:
> Also, IsScrollableNode unconditionally calls UpdateStyleAndLayout which is a problem because we sh […]
You make a good point that there would be too many tab stops for the second option. I hadn't considered that. Will stop working on that CL.
As for returning false if it doesn't have layout yet: maybe, but this CL doesn't do it because it still calls IsFocusable. I am thinking about how to do this better now.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
I like this approach the best. It's basically what this patch will do, effectively. If the layout is stale (or nonexistent) then return false.
There are likely corner cases that aren't 100% "correct" but this will be so much less noticeable to users than any of the options above. #1 slows everyone down, and #2 adds tab stops for too many boxes.
Based on this discussion, I agree with this. However, I would like to make sure we add a comment in the a11y code that explains what we are doing, and why, and the fact that it would only affect far-away scrollables, which we think is ok.
Patchset:
+1 with the addition of a
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #5, Line 6044:
if (UNLIKELY(disallow_layout_updates_for_accessibility_only)) {Will this cause all display locked nodes to be marked as not focusable?
Could we instead only change the results unlayedout scrollables? Everything else should have enough to use the layout object, since they only need computed style at most.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
2 comments:
Patchset:
As for returning false if it doesn't have layout yet: maybe, but this CL doesn't do it because it still calls IsFocusable. I am thinking about how to do this better now.
Are you saying it still updates layout via the SupportsFocus() call? I've been assuming that this chunk:
```
if (DisplayLockUtilities::ShouldIgnoreNodeDueToDisplayLock(
*this, DisplayLockActivationReason::kUserFocus)) {
return false;
}
```
does exactly that, and avoids us calling SupportsFocus().
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #5, Line 6044:
if (UNLIKELY(disallow_layout_updates_for_accessibility_only)) {Will this cause all display locked nodes to be marked as not focusable?
No, this won't do that. It'll just cause the focusability information for display locked trees to use possibly-stale layout information.
Note that this change does not break any of the display locking / a11y test cases.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
Mason Freed uploaded patch set #6 to this change.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Patchset:
> As for returning false if it doesn't have layout yet: maybe, but this CL doesn't do it because it […]
1. ShouldIgnoreNodeDueToDisplayLock(kUserFocus) only skips content-visibility:hidden subtrees, not content-visibility:auto.
2. I'm talking about SupportsFocus for contentent-visibility:auto subtrees.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
Patch set 6:Auto-Submit +1
1 comment:
Patchset:
1. […]
I will admit I still haven't wrapped my head around the difference between `ShouldIgnoreNodeDueToDisplayLock` and `NeedsLayoutTreeUpdateForNodeIncludingDisplayLocked` (which is what `UpdateStyleAndLayoutTreeForNode` checks). But it sounds like you think neither of those is the right check to avoid `content-visibility:auto`? If not, what's the right check?
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
1 comment:
Patchset:
I will admit I still haven't wrapped my head around the difference between `ShouldIgnoreNodeDueToDis […]
You should use DisplayLockUtilities::IsDisplayLockedPreventingPaint for that.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
1 comment:
Patchset:
You should use DisplayLockUtilities::IsDisplayLockedPreventingPaint for that.
Interesting. The `PreventingPaint` part of that threw me, since it sounds like it's preventing `Layout`.
Should we use that here in `IsFocusableStyleNeverLayoutForAccessibilityOnly`, then?
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
Mason Freed uploaded patch set #7 to this change.
27 files changed, 89 insertions(+), 56 deletions(-)
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Chris Harrelson.
Patch set 7:Auto-Submit +1
1 comment:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #7, Line 6029:
DCHECK(!NeedsStyleRecalc()) << thisPer offline conversation - I removed the `if()` block and downgraded to a DCHECK here.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
Patch set 7:Code-Review +1
1 comment:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #5, Line 6044:
if (UNLIKELY(disallow_layout_updates_for_accessibility_only)) {> Will this cause all display locked nodes to be marked as not focusable? […]
Done
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.
Chris Harrelson (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
Patch set 7:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Add argument to IsFocusable() for a11y use only
This CL adds this:
virtual bool IsFocusable(
bool disallow_layout_updates_for_accessibility_only = false);
for use by a11y code, which sometimes hits the CHECK for clean
layout. When this bool is true (default false), layout will not
be updated to check focusability, and possibly-stale layout objects
will be used.
Bug: 1485059, 1489580, 1492703
Change-Id: I7f8b73a74341e6dad04102c327b1b8ce99a2ef1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4959753
Auto-Submit: Mason Freed <mas...@chromium.org>
Commit-Queue: Mason Freed <mas...@chromium.org>
Reviewed-by: Chris Harrelson <chri...@chromium.org>
Commit-Queue: Chris Harrelson <chri...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1213856}
Mason Freed (Gerrit)
Mason Freed has created a revert of this change.
To view, visit change 4959753. To unsubscribe, or for help writing mail filters, visit settings.