Groups

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable [chromium/src : main]

1 view

Skip to first unread message

Mason Freed (Gerrit)

unread,

Aug 18, 2023, 7:44:29 PM8/18/23

to Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang

Attention is currently required from: Aaron Leventhal.

Mason Freed would like Aaron Leventhal to review this change.

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable

This CL cleans up three functions in Element:

 - SupportsFocus: true if the element is *capable* of being focused.  An
   element supports focus if, e.g. it has a tabindex attribute, or it is
   editable, or other conditions. Note that the element might *support*
   focus while not *being focusable*, for example if the element is
   disconnected from the document.

 - IsMouseFocusable: true if the element SupportsFocus() and is
   currently focusable using the mouse.

 - IsKeyboardFocusable: true if the element IsMouseFocusable(), and is
   currently focusable using the keyboard.

This also adds IsScrollableContainerThatShouldBeKeyboardFocusable(),
which returns true for scrollers that should be made keyboard focusable if the feature flag for that behavior is enabled.

This also adds a DCHECK that SupportsFocus() does not update the
rendering lifecycle.

This patch borrows heavily from this one:
https://chromium-review.googlesource.com/c/chromium/src/+/4518938

Bug: 1444450
Change-Id: Ieeb5f0bde2cf2130c19cd15df1bf4f9c0aa21e19
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
4 files changed, 75 insertions(+), 68 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 18, 2023, 7:44:34 PM8/18/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal.

2 comments:

File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #5, Line 5462:
```
It might be
// a good idea to cache this bit on the element to avoid having to
// recompute it. That would require marking that bit dirty whenever
// a node in the subtree was mutated, or when styles for the subtree
// were recomputed.
```
  I'm not sure whether this will be necessary, but it might. Checking for focusability shouldn't happen in tight loops, but there are some cases where it happens I think. For example when the focus is currently on a scroller, each mouse move triggers a check. Perhaps let's wait and see what regressions show up?
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
- Patch Set #5, Line 4135:
```
 We can't use `Element::IsKeyboardFocusable()` since the downstream
// `Element::IsFocusableStyle()` call will reset the document lifecycle.
```
  It's generally bad that we have a copy/paste version of IsKeyboardFocusable in ax_object. It's a bug waiting to happen. Not sure what to do about that, given the limitation that we can't update the lifecycle, so just calling it out.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 21, 2023, 10:40:47 AM8/21/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Mason Freed.

7 comments:

File third_party/blink/renderer/core/dom/element.h:
- Patch Set #5, Line 793: virtual bool SupportsFocus() const;
  I mentioned in the other CL, but this is still vague to me. I wonder if it can be a private method. It sounds like an implementation detail of the higher-level public methods, and I'm not sure why it would be useful outside of that.
- Patch Set #5, Line 798: virtual bool IsMouseFocusable() const;
  Is it worth keeping both names for this function?
- Patch Set #5, Line 801: // currently focusable using the keyboard. This method can be called when
  Optional: another way to say this is that IsKeyboardFocusable() is the subset if mouse focusable elements that are in the tab cycle. (I often call this tabbable elements, but I'm ok with IsKeyboardFocusable).
- Patch Set #5, Line 803: virtual bool IsKeyboardFocusable() const;
  If we make sure that it doesn't do a lifecycle update when layout is already clean, then a11y can use it.
- Patch Set #5, Line 805: // An element is focusable iff it is mouse focusable.
  Nit: iff
File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #5, Line 5503: auto starting_lifecycle = GetDocument().Lifecycle().GetState();
  I believe you can use
  `DocumentLifecycle::DisallowTransitionScope scoped(GetDocument().Lifecycle());`
File third_party/blink/renderer/modules/accessibility/ax_object.cc:

Patch Set #5, Line 4135:

 We can't use `Element::IsKeyboardFocusable()` since the downstream
// `Element::IsFocusableStyle()` call will reset the document lifecycle.

- It's generally bad that we have a copy/paste version of IsKeyboardFocusable in ax_object. […]
  This code is only called when layout is clean, because it's only called from UpdateCachedAttributeValuesIfNeeded() which has this DCHECK:
  https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=3124
  Why should Element::IsFocusableStyle() be changing the life cycle if layout is already clean?
  Maybe that's no longer true?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Di Zhang (Gerrit)

unread,

Aug 21, 2023, 1:56:39 PM8/21/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Mason Freed.

2 comments:

Patchset:
- Patch Set #5:
  Thanks for taking a look at this refactoring! The described logic make sense. We also need to be careful of side effects like changing SVGAElement::isKeyboardFocusable.
File third_party/blink/renderer/core/dom/element.h:
- Patch Set #5, Line 803: virtual bool IsKeyboardFocusable() const;
  If we make sure that it doesn't do a lifecycle update when layout is already clean, then a11y can us […]
  I think the lifecycle and layout tree is getting modified by IsFocusableStyleAfterUpdate, which gets called by IsMouseFocusable (and IsKeyboardFocusable that calls it) (?)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 21, 2023, 2:47:35 PM8/21/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

1 comment:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 803: virtual bool IsKeyboardFocusable() const;

- I think the lifecycle and layout tree is getting modified by IsFocusableStyleAfterUpdate, which gets […]
  The a11y code only calls in when layout is already clean.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 21, 2023, 10:12:25 PM8/21/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang.

8 comments:

File third_party/blink/renderer/core/dom/element.h:
- Patch Set #5, Line 793: virtual bool SupportsFocus() const;
  I mentioned in the other CL, but this is still vague to me. I wonder if it can be a private method. […]
  After looking through the call sites, and particularly after trying to get tests to pass with this small refactor, I agree with you. This is a mess.
  But would you mind if I take a look at that part of the refactor after this CL lands? I've already got another queued up after this one to remove `IsBaseElementFocusable` which is also not needed. And then I can focus on making SupportsFocus private.

- Patch Set #5, Line 798: virtual bool IsMouseFocusable() const;
  Is it worth keeping both names for this function?

I did toy with removing one of the two, but I kind of feel like it confuses things for folks who aren't as familiar with focus. The three "situations" they might encounter:
 1. Is it focusable with the mouse?
 2. Is it focusable with the keyboard?
 3. Is it focusable at all?
 
Yes, 1 and 3 are the same, but let's help people by offering them 3 methods that line up with their expectations. For example, I found a number of places where people do `IsMouseFocusable() || IsKeyboardFocusable()` which leads me to believe they don't understand.

Sound ok?

- Patch Set #5, Line 801: // currently focusable using the keyboard. This method can be called when

- Optional: another way to say this is that IsKeyboardFocusable() is the subset if mouse focusable ele […]
  Done

- Patch Set #5, Line 803: virtual bool IsKeyboardFocusable() const;

- The a11y code only calls in when layout is already clean.
  I added more to the comments about this, but it shouldn't update the lifecycle if it's already clean. There's no great way to DCHECK that fact, unfortunately.

- Patch Set #5, Line 805: // An element is focusable iff it is mouse focusable.
  Nit: iff

- I meant "iff". As in "if and only if":
  https://en.wikipedia.org/wiki/If_and_only_if
File third_party/blink/renderer/core/dom/element.cc:
- Patch Set #5, Line 5462:

It might be
// a good idea to cache this bit on the element to avoid having to
// recompute it. That would require marking that bit dirty whenever
// a node in the subtree was mutated, or when styles for the subtree
// were recomputed.

- I'm not sure whether this will be necessary, but it might. […]
  Done

- Patch Set #5, Line 5503: auto starting_lifecycle = GetDocument().Lifecycle().GetState();

- I believe you can use […]
  Yep, better, thanks.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:

Patch Set #5, Line 4135:

 We can't use `Element::IsKeyboardFocusable()` since the downstream
// `Element::IsFocusableStyle()` call will reset the document lifecycle.

- This code is only called when layout is clean, because it's only called from UpdateCachedAttributeVa […]
  I believe that shouldn't be true. If we're at layout-clean, I don't know what/why it would go backwards.
  So you're saying we might be able to replace all of this code with a call to `element->IsKeyboardFocusable()`?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 22, 2023, 10:13:32 AM8/22/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

Patch set 6:Code-Review +1

4 comments:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- After looking through the call sites, and particularly after trying to get tests to pass with this s […]
  Sounds reasonable to me.

- Patch Set #5, Line 798: virtual bool IsMouseFocusable() const;

- I did toy with removing one of the two, but I kind of feel like it confuses things for folks who are […]
  Ok

- Patch Set #5, Line 805: // An element is focusable iff it is mouse focusable.

- I meant "iff". As in "if and only if": […]
  Wow! Learned. Question is, will wife allow in scrabble.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:

Patch Set #5, Line 4135:

 We can't use `Element::IsKeyboardFocusable()` since the downstream
// `Element::IsFocusableStyle()` call will reset the document lifecycle.

- I believe that shouldn't be true. […]
  I guess it's worth giving that a try to see what breaks, in a follow-up CL.
  It probably wasn't always in layout clean when we first wrote this method. We've gotten better at that kind of thing. Now it's guaranteed.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 23, 2023, 12:16:54 AM8/23/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org

Attention is currently required from: Di Zhang, Mason Freed.

Mason Freed uploaded patch set #7 to this change.

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable

This CL cleans up three functions in Element:

 - SupportsFocus: true if the element is *capable* of being focused.  An
   element supports focus if, e.g. it has a tabindex attribute, or it is
   editable, or other conditions. Note that the element might *support*
   focus while not *being focusable*, for example if the element is
   disconnected from the document.

 - IsMouseFocusable: true if the element SupportsFocus() and is
   currently focusable using the mouse.

 - IsKeyboardFocusable: true if the element IsMouseFocusable(), and is

   currently focusable using the keyboard.

This also adds IsScrollableContainerThatShouldBeKeyboardFocusable(),
which returns true for scrollers that should be made keyboard focusable if the feature flag for that behavior is enabled.

This also adds a DCHECK that SupportsFocus() does not update the
rendering lifecycle.

This patch borrows heavily from this one:
https://chromium-review.googlesource.com/c/chromium/src/+/4518938

Bug: 1444450, 1474971


Change-Id: Ieeb5f0bde2cf2130c19cd15df1bf4f9c0aa21e19
---
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/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
9 files changed, 83 insertions(+), 81 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 23, 2023, 10:31:23 AM8/23/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed.

Mason Freed uploaded patch set #10 to this change.

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable

This CL cleans up three functions in Element:

 - SupportsFocus: true if the element is *capable* of being focused.  An
   element supports focus if, e.g. it has a tabindex attribute, or it is
   editable, or other conditions. Note that the element might *support*
   focus while not *being focusable*, for example if the element is
   disconnected from the document.

 - IsMouseFocusable: true if the element SupportsFocus() and is
   currently focusable using the mouse.

 - IsKeyboardFocusable: true if the element IsMouseFocusable(), and is
   currently focusable using the keyboard.

This also adds IsScrollableContainerThatShouldBeKeyboardFocusable(),
which returns true for scrollers that should be made keyboard focusable if the feature flag for that behavior is enabled.

This also adds a DCHECK that SupportsFocus() does not update the
rendering lifecycle.

Note crbug.com/1474971 and the new baselines for the
video-playback-speed-menu.html test. That focus ring was there due
to a confluence of weirdness in the media controls code, and that
will need to be tackled separately, if desired.



This patch borrows heavily from this one:
https://chromium-review.googlesource.com/c/chromium/src/+/4518938

Bug: 1444450, 1474971
Change-Id: Ieeb5f0bde2cf2130c19cd15df1bf4f9c0aa21e19
---

M content/test/data/accessibility/event/scroll-horizontal-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/event/scroll-vertical-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/html/overflow-actions-expected-android-external.txt
M content/test/data/accessibility/html/overflow-actions-expected-android.txt
M content/test/data/accessibility/html/scrollable-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-auralinux.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-blink.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/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h

M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
32 files changed, 128 insertions(+), 119 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 23, 2023, 10:34:29 AM8/23/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang.

Patch set 10:Commit-Queue +1

3 comments:

Patchset:
- Patch Set #10:
  Thanks for the review. It took a lot of wrangling of the media code to get this to pass the tests, but I think it's there now. I'll need a review of that from the media folks. But I *think* this patchset (10) should pass the bots. Maybe wait for green bots before giving it another +1.
File third_party/blink/renderer/core/dom/element.h:
- Patch Set #5, Line 805: // An element is focusable iff it is mouse focusable.
  Wow! Learned. Question is, will wife allow in scrabble.
  I definitely believe this should be scrabble-legal.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:

Patch Set #5, Line 4135:

 We can't use `Element::IsKeyboardFocusable()` since the downstream
// `Element::IsFocusableStyle()` call will reset the document lifecycle.

- I guess it's worth giving that a try to see what breaks, in a follow-up CL. […]
  Ok, will do.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 23, 2023, 6:33:05 PM8/23/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang.

1 comment:

Patchset:

aleventhal@, there is still one pesky test that I cannot get to pass with this CL:

    WebContentsAccessibilityTest#testNodeInfo_Actions_OverflowScroll
    
You can see a failure here: 
  https://ci.chromium.org/ui/p/chromium/builders/try/android-nougat-x86-rel/367181/overview
  
Any ideas? It can't for some reason, find the two contained <p> nodes any longer.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 24, 2023, 10:18:10 AM8/24/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Mason Freed

Attention is currently required from: Di Zhang, Mason Freed.

Mason Freed has uploaded this change for review.

M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates.html


M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png

33 files changed, 129 insertions(+), 124 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 24, 2023, 10:18:18 AM8/24/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

1 comment:

Patchset:
- Patch Set #15:
  aleventhal@, there is still one pesky test that I cannot get to pass with this CL: […]
  I'm not familiar with how waitForNodeMatching() works, but it looks like it's trying to find an accessibility node with that id.
  My idea: use a command like this to test your local source using a trybot:
  `autoninja -C out/android-debug content_shell_test_ak && out/android-debug/bin/run_content_shell_test_apk --gtest_filter="org.chromium.content.browser.accessibility.WebContentsAccessibilityTest#testNodeInfo_Actions_OverflowScroll"`
  You can then reduce the changes your CL makes a little bit at a time, and keep trying until you find what part of your change caused it. (Or the reverse, start with main and add little parts of your change one at a time, until it fails).
  @mschi...@google.com would maybe be able to help with Android debugging or test infrastucture questions.
  If this isn't helpful enough let me know and I'll spend more time on it.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 24, 2023, 10:24:13 AM8/24/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

1 comment:

Patchset:

I'm not familiar with how waitForNodeMatching() works, but it looks like it's trying to find an acce […]

Another idea: load the content from the test and check the whole Blink tree before and after your change. There must be something that we're exposing differently. Perhaps it's making Android decide that the paragraph objects aren't interesting enough to expose. The nice thing about this approach is you can find the fix without testing on Android at all.
```
<div id='div1' title='1234' style='overflow:scroll; width: 200px; height:50px'>
  <p id='p1'>Example Paragraph 1</p>
  <p id='p2'>Example Paragraph 2</p>
</div>
```

Here are some ways of dumping the tree:

Put `Root()->ShowAXTreeForThis();` somewhere, e.g. at the end of AXObjectCacheImpl::SerializeDirtyObjectsAndEvents()
Create a dump tree test with that content and put @BLINK-ALLOW:* in a comment at the top.
Load it up in chrome and look at chrome://accessibility. You can just use Canary for the baseline.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 24, 2023, 5:50:41 PM8/24/23

to Mark Schillaci, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

Mason Freed would like Mark Schillaci to review this change.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 24, 2023, 5:50:49 PM8/24/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

2 comments:

Patchset:
- Patch Set #15:
  I'm moving mschillaci@ to a reviewer for attention. I'm stuck without guidance on how to fix this a11y test.
- Patch Set #15:
  Another idea: load the content from the test and check the whole Blink tree before and after your ch […]
  mschillaci@ if you have any ideas, I'm all ears.
  aleventhal@ thanks for the suggestions. The easiest seemed to be looking at chrome://accessibility. Comparing Canary with this patch, the *only* differences are:
  - All elements: AXFocusableAncestor goes from NULL to a value, and AXSelectedTextMarkerRange goes from a value to NULL.
  - AXWebArea: AXFocused goes from 0 to 1.
  - div1: AXDescription goes from 'Example Paragraph 1 Example Paragraph 2' to '1234', and AXHelp goes from '1234' to NULL.
  - p1 and p2: no other changes.
  Here's the Canary tree:
```
AXWebArea AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:2] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXLoaded=1 AXLoadingProgress=1 AXParent='<RenderWidgetHostViewCocoa: 0x12c01bd7600>' AXPosition='NSPoint: {-1912, 93}' AXRoleDescription='HTML content' AXSelected=0 AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSize={w: 150, h: 870} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='9' LocalPosition={x: 0, y: 0}
++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:3, :5] AXDOMClassList=[] AXDOMIdentifier='div1' AXDescription='Example Paragraph 1 Example Paragraph 2' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=NULL AXFocused=1 AXHelp='1234' AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:1 AXPosition='NSPoint: {-1904, 905}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSize={w: 142, h: 50} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='12' LocalPosition={x: 8, y: 8}
++++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:4] AXDOMClassList=[] AXDOMIdentifier='p1' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=NULL AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:2 AXPosition='NSPoint: {-1904, 921}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSize={w: 142, h: 18} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='13' LocalPosition={x: 8, y: 24}
++++++AXStaticText AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=NULL AXFocused=0 AXHelp=NULL AXInsertionPointLineNumber=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXNumberOfCharacters=NULL AXParent=:3 AXPlaceholderValue=NULL AXPosition='NSPoint: {-1904, 921}' AXRoleDescription='text' AXSelected=0 AXSelectedText=NULL AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSelectedTextRange=NULL AXSize={w: 138, h: 18} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXValue='Example Paragraph 1' AXVisibleCharacterRange=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='15' LocalPosition={x: 8, y: 24}
++++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:6] AXDOMClassList=[] AXDOMIdentifier='p2' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=NULL AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:2 AXPosition='NSPoint: {-1904, 905}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSize={w: 142, h: 1} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='14' LocalPosition={x: 8, y: 57}
++++++AXStaticText AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=NULL AXFocused=0 AXHelp=NULL AXInsertionPointLineNumber=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXNumberOfCharacters=NULL AXParent=:5 AXPlaceholderValue=NULL AXPosition='NSPoint: {-1904, 905}' AXRoleDescription='text' AXSelected=0 AXSelectedText=NULL AXSelectedTextMarkerRange={anchor: {:6, 19, down}, focus: {:6, 19, down}} AXSelectedTextRange=NULL AXSize={w: 138, h: 1} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXValue='Example Paragraph 2' AXVisibleCharacterRange=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='16' LocalPosition={x: 8, y: 57}
```
  Here's the "with patch" tree:
```
AXWebArea AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:2] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocused=1 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXLoaded=1 AXLoadingProgress=1 AXParent='<RenderWidgetHostViewCocoa: 0x12f42fa00>' AXPosition='NSPoint: {-1557, 207}' AXRoleDescription='HTML content' AXSelected=0 AXSelectedTextMarkerRange=NULL AXSize={w: 1440, h: 733} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='1' LocalPosition={x: 0, y: 0}
++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:3, :5] AXDOMClassList=[] AXDOMIdentifier='div1' AXDescription='1234' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=:2 AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:1 AXPosition='NSPoint: {-1549, 882}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange=NULL AXSize={w: 200, h: 50} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='4' LocalPosition={x: 8, y: 8}
++++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:4] AXDOMClassList=[] AXDOMIdentifier='p1' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=:2 AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:2 AXPosition='NSPoint: {-1549, 898}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange=NULL AXSize={w: 185, h: 18} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='5' LocalPosition={x: 8, y: 24}
++++++AXStaticText AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=:2 AXFocused=0 AXHelp=NULL AXInsertionPointLineNumber=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXNumberOfCharacters=NULL AXParent=:3 AXPlaceholderValue=NULL AXPosition='NSPoint: {-1549, 898}' AXRoleDescription='text' AXSelected=0 AXSelectedText=NULL AXSelectedTextMarkerRange=NULL AXSelectedTextRange=NULL AXSize={w: 138, h: 18} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXValue='Example Paragraph 1' AXVisibleCharacterRange=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='7' LocalPosition={x: 8, y: 24}
++++AXGroup AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[:6] AXDOMClassList=[] AXDOMIdentifier='p2' AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=:2 AXFocused=0 AXHelp=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXParent=:2 AXPosition='NSPoint: {-1549, 882}' AXRoleDescription='group' AXSelected=0 AXSelectedTextMarkerRange=NULL AXSize={w: 185, h: 1} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='6' LocalPosition={x: 8, y: 57}
++++++AXStaticText AXSubrole=_const_NULL AXBlockQuoteLevel=0 AXChildren=[] AXDOMClassList=[] AXElementBusy=0 AXEnabled=1 AXEndTextMarker={:unknown, 19, down} AXFocusableAncestor=:2 AXFocused=0 AXHelp=NULL AXInsertionPointLineNumber=NULL AXLanguage='en-US' AXLinkedUIElements=[] AXNumberOfCharacters=NULL AXParent=:5 AXPlaceholderValue=NULL AXPosition='NSPoint: {-1549, 882}' AXRoleDescription='text' AXSelected=0 AXSelectedText=NULL AXSelectedTextMarkerRange=NULL AXSelectedTextRange=NULL AXSize={w: 138, h: 1} AXStartTextMarker={:1, 0, down} AXTopLevelUIElement=NULL AXValue='Example Paragraph 2' AXVisibleCharacterRange=NULL AXVisited=0 AXWindow=NULL ChromeAXNodeId='8' LocalPosition={x: 8, y: 57}
```

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 24, 2023, 6:42:56 PM8/24/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

File third_party/blink/renderer/core/dom/element.h:
- Patch Set #5, Line 793: virtual bool SupportsFocus() const;
  Sounds reasonable to me.
  In looking at this in a separate CL, it looks like this call right here:
  https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=4128;drc=4ebd2b7ffe0ff01e25de59c7d8b8bb2ee362caad
  is called without clean layout. See this CL:
  https://chromium-review.googlesource.com/c/chromium/src/+/4807704/3
  which necessarily has to change that `SupportsFocus()` to an `IsFocusable()` which violates the comment at the top of `AXObject::ComputeCanSetFocusAttribute()` and does cause test failures due to trying to advance the lifecycle. Thoughts?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 24, 2023, 6:43:43 PM8/24/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- In looking at this in a separate CL, it looks like this call right here: […]
  I do believe the rest of blink doesn't need `SupportsFocus` so maybe it needs to be `SupportsFocusForAccessibilityUseCasesOnly()`?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mark Schillaci (Gerrit)

unread,

Aug 24, 2023, 7:23:47 PM8/24/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed.

1 comment:

Patchset:
- Patch Set #15:
  mschillaci@ if you have any ideas, I'm all ears. […]
  I took a quick look at this. The children are not being populated into the Android tree, so they are all present in the blink tree, but not by the time it makes it to the Android java-side tree. Here is what the tree looks like (pulled from chrome://accessibility after opening the test html):
```
WebView focusable focused scrollable actions:[CLEAR_FOCUS, CLEAR_AX_FOCUS] bundle:[chromeRole="rootWebArea"]
++View text:"1234" focusable scrollable actions:[FOCUS, AX_FOCUS, NEXT, PREVIOUS] bundle:[chromeRole="genericContainer"]
```
  I didn't investigate the root cause, but if I were to guess, it would be because browser_accessibility_android.cc is returning false in IsInterestingOnAndroid:
  https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_android.cc;bpv=1;bpt=1;l=354?q=browser_accessibility_android.cc&ss=chromium&gsn=IsInterestingOnAndroid&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dcontent%2Fbrowser%2Faccessibility%2Fbrowser_accessibility_android.cc%23PA7TeJEc89khAb9Wr4KFyQIqw9oQtein212mKu3SpvU
  It /might/ be because IsLeaf has changed for the nodes, but in that case I would expect the content to be appended to the end of the parent node. Since the text isn't appearing anywhere in the tree, it suggests the node is no longer considered "interesting" to Android, and so we are not populating it.
  https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_android.cc;drc=94789962d91c9510fa0854d8e880e3f917c9e45c;bpv=1;bpt=1;l=534?q=browser_accessibility_android.cc&ss=chromium&gsn=IsLeaf&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dcontent%2Fbrowser%2Faccessibility%2Fbrowser_accessibility_android.cc%23vLIZT8iv6ljiqzpNZ8vyRa5QV8cgYvHt8CG0VlkCXb4

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 24, 2023, 8:01:47 PM8/24/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed.

After hours here but, main, I noticed that you are likely using the platform tree in Chrome://accessibility. I should have nevermind there is a checkbox to look at the raw blink tree.

1 comment:

Patchset:
- Patch Set #15:
  After hours here but, main, I noticed that you are likely using the platform tree in Chrome://accessibility. I should have nevermind there is a checkbox to look at the raw blink tree.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 24, 2023, 9:34:45 PM8/24/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

Patchset:
- Patch Set #15:
  I took a quick look at this. […]
  Thanks for taking a look! `BrowserAccessibilityAndroid` is an entire world I didn't know existed. It's a bit unfortunate to discover another set of definitions of "focusable". 😊 I'm guessing the problem is somewhere in there - a mismatch between the new behavior in the renderer (scrollable containers are now focusable) and the assumptions in this code. Maybe not though.
  Nothing about the "missing" `<p>` tags has changed. Only the container they're in, which is still a genericContainer, just a focusable one. Based on that, and looking through the code (I don't have an Android device to test on), this seems like maybe the culprit?
  https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_android.cc;l=580;drc=bc4cea16a2cf084539bd4cf74807accd7de64874
  That's a condition in `BrowserAccessibilityAndroid::IsLeaf()` which I believe will return `true` for the **container scrollable node** in this example, but wouldn't have before. It says (essentially):
```
```
   // Focusable nodes with text can drop their children (with exceptions).
   if (HasState(ax::mojom::State::kFocusable) && !name.empty()) {
     return IsLeafConsideringChildren();
   }
```
   
With this patch, the `<div>` container will be focusable, and has a non-empty name "1234", and `IsLeafConsideringChildren()` will scan `<div>`'s children and find nothing interesting, so it will return `true`.
```
  Is that the culprit? If so, what should I do about it? Perhaps it's "correct" and the fix is to just add `tabindex` to the `<p>` tags in the test to make them interesting?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mark Schillaci (Gerrit)

unread,

Aug 24, 2023, 11:44:09 PM8/24/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed.

1 comment:

Patchset:
- Patch Set #15:
  Thanks for taking a look! `BrowserAccessibilityAndroid` is an entire world I didn't know existed. […]
  No problem! I'm not sure why there is a title for the <div> in the test, it doesn't seem to serve a purpose. The test is making a div that is scrollable, and then checking that it has scroll actions but none of the children have scroll actions. I think the test would work without the title there and be just as meaningful.
  That being said, this still might be an edge case with this impl that needs to be fixed. I'm curious how this change is affecting the end user behavior for an edge case like this, and if it has changed, what do we consider the correct behavior?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 25, 2023, 9:07:27 AM8/25/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

1 comment:

Patchset:
- Patch Set #17:
  I thought this CL was just cleaning up code and not changing any behavior? We should probably put behavioral changes in a separate CL, if there are any that we want.
  That's why it's probably good to put this code into a dump tree test with blink expectations. It will allow us more easily debug this on any OS and if we land it, will prevent us from accidentally changing how this case works.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 25, 2023, 12:34:06 PM8/25/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

2 comments:

Patchset:
- Patch Set #15:
  No problem! I'm not sure why there is a title for the <div> in the test, it doesn't seem to serve a […]
  Yeah, that's a good question. I've extracted the a11y changes from this CL into a fresh one, and added a dump tree test. Perhaps we can continue on that CL, once it's ready (later today).
Patchset:
- Patch Set #17:
  I thought this CL was just cleaning up code and not changing any behavior? We should probably put be […]
  Yeah, that's a good point. I've extracted the a11y changes to a new CL:
  https://chromium-review.googlesource.com/c/chromium/src/+/4813829
  I suspect we'll have to pick up this discussion there, as the failures will be the same. I've added a dump tree test to that CL, so we'll see what it says.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 25, 2023, 2:17:16 PM8/25/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci, Mason Freed.

Patch set 18:Commit-Queue +1

2 comments:

Patchset:
- Patch Set #15:
  Yeah, that's a good question. […]
  Done
Patchset:
- Patch Set #17:
  Yeah, that's a good point. I've extracted the a11y changes to a new CL: […]
  Ok, I'm about an hour from giving up on KeyboardFocusableScrollers entirely.
  The changes in ax_object.cc are not the culprit for the changes in output. The reason is fundamentally because a11y code is using `SupportsFocus()` to mean `IsFocusable()` and this CL necessarily has to change what `SupportsFocus()` does. Namely, scrollers without focusable children will now return `true` from `SupportsFocus()`, whereas they would previously return `false`. Those same scrollers have been had `IsFocusable()` returning `true` all along.
  We have two choices: keep `SupportsFocus()` but call it `FakeIsFocusableForAccessibilityOnly()` and stop using it elsewhere, or push forward with the behavioral changes this CL implies. I need to know which one to pursue.
  For now, I'm going to re-merge the a11y change stuff back in here, because it can't pass without it.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 25, 2023, 2:31:09 PM8/25/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

File content/test/data/accessibility/html/overflow-actions-expected-android.txt:
- Patch Set #20, Line 2: interesting
  Note, by the way, you can see the behavior change in this existing Android dump test. The scroller becomes focusable, and therefore `interesting`.
  (By the way, it is **actually** becoming focusable, and has been for a long while with KeyboardFocusableScrollers enabled, so this change just makes the a11y code aware of the actual/correct focusable state. The prior situation was that this scroller **was** focusable, but the a11y code **thought it wasn't**.)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 25, 2023, 2:32:08 PM8/25/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- I do believe the rest of blink doesn't need `SupportsFocus` so maybe it needs to be `SupportsFocusFo […]
  I'm really starting to lean toward this solution.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Aaron Leventhal (Gerrit)

unread,

Aug 25, 2023, 3:30:35 PM8/25/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mark Schillaci, Mason Freed.

1 comment:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- I'm really starting to lean toward this solution.
  I'm a bit confused. Can we improve it incrementally?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 25, 2023, 6:17:28 PM8/25/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Mark Schillaci, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci.

1 comment:

File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- I'm a bit confused. […]
  Did you see my other comment? It goes into the details. The only incremental way I know is to add `FakeIsFocusableForAccessibilityOnly` that is only used by a11y code. And that pretty much negates the point of this CL, I think, right?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 26, 2023, 2:27:52 PM8/26/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org

Attention is currently required from: Aaron Leventhal, Di Zhang, Mark Schillaci, Mason Freed.

Mason Freed uploaded patch set #23 to this change.

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable

This CL cleans up three functions in Element:

 - SupportsFocus: true if the element is *capable* of being focused.  An
   element supports focus if, e.g. it has a tabindex attribute, or it is
   editable, or other conditions. Note that the element might *support*
   focus while not *being focusable*, for example if the element is
   disconnected from the document.

 - IsMouseFocusable: true if the element SupportsFocus() *and* is


   currently focusable using the mouse.

 - IsKeyboardFocusable: true if the element IsMouseFocusable(), *and*


   is currently focusable using the keyboard.

Note that each method is a subset of the one above it, by
construction.



This also adds IsScrollableContainerThatShouldBeKeyboardFocusable(),
which returns true for scrollers that should be made keyboard focusable if the feature flag for that behavior is enabled.

This also adds a `DocumentLifecycle::DisallowTransitionScope` to
make sure SupportsFocus() does not update the rendering lifecycle.



Note crbug.com/1474971 and the new baselines for the
video-playback-speed-menu.html test. That focus ring was there due
to a confluence of weirdness in the media controls code, and that

will need to be tackled separately, if desired. The new behavior
might also be ok.

Note that accessibility code was using `SupportsFocus()` to mean
`IsFocusable()`, but prior to this CL, there were cases in which
`SupportsFocus()` returned false and yet `IsFocusable()` returned
true. That has been fixed in this CL everywhere except a11y code,
which still uses a new `FakeIsFocusableForAccessibilityOnly()`
function that does what the old `SupportsFocus()` did. A followup
CL [2] re-removes FakeIsFocusableForAccessibilityOnly() and deals
with the behavioral changes that ensue.

This patch borrows heavily from [1].

[1] https://chromium-review.googlesource.com/c/chromium/src/+/4518938
[2] https://chromium-review.googlesource.com/c/chromium/src/+/4813829



Bug: 1444450, 1474971
Change-Id: Ieeb5f0bde2cf2130c19cd15df1bf4f9c0aa21e19
---

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/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h
M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h

M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates-expected.txt
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates.html


M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png

24 files changed, 134 insertions(+), 130 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 26, 2023, 2:30:18 PM8/26/23

to Tommy Steimel, Mark Schillaci, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Aaron Leventhal

Attention is currently required from: Aaron Leventhal, Di Zhang, Tommy Steimel.

Mason Freed would like Tommy Steimel to review this change.

Mason Freed removed Mark Schillaci from this change.

Mason Freed (Gerrit)

unread,

Aug 26, 2023, 2:30:28 PM8/26/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Tommy Steimel.

Patch set 23:Auto-Submit +1Commit-Queue +1

1 comment:

Patchset:
- Patch Set #23:
  +steimel@ for renderer/modules/media_controls/elements/* and to take note of crbug.com/1474971.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 26, 2023, 2:51:10 PM8/26/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Tommy Steimel.

Patch set 24:Auto-Submit +1

3 comments:

Patchset:
- Patch Set #17:
  Ok, I'm about an hour from giving up on KeyboardFocusableScrollers entirely. […]
  Re-removed, per my other comment.
File content/test/data/accessibility/html/overflow-actions-expected-android.txt:

- Patch Set #20, Line 2: interesting

- Note, by the way, you can see the behavior change in this existing Android dump test. […]
  Acknowledged
File third_party/blink/renderer/core/dom/element.h:

- Patch Set #5, Line 793: virtual bool SupportsFocus() const;

- Did you see my other comment? It goes into the details. […]
  Alright, I've once again moved all of the a11y code out into this CL:
  https://chromium-review.googlesource.com/c/chromium/src/+/4813829
  That requires just the rename of `SupportsFocus` to `FakeIsFocusableForAccessibilityOnly()` here and no behavioral changes a11y-wise. And it unblocks this CL to land once I get media team LGTM.
  I'm going to close this set of comments, and we can continue them on the CL above.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 26, 2023, 5:47:54 PM8/26/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed, Tommy Steimel.

Mason Freed uploaded patch set #27 to this change.

Rationalize the behavior of SupportsFocus, Is{keybd|mouse}Focusable

This CL cleans up three functions in Element:

 - SupportsFocus: true if the element is *capable* of being focused.  An
   element supports focus if, e.g. it has a tabindex attribute, or it is
   editable, or other conditions. Note that the element might *support*
   focus while not *being focusable*, for example if the element is
   disconnected from the document.

 - IsMouseFocusable: true if the element SupportsFocus() *and* is
   currently focusable using the mouse.

 - IsKeyboardFocusable: true if the element IsMouseFocusable(), *and*
   is currently focusable using the keyboard.

Note that each method is a subset of the one above it, by
construction.

This also adds IsScrollableContainerThatShouldBeKeyboardFocusable(),
which returns true for scrollers that should be made keyboard focusable if the feature flag for that behavior is enabled.

This also adds a `DocumentLifecycle::DisallowTransitionScope` to
make sure SupportsFocus() does not update the rendering lifecycle.

Note crbug.com/1474971 and the new baselines for the
video-playback-speed-menu.html test. That focus ring was there due
to a confluence of weirdness in the media controls code, and that
will need to be tackled separately, if desired. The new behavior
might also be ok.

Note that accessibility code is using `SupportsFocus()` to mean


`IsFocusable()`, but prior to this CL, there were cases in which
`SupportsFocus()` returned false and yet `IsFocusable()` returned
true. That has been fixed in this CL everywhere except a11y code,

which still uses `SupportsFocus()`. In addition, to avoid breaking
a11y code, `SupportsFocus()` is still not calling the new
`IsScrollableContainerThatShouldBeKeyboardFocusable()` method, which
means for focusable scrollers, SupportsFocus will be `false` while
`IsFocusable()` will be true. This will be fixed in a followup
CL [2].



This patch borrows heavily from [1].

[1] https://chromium-review.googlesource.com/c/chromium/src/+/4518938
[2] https://chromium-review.googlesource.com/c/chromium/src/+/4813829

Bug: 1444450, 1474971
Change-Id: Ieeb5f0bde2cf2130c19cd15df1bf4f9c0aa21e19
---
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/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/core/page/focus_controller.cc

M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h
M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates-expected.txt
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates.html
M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png

23 files changed, 109 insertions(+), 127 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Blink W3C Test Autoroller (Gerrit)

unread,

Aug 26, 2023, 7:30:42 PM8/26/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel, Aaron Leventhal, Di Zhang, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Aaron Leventhal, Di Zhang, Mason Freed, Tommy Steimel.

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/41653.

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

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 27, 2023, 1:34:25 AM8/27/23

to Di Zhang, Aaron Leventhal, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel

Attention is currently required from: Di Zhang, Tommy Steimel.

Mason Freed would like Di Zhang to review this change.

Mason Freed removed Aaron Leventhal from this change.

M third_party/blink/renderer/core/input/event_handler_test.cc


M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h
M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h

M third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-event-fires-to-iframe-inner-frame.html


M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates-expected.txt
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates.html
M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png

M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-tabindex.html
27 files changed, 118 insertions(+), 133 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 27, 2023, 1:34:31 AM8/27/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Tommy Steimel.

Patch set 28:Auto-Submit +1

1 comment:

Patchset:
- Patch Set #28:
  Di, can you PTAL at everything but the media stuff?

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Tommy Steimel (Gerrit)

unread,

Aug 28, 2023, 1:30:34 AM8/28/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Mason Freed.

2 comments:

Patchset:
- Patch Set #28:
  The visual changes seem okay to me, but I'd rather not lose the ability to scroll via clicking and dragging the scrollbar. Is there a way to avoid losing that ability while still fixing SupportsFocus?
File third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h:
- Patch Set #28, Line 30: // When clicking the scroll bar, chrome will find its first focusable parent
  By removing this, using the mouse to use the scrollbar just closes the popup

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 28, 2023, 11:55:22 AM8/28/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Tommy Steimel.

Patch set 29:Auto-Submit +1

1 comment:

File third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h:

- Patch Set #28, Line 30: // When clicking the scroll bar, chrome will find its first focusable parent
  By removing this, using the mouse to use the scrollbar just closes the popup

- Ahh good catch, thanks. I've fixed this by adding a `setTabindex(0)` to the popup menu constructor. I verified locally that this fixes the problem. I also added a TODO that there's apparently no test for this behavior.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Di Zhang (Gerrit)

unread,

Aug 28, 2023, 1:44:07 PM8/28/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Mason Freed, Tommy Steimel.

2 comments:

Patchset:
- Patch Set #29:
  From a quick look, the changes make sense. Might be easier to review once the other CLs are merged.
File third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html:
- Patch Set #29, Line 61: // TODO(crbug.com/1444450) Put this back once scrollers are fully supported again.
  Would it be better to have the test in TestExpectations instead? I fear TODO in tests might get forgotten. Or, add a crbug comment about re-enabling these tests.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 28, 2023, 7:30:09 PM8/28/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Di Zhang, Tommy Steimel.

2 comments:

Patchset:

- Patch Set #29:
  From a quick look, the changes make sense. Might be easier to review once the other CLs are merged.

- Hmm - I do need a review for this one first, since it's the *first* in the chain of CLs. Is there something I can help with?
File third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html:

- Patch Set #29, Line 61: // TODO(crbug.com/1444450) Put this back once scrollers are fully supported again.

- Would it be better to have the test in TestExpectations instead? I fear TODO in tests might get forg […]
  Yeah, I definitely don't want to forget about this. But I did add a TODO(crbug) comment (see line 61), and the next CL in the stack re-removes this case:
  https://chromium-review.googlesource.com/c/chromium/src/+/4813829/14/third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html
  I definitely prefer commenting this line out rather than using TestExpectations, since TestExpectations will completely disable the test and we lose all coverage. This way, we're testing the new behavior still.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Di Zhang (Gerrit)

unread,

Aug 28, 2023, 7:47:18 PM8/28/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Mason Freed, Tommy Steimel.

Patch set 29:Code-Review +1

2 comments:

Patchset:
- Patch Set #29:
  Hmm - I do need a review for this one first, since it's the *first* in the chain of CLs. […]
  Oh, I didn't realize. I thought we could merge 4794984 into this. And was waiting for that before approving.
File third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html:

- Patch Set #29, Line 61: // TODO(crbug.com/1444450) Put this back once scrollers are fully supported again.

- Yeah, I definitely don't want to forget about this. […]
  I see, missed patch 4813829. If it is getting fixed within the next few patches, then no need adding to TestExpectations.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Aug 28, 2023, 10:01:51 PM8/28/23

to abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Tommy Steimel, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Tommy Steimel.

2 comments:

Patchset:
- Patch Set #29:
  Oh, I didn't realize. I thought we could merge 4794984 into this. […]
  Ahh, I see. Yeah, stacked changes like this get landed in order, rather than merged into one and then landed together. So this patch will land first, and then 4794984 (and maybe 4813829) and then 4807704. So when you get a set of stacked CLs to review, the first one to review is the one listed at the *bottom* of the "Relation chain" section there. Just FYI for later! 😊
- Patch Set #29:
  Thanks dizhangg@!
  So this just needs a review for the renderer/modules/media_controls/* changes.

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Tommy Steimel (Gerrit)

unread,

Aug 29, 2023, 12:25:26 PM8/29/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Di Zhang, Blink W3C Test Autoroller, Chromium LUCI CQ, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Attention is currently required from: Mason Freed.

Patch set 29:Code-Review +1Commit-Queue +2

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Aug 29, 2023, 1:23:53 PM8/29/23

to Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel, Di Zhang, Blink W3C Test Autoroller, Akihiro Ota, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

Chromium LUCI CQ submitted this change.

Approvals:
  Tommy Steimel: Looks good to me; Commit
  Di Zhang: Looks good to me
  Mason Freed: Send CL to CQ automatically after approval

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4795287
Reviewed-by: Tommy Steimel <ste...@chromium.org>
Commit-Queue: Tommy Steimel <ste...@chromium.org>
Auto-Submit: Mason Freed <mas...@chromium.org>
Reviewed-by: Di Zhang <dizh...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1189598}


---
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/media/html_media_element.cc
M third_party/blink/renderer/core/html/media/html_media_element.h
M third_party/blink/renderer/core/input/event_handler_test.cc
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
M third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.h
M third_party/blink/renderer/modules/media_controls/elements/media_control_playback_speed_list_element.cc

M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc


M third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
M third_party/blink/web_tests/external/wpt/dom/events/scrolling/scrollend-event-fires-to-iframe-inner-frame.html
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates-expected.txt
M third_party/blink/web_tests/fast/scroll-behavior/overflow-scroll-animates.html
M third_party/blink/web_tests/platform/linux/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac11-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac12-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac-mac13-arm64/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/controls-refresh-hc/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/media/video-playback-speed-menu-expected.png
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-tabindex.html

28 files changed, 124 insertions(+), 133 deletions(-)

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Blink W3C Test Autoroller (Gerrit)

unread,

Aug 29, 2023, 2:00:10 PM8/29/23

to Chromium LUCI CQ, Mason Freed, abigailbk...@google.com, aleventhal...@chromium.org, blink-rev...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, hirokisa...@chromium.org, josiah...@chromium.org, kyungjunle...@google.com, nektar...@chromium.org, sarakat...@chromium.org, yuzo+...@chromium.org, Tommy Steimel, Di Zhang, chromium...@chromium.org, (Julie)Jeongeun Kim, Kevin Babbitt, Nektarios Paisios

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/41653

To view, visit change 4795287. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu