fix(aria-errormessage): adds support for aria-errormessage #517

AutoSponge · 2017-09-09T23:02:29Z

closes #499
see https://www.w3.org/TR/wai-aria-1.1/#aria-errormessage

CLAassistant · 2017-09-09T23:02:36Z

All committers have signed the CLA.

AutoSponge · 2017-09-11T19:40:55Z

I noticed #497 (comment); if you need me to retarget this PR to a feature branch (not develop), have a maintainer create the target branch and I will update this PR. This PR can be accepted then someone can create a new PR from feature TBD to develop.

marcysutton

Targeting develop is fine. We'll cherry-pick it onto develop-2x too so the changes go into 2x as well as 3x.

The test will need .only removed, and it would be good to link to the spec documentation in your commit.

Can you add some DOM elements to the integration tests? I just had to do something similar: 9b4d2ee

marcysutton · 2017-09-11T20:29:48Z

test/checks/aria/valid-attr-value.js

@@ -60,6 +60,23 @@ describe('aria-valid-attr-value', function () {
 		assert.isNull(checkContext._data);
 	});

+	it.only('should return true if aria-errormessage id is alert or aria-describedby', function () {


Need to remove the .only here.

ah. development artifact.

WilcoFiers · 2017-09-12T10:42:01Z

lib/commons/aria/attributes.js

+ * @param  {String} value The value of the attribute
+ * @return {Boolean}
+ */
+aria.validateIdrefType = function (doc, node, attr, value) {


The logic looks excellent to me. What I'm wondering is if there's a better way of communicating what's going on to users. The proposed implementation doesn't give any sort of feedback about why in one case aria-errormessage is passed and in another it is failed. It'd be great if we could expose that somehow. I'm not too sure how we'd go about doing that though... Perhaps we could have a separate check for aria-errormessage? It could be a none check, that raises an error if none of these three cases are met. What do you think?

So, if this fn throws and I include a test for throws...

throw new Error('The value of aria-errormessage must be supported using an additional technique that announces the message (e.g., aria-live, aria-describedby, role=alert, etc.)')

Is that enough? Too much?

The tests are passing. The only thing I haven't tried to do is:

Perhaps we could have a separate check for aria-errormessage?

Given my lack of familiarity with the testing harness, I'm not sure how I would go about that. Is it possible to make that an additional feature?

dylanb · 2017-09-12T12:43:13Z

@AutoSponge can you also remove the wcag2.1 from the commit message when you make this change

marcysutton · 2017-09-12T15:02:40Z

I asked him to put a link to the 2.1 spec in the commit, I should have been more specific. The commit body has a lot more room for stuff like that. The first line of the commit message should be under 100 characters.

AutoSponge · 2017-09-12T22:48:27Z

@marcysutton I was thinking of aria1.1 but typed wcag2.1. I'll fix it.

AutoSponge · 2017-09-13T12:53:21Z

Circle CI isn't showing me the failure (test section has infinite spinner) and I don't have permissions to trigger a build.

dylanb · 2017-09-13T13:03:46Z

@AutoSponge

 1) aria-allowed-attr aria-allowed-attr passing tests passes should find ["#pass69"]:
     Element not found
  2) aria-allowed-attr aria-allowed-attr passing tests passes should find ["#pass70"]:
     Element not found
  3) aria-allowed-attr aria-allowed-attr passing tests passes should find ["#pass71"]:
     Element not found

WilcoFiers · 2017-10-04T10:13:52Z

@AutoSponge Any updates on this?

AutoSponge · 2017-10-04T12:59:41Z

@WilcoFiers Github hasn't helped with the order of comments. My last comment was ~3 weeks ago:

The tests are passing. The only thing I haven't tried to do is:

Perhaps we could have a separate check for aria-errormessage?

Given my lack of familiarity with the testing harness, I'm not sure how I would go about that. Is it possible to make that an additional feature?

I was waiting on input from you guys but @dylanb said you were on vacation so I didn't bug you :)

Integration tests are failing, but I couldn't figure out why. The union of the two checks seems to be causing problems

marcysutton · 2017-10-12T00:24:55Z

I do think we need a new aria-errormessage check to provide appropriate feedback to the user.

I started to fix this but I got hung up on the integration tests. The two checks are interfering with each other...it's unclear to me what exactly is happening. @AutoSponge you can see what I did here: 4ae3fc3?diff=unified

I have to move on to other stuff, so if someone else can pick this up that would help a lot!

AutoSponge · 2017-10-13T02:44:28Z

@marcysutton I commented on your commit but it doesn't show here. So, just commenting to make sure you see: 4ae3fc3?diff=unified. When I made those changes it worked locally but I can't push to your branch.

AutoSponge · 2017-10-14T16:05:45Z

@marcysutton I realized I could merge your changes here :)

marcysutton · 2017-10-25T01:21:49Z

Thanks for the contribution, @AutoSponge!

* fix(aria-errormessage): adds support for aria-errormessage * fix(aria-valid-attr-value): switches from `any` mode to `all`

…equelabs#517) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

marcysutton suggested changes Sep 11, 2017

View reviewed changes

WilcoFiers reviewed Sep 12, 2017

View reviewed changes

dylanb changed the title ~~fix(aria-errormessage): adds support for wcag2.1 aria-errormessage~~ fix(aria-errormessage): adds support for aria-errormessage Sep 12, 2017

AutoSponge force-pushed the fix-aria-errormessage branch 2 times, most recently from 0d1c7d5 to 9361359 Compare September 13, 2017 00:02

AutoSponge force-pushed the fix-aria-errormessage branch 6 times, most recently from 6a5baa4 to 836e5a8 Compare September 14, 2017 22:32

fix(aria-errormessage): adds support for aria-errormessage

cb8e39c

AutoSponge force-pushed the fix-aria-errormessage branch from 836e5a8 to cb8e39c Compare September 14, 2017 22:40

WIP: migrate aria-errormessage to its own check

ae6d887

Integration tests are failing, but I couldn't figure out why. The union of the two checks seems to be causing problems

fix(aria-valid-attr-value): switches from any mode to all

7781f7d

marcysutton approved these changes Oct 25, 2017

View reviewed changes

marcysutton merged commit c96f58c into dequelabs:develop Oct 25, 2017

marcysutton pushed a commit that referenced this pull request Oct 25, 2017

fix(aria-errormessage): adds support for aria-errormessage (#517)

caf9851

* fix(aria-errormessage): adds support for aria-errormessage * fix(aria-valid-attr-value): switches from `any` mode to `all`

marcysutton mentioned this pull request Oct 25, 2017

aria-errormessage for the 2x branch #583

Merged

marcysutton pushed a commit that referenced this pull request Oct 25, 2017

fix(aria-errormessage): adds support for aria-errormessage (#517)

465d41d

* fix(aria-errormessage): adds support for aria-errormessage * fix(aria-valid-attr-value): switches from `any` mode to `all`

WilcoFiers mentioned this pull request Aug 5, 2018

False positive: 'aria-allowed-attr' fails when using 'aria-errormessage' attribute #1043

Closed

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

chore(deps): bump ansi-regex from 3.0.0 to 3.0.1 in /axe_core_test (d…

c67f3cc

…equelabs#517) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aria-errormessage): adds support for aria-errormessage #517

fix(aria-errormessage): adds support for aria-errormessage #517

AutoSponge commented Sep 9, 2017 •

edited

Loading

CLAassistant commented Sep 9, 2017 •

edited

Loading

AutoSponge commented Sep 11, 2017 •

edited

Loading

marcysutton left a comment

marcysutton Sep 11, 2017

AutoSponge Sep 12, 2017

WilcoFiers Sep 12, 2017

AutoSponge Sep 12, 2017

AutoSponge Sep 14, 2017

dylanb commented Sep 12, 2017

marcysutton commented Sep 12, 2017 •

edited

Loading

AutoSponge commented Sep 12, 2017

AutoSponge commented Sep 13, 2017

dylanb commented Sep 13, 2017

WilcoFiers commented Oct 4, 2017

AutoSponge commented Oct 4, 2017

marcysutton commented Oct 12, 2017 •

edited

Loading

AutoSponge commented Oct 13, 2017

AutoSponge commented Oct 14, 2017 •

edited

Loading

marcysutton commented Oct 25, 2017

fix(aria-errormessage): adds support for aria-errormessage #517

fix(aria-errormessage): adds support for aria-errormessage #517

Conversation

AutoSponge commented Sep 9, 2017 • edited Loading

CLAassistant commented Sep 9, 2017 • edited Loading

AutoSponge commented Sep 11, 2017 • edited Loading

marcysutton left a comment

Choose a reason for hiding this comment

marcysutton Sep 11, 2017

Choose a reason for hiding this comment

AutoSponge Sep 12, 2017

Choose a reason for hiding this comment

WilcoFiers Sep 12, 2017

Choose a reason for hiding this comment

AutoSponge Sep 12, 2017

Choose a reason for hiding this comment

AutoSponge Sep 14, 2017

Choose a reason for hiding this comment

dylanb commented Sep 12, 2017

marcysutton commented Sep 12, 2017 • edited Loading

AutoSponge commented Sep 12, 2017

AutoSponge commented Sep 13, 2017

dylanb commented Sep 13, 2017

WilcoFiers commented Oct 4, 2017

AutoSponge commented Oct 4, 2017

marcysutton commented Oct 12, 2017 • edited Loading

AutoSponge commented Oct 13, 2017

AutoSponge commented Oct 14, 2017 • edited Loading

marcysutton commented Oct 25, 2017

AutoSponge commented Sep 9, 2017 •

edited

Loading

CLAassistant commented Sep 9, 2017 •

edited

Loading

AutoSponge commented Sep 11, 2017 •

edited

Loading

marcysutton commented Sep 12, 2017 •

edited

Loading

marcysutton commented Oct 12, 2017 •

edited

Loading

AutoSponge commented Oct 14, 2017 •

edited

Loading