Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chrome WPT failures may be due to incorrect implementation in ChromeDriver ExecuteGetComputedLabel #65

Closed
zcorpan opened this issue Oct 10, 2023 · 24 comments
Assignees

Comments

@zcorpan
Copy link
Member

zcorpan commented Oct 10, 2023

See web-platform-tests/interop#526 (comment) by @dtsengchromium

I think there's issues with the tests themselves and the way they fetch data from Chrome. One issue I've written up follows.

WPT calls into webdriverio https://github.com/webdriverio/webdriverio in webdriverio/packages/devtools/src/commands/getElementComputedRole.ts accessibility.snapshot

which is Puppeteer https://github.com/puppeteer/puppeteer puppeteer/packages/puppeteer-core/src/cdp/Accessibility.ts which sends the CDP command Accessibility.getFullAXTree with the interesting option defaulted to true.

In Chrome, getFullAXTree InspectorAccessibilityAgent::getFullAXTree gets called and returns the tree back over CDP.

Back in Puppeteer, puppeteer/packages/puppeteer-core/src/cdp/Accessibility.ts Accessibility.snapshot

the tree is filtered for "interesting nodes". This amounts to calling into its own wrapper AXNode ts object isInteresting method.

That method has some definition of interesting which includes only some elements e.g. controls, leaves (with text), etc. That explains a lot of failures within the dashboard pertaining to roles.

I'm therefore not sure we can use the test results here until the underlying tooling is reliable/corrected. Specifically here, the simplest thing is for WPT to pass interesting to false https://pptr.dev/api/puppeteer.accessibility.snapshot

@cookiecrook
Copy link
Collaborator

cookiecrook commented Oct 10, 2023

I think this might only be a problem for one investigation issue, which is intended to carry over into the 2024 investigation:

@cookiecrook
Copy link
Collaborator

cookiecrook commented Oct 10, 2023

IOW, I don't understand how it would affect any of the current WPT tests as written.

@zcorpan
Copy link
Member Author

zcorpan commented Oct 11, 2023

I thought it was about how the webdriver Get Computed Role command was implemented for Chrome, but maybe @dtsengchromium can clarify.

@dtsengchromium
Copy link

dtsengchromium commented Oct 11, 2023

@zcorpan is correct. WPT asks for an accessibility tree snapshot from webdriver which prunes "uninteresting" nodes a lot of which WPT attempts to test for e.g. <code>. Simple fix is to ask for those uninteresting nodes as well.

@zcorpan
Copy link
Member Author

zcorpan commented Oct 12, 2023

cc @chrishtr @aleventhal

@boazsender
Copy link
Contributor

boazsender commented Oct 12, 2023

Ah, I understand now. Thanks for clarifying.

@cookiecrook
Copy link
Collaborator

cookiecrook commented Oct 13, 2023

Yes, thanks for clarifying. Here's the direct link to the <code> failure David mentioned as an example: HTML-AAM: el-code

@cookiecrook
Copy link
Collaborator

@jugglinmike since you are investigating, would you be the right person to take assignement of this issue and potentially provide a patch.

@cookiecrook
Copy link
Collaborator

cookiecrook commented Nov 7, 2023

Actually, if I am reading this subset of @jugglinmike's call stack investigation correctly, isn't this a bug in Chromium's implementation of WebDriver computedlabel, not requiring a fix in the WPT project? @dtsengchromium mentioned ExecuteGetComputedLabel may need to override the argument for whether the node was "interesting" or not.

@dtsengchromium
Copy link

dtsengchromium commented Nov 8, 2023

It's not really a bug. The current set of users of Chrome driver / Puppeteer simply don't care about a large majority of the nodes. Presumably they are using it for test automation purposes.

I think that's true for most usages of CDP today. The fix from a high level is for WPT to ask for uninteresting nodes from Chrome driver or Puppeteer.

I'm also a bit unclear on what get computed role/label mean in these contexts and who added that support. Is label supposed to mean accessible name computation or something else?

@jgraham
Copy link
Contributor

jgraham commented Nov 8, 2023

The WebDriver spec provides get computed role and get computed label (https://w3c.github.io/webdriver/#get-computed-role) and it's more or less up to Chrome/ChromeDriver to implement those correctly.

https://wpt.fyi/results/webdriver/tests/classic/get_computed_role/get.py?label=master&label=experimental&aligned suggests that at least some cases are implemented correctly. But the spec doesn't have any concept of "interesting nodes" so if there's currently filtering going on, you may need to adjust the ChromeDriver implementation to disable it. However wpt doesn't know anything about this.

@dtsengchromium
Copy link

To clarify, I (personally or work wise) don't have any tie to Chrome driver. If there's a discrepancy between some spec and Chrome driver, it seems like that should be resolved by someone from Chrome driver and the spec authors. I can see an argument that the implementation is wrong or that the spec is wrong.

I simply pointed out the issue lies within the manner in which WPT snapshots the a11y tree whether it's WPT, one of its deps, or some combination at fault.

@cookiecrook
Copy link
Collaborator

cookiecrook commented Nov 9, 2023

@dtsengchromium wrote:

the issue lies within the manner in which [the chromium tests return the role/label for a node], whether it's WPT, one of its deps, or some combination at fault.

Yes, from the discussion, and the WPT code, and mostly from @jugglinmike's call stack diagnosis, this seems like an issue in Chromium's implementation of WebDriver rather than in WPT, so likely an implementation detail not blocking the tests. I'll spawn this as a new issue on Chromium to resolve.

With regards to the tests being considered for the Interop 2024 Accessibility Focus Area, I believe they are all valid, but we can exclude some on a file-by-file basis, so if there are specific tests that prove particularly challenging, risky, or low-ROI to implement in Chromium, we can discuss excluding them.

I'm also a bit unclear on what get computed role/label mean in these contexts

ARIA uses label more or less interchangeably with how the Windows APIs use name. label was chosen in part because "name" has so many other unrelated definitions in the web platform. The WebDriver spec references the AccName spec in the definition of computedlabel and references the ARIA spec in the definition of computedrole.

… and who added that support.

Not sure in which project (WPT, WebDriver, or the engines) you are referring to, but…

  • WebDriver computedlabel and computedrole came out of TPAC 2019 discussions in Fukuoka with the Browser Testing and Tools group. Many of the people actively working on this investigation project were in that initial discussion.
  • WebKit and Chromium added support for the new WebDriver methods in the last few years. I don't have the patch links handy, but I can find more history/detail if needed.
  • I added the WPT support back in February with Update test driver to use computedrole/computedlabel from webdriver #5 and a few later patches.
  • After this year's investigation started, Mozilla added support for those in Gecko bug 1585622 (webdriver computedlabel/computedrole updates) and Gecko bug 1832148 (marionette updates).

@cookiecrook cookiecrook self-assigned this Nov 9, 2023
@cookiecrook cookiecrook changed the title Test failures in Chrome may be due to incorrect infra implementation in wpt Chrome WPT failures may be due to incorrect implementation in ChromeDriver ExecuteGetComputedLabel Nov 9, 2023
@cookiecrook
Copy link
Collaborator

@cookiecrook
Copy link
Collaborator

@dtsengchromium I think this WPT tracker can be closed now, but I'll leave that decision up to you.

@dtsengchromium
Copy link

dtsengchromium commented Nov 9, 2023

With regards to the tests being considered for the Interop 2024
Accessibility Focus Area, I believe they are all valid, but we can
exclude some on a file-by-file basis, so if there are specific tests
that prove particularly challenging, risky, or low-ROI to implement in
Chromium, we can discuss excluding them.

I don't think the tests were ever invalid. The tooling under them seems flawed. I can expand on that a bit more below.

Yes, in ARIA parlance, label is more or less interchangeable with name,
and was used because "name" has so many other vocabulary meanings in
the web platform. The WebDriver spec references the AccName spec in the
definition of computedlabel and references the ARIA spec in the
definition of computedrole

GOod history here and below.

Some of this might have been discussed previously, but as is, accessible name and role computation cannot be satisfied with the way snapshoting works overall.

Snapshoting currently within Blink (and likely WebKit) pertain to a specific renderer. That means any specs requiring crossing frame boundaries would not be covered. Examples.

  • aria hidden applied to an ancestor frame
  • role presentation applied to ancestoring frames
  • node refs that cross frames (e.g. aria labelledby)

Some permutations of things I'd worry about with regarding to testing this layer as is:

  • test fails here, test succeeds with browser a11y tree: e.g. shows up fine in Chrome's browser a11y tree, OS specific a11y tree
  • test passes here, test fails in browser/OS a11y tree: Chrome (and likely others) transform the renderer side a11y tree when they serialize/deserialize it into the browser/UI process. For example, we add additional nodes (for table cell columns), add additional computations for name that are less expensive done on demand, etc.
  • test fails in both: clear signal though root cause analysis is still a fair amount of work for browser implementers
  • test passes in both: good to go

Finally, what it looks like is that there's an additional accessibility api being added here (get computed label, role). I'm sure this was also discussed, but this does increase yet again the surfaces on which browsers have to support a11y.

Ultimately, what's probably right for the tooling and browsers to do is to surface the complete composite browser/UI process tree through this informal "debugging" JSON protocol which WebKit and subsequently Chrome have adopted and made into something for the various web drivers, test automation to consume, use.

@chrishtr FYI.

@cookiecrook
Copy link
Collaborator

@dtsengchromium wrote:

Snapshotting currently within Blink (and likely WebKit) pertain to a specific renderer.

Are you assuming this affects WebKit because Blink/Chromium forked from WebKit in 2013, or some other reason?

I added AccessibilityObject::computedRoleString() in WebKit in 2014 (after the Blink fork) in order to support the WebKit Accessibility Inspector. WebKit's WebDriver implementation uses these same methods that come from the live internal accessibility tree.

I recall the Accessibility additions for CDP were a few years later, so I don't have a reason to believe the snapshotting decision affects the other engines. Since you mentioned <code> as an example of where this breaks in Chrome and Edge, it's worth noting that same test passes in Firefox and Safari.

Or if there is some other reason this might affect WebKit, please elaborate. Thanks.

@cookiecrook
Copy link
Collaborator

cookiecrook commented Nov 9, 2023

@dtsengchromium wrote:

Finally, what it looks like is that there's an additional accessibility api being added here (get computed label, role). I'm sure this was also discussed, but this does increase yet again the surfaces on which browsers have to support a11y.

The WebDriver API for computedrole and computedlabel was added in 2019 and is already implemented in all engines. The new tests in WPT just use that pre-existing API to verify expectations from AccName, ARIA, and other specs.

Ultimately, what's probably right for the tooling and browsers to do is to surface the complete composite browser/UI process tree through this informal "debugging" JSON protocol which WebKit and subsequently Chrome have adopted and made into something for the various web drivers, test automation to consume, use.

I believe that's what WebKit and Gecko are already doing.

Since Chrome isn't going off the live tree, I filed this implementation bug in Chromium.

@dtsengchromium
Copy link

I can't speak to the other engines e.g. FF post cache the world work, wk2, etc but did Blink have a informed stakeholder within those proceedings?
Seems like a pretty large oversight and not really agreeable given the amount of work actually required.
Also to be clear, this (CDP) is the live a11y tree, just renderer specific.
A completely new a11y api over a debugging protocol though is pretty unsustainable, but I'm just catching up with some of this workstream, so am probably missing a whole lot of context since most of our engineers have been focused on OS + AT work. In that sense, the tests here don't really capture much of the actual issues we're seeing.
Probably better to take this in a much less latency sensitive, higher bandwidth form of comms. Will reach out.

@dtsengchromium
Copy link

dtsengchromium commented Nov 10, 2023

Are you assuming this affects WebKit because Blink/Chromium forked from WebKit in 2013, or some other reason?

I never stated any such tthing (see above).

I added AccessibilityObject::computedRoleString() in WebKit in 2014 (after the Blink fork) in order to support the WebKit Accessibility Inspector. WebKit's WebDriver implementation uses these same methods that come from the live internal accessibility tree.

Great. How does that work after WebKit went multi process?

I recall the Accessibility additions for CDP were a few years later, so
I don't have a reason to believe the snapshotting decision affects the
other engines. Since you mentioned <code> as an example of where this
breaks in Chrome and Edge, it's worth noting that same test passes in
Firefox and Safari.

That's good as well, but practically irrelevant since the OS tree is correct on Windows, Mac, Linux, and Android for Chrome.

@cookiecrook
Copy link
Collaborator

cookiecrook commented Nov 10, 2023

Yes let's chat offline. It's good that a potential Chromium issue has come out of this, but I think this WPT issue lacks clarity as to what, if anything, needs to change in WPT.

@cookiecrook
Copy link
Collaborator

@dtsengchromium seems to indicate in this comment on the Focus Area tracker that he now agrees this is a Chromium issue, not a WPT issue. Assuming that's correct David, can this issue be closed since Chromium 1500862 is tracking any changes needed?

@dtsengchromium
Copy link

dtsengchromium commented Jan 6, 2024 via email

@cookiecrook
Copy link
Collaborator

Closing. Tracked in Chromium 1500862

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants