733382 - editable state bit should be presented on readonly inputs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

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

Jamie, would removing the editable state in this case gain us anything? Would we lose anything?

Alexander, what do other browsers do?

Comment 3

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

Personally I think the disabled state turns the other states into "would be" states. For example STATE_CHECKED - would we remove that too?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)
> Alexander, what do other browsers do?

not sure if it gives something since we could refer to IE if it implemented IA2. Webkit mostly copied Firefox for ATK.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #3)
> Personally I think the disabled state turns the other states into "would be"
> states. For example STATE_CHECKED - would we remove that too?

not STATE_CHECKED, but probably STATE_CHECKABLE

but this is a good point, it might be good to keep "would be" state regardless of unavailable state. Same applicable for editable state iif editable state is not sort of special state.

Comment 6

[alexander :surkov (:asurkov)]

IA2 and ATK says:
Indicates the user can change the contents of this object.
IA2 link:
http://accessibility.linuxfoundation.org/a11yspecs/ia2/archive/ia2-1.0.1/docs/html/group__grp_states.html#g0ae029f7da91857b56b46309876feddc
ATK link:
http://www.pygtk.org/docs/pygtk/atk-constants.html#atk-state-type-constants

we remove editable state bit when readonly state is presented starting from big reorg bug 340829. If we want to be consistent and follow the specs then this bug as it's summarized should be fixed.

Comment 7

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

How does this work in UIA? (control types/patterns)

Does a disabled state block "Control Pattern Availability Property Identifiers"?

Comment 8

[James Teh [:Jamie]]

This one is tricky.
1. The editable state is probably redundant for controls that are inherently editable but aren't marked as read-only; e.g. editable text fields.
2. However, for controls which aren't inherently editable, the editable state is possibly more important so you know they might become editable in the future; e.g.
<p contenteditable="true" aria-disabled="true">blah</p>
If this bug is fixed, that paragraph will lose its editable state, but it'll just look like a normal paragraph to us instead of a paragraph that could become editable.
3. Unfortunately, to make things even more confusing, a control with ROLE_SYSTEM_TEXT but STATE_SYSTEM_READONLY that isn't focusable is a text leaf node. So, if we have:
<input type="text" readonly disabled>
Now we can't tell that this is actually a disabled read-only editable text field instead of a disabled text leaf node.
4. You could argue that if the disabled state is present on a control that might be editable, it should be assumed that it may become editable. So, for (2) and (3), because the disabled state is present and normally wouldn't be if the controls weren't editable, we should treat them as disabled editable controls. However, imo, this is really confusing.

For reference, NVDA doesn't use the editable state for editable text fields at present. However, this doesn't make the issues above any less valid for correctness.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #7)
> How does this work in UIA? (control types/patterns)

Neither IA2 nor ATK should be driven by UIA. These specs should be driven by what is reasonable and what AT need.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #8)
> This one is tricky.
> 1. The editable state is probably redundant for controls that are inherently
> editable but aren't marked as read-only; e.g. editable text fields.

that's interesting because we do unperformant things to expose that. Don't you need to know if the user is inside the editable area? Should AT be always driven by focus so you need to know editable state on editable container only.

> 2. However, for controls which aren't inherently editable, the editable
> state is possibly more important so you know they might become editable in
> the future; e.g.
> <p contenteditable="true" aria-disabled="true">blah</p>
> If this bug is fixed, that paragraph will lose its editable state, but it'll
> just look like a normal paragraph to us instead of a paragraph that could
> become editable.
> 3. Unfortunately, to make things even more confusing, a control with
> ROLE_SYSTEM_TEXT but STATE_SYSTEM_READONLY that isn't focusable is a text
> leaf node. So, if we have:
> <input type="text" readonly disabled>
> Now we can't tell that this is actually a disabled read-only editable text
> field instead of a disabled text leaf node.

Why would you need to distinguish potentially editable paragraph or textbox from ordinal paragraph and text?

How is "setting @contenteditable attribute on ordinal paragraph" different from "removing @aria-hidden attribute on contenteditable paragraph"? It sounds like from user perspective these are equivalent things.

Comment 11

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #10)
> > 1. The editable state is probably redundant for controls that are inherently
> > editable but aren't marked as read-only; e.g. editable text fields.
> that's interesting because we do unperformant things to expose that. Don't
> you need to know if the user is inside the editable area?
Of course, but an editable text field (ROLE_SYSTEM_TEXT without the read-only state) is inherently editable (as opposed to, say, a paragraph). That's all I mean. I don't see any reason to change the current behaviour, though.

> Should AT be
> always driven by focus so you need to know editable state on editable
> container only.
We need to know the editable state on anything that isn't inherently editable (e.g. editable paragraph). Otherwise, we don't know it's editable. This is needed for all objects, as the user might be exploring with the review cursor.

> Why would you need to distinguish potentially editable paragraph or textbox
> from ordinal paragraph and text?
So the AT knows the user can potentially interact with the former.

> How is "setting @contenteditable attribute on ordinal paragraph" different
> from "removing @aria-hidden attribute on contenteditable paragraph"? It
> sounds like from user perspective these are equivalent things.
I don't understand the question, particularly the reference to aria-hidden. :(

Comment 12

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #11)
> (In reply to alexander :surkov from comment #10)
> > > 1. The editable state is probably redundant for controls that are inherently
> > > editable but aren't marked as read-only; e.g. editable text fields.
> > that's interesting because we do unperformant things to expose that. Don't
> > you need to know if the user is inside the editable area?
> Of course, but an editable text field (ROLE_SYSTEM_TEXT without the
> read-only state) is inherently editable (as opposed to, say, a paragraph).

I'm not sure what "inherently editable" means from implementation point of view.

do you mean that paragraph should expose editable state if it's a part of editable container?

> That's all I mean. I don't see any reason to change the current behaviour,
> though.
> 
> > Should AT be
> > always driven by focus so you need to know editable state on editable
> > container only.
> We need to know the editable state on anything that isn't inherently
> editable (e.g. editable paragraph). Otherwise, we don't know it's editable.
> This is needed for all objects, as the user might be exploring with the
> review cursor.

ok, I see.

> > Why would you need to distinguish potentially editable paragraph or textbox
> > from ordinal paragraph and text?
> So the AT knows the user can potentially interact with the former.
> 
> > How is "setting @contenteditable attribute on ordinal paragraph" different
> > from "removing @aria-hidden attribute on contenteditable paragraph"? It
> > sounds like from user perspective these are equivalent things.
> I don't understand the question, particularly the reference to aria-hidden.
> :(

I meant aria-disabled. I wanted to say that
<p> + p.setAttribute("contentEditable", "true") is sort of equivalent to
<p contentEditable="true" aria-disabled="true"> + p.removeAttribute("aria-disabled").

Comment 13

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #12)
> > > > 1. The editable state is probably redundant for controls that are inherently
> > > > editable but aren't marked as read-only; e.g. editable text fields.
> I'm not sure what "inherently editable" means from implementation point of
> view.
ROLE_SYSTEM_TEXT without read-only is probably the only inherently editable control as far as text is concerned. I mean that if you have no read-only state, ATs tend to assume that ROLE_SYSTEM_TEXT is editable even if the editable state isn't present (there was no editable state in MSAA). This is probably more a point of interest than anything that needs ot be changed. Sorry for the confusion.

> do you mean that paragraph should expose editable state if it's a part of
> editable container?
I'm pretty sure it does already. It should continue to do so.

> I meant aria-disabled. I wanted to say that
> <p> + p.setAttribute("contentEditable", "true") is sort of equivalent to
> <p contentEditable="true" aria-disabled="true"> +
> p.removeAttribute("aria-disabled").
I guess I see them as different because the former creates a normal paragraph and then makes it editable, whereas the latter creates a disabled editable paragraph and then enables it. I guess I'm arguing that an editable paragraph is different to a normal paragraph, even when it's disabled, similar to the way an editable text control is still an editable text control even when it is disabled. However, maybe that's not really the case.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #13)
> > do you mean that paragraph should expose editable state if it's a part of
> > editable container?
> I'm pretty sure it does already. It should continue to do so.

out of scope of the bug but let's continue discussion.
what about other elements like html:img or html:button placed inside editable area. Should they expose editable state or not?

Jamie, would you mind to file a request to IA2 group to change the spec? I'm fine to treat controls as editable, checkable, whatever else able (expect perhaps focusable since it look like it's opposite to unavailable state) when they are disabled if you need that.

Comment 15

[alexander :surkov (:asurkov)]

I filed question to ATK group (http://mail.gnome.org/archives/gnome-accessibility-devel/2012-March/msg00003.html).

Comment 16

[alexander :surkov (:asurkov)]

Hm, that's sort of strange if we start to expose editable state on readonly textboxes. But if we don't then it's strange if we expose it on disabled textboxes.

Comment 17

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #6)
> IA2 and ATK says:
> Indicates the user can change the contents of this object.
Actually, IA2 now says:
> An object with this state has a caret and implements the IAccessibleText interface.
> Such fields may be read-only, so STATE_SYSTEM_READONLY is valid in combination with IA2_STATE_EDITABLE.
http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/_accessible_states_8idl.html#afecbfb195aaf7050dc8f75a7833bd5f2e08a5b43eb5b522c96851cee9f7e271
I don't think this conflicts with the disabled state.

(In reply to alexander :surkov from comment #16)
> Hm, that's sort of strange if we start to expose editable state on readonly
> textboxes.
See the description above. Perhaps the editable state would be better described as an interactiveText or explorableText state. :)

Comment 18

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #17)
> Actually, IA2 now says:
For reference, this changed in IA2 1.2.1.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #17)
> (In reply to alexander :surkov from comment #6)
> > IA2 and ATK says:
> > Indicates the user can change the contents of this object.
> Actually, IA2 now says:
> > An object with this state has a caret and implements the IAccessibleText interface.
> > Such fields may be read-only, so STATE_SYSTEM_READONLY is valid in combination with IA2_STATE_EDITABLE.
> http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/
> _accessible_states_8idl.
> html#afecbfb195aaf7050dc8f75a7833bd5f2e08a5b43eb5b522c96851cee9f7e271
> I don't think this conflicts with the disabled state.

conflicts, disabled textbox doesn't have a caret.

> (In reply to alexander :surkov from comment #16)
> > Hm, that's sort of strange if we start to expose editable state on readonly
> > textboxes.
> See the description above. Perhaps the editable state would be better
> described as an interactiveText or explorableText state. :)

ah, old docs :) thank you for pointing it out. 

interactiveText - no since disabled is not interactive. explorableText - not sure what it means

Comment 20

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #19)
> > I don't think this conflicts with the disabled state.
> conflicts, disabled textbox doesn't have a caret.
No, but it would if it weren't disabled.

To take your logic to extremes for the sake of example, when a button is disabled, one might argue that it is not a button any more because it doesn't behave like a button; i.e. you can't click it and you can't focus it. The point is that it's still a button; it's just disabled. If it was enabled, normal behaviour would resume. The same is true for an editable text control. It's still an editable text control; it's just disabled.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #20)
> (In reply to alexander :surkov from comment #19)
> > > I don't think this conflicts with the disabled state.
> > conflicts, disabled textbox doesn't have a caret.
> No, but it would if it weren't disabled.

then spec should say this like "has a caret of would have a caret if it weren't disabled"

> To take your logic to extremes for the sake of example, when a button is
> disabled, one might argue that it is not a button any more because it
> doesn't behave like a button; i.e. you can't click it and you can't focus
> it. The point is that it's still a button; it's just disabled. If it was
> enabled, normal behaviour would resume. The same is true for an editable
> text control. It's still an editable text control; it's just disabled.

you can extreme examples for any logic like let's expose editable and disabled state on ordinal paragraph because it becomes editable if somebody put contentEditable on it. But it doesn't prove that the logic was weird.

Comment 22

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #21)
> then spec should say this like "has a caret of would have a caret if it
> weren't disabled"
How far do we go, though? Does IA2_ROLE_CHECK_MENU_ITEM need to document that it isn't really checkable while it's disabled? The disabled state is the abnormal case here.

> you can extreme examples for any logic like let's expose editable and
> disabled state on ordinal paragraph because it becomes editable if somebody
> put contentEditable on it.
The disabled state disables the normal functionality of the control. Disabled is therefore the abnormal case. You wouldn't bother disabling a normal paragraph, but you might disable an editable paragraph. That suggests you are modifying its normal functionality, but its normal functionality isn't clear without the editable state. I think of role paragraph with state editable as "an editable paragraph", like role text without read-only is "an editable text field". Disabled then "disables" that control.

To be honest, I don't really have a strong enough opinion on this to push too hard if it's decided this change should be implemented. It probably won't affect us or our users too much, as editable paragraphs, etc. aren't too common in the middle of normal content.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #22)
> (In reply to alexander :surkov from comment #21)
> > then spec should say this like "has a caret of would have a caret if it
> > weren't disabled"
> How far do we go, though? Does IA2_ROLE_CHECK_MENU_ITEM need to document
> that it isn't really checkable while it's disabled? The disabled state is
> the abnormal case here.

I'd say 'able' state should mean potentially able except the focusable thing. So disabled checkbox should has checkable state. Otherwise I'm feeling like we get more or more mess.

> > you can extreme examples for any logic like let's expose editable and
> > disabled state on ordinal paragraph because it becomes editable if somebody
> > put contentEditable on it.
> The disabled state disables the normal functionality of the control.
> Disabled is therefore the abnormal case. You wouldn't bother disabling a
> normal paragraph, but you might disable an editable paragraph. That suggests
> you are modifying its normal functionality, but its normal functionality
> isn't clear without the editable state. I think of role paragraph with state
> editable as "an editable paragraph", like role text without read-only is "an
> editable text field". Disabled then "disables" that control.
> 
> To be honest, I don't really have a strong enough opinion on this to push
> too hard if it's decided this change should be implemented. It probably
> won't affect us or our users too much, as editable paragraphs, etc. aren't
> too common in the middle of normal content.

Well, I didn't mean that's something it makes sense to do. I just exampled that extreme examples might be not good :)

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 25

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #24)
> Created attachment 603701 [details] [diff] [review]
> patch

btw, the patch reworks the fix from bug 467387

Comment 26

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

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

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

::: accessible/src/base/nsARIAMap.cpp
@@ +828,5 @@
> +  // Non ARIA attribute case. Expose default state until excluding state is
> +  // presented.
> +  if (!entry.mAttributeName) {
> +    if (!(*aState & entry.mExcludingState))
> +      *aState |= entry.mDefaultState;

Not sure if better, but you should be able to replace the last two lines with:
*aState ^= entry.mExcludingState;

Comment 27

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #26)
> > +  if (!entry.mAttributeName) {
> > +    if (!(*aState & entry.mExcludingState))
> > +      *aState |= entry.mDefaultState;
> 
> Not sure if better, but you should be able to replace the last two lines
> with:
> *aState ^= entry.mExcludingState;

that removes entry.mExcludingState if it was presented or adds if it wasn't. That's not equivalent to the code.

Comment 28

[Trevor Saunders (:tbsaunde)]

fyi

Comment 29

[Trevor Saunders (:tbsaunde)]

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

Alex I seem to remember you pushing for cleanup of the aria mapping stuff the last time someone tried to do something like this, but I gues this doesn't make things too much worse so whatever.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #29)
> Comment on attachment 603701 [details] [diff] [review]
> patch
> 
> Alex I seem to remember you pushing for cleanup of the aria mapping stuff
> the last time someone tried to do something like this, but I gues this
> doesn't make things too much worse so whatever.

hmm, would be nice to have a bug number. I don't recall somebody filed similar stuffs.

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #30)
> (In reply to Trevor Saunders (:tbsaunde) from comment #29)
> > Comment on attachment 603701 [details] [diff] [review]
> > patch
> > 
> > Alex I seem to remember you pushing for cleanup of the aria mapping stuff
> > the last time someone tried to do something like this, but I gues this
> > doesn't make things too much worse so whatever.
> 
> hmm, would be nice to have a bug number. I don't recall somebody filed
> similar stuffs.

bug 653601 is the first one I find, but maybe not the only one :)

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #31)
> (In reply to alexander :surkov from comment #30)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #29)
> > > Comment on attachment 603701 [details] [diff] [review]
> > > patch
> > > 
> > > Alex I seem to remember you pushing for cleanup of the aria mapping stuff
> > > the last time someone tried to do something like this, but I gues this
> > > doesn't make things too much worse so whatever.
> > 
> > hmm, would be nice to have a bug number. I don't recall somebody filed
> > similar stuffs.
> 
> bug 653601 is the first one I find, but maybe not the only one :)

big amount of patches and long discussion :) anything specific please?

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #32)
> (In reply to Trevor Saunders (:tbsaunde) from comment #31)
> > (In reply to alexander :surkov from comment #30)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #29)
> > > > Comment on attachment 603701 [details] [diff] [review]
> > > > patch
> > > > 
> > > > Alex I seem to remember you pushing for cleanup of the aria mapping stuff
> > > > the last time someone tried to do something like this, but I gues this
> > > > doesn't make things too much worse so whatever.
> > > 
> > > hmm, would be nice to have a bug number. I don't recall somebody filed
> > > similar stuffs.
> > 
> > bug 653601 is the first one I find, but maybe not the only one :)
> 
> big amount of patches and long discussion :) anything specific please?

comments 12 and 20? anyway I'm ok with this it'd just be nice to have something nicer

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)

> > > > > Alex I seem to remember you pushing for cleanup of the aria mapping stuff
> > > > > the last time someone tried to do something like this, but I gues this
> > > > > doesn't make things too much worse so whatever.
> > > > 
> > > > hmm, would be nice to have a bug number. I don't recall somebody filed
> > > > similar stuffs.
> > > 
> > > bug 653601 is the first one I find, but maybe not the only one :)
> > 
> > big amount of patches and long discussion :) anything specific please?
> 
> comments 12 and 20? anyway I'm ok with this it'd just be nice to have
> something nicer

Well, I am against of more if/else stuffs in nsAccessible for ARIA. I think it's better to keep the ARIA logic inside of nsARIAMap file. Technically that's what I do in this patch - move the logic from nsAccessible to nsARIAMap.

Comment 35

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

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

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

::: accessible/tests/mochitest/states.js
@@ -109,5 @@
> -
> -  if (extraState & EXT_STATE_EDITABLE)
> -    isState(state & STATE_READONLY, 0, true,
> -            "Editable " + id + " cannot be readonly!");
> -

Why did you remove this?

Comment 36

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #35)

> > -  if (extraState & EXT_STATE_EDITABLE)
> > -    isState(state & STATE_READONLY, 0, true,
> > -            "Editable " + id + " cannot be readonly!");
> > -
> 
> Why did you remove this?

because editable can be presented on readonly control (see bug summary)

Comment 37

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

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

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

r=me. Sorry for delay.

I'm not a super big fan of formatting-only changes (test_textbox.xul) but if you think it is worth it that's your call :)

::: accessible/src/base/nsARIAMap.h
@@ +187,5 @@
>    eARIAReadonly,
>    eARIAReadonlyOrEditable,
>    eARIARequired,
> +  eARIASelectable,
> +  eReadonlyUntilEditable

Feels funny but I don't have a better suggestion.

Comment 38

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4ce5c2cb97

Comment 39

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1d4ce5c2cb97