341747 - Remove #ifdef/ifndef from nsIAccessible.idl

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We need consistent use of roles in the cross platform code.

The mapping to platform role constants should occur in the nsAccessibleWrap classes.

Comment 1

[Aaron Leventhal]

State constants as well.

Comment 2

[Aaron Leventhal]

The ultimate goal should be to create the exact same nsIAccessible hierarchy no matter what platform we're on, and to make any platform tweaks necessary in the final mapping to MSAA, ATK and UA.

However, an extension like Fire Vox should see the same thing on all platforms.

Comment 3

[Mike Gao]

For ROLEs, should we use the values defined in ATK? 

And how about those set to ATK_ROLE_UNKNOWN and 50000U(which are none-matching ATK roles)? Assign different values from just ATK_ROLE_UNKNOWN or 50000U ?

Comment 4

[Aaron Leventhal]

(In reply to comment #3)
> For ROLEs, should we use the values defined in ATK? 
> 
> And how about those set to ATK_ROLE_UNKNOWN and 50000U(which are none-matching
> ATK roles)? Assign different values from just ATK_ROLE_UNKNOWN or 50000U ?

Let's just define our own nsIAccessible role & state constants, and use those completely. Don't use MSAA or ATK constants there at all. Then map those to MSAA/ATK in nsAccessibleWrap.

Comment 5

[Mike Gao]

Attachment: patch of this bug.

Comment 6

[Mike Gao]

sorry. I didn't mean to add comment#3. It was typed before I made the patch and submitted by mistake I think.

I finished compiling with the patch I submitted just now.

Comment 7

[Aaron Leventhal]

Comment on attachment 227399 [details] [diff] [review]
patch of this bug.

The role constant that comes out of MSAA and ATK needs to be what it always was.

You could have a mapping array, and then have code in msaa/nsAccessibleWrap which does msaaRoleMap[xpRole];
In atk/nsAccessible wrap it would do:
atkRole = atkRoleMap[xpRole]

This way the MSAA and ATK results are the same as they always were, but no matter what platform you use our internal nsIAccessible from, you would get the same cross platform role results.

Need the same thing for state and relation.

Comment 8

[Mike Gao]

Attachment: patch for bug341747 and 214580

Comment 9

[Aaron Leventhal]

Comment on attachment 228946 [details] [diff] [review]
patch for bug341747 and 214580

Comments via IM

Comment 10

[Mike Gao]

Attachment: patch for bug 341747 and 214580

Compiled passed on XP and FC5.

But need to confirm the Role mapping when USE_ATK_ROLE_* is not defined in atk/nsAccessibleWrap.cpp.

Comment 11

[Aaron Leventhal]

This code needs to be removed from get_accRole()
  if (role == ROLE_ENTRY || role == ROLE_PASSWORD_TEXT) {
    role = ROLE_TEXT_LEAF;
  }
and that mapping needs to happen in the msaaRoleMap.

Also, changing ROLE_NOTHING in the msaaRoleMap to USE_ROLE_STRING -- it's a lot clearer. It's not really that there is no role at all, but we will be using a string to expose it instead  of an enum. You'll have to change get_accRole to make that happen. Also it makes no sense to compare to ROLE_CLIENT in get_accRole(). You'll have to change ROLE_SYSTEM_CLIENT to USE_ROLE_STRING in the table.

Here are some cleanups for the MSAA table:
+// Make up for ATK roles that we don't have in MSAA
+// When in doubt map them to ROLE_NOTHING so that the role string is exposed
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ACCEL_LABEL
ROLE_LABEL
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ARROW
ROLE_SYSTEM_INDICATOR
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_CANVAS
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_CHECK_MENU_ITEM
ROLE_SYSTEM_MENUITEM
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_COLOR_CHOOSER
ROLE_SYSTEM_DIALOG
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_DATE_EDITOR
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_DESKTOP_ICON
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_DESKTOP_FRAME
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_DIRECTORY_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FILE_CHOOSER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FILLER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FONT_CHOOSER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FRAME
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_GLASS_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_HTML_CONTAINER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ICON
ROLE_SYSTEM_BUTTON
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_INTERNAL_FRAME
+    ROLE_SYSTEM_STATICTEXT,         // nsIAccessible::ROLE_LABEL
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_LAYERED_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_OPTION_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_PASSWORD_TEXT
ROLE_SYSTEM_TEXT
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_POPUP_MENU
Remove and replace all use with ROLE_MENUPOPUP
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_RADIO_MENU_ITEM
ROLE_SYSTEM_MENUITEM
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ROOT_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_SCROLL_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_SPLIT_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TABLE_COLUMN_HEADER
Remove and replace all usage with ROW_COLUMNHEADER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TABLE_ROW_HEADER
Remove and replace all usage with ROW_ROWHEADER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TEAR_OFF_MENU_ITEM
ROLE_SYSTEM_MENUITEM
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TERMINAL
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TEXT_CONTAINER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TOGGLE_BUTTON
ROLE_SYSTEM_BUTTON
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TREE_TABLE
ROLE_SYSTEM_TREE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_VIEWPORT
ROLE_SYSTEM_PANE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_HEADER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FOOTER
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_PARAGRAPH
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_RULER
+    ROLE_SYSTEM_COMBOBOX,           // nsIAccessible::ROLE_AUTOCOMPLETE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_EDITBAR
ROLE_SYSTEM_TEXT
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_EMBEDDED
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ENTRY
ROLE_SYSTEM_TEXT
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_CAPTION
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_DOCUMENT_FRAME
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_HEADING
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_PAGE
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_SECTION
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_REDUNDANT_OBJECT
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_FORM
+    ROLE_NOTHING                    // nsIAccessible::ROLE_IME
+};


I don't like this change:
-    if (!IsEmbeddedObject(this)) {
+    if (Role(this) == ROLE_TEXT_LEAF) {
Why did you do that? In fact I was going to tell you in the static text bug to make sure nsAccessible::IsEmbeddedObject() returns PR_FALSE for ROLE_STATIC_TEXT.

Comment 12

[Aaron Leventhal]

Another thing. In new-atk it doesn't make much sense to use the "Hyper link" string role anymore. Let's get rid of the #ifndef USE_ATK_ROLE_LINK section in getRoleCB() and just use the aktRoleMap for that.
You can put in the atkRoleMap
#ifdef USE_ATK_ROLE_LINK
ATK_ROLE_LINK,
#else
ATK_ROLE_BUTTON,
#endif

Then, in getRoleCB() you only need 1 line for 
atkRole = atkRoleMap[accRole]; // map to the actual value
Please use atkRole and msaaRole for the final role, keep them separate. What I meant before was there was no need to change every occurance of the old role type to xpRole, that made the patch too big.

Comment 13

[Mike Gao]

For ROLE_SYSTEM_BUTTON and ROLE_SYSTEM_TREE are not defined in msaa, I mapped them to ROLE_SYSTEM_PUSHBUTTON and ROLE_SYSTEM_OUTLINE. Is this ok?

+    ROLE_NOTHING,                   // nsIAccessible::ROLE_ICON
ROLE_SYSTEM_BUTTON
 -> 
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TOGGLE_BUTTON
ROLE_SYSTEM_BUTTON
 -> ROLE_SYSTEM_PUSHBUTTON
+    ROLE_NOTHING,                   // nsIAccessible::ROLE_TREE_TABLE
ROLE_SYSTEM_TREE
 -> ROLE_SYSTEM_OUTLINE

> I don't like this change:
> -    if (!IsEmbeddedObject(this)) {
> +    if (Role(this) == ROLE_TEXT_LEAF) {
> Why did you do that? In fact I was going to tell you in the static text bug to
> make sure nsAccessible::IsEmbeddedObject() returns PR_FALSE for
> ROLE_STATIC_TEXT.

sorry, this line is not changed by me, but I made the change based on 1.41 which  is not the latest version:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/accessible/src/atk&command=DIFF_FRAMESET&file=nsAccessibleWrap.cpp&rev2=1.42&rev1=1.41

I need to avoid this in the future.

Comment 14

[Mike Gao]

Attachment: new version of the patch

Comment 15

[Mike Gao]

Attachment: new version of the patch

updated for a typo.

Comment 16

[Aaron Leventhal]

Comment on attachment 229237 [details] [diff] [review]
new version of the patch

We are getting very close.
* There is a typo where it says ROLE_SYSTEM_MENUITE and is missing the final M
* For the atk role map please shorten the comments on each line to be more like the msaa role map. In other words:
// nsIAccessible::ROLE_NOTHING -> ATK_ROLE_UNKNOWN               0
You don't need to -> ATK_ROLE_UNKNOWN since that is redundant. You can keep the numeric values in there, that's very useful.
* Please create a file called nsRoleMap.h in both src/atk and src/msaa and put these mapping tables in there. Eventually we should do the same thing for the role attribute map in nsAccessibleWrap, but we'll do that later.

Comment 17

[Aaron Leventhal]

I just compiled and test it, and it works well on Windows. Once you post a new patch, request r= from myself and second review from [e-mail]. Hwaara also says it looks good to him.

Comment 18

[Mike Gao]

Attachment: patch VFriday

Compiled on FC5 just now. will set review flags when finished on Windows.

Comment 19

[Aaron Leventhal]

Comment on attachment 229252 [details] [diff] [review]
patch VFriday

r=aaronlev, but I'd like Ginn to look at it as well.

Comment 20

[Ginn Chen]

Comment on attachment 229252 [details] [diff] [review]
patch VFriday

r=me

Comment 21

[Mike Gao]

Re-open for the code is not checked in yet.

Comment 22

[Aaron Leventhal]

I changed 3 things before checking in.

1) Got rid of the redundant line (removed the else):
            atkRole = atkRoleMap[accRole]; // map to the actual value
        }
        else {
            atkRole = atkRoleMap[accRole]; // map to the actual value
        }

2) Added an assertion and comment to help ensure the role map tables don't accidentally get skewed from their intended nsIAccessible roles.

3) I hadn't seen this before:
-  if (role != ROLE_NOTHING && role != ROLE_CLIENT) {
+  if (msaaRole != USE_ROLE_STRING) {
We need to keep the same logic, so I changed it to:
+    if (msaaRole != ROLE_CLIENT && GetRoleAttribute(content, roleString)) {

Comment 23

[Aaron Leventhal]

This busted the tree after I checked in. I had to add
USE_ATK_ROLE_EMBEDDED  and  USE_ATK_ROLE_EDITBAR

and make some other tweaks.

Comment 24

[Aaron Leventhal]

Had to fix more roles over on the seamonkey tree:
nsRoleMap.h:63: error: `ATK_ROLE_APPLICATION' was not declared in this scope
nsRoleMap.h:159: error: `ATK_ROLE_HEADER' was not declared in this scope
nsRoleMap.h:160: error: `ATK_ROLE_FOOTER' was not declared in this scope
nsRoleMap.h:161: error: `ATK_ROLE_PARAGRAPH' was not declared in this scope
nsRoleMap.h:162: error: `ATK_ROLE_RULER' was not declared in this scope

Comment 25

[Aaron Leventhal]

Attachment: Code readability patch

Mike, I haven't tried to build or test this, but if it works it's more readable. The ATK role map was getting a little messy because of all of the ifdefs. Will you try it out and review it?

Comment 26

[Mike Gao]

Comment on attachment 229494 [details] [diff] [review]
Code readability patch

sorry. We can't have preprocessor directives in a #define.

Comment 27

[Mike Gao]

Attachment: 3 tweaks to the previous patch

Comment 28

[Aaron Leventhal]

Comment on attachment 229786 [details] [diff] [review]
3 tweaks to the previous patch

Is this line a mistake? Or, what does it do?

+#define paster( n ) printf_s( "token" #n " = %d", token##n )
+

Comment 29

[Aaron Leventhal]

Comment on attachment 229786 [details] [diff] [review]
3 tweaks to the previous patch

Doesn't make sense to me.

Comment 30

[Mike Gao]

(In reply to comment #28)
> (From update of attachment 229786 [details] [diff] [review] [edit])
> Is this line a mistake? Or, what does it do?
> 
> +#define paster( n ) printf_s( "token" #n " = %d", token##n )
> +
> 

This line is from your previous patch, I thought you would use it later.

Comment 31

[Aaron Leventhal]

Oops! Sorry, that was accidental.

Comment 32

[Ginn Chen]

238 #ifdef USE_ATK_ROLE_INPUT_METHOD_WINDOW
239     ATK_ROLE_INPUT_METHOD_WINDOW  // nsIAccessible::ROLE_IME               113
240 #else
241     ATK_ROLE_INVALID              // nsIAccessible::ROLE_IME               113
242 #endif

we missed 2 commas here.
so it skewed.
Aaron's patch in bug 275010 will fix it.

Comment 33

[Aaron Leventhal]

Actually that bug won't fix it, it  was an accidental part of the diff. Ming already looked at that change and saw a problem.

Comment 34

[Aaron Leventhal]

Attachment: Fix missing commas

Comment 35

[Aaron Leventhal]

Ginn, can you check this in for me?

Comment 36

[Ginn Chen]

(In reply to comment #35)
> Ginn, can you check this in for me?
> 

I'd already checked in last week, but I forgot to comment here.