fix(a11y-wand): add handling of nested aria-hidden elements #134

ericrallen · 2019-03-09T17:08:15Z

We needed to set the recursive argument in the findTextAlternatives() method to true so that accessibility-developer-tools would check for nested hidden elements.

This also adds two simple tests to make sure that this functionality isn't broken in the future.

Closes #133

khanbot · 2019-03-09T17:08:24Z

Hey @ericrallen,

Thanks for the PR! Mind signing our Contributor License Agreement?

When you've done so, go ahead and comment [clabot:check] and I'll check again.

Yours truly,
khanbot

ericrallen · 2019-03-09T17:09:32Z

[clabot:check]

khanbot · 2019-03-09T17:09:33Z

CLA signature looks good 👍

ericrallen · 2019-03-10T23:41:52Z

package.json

@@ -20,6 +20,7 @@
    "handlebars-loader": "^1.0.0",
    "jquery": "^2.1.3",
    "jsdom": "^8.1.0",
+    "jsdom-global": "^2.1.1",


This seemed to be necessary to get the a11y-text-wand plugin to run due to needing to access something on window that wasn't available without jsdom-global.

Do you recall what it was that was missing?

I'll pull it out run the test again and report back.

Without jsdom-global:

ReferenceError: window is not defined at InfoPanel.initAndPosition (plugins/shared/info-panel/index.js:127:9) at InfoPanel.render (plugins/shared/info-panel/index.js:346:18) at A11yTextWand.run (plugins/a11y-text-wand/index.js:23:20) at Context.<anonymous> (test/a11y-wand-test.js:29:16)

I'm not super familiar with jsdom, but some brief research lead me to the jsdom-global and --require jsdom-global/register solution for resolving this error.

Thanks for checking into that. This is not a request for you, just an outloud musing; I wonder if upgrading jsdom as well as setting pretendToBeVisual to true in its setup would address this too.

A quick glance at the jsdom docs seem to indicate that might be the case. It looks like the usage of jsdom has changed a good bit and jsdom.env no longer exists and this change might require some more significant refactoring.

This same error shows up for all four of the tests this PR adds and none of the other tests run.

1) Screen Reader Wand "before all" hook: TypeError: jsdom.env is not a function at Object.exports.createDom (test/mock-dom/index.js:30:11) at Context.<anonymous> (test/a11y-wand-test.js:12:17)

I misspoke, it does try to run all the tests, but they all fail with the same error from above.

I spent a little time trying to quickly refactor the mock-dom file, but it looks like something with how the annotations are handled might be causing an issue?

I don't think I'm familiar enough with jsdom or the way this is being handled to resolve it without a good deal more research.

I can work on upgrading JSDOM, if you'd like. I have quite a bit of experience with it if late. I can put you on the review and we can collaborate so that all gets landed.

I've already been upgrading other dependencies yesterday, so this fits pretty well with that.

What do you think?

Sure thing.

We can put this one on pause and rebase once the JSDOM updates are merged in.

plugins/a11y-text-wand/index.js

ericrallen · 2019-03-10T23:44:11Z

plugins/a11y-text-wand/index.js


            let textAlternative = axs.properties.findTextAlternatives(
-                element, {});
+                element, {}, true);


This was the real fix to the issue, the true tells findTextAlternatives to run recursively on the Element and it's children. Without this, nested aria-hidden elements will show up in the Screen Reader text that is displayed in tota11y's info panel.

ericrallen · 2019-03-13T02:52:01Z

@somewhatabstract I know this repo hasn't been touched in quite awhile, but this is kind of a big issue with how the a11y-text-wand operates. There are a few PRs that have been sitting out there for awhile. Is there anything we can help with to get this or any of the other PRs across the finish line?

somewhatabstract · 2019-03-13T17:27:57Z

@somewhatabstract I know this repo hasn't been touched in quite awhile, but this is kind of a big issue with how the a11y-text-wand operates. There are a few PRs that have been sitting out there for awhile. Is there anything we can help with to get this or any of the other PRs across the finish line?

Hey, @ericrallen, thanks for pinging me and for contributing! I will make sure to put time aside this week so we can get this reviewed and merged. We are working on how to better manage our open source like tota11y; I'm sorry it's been a sub-optimal experience for this repo of late. I would like to contribute more and get tota11y on a regular update cadence; I'll work to get some time to do that.

Thanks again for reaching out.

test/a11y-wand-test.js

somewhatabstract

Thank you again for this contribution, @ericrallen. I've left some comments inline. Let me know what you think.

ericrallen · 2019-03-15T23:46:08Z

@somewhatabstract Thanks for taking a look. I believe I've addressed everything in your comments, but had one question about the testing AAA comments.

somewhatabstract · 2019-04-22T16:58:35Z

I haven't forgotten this. I will try to make time this week to give feedback. Thank you for your patience.

ericrallen · 2019-04-23T19:19:38Z

No worries. I know how easy it is to get busy with stuff.

I see there were more changes merged in so I'll resolve the conflicts and get this conflict-free, too.

somewhatabstract · 2019-05-17T17:55:24Z

Thanks for your patience, @ericrallen.

I'll work on the JSDOM upgrade today, but I think this is looking pretty good otherwise. It needs rebasing to the latest master as we're no longer committing build artifacts (yay!).

ericrallen · 2019-05-17T18:06:14Z

@somewhatabstract Rebase is taken care of. Looking forward to the JSDom upgrade and getting this one merged in.

Thanks for taking a look. I know it's tough to find time for this stuff when things get busy.

somewhatabstract · 2019-05-18T00:11:19Z

@somewhatabstract Rebase is taken care of. Looking forward to the JSDom upgrade and getting this one merged in.

Well, I spent a few hours with this today. Just updating JSDOM was a journey since its API has changed, making the way we import files more difficult. I thought I'd see if I could leverage jest and its use of JSDOM to make things work, but that proved difficult. It changed how things were getting imported leading to different behavior from axs. I'd rather remove hacks than add more, so I'm a bit reluctant to just hack it to work. I'm going to think about it some more and see if we can get a path forward.

Thanks for taking a look. I know it's tough to find time for this stuff when things get busy.

No worries. I am really sorry for taking so darn long. Thank you so much for being patient and for making contributions to this project.

somewhatabstract · 2019-05-18T01:00:48Z

If you have chance to look at what I'm looking at @ericrallen, it's this.

At the CLI, run node REPL.
const axs = require("accessibility-developer-tools")
And then just show what that looks like; you should see it's missing a whole bunch of things like AuditRules

If you do the same inside of our tests or the browser, then all those things are there.

This different behavior in the two worlds is making it difficult to move forward with the JSDOM update. My current thinking is that we may want to remove our dependency on this no longer maintained library. Not an insurmountable task but definitely weightier than I envisioned when I first said we should update JSDOM.

So, I'm going to roll back on that suggestion for now and let's move forward with jsdom-global as you need it for this work (and it's only for our test environment).

We should then strategize how we can deal with our hard dependency on accessibility-developer-tools.

somewhatabstract · 2019-05-18T01:12:49Z

I think I know how to resolve this after all. I'll try to get a PR together.

We needed to set the recursive argument in the findTextAlternatives method to true so that accessibility-developer-tools would check for nested hidden elements. This also adds two simple tests to make sure that this functionalit isn't broken in the future. Closes jdan#133

ericrallen commented Mar 10, 2019

View reviewed changes

plugins/a11y-text-wand/index.js Show resolved Hide resolved

ericrallen commented Mar 10, 2019

View reviewed changes

ericrallen mentioned this pull request Mar 12, 2019

Elements with aria-hidden="true" still included in output of Screen Reader Wand #133

Open

somewhatabstract self-requested a review March 13, 2019 17:28

somewhatabstract reviewed Mar 15, 2019

View reviewed changes

test/a11y-wand-test.js Show resolved Hide resolved

somewhatabstract reviewed Mar 15, 2019

View reviewed changes

test/a11y-wand-test.js Show resolved Hide resolved

somewhatabstract reviewed Mar 15, 2019

View reviewed changes

test/a11y-wand-test.js Outdated Show resolved Hide resolved

somewhatabstract suggested changes Mar 15, 2019

View reviewed changes

ericrallen force-pushed the feature/a11y-wand-aria-hidden branch from 07d1068 to 0887719 Compare March 15, 2019 23:37

ericrallen force-pushed the feature/a11y-wand-aria-hidden branch 2 times, most recently from 0ced4a0 to e184248 Compare March 16, 2019 03:12

ericrallen force-pushed the feature/a11y-wand-aria-hidden branch from e184248 to 2841a42 Compare April 15, 2019 19:30

somewhatabstract assigned ericrallen May 17, 2019

ericrallen force-pushed the feature/a11y-wand-aria-hidden branch from 2841a42 to ee94325 Compare May 17, 2019 18:03

ericrallen force-pushed the feature/a11y-wand-aria-hidden branch from ee94325 to e70cd13 Compare May 19, 2019 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(a11y-wand): add handling of nested aria-hidden elements #134

fix(a11y-wand): add handling of nested aria-hidden elements #134

ericrallen commented Mar 9, 2019

khanbot commented Mar 9, 2019

ericrallen commented Mar 9, 2019

khanbot commented Mar 9, 2019

ericrallen Mar 10, 2019

somewhatabstract Mar 15, 2019

ericrallen Mar 15, 2019

ericrallen Mar 15, 2019

somewhatabstract Mar 16, 2019

ericrallen Mar 16, 2019

ericrallen Mar 16, 2019

ericrallen Mar 16, 2019

somewhatabstract Mar 16, 2019

ericrallen Mar 16, 2019

ericrallen Mar 10, 2019

ericrallen commented Mar 13, 2019

somewhatabstract commented Mar 13, 2019 •

edited

Loading

somewhatabstract left a comment

ericrallen commented Mar 15, 2019

somewhatabstract commented Apr 22, 2019

ericrallen commented Apr 23, 2019

somewhatabstract commented May 17, 2019

ericrallen commented May 17, 2019

somewhatabstract commented May 18, 2019

somewhatabstract commented May 18, 2019 •

edited

Loading

somewhatabstract commented May 18, 2019

fix(a11y-wand): add handling of nested aria-hidden elements #134

Are you sure you want to change the base?

fix(a11y-wand): add handling of nested aria-hidden elements #134

Conversation

ericrallen commented Mar 9, 2019

khanbot commented Mar 9, 2019

ericrallen commented Mar 9, 2019

khanbot commented Mar 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericrallen commented Mar 13, 2019

somewhatabstract commented Mar 13, 2019 • edited Loading

somewhatabstract left a comment

Choose a reason for hiding this comment

ericrallen commented Mar 15, 2019

somewhatabstract commented Apr 22, 2019

ericrallen commented Apr 23, 2019

somewhatabstract commented May 17, 2019

ericrallen commented May 17, 2019

somewhatabstract commented May 18, 2019

somewhatabstract commented May 18, 2019 • edited Loading

somewhatabstract commented May 18, 2019

somewhatabstract commented Mar 13, 2019 •

edited

Loading

somewhatabstract commented May 18, 2019 •

edited

Loading