241055 - nsHTMLTextFieldAccessible need "ATK_STATE_EDITABLE" state and "Activate" action.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

This bug is to refine ATK information exported from "nsHTMLTextFieldAccessible".
1. If inputbox in HTML is not readonly, it should export "ATK_STATE_EDITABLE" state.
2. In gail implementation, inputbox in HTML has "Activate" action. We should
support in mozilla.

Comment 1

[Louie Zhao]

Attachment: add editable state

Comment 2

[Louie Zhao]

Attachment: add activate action

Comment 3

[Philip K. Warren]

Comment on attachment 146563 [details] [diff] [review]
add editable state

>Index: accessible/public/nsIAccessible.idl
...
>   const unsigned long  STATE_TRANSIENT          = 0x40000000;  // Tells accessibility aid "Don't add event listener - this object doesn't generate any". For example, could be used with higher level containers.    
>   const unsigned long  STATE_VERTICAL           = 0x80000000;  // Especially used for sliders and scrollbars  
>+  const unsigned long  STATE_EDITABLE           = 0x00080000;  // used for HTML input (type = text,password) element

Can you please move STATE_EDITABLE up to the top of the extended state
constants, so they are in order of increasing value like in other places in
this file. Also, any reason on why you chose 0x00080000 instead of 0x00100000?

>Index: accessible/src/html/nsHTMLFormControlAccessible.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/html/nsHTMLFormControlAccessible.cpp,v
>retrieving revision 1.42
>diff -u -r1.42 nsHTMLFormControlAccessible.cpp
>--- accessible/src/html/nsHTMLFormControlAccessible.cpp	31 Jul 2003 08:09:24 -0000	1.42
>+++ accessible/src/html/nsHTMLFormControlAccessible.cpp	19 Apr 2004 10:51:32 -0000
>@@ -375,6 +375,8 @@
>   elt->HasAttribute(NS_LITERAL_STRING("readonly"), &isReadOnly);
>   if (isReadOnly)
>     *_retval |= STATE_READONLY;
>+  else
>+    *_retval |= STATE_EDITABLE;
> 
>   // Get current selection and find out if current node is in it
>   nsCOMPtr<nsIPresShell> shell(GetPresShell());

I don't think the way we are currently dealing with states is correct.
STATE_EDITABLE is an extended state (listed in the second section), so it
should be set in an implementation of GetAccExtState. However, this will not
work in the current ATK code because of the following (from
nsAccessibleWrap.cpp):

    PRUint32 accState = 0;
    nsresult rv = accWrap->GetState(&accState);
    NS_ENSURE_SUCCESS(rv, state_set);

    if (accState == 0) {
	nsresult rv = accWrap->GetExtState(&accState);
	//NS_ENSURE_SUCCESS(rv, state_set);
	if (accState == 0)
	    return state_set;
    }
    nsAccessibleWrap::TranslateStates(accState, state_set);

Both the state and the extended state could pass valuable information, however
we are only checking the extended state if the state is not set.

I have spoken to Aaron some more today about the whole state/extended state
problems. I think the cleanest solution would be to rename the extended state
constants in the idl file with an EXT_ prefix to prevent confusion between the
two.

Comment 4

[Louie Zhao]

Attachment: add editable state 2

> Can you please move STATE_EDITABLE up to the top of the extended state
> constants, so they are in order of increasing value like in other places in
> this file.
Done.

> Also, any reason on why you chose 0x00080000 instead of 0x00100000?
If two "STATE"s have the same value, mozilla will be confused while using them
simultaneously. MSAA States consume almost all of bitfield. So Extended states
can only pick up the bitfields which have not been used. "0x00080000" is a good
candidate. "0x00100000" is STATE_FOCUSABLE, which has been used widely.

> I don't think the way we are currently dealing with states is correct.
> STATE_EDITABLE is an extended state (listed in the second section), so it
> should be set in an implementation of GetAccExtState. However, this will not
> work in the current ATK code

> I have spoken to Aaron some more today about the whole state/extended state
> problems. I think the cleanest solution would be to rename the extended state

> constants in the idl file with an EXT_ prefix to prevent confusion between
the
> two.

Actually, "GetAccExtState" isn't implemented by any object at present, that is
to say, "state/extended state" mechanism has never been used. It's really a
problem because MSAA only use "GetState". If we put extended state in
"GetState", MSAA may not work properly. Philip, can you file another bug to fix
this issue ? Thanks.

Comment 5

[Louie Zhao]

Attachment: add editable state 2

Updated patch.

Since STATE_EDITABLE is "extend state", which shouldn't be used in
"accessible/src/html". Otherwise, MASS may not work properly because MASS may
confuse STATE_EDITABLE with other MASS states. This patch move STATE_EDITABLE
to "accessible/src/atk".

Comment 6

[Philip K. Warren]

The problem I see with setting the state to include an extended state flag is 
that ATK may work properly, but the MSAA implementation will think the object 
has a state which it does not. Since nsIAccessible::STATE_EDITABLE and 
nsIAccessible::STATE_SELFVOICING have the same value, the ATK code will work 
properly when encountering this state (by setting ATK_STATE_EDITABLE), but the 
MSAA code will set the improper state STATE_SELFVOICING for this object.

The reason we have not encountered this problem so far is that we are not 
setting any of the extended states currently.

Comment 7

[Louie Zhao]

All the extended states are only used in ATK code. MSAA implementation has never
used them and isn't supposed to use "extended state".

There will be problem when both MSAA and ATK want to use 2 states who have the
same value at the same time. Fortunately, this won't happen because MSAA only
use "MSAA State flags" and ATK replaces unused "MSAA State flags" with extended
state. If using more states (for both MSAA and ATK) makes confliction in the
future, we can file bug to deal with it.

Comment 8

[Aaron Leventhal]

I want to move us in the direction of clean API's. Let's not mix extended states
and regular states. That's not the way regular and extended states were meant to
be used. They should always be separate in separate fields until the final
mapping to MSAA and ATK states.

Let's use some type safety techniques to ensure states don't get mixed together.

Comment 9

[Henry Jia]

Comment on attachment 146564 [details] [diff] [review]
add activate action

sr=Henry

Comment 10

[Louie Zhao]

Thanks for r and sr. "add activate action" PATCH has been checked in.

Comment 11

[Louie Zhao]

filed bug 278951 to deal with "extended state flags" issue.

Comment 12

[Louie Zhao]

Attachment: add editable state

Comment 13

[Aaron Leventhal]

Comment on attachment 176575 [details] [diff] [review]
add editable state

The changes are minor from the patch you already have r= and sr= for, so just
check it in with r=pkw,aaronlev and sr=henry.

Comment 14

[Aaron Leventhal]

Comment on attachment 176575 [details] [diff] [review]
add editable state

Actually this patch is fixing a different problem than the first page.

Comment 15

[Louie Zhao]

The patch for "add edtiable state" has been checked in.