Summary: | Web Inspector: Audit: provide a way to query for all nodes with a given computed Accessibility role | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, hi, inspector-bugzilla-changes, jcraig, jdiggs, joepeck, samuel_white, simon.fraser, tsavell, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=193594 | ||||||||||
Bug Depends on: | 190754, 193149 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Devin Rousso
2019-01-07 21:38:24 PST
Created attachment 358572 [details]
Patch
Created attachment 358850 [details]
Patch
Throw an error when called outside of a test
Comment on attachment 358850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358850&action=review r=me > LayoutTests/inspector/audit/run-accessibility-expected.txt:22 > +-- Running test case: Audit.run.Accessibility.getElementsByComputedRole.button > +Audit setup... > +Audit run `AUDIT.Accessibility.getElementsByComputedRole("button")`... > +Result: ["#button"] > +Audit teardown... Lets have a test that returns multiple elements along side this one which just returns one. > LayoutTests/inspector/audit/run-accessibility.html:29 > + function evaluateStringForTest(func, target, role) { > + if (target) { > + if (role) > + return `Accessibility.${func}("${role}", document.querySelector("#${target}"))`; > + return `Accessibility.${func}(document.querySelector("#${target}"))`; > + } > + > + if (role) > + return `Accessibility.${func}("${role}")`; > + } Having read a few of these now, I think having `AUDIT.Accessibility` here would be better for searchability as well instead of putting the AUDIT part later. Up to you. > Source/WebCore/inspector/InspectorAuditAccessibilityUtilities.cpp:42 > +#define ERROR_IF_NO_ACTIVE_AUDIT() if (!m_auditAgent.hasActiveAudit()) return Exception { NotAllowedError, "Unable to run outside of an Inspector Audit"_s }; Style: Make this multiple lines to improve readability. Created attachment 359148 [details]
Patch
Comment on attachment 359148 [details] Patch Clearing flags on attachment: 359148 Committed r239986: <https://trac.webkit.org/changeset/239986> All reviewed patches have been landed. Closing bug. The new test inspector/audit/run-accessibility.html added in https://trac.webkit.org/changeset/239986/webkit is a flakey failure on Mac. History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Faudit%2Frun-accessibility.html Diff: --- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/inspector/audit/run-accessibility-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/inspector/audit/run-accessibility-actual.txt @@ -18,7 +18,7 @@ -- Running test case: Audit.run.Accessibility.getElementsByComputedRole.tree Audit setup... Audit run `WebInspectorAudit.Accessibility.getElementsByComputedRole("tree")`... -Result: ["#parent"] +Result: [] Audit teardown... -- Running test case: Audit.run.Accessibility.getElementsByComputedRole.tree.parent @@ -30,7 +30,7 @@ -- Running test case: Audit.run.Accessibility.getElementsByComputedRole.button Audit setup... Audit run `WebInspectorAudit.Accessibility.getElementsByComputedRole("button")`... -Result: ["#button","#link"] +Result: [] Audit teardown... -- Running test case: Audit.run.Accessibility.getElementsByComputedRole.button.parent (In reply to Truitt Savell from comment #8) > The new test inspector/audit/run-accessibility.html > > added in https://trac.webkit.org/changeset/239986/webkit > > is a flakey failure on Mac. This test only seems to fail on my machine if you run it in concert with other tests (or repeated iterations). Running it by itself has always passed, but it doesn't seem to work with more than one run. $ run-webkit-tests --no-build --no-sample --no-retry-failures --time-out-ms=5000 --force --verbose --iterations=100 --exit-after-n-failures=1 inspector/audit/run-accessibility.html That seems to consistently fail on the 2nd or 3rd iteration. Based on what I've been able to figure out so far, it seems that we aren't creating a render tree for the new document, meaning that we early-return for any accessibility related calls. (In reply to Devin Rousso from comment #9) > (In reply to Truitt Savell from comment #8) > > The new test inspector/audit/run-accessibility.html > > > > added in https://trac.webkit.org/changeset/239986/webkit > > > > is a flakey failure on Mac. > This test only seems to fail on my machine if you run it in concert with > other tests (or repeated iterations). Running it by itself has always > passed, but it doesn't seem to work with more than one run. > > $ run-webkit-tests --no-build --no-sample --no-retry-failures > --time-out-ms=5000 --force --verbose --iterations=100 > --exit-after-n-failures=1 inspector/audit/run-accessibility.html > > That seems to consistently fail on the 2nd or 3rd iteration. > > Based on what I've been able to figure out so far, it seems that we aren't > creating a render tree for the new document, meaning that we early-return > for any accessibility related calls. It looks like the problem was "higher up" than the Document. The implementation of `getElementsByComputedRole` used `[CallWith=Document]` so that it could work without hassle inside any environment (rather than relying on the inspected page's main frame's document). The issue was that the backing WebCore `InspectorAuditAccessibilityObject` was being reused across test runs, meaning that even though the page changed, the underlying object didn't. My guess is that the reference to the JavaScript value was somehow being cached between these runs, and when we `toJS` the `InspectorAuditAccessibilityObject`, we get the old page's JavaScript value instead of the new one, which no longer has a render tree, meaning it wouldn't generate any accessibility objects. <https://webkit.org/b/193594> |