559275 - map attribute required to STATE_REQUIRED

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 345822.

Mounir, what input types is @required attribute applicable to?

Comment 1

[Mounir Lamouri (:mounir)]

It apply for all types except: hidden, button, image, submit, rest and two which are not implemented yet: color and range.

If you have access to nsHTMLInputElement, you can use 'DoesRequiredApply()' and you will not have to guess if the type accepts @required ;)

By the way, @required is applicable for textarea element too.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Mounir I don't think we have access to nsHTMLInputElement. Do you know of a public interface we can use? Maybe there should be a virtual PRBool IsRequired on nsIFormControl or nsIDOMHTMLInputElement?

Comment 3

[Mounir Lamouri (:mounir)]

No need to have IsRequired.
You can use NS_EVENT_STATE_REQUIRED or the required attribute (which is in nsIDOMHTMLInputElement).

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> No need to have IsRequired.
> You can use NS_EVENT_STATE_REQUIRED or the required attribute (which is in
> nsIDOMHTMLInputElement).

Oups, I did not see my own comment (comment 2). I think NS_EVENT_STATE_REQUIRED would be fine because it will not apply on types which can't be required.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

We really need the ability to query for the 'required' state. Are there any instances of nsIDOMHTMLInputElement for which the required attribute does not apply?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> We really need the ability to query for the 'required' state. Are there any
> instances of nsIDOMHTMLInputElement for which the required attribute does not
> apply?

Yes, for some types, see comment 2.

But NS_EVENT_STATE_REQUIRED, if set, means that the element is currently required. If NS_EVENT_STATE_OPTIONAL is set, that means the elemnet is currently optional (which means it could be required). If none are sot, for <input>, that means the element can't be required.
That sounds the easiest way to do it.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh I see. I thought that was some at-event-time-only thing.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

OK easy fix.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

(removed unnecessary include)

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 478022 [details] [diff] [review]
patch


>+ACCESSIBILITY_ATOM(required, "required") // HTML5 inputs

I don't see where it's used

>+
>+    // expose 'required' state for inputs
>+    if (mContent->IntrinsicState() & NS_EVENT_STATE_REQUIRED) {
>+        *aState |= nsIAccessibleStates::STATE_REQUIRED;
>+    }

nit: no braces around single if
nit: two space indent, you have four.

this place is not ready to add this code because it's about disabled state, not about HTML specific states. I'd like to see it reworked in some nice way.

btw, it might be nice to start to use IntristicState() instead of disabled attribute check.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> btw, it might be nice to start to use IntristicState() instead of disabled
> attribute check.

Since bug 557087 has been fixed, it's would be more than nice, it's required ;)
I've open bug 599163 for that.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (In reply to comment #10)
> > btw, it might be nice to start to use IntristicState() instead of disabled
> > attribute check.
> 
> Since bug 557087 has been fixed, it's would be more than nice, it's required ;)
> I've open bug 599163 for that.

thank you. But while we are here, does it work for XUL as well?

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > btw, it might be nice to start to use IntristicState() instead of disabled
> > > attribute check.
> > 
> > Since bug 557087 has been fixed, it's would be more than nice, it's required ;)
> > I've open bug 599163 for that.
> 
> thank you. But while we are here, does it work for XUL as well?

If you are speaking of the disabled state, I've just wrote a patch which is going to make that working in XUL. Related bug: bug 598329.

Comment 14

[alexander :surkov (:asurkov)]

yes, thanks again

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 478022 [details] [diff] [review]
patch

>+  <title>Test document hierarchy</title>

Nit: Please change the title to "HTML5 inputs" or something to that end to reflect the actual tests being run.

r=me thanks!

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #10)
> >+ACCESSIBILITY_ATOM(required, "required") // HTML5 inputs
> 
> I don't see where it's used

Good catch - it is cruft.

> 
> >+
> >+    // expose 'required' state for inputs
> >+    if (mContent->IntrinsicState() & NS_EVENT_STATE_REQUIRED) {
> >+        *aState |= nsIAccessibleStates::STATE_REQUIRED;
> >+    }
> 
> nit: no braces around single if

I don't think I will ever get used to no braces ;)

> this place is not ready to add this code because it's about disabled state, not
> about HTML specific states. I'd like to see it reworked in some nice way.

I don't understand.

> btw, it might be nice to start to use IntristicState() instead of disabled
> attribute check.

Yep :)

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #15)
> Comment on attachment 478022 [details] [diff] [review]
> patch
> 
> >+  <title>Test document hierarchy</title>
> 
> Nit: Please change the title to "HTML5 inputs" or something to that end to
> reflect the actual tests being run.

Done.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> > nit: no braces around single if
> 
> I don't think I will ever get used to no braces ;)

isn't it nicer? ;)

> 
> > this place is not ready to add this code because it's about disabled state, not
> > about HTML specific states. I'd like to see it reworked in some nice way.
> 
> I don't understand.

we have something like

// Expose disabled state
if (isHTML()) {
  disabled = isHTMLDIsabled();
} else {
  disabled = isXULDisabled();
}
if (disabled) {
  setDisabledState();
}

you do

// Expose disabled state
if (isHTML()) {
  disabled = isHTMLDIsabled();
  if (isHTMLRequired())
    setRequired();
} else {
  disabled = isXULDisabled();
}
...

I mean at least you should change comments to make this hunk of code not about disabled state only, but perhaps it makes sense to rethink it at all. For example, does it makes sense to check HTML namespace before to check IntristicState() & REQUIRED, taking into account ongoing disabled state changes (bug 599163). Btw, it might make sense to work on these bugs the same time in one patch.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

(In reply to comment #18)
Got you. Agreed :)

Note I have a simple patch for intrinsic invalid state too, but would rather do that on another bug, with additional tests.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh hmmm we don't have xul disabled state tests.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 478808 [details] [diff] [review]
patch 2


>   // unavailable
>   if (state & STATE_UNAVAILABLE) {
>-    var role = getRole(aAccOrElmOrID);
>-    if (role != ROLE_GROUPING && role != ROLE_EMBEDDED_OBJECT)
>-      isState(state & STATE_FOCUSABLE, STATE_FOCUSABLE, false,
>-              "Disabled " + id + " must be focusable!");
>+    isState(state & STATE_FOCUSABLE, 0, false,
>+            "Disabled " + id + " s not be focusable!");

I must miss something, it sounds disabled state code logic is preserved (not taking into account that XUL bug) but test changes has opposite logic. Could you comment this please?

Comment 22

[Mounir Lamouri (:mounir)]

Btw, XUL elements will not support :disabled for performance reasons. (follow-up from comments 12 and 13)

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Btw, XUL elements will not support :disabled for performance reasons.
> (follow-up from comments 12 and 13)

so, we can't get disabled state from IntristicStates(), correct?

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> (In reply to comment #22)
> > Btw, XUL elements will not support :disabled for performance reasons.
> > (follow-up from comments 12 and 13)
> 
> so, we can't get disabled state from IntristicStates(), correct?

For HTML elements you can. For XUL elements, you will have to check that @disable='true' but I guess you are already doing that.

Comment 25

[alexander :surkov (:asurkov)]

right, thanks

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #21)
> Comment on attachment 478808 [details] [diff] [review]
> patch 2
> 
> 
> >   // unavailable
> >   if (state & STATE_UNAVAILABLE) {
> >-    var role = getRole(aAccOrElmOrID);
> >-    if (role != ROLE_GROUPING && role != ROLE_EMBEDDED_OBJECT)
> >-      isState(state & STATE_FOCUSABLE, STATE_FOCUSABLE, false,
> >-              "Disabled " + id + " must be focusable!");
> >+    isState(state & STATE_FOCUSABLE, 0, false,
> >+            "Disabled " + id + " s not be focusable!");
> 
> I must miss something, it sounds disabled state code logic is preserved (not
> taking into account that XUL bug) but test changes has opposite logic. Could
> you comment this please?

I think the original isState check was never exercised before. The only existing test we have for STATE_UNAVAILABLE was for an embedded object which bails out before this check.

What were you intending to test here originally?

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh nevermind. Was thinking it was an 'aState' check.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 3

Comment 29

[Marco Zehe (:MarcoZ)]

Comment on attachment 479476 [details] [diff] [review]
patch 3

Thanks for the additional tests, these look good!

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 479476 [details] [diff] [review]
patch 3

Marco, I should get your updated r+ since this patch has grown.

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 479476 [details] [diff] [review]
patch 3


>-  else if (mContent->IsElement()) {
>-    nsIFrame *frame = GetFrame();
>-    if (frame && frame->IsFocusable()) {
>-      *aState |= nsIAccessibleStates::STATE_FOCUSABLE;
>-    }
>-
>-    if (gLastFocusedNode == mContent) {
>-      *aState |= nsIAccessibleStates::STATE_FOCUSED;
>+    if (mContent->IsElement()) {
>+      nsIFrame *frame = GetFrame();
>+      if (frame && frame->IsFocusable()) {
>+        *aState |= nsIAccessibleStates::STATE_FOCUSABLE;
>+      }
>+  
>+      if (gLastFocusedNode == mContent) {
>+        *aState |= nsIAccessibleStates::STATE_FOCUSED;
>+      }

I don't see why this change is.

>+  // Note: when we propogate aria-disabled down to descendants, we don't
>+  // currently unset the focusable state. Therefore we don't have a consistency
>+  // check here regarding STATE_UNAVAILABLE->!STATE_FOCUSABLE
> }

it sounds we shouldn't, moreover, we propagate state disabled to focusable items only. see bug 429285.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #31)
> Comment on attachment 479476 [details] [diff] [review]
> patch 3
> 
> 
> >-  else if (mContent->IsElement()) {
> 
> I don't see why this change is.

Ah yes I think I can preserve the original blame here.

> 
> >+  // Note: when we propogate aria-disabled down to descendants, we don't
> >+  // currently unset the focusable state. Therefore we don't have a consistency
> >+  // check here regarding STATE_UNAVAILABLE->!STATE_FOCUSABLE
> > }
> 
> it sounds we shouldn't, moreover, we propagate state disabled to focusable
> items only. see bug 429285.

I think we need to trust the intrinsic state which will sometimes marry unavailable and !focusable.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> I think we need to trust the intrinsic state which will sometimes marry
> unavailable and !focusable.

that's ok for intristic case, but we have different logic for ARIA disabled.

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 4

This patch improves blame preservation and covers bug 429285 under a more specific test. Prior to this patch we would have enforced disabled state to require focusable state which was strange, but wasn't an issue by shear luck.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 480128 [details] [diff] [review]
patch 4


>+  // set unavailable state based on disabled state, otherwise set focus states

nit: capital 'S' letter, point in the end of sentence.

>-  // unavailable
>-  if (state & STATE_UNAVAILABLE) {
>-    var role = getRole(aAccOrElmOrID);
>-    if (role != ROLE_GROUPING && role != ROLE_EMBEDDED_OBJECT)
>-      isState(state & STATE_FOCUSABLE, STATE_FOCUSABLE, false,
>-              "Disabled " + id + " must be focusable!");
>-  }

>   <script type="application/javascript">
>+    function testAriaDisabledTree(aAccOrElmOrID)
>+    {

can we make this a part of testStatesInSubtree(), for example, by providing additional argument on testing function? Especially it makes sense because testStatesInSubtree is used for disabled state tests only. I like the idea to keep it "pure" but perhaps it's really worth to combine these functions.

btw, this function doesn't care about text leafs, it should I guess.


>+      <a target="_blank"
>+         href="https://bugzilla.mozilla.org/show_bug.cgi?id=559275"
>+         title="map attribute required to STATE_REQUIRED">
>+        Mozilla Bug 559275

nit: technically xul test is not about required attribute

r=me, please feel to rereqeust review for testStatesInSubtree code if you think it's necessary.

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #35)

Thanks for nits.

> >+    function testAriaDisabledTree(aAccOrElmOrID)
> >+    {
> 
> can we make this a part of testStatesInSubtree(), for example, by providing
> additional argument on testing function?

I had thought of doing this (it is less work for me), but I prefer not since it is so specific, and introduces ugly code to a util function. Do you feel strongly about it?

Comment 37

[alexander :surkov (:asurkov)]

No, but it might be nice

function testStatesInSubtree(aID, aState, aExtraState, aSpecificTest)
{
  var acc = getAccessible(aID);
  var [state, extState= getStates(acc);
  if (aSpecificTest)
   aSpecificTest.call(acc, state, extState);

  ...
}

function testARIADisabledState(aAcc, aState, aExtraState)
{
  ...
}

testStatesIsSubtree("group", STATE_UNAVAILABLE, 0, testARIADisabledState);

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/d925cd5764b5