1374697 - Add ATK support for aria-details and aria-errormessage

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Joanmarie Diggs]

The core and MSAA/IA2 support were added as part of bug 1310794. We need to add the ATK mappings.

Comment 1

[Joanmarie Diggs]

Attachment: proposed patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8880038 [details] [diff] [review]
proposed patch

Review of attachment 8880038 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/Accessible.cpp
@@ +1779,5 @@
>  
>      case RelationType::ERRORMSG:
> +      // Authors MUST use aria-invalid in conjunction with aria-errormessage.
> +      // But it is easy enough for us to handle instances when they fail to.
> +      if (State() & states::INVALID)

it's ok for now, but State() is a heavy call, it'd be nice to have Invalid() method similar to Unavailable().

::: accessible/tests/mochitest/relations/test_general.html
@@ +425,5 @@
> +  <section id="error_false"></section>
> +  <input id="has_error_invalid_empty" aria-invalid="" aria-errormessage="error_empty">
> +  <section id="error_empty"></section>
> +  <input id="has_error_invalid_absent" aria-errormessage="error_absent">
> +  <section id="error_absent"></section>

it'd be nice to have a test case of natively invalid input, but simple <input type="email" value="hoho"> probably wont' work, so feel free to ignore it.

Comment 3

[Joanmarie Diggs]

Attachment: proposed patch - addresses review comments

(In reply to alexander :surkov from comment #2)
> Comment on attachment 8880038 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 8880038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.cpp
> @@ +1779,5 @@
> >  
> >      case RelationType::ERRORMSG:
> > +      // Authors MUST use aria-invalid in conjunction with aria-errormessage.
> > +      // But it is easy enough for us to handle instances when they fail to.
> > +      if (State() & states::INVALID)
> 
> it's ok for now, but State() is a heavy call, it'd be nice to have Invalid()
> method similar to Unavailable().

How about IsARIAInvalid()? In this particular case, I don't think we want to take native attributes into account -- unless they map to aria-invalid.

Anyway, this patch adds that method.

> ::: accessible/tests/mochitest/relations/test_general.html
> @@ +425,5 @@
> > +  <section id="error_false"></section>
> > +  <input id="has_error_invalid_empty" aria-invalid="" aria-errormessage="error_empty">
> > +  <section id="error_empty"></section>
> > +  <input id="has_error_invalid_absent" aria-errormessage="error_absent">
> > +  <section id="error_absent"></section>
> 
> it'd be nice to have a test case of natively invalid input, but simple
> <input type="email" value="hoho"> probably wont' work, so feel free to
> ignore it.

That it won't work (i.e. that there is no relationship resulting from the above user-agent validated and in error) is the expectation as I understand it. And that the relationship is absent because there is no aria-invalid attribute is certainly testable. Test case added.

Thanks for the review!

(Not sure if this counts as a "minor" change or not, so setting the r? flag again.)

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8880116 [details] [diff] [review]
proposed patch - addresses review comments

Review of attachment 8880116 [details] [diff] [review]:
-----------------------------------------------------------------

no actions is required, but this patch addresses two independent things:
1) atk implementation of aria-details and errormessage
2) aria-invalid thing

the hg/git history would be more readable if those were two separate bugs

::: accessible/generic/Accessible.h
@@ +299,5 @@
> +  bool IsARIAInvalid() const
> +  {
> +    uint64_t state = 0;
> +    ApplyARIAState(&state);
> +    return state & states::INVALID;

agreed, it's good idea to use common code paths, but ApplyARIAState has many extras, I'd love to avoid. aria-invalid is a global state, and thus

bool IsARIAInvalid() const
{
  return aria::UniversalStatesFor(mContent) & states::INVALID;
}

should do a trick too.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #3)

> > it'd be nice to have a test case of natively invalid input, but simple
> > <input type="email" value="hoho"> probably wont' work, so feel free to
> > ignore it.
> 
> That it won't work (i.e. that there is no relationship resulting from the
> above user-agent validated and in error) is the expectation as I understand
> it. And that the relationship is absent because there is no aria-invalid
> attribute is certainly testable. Test case added.

I didn't know I pointed out to the bug when I gave that example :) It feels a bit strange with me that aria-errormessage cannot be used with invalid HTML input semantcis, since usually ARIA is complimentary to HTML (or it was?), but it is ok, since you follow the spec.

> Thanks for the review!

yw :) thanks to you

> (Not sure if this counts as a "minor" change or not, so setting the r? flag
> again.)

please always r? if you are not sure, and skip it if you are

Comment 6

[Joanmarie Diggs]

(In reply to alexander :surkov from comment #5)

> I didn't know I pointed out to the bug when I gave that example :) It feels
> a bit strange with me that aria-errormessage cannot be used with invalid
> HTML input semantcis, since usually ARIA is complimentary to HTML (or it
> was?), but it is ok, since you follow the spec.

I *believe* something in one of the HTML specs needs to indicate there is an implicit mapping from <thing in their spec> to <thing in ARIA spec>. I've not found such a mapping or any related statement. But I searched some more and found this: https://github.com/w3c/html-aam/issues/47 :) I've since commented. Perhaps the fix is for them to map the failed validation to aria-invalid="true". And then we're set. :)

Related to this, along with a potential exit-criteria test failure related to sanity checking, I think I will split the sanity checking (and associated IsARIAInvalid() and test case we're discussing) out.

Comment 7

[Joanmarie Diggs]

Attachment: Patch for landing - ATK mappings only

This patch splits out the ATK support from the r+ed earlier patches. Bug 1375290 has been opened for the sanity-checking.

Since I don't seem to have editbugs privs yet, it would be great if this could be given checkin-needed. Thanks!

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #7)

> Since I don't seem to have editbugs privs yet, it would be great if this
> could be given checkin-needed. Thanks!

you probably need to request them, it'd be easier for all of us :)

Comment 9

[Joanmarie Diggs]

(In reply to alexander :surkov from comment #8)
> (In reply to Joanmarie Diggs from comment #7)
> 
> > Since I don't seem to have editbugs privs yet, it would be great if this
> > could be given checkin-needed. Thanks!
> 
> you probably need to request them, it'd be easier for all of us :)

I did (bug 1374652).

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #6)
> (In reply to alexander :surkov from comment #5)
> 
> > I didn't know I pointed out to the bug when I gave that example :) It feels
> > a bit strange with me that aria-errormessage cannot be used with invalid
> > HTML input semantcis, since usually ARIA is complimentary to HTML (or it
> > was?), but it is ok, since you follow the spec.
> 
> I *believe* something in one of the HTML specs needs to indicate there is an
> implicit mapping from <thing in their spec> to <thing in ARIA spec>.

So it's not on propose and not a result of deliberate decision of ARIA group. HTML a11y spec structure doesn't fit yet anything like pseudo styles and similar stuff.

> I've
> not found such a mapping or any related statement. But I searched some more
> and found this: https://github.com/w3c/html-aam/issues/47 :) I've since
> commented. Perhaps the fix is for them to map the failed validation to
> aria-invalid="true". And then we're set. :)

Yeah, I believe that HTML's invalid has same semantics as aria-invalid. I suppose they (we :) ) will fix that. What is strategy we take here? Go with ARIAInvalid() for now?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddea63224d2
Add ATK support for aria-details and aria-errormessage. r=surkov

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7ddea63224d2