1) create states.h file and put 64bit accessible states under states namespace 2) introduce new state methods and start to use them, use new constants 2.1) add NativeState method instead GetStatesInternal: PRUint64 NativeState(bool aIncludeExtraStates) - aIncludeExtraStates to avoid possible perf problems since GetStatesInternal is used with null aExtraStates argument 2.2) do the same for GetARIAState 2.3) do the same for GetState (additionally add new GetState XPCOM version to map states into nsIAccessibleStates) 2.4) Replace all methods to new ones 2.5) new constants should be used 5) fix atk/nsStateMap to have one array.

(In reply to comment #1) > Created attachment 512727 [details] [diff] [review] > add states.h defining accessible states please fix licensing block remove dupe file following an idea to expose a11y headers in a11y namespace, I think it's worth to name namespace for states as "states". don't keep never used states as marqueed follow bug 344674 in states commenting

(In reply to comment #2) > Created attachment 512728 [details] [diff] [review] > (wip) start implementing nativeState() PRUint32 nativeState(PRBool aExtraStates) should be PRUint32 NativeState(PRBool aIncludeExtraStates) or aSkipExtraStates you should do NativeState up on top of GetStatesInternal to keep history please make sure you declare a method in header file in proper place

(In reply to comment #4) > (In reply to comment #2) > > Created attachment 512728 [details] [diff] [review] > > (wip) start implementing nativeState() > > PRUint32 nativeState(PRBool aExtraStates) Just do PRUint64 NativeState() - see bug 462983. Let's plan this work after fx4. So feel free to change interfaces and change the code more radically. Split, the work on small parts, don't do two different changes in one patch.

Comment on attachment 513944 [details] [diff] [review] add states.h defining accessible states >+ * Contributor(s): >+ * Trevor Saunders <trev.saunders@gmail.com> two spaces indent relative Contributor(s), add (original author) after email >+ /** >+ * Disabled, maps to opposite of Java ENABLED, Gnome/ATK SENSITIVE? >+ */ >+ const PRUint64 UNAVAILABLE = 0x1; // nit // at the end of line The object is disabled, opposite to to enabled and sensitive >+ >+ /** >+ * the object is selected. >+ */ >+ const PRUint64 SELECTED = 0x2; the -> The >+ >+ /** >+ * The object has the keyboard focus. >+ */ >+ const PRUint64 FOCUSED = 0x4; perhaps "has the focus" >+ >+ /** >+ * The object is pressed. >+ */ >+ const PRUint64 PRESSED = 0x8; >+ >+ /** >+ * The objects check box is selected. >+ */ >+ const PRUint64 CHECKED = 0x10; perhaps: The checkbox control is checked >+ >+ /** >+ * Indicates that the state of a three-state check box or tool bar button is >+ * undetermined. The check box is neither checked and is >+ * Therefore in the third or mixed state. >+ */ >+ const PRUint64 MIXED = 0x20; please check English in last two sentences. >+ /** >+ * The object is designated read-only >+ */ >+ const PRUint64 READONLY = 0x40; // Maps to opposite of Java/Gnome/ATK EDITABLE state combine two comments, "opposite to editable state" >+ /** >+ * The object is hot-tracked by the mouse, which means that its appearance >+ * has changed to indicate that the mouse pointer is located over it. >+ */ >+ const PRUint64 HOTTRACKED = 0x80; point that it's not used >+ >+ /** >+ * Children of this object that have the ROLE_OUTLINEITEM role are displayed. >+ */ >+ const PRUint64 EXPANDED = 0x200; perhaps: Items of the control are displayed, opposite to collapsed >+ >+ /** >+ * children of this object that have the ROLE_OUTLINEITEM role are hidden. >+ */ >+ const PRUint64 COLLAPSED = 0x400; same >+ >+ /** >+ * The control can not accept input at this time. >+ */ >+ const PRUint64 BUSY = 0x800; the control or document >+ >+ /** >+ * children "owned" not "contained" by parent, that is the object is not >+ * clipped to the boundary of its parent object and does not move >+ * automatically when the parent moves. >+ */ >+ const PRUint64 FLOATING = 0x1000; perhaps: the object is out of normal flow, may be outside of boundaries of its parent >+ const PRUint64 CHECKABLE = 0x2000; comment for checkable. Please add comment to MSAA part its mapped to marquee state. >+ >+ const PRUint64 ANIMATED = 0x4000; comment? >+ >+ /** >+ * programatically hidden >+ */ >+ const PRUint64 INVISIBLE = 0x8000; perhaps: The object is hidden, can't be made visible by user action (like scrolling, tabs switching). >+ >+ /** >+ * scrolled off screen >+ */ >+ const PRUint64 OFFSCREEN = 0x10000; perhaps: the object is off screen, can be made visible by user action (like scrolling, tabs switching) >+ const PRUint64 SIZEABLE = 0x20000; >+ >+ const PRUint64 MOVEABLE = 0x40000; >+ >+ const PRUint64 SELFVOICING = 0x80000; >+ >+ const PRUint64 FOCUSABLE = 0x100000; >+ >+ const PRUint64 SELECTABLE = 0x200000; >+ >+ const PRUint64 LINKED = 0x400000; >+ >+ const PRUint64 TRAVERSED = 0x800000; comment for all of these please >+ /** >+ * supports multiple selection >+ */ >+ const PRUint64 MULTISELECTABLE = 0x1000000; // Supports multiple selection the control is multiselectable, combine comments >+ /** >+ * supports extended selection >+ */ >+ const PRUint64 EXTSELECTABLE = 0x2000000; Supports extended selection, see nsIAccessible::ExtendSelection. Always exposed with MULTISELECTABLE state. Makes sense for MSAA only. Move it there eventually. File a bug. >+ const PRUint64 REQUIRED = 0x4000000; >+ >+ const PRUint64 ALERT = 0x8000000; >+ >+ const PRUint64 INVALID = 0x10000000; comment these >+ /** >+ * Maps to Gnome's *Role* ATK_ROLE_PASSWD_TEXT, nothing for Java? >+ */ >+ const PRUint64 PROTECTED = 0x20000000; perhaps: The control value can't be obtained and exposed as set of asterisks. >+ >+ const PRUint64 HASPOPUP = 0x40000000; comment >+ /** >+ * * Extended state flags (for now non-MSAA, for Java and Gnome/ATK support) >+ * * "Extended state flags" has separate value space from "MSAA State flags". >+ * */ do you need that? >+ /** >+ * For editable areas that have any kind of autocompletion >+ */ >+ const PRUint64 SUPPORTS_AUTOCOMPLETION = 0x100000000; perhaps: The editable area that has >+ /** >+ * object no longer exists >+ */ >+ const PRUint64 DEFUNCT = 0x200000000; The object no longer exists, the information it exposes is obsolete >+ /** >+ * For text which is selectable, object must implement nsIAccessibleText >+ */ >+ const PRUint64 SELECTABLE_TEXT = 0x400000000; The text object that supports text selection? >+ /** >+ * Implements nsIAccessibleEditableText >+ */ >+ const PRUint64 EDITABLE = 0x800000000; The text object that the text editing is allowed for? >+ /** >+ * This window is currently the active window >+ */ >+ const PRUint64 ACTIVE = 0x1000000000; >+ >+ /** >+ * Must do something with control before leaving it >+ */ >+ const PRUint64 MODAL = 0x2000000000; perhaps take IA2 wording: Indicates that an object is modal. Modal objects have the behavior that something must be done with the object before the user can interact with an object in a different window. >+/** >+ * Edit control that can take multiple lines >+ */ >+ const PRUint64 MULTI_LINE = 0x4000000000; don't forget to set point in the end of comment. >+ >+ const PRUint64 TRANSIENT = 0x40000000000; ? >+ /** >+ * uses veritcal layout? >+ * Especially used for sliders and scrollbars >+ */ >+ const PRUint64 VERTICAL = 0x80000000000; why's ? here. >+ /** >+ * A widget that is not unavailable >+ */ >+ const PRUint64 ENABLED = 0x200000000000; >+ >+ /** >+ * same as enabled for now??? >+ */ >+ const PRUint64 SENSITIVE = 0x400000000000; yes, perhaps we need it for specific toolkit, if so please file a bug for that >+ >+ /** >+ * if expanded or collapsed >+ * can be expanded >+ */ >+ const PRUint64 EXPANDABLE = 0x800000000000; can be expanded or collapsed, see expanded, collsapsed states >+} >+ >+#endif no end empty line?

(In reply to comment #13) > Comment on attachment 513944 [details] [diff] [review] > add states.h defining accessible states > >+ /** > >+ * The object has the keyboard focus. > >+ */ > >+ const PRUint64 FOCUSED = 0x4; > > perhaps "has the focus" That's what I currently have and it seems reasonable, but fyi the msdn docs use the old wording word for word as it was in that comment. > >+ /** > >+ * children "owned" not "contained" by parent, that is the object is not > >+ * clipped to the boundary of its parent object and does not move > >+ * automatically when the parent moves. > >+ */ > >+ const PRUint64 FLOATING = 0x1000; > > perhaps: the object is out of normal flow, may be outside of boundaries of its > parent Ok, added that to the comment, if you think the old comment is inaccurate I can remove it, but it seems to me like it may be useful. > > >+ /** > >+ * supports extended selection > >+ */ > >+ const PRUint64 EXTSELECTABLE = 0x2000000; > > Supports extended selection, see nsIAccessible::ExtendSelection. Always exposed > with MULTISELECTABLE state. Makes sense for MSAA only. Move it there > eventually. File a bug. ok, bug 635690 > >+ const PRUint64 ALERT = 0x8000000; neither ia2 or msaa have this state how do we want to describe it? > >+ * * Extended state flags (for now non-MSAA, for Java and Gnome/ATK support) > >+ * * "Extended state flags" has separate value space from "MSAA State flags". > >+ * */ > > do you need that? I don't think so, I've changed it so where the extended ones start is immediately after the first set without skipping to bit 32. > > >+ /** > >+ * For editable areas that have any kind of autocompletion > >+ */ > >+ const PRUint64 SUPPORTS_AUTOCOMPLETION = 0x100000000; > > perhaps: The editable area that has I'm not sure I see what your suggesting, imvho the current description is reasonable. > >+ * For text which is selectable, object must implement nsIAccessibleText > >+ */ > >+ const PRUint64 SELECTABLE_TEXT = 0x400000000; > > The text object that supports text selection? The original seems clearer to me :) > >+ /** > >+ * Implements nsIAccessibleEditableText > >+ */ > >+ const PRUint64 EDITABLE = 0x800000000; > > The text object that the text editing is allowed for? how about The text in this object can be edited? > >+ const PRUint64 TRANSIENT = 0x40000000000; > > ? I assume you wanted a comment? > > >+ /** > >+ * uses veritcal layout? > >+ * Especially used for sliders and scrollbars > >+ */ > >+ const PRUint64 VERTICAL = 0x80000000000; > > why's ? here. I wasn't completely sure that was the case :) > >+ /** > >+ * same as enabled for now??? > >+ */ > >+ const PRUint64 SENSITIVE = 0x400000000000; > > yes, perhaps we need it for specific toolkit, if so please file a bug for that my reading of the comment is that someone had some idea to change the meaning to something different from sensitive at some point. However I can't think of something it could mean other than the same as enabled so... > >+#endif > > no end empty line? should there be one? other files don't seem to always have one.

(In reply to comment #14) > > perhaps "has the focus" > > That's what I currently have and it seems reasonable, but fyi the msdn docs > use the old wording word for word as it was in that comment. what does ATK say? > > > >+ /** > > >+ * children "owned" not "contained" by parent, that is the object is not > > >+ * clipped to the boundary of its parent object and does not move > > >+ * automatically when the parent moves. > > >+ */ > > >+ const PRUint64 FLOATING = 0x1000; > > > > perhaps: the object is out of normal flow, may be outside of boundaries of its > > parent > > Ok, added that to the comment, if you think the old comment is inaccurate I can > remove it, but it seems to me like it may be useful. "owned" vs "contained" doesn't sound descriptive, contain word is used in geometrical meaning, I'd like to have it clear (vs other contain word meanings). This state is about CSS floats property (http://www.w3.org/TR/CSS2/visuren.html#floats) and we should rely on it to describe this state. > > >+ const PRUint64 ALERT = 0x8000000; > > neither ia2 or msaa have this state how do we want to describe it? msaa must have it, alert_medium? > > > >+ * * Extended state flags (for now non-MSAA, for Java and Gnome/ATK support) > > >+ * * "Extended state flags" has separate value space from "MSAA State flags". > > >+ * */ > > > > do you need that? > > I don't think so, I've changed it so where the extended ones start is > immediately after the first set without skipping to bit 32. I'm not sure we need it at all, but let's see next patch version > > > > >+ /** > > >+ * For editable areas that have any kind of autocompletion > > >+ */ > > >+ const PRUint64 SUPPORTS_AUTOCOMPLETION = 0x100000000; > > > > perhaps: The editable area that has > > I'm not sure I see what your suggesting, imvho the current description is > reasonable. the point was that other descriptions starts from "the object ...", I just suggested to make this comment closer to this template. > > > >+ * For text which is selectable, object must implement nsIAccessibleText > > >+ */ > > >+ const PRUint64 SELECTABLE_TEXT = 0x400000000; > > > > The text object that supports text selection? > > The original seems clearer to me :) the same reason, I don't insist on this wording exactly but it's nice to keep it common. > > > >+ /** > > >+ * Implements nsIAccessibleEditableText > > >+ */ > > >+ const PRUint64 EDITABLE = 0x800000000; > > > > The text object that the text editing is allowed for? > > how about The text in this object can be edited? perhaps more readable comment is preferable over template :) ok. > > > >+ const PRUint64 TRANSIENT = 0x40000000000; > > > > ? > > I assume you wanted a comment? sure > > > > >+ /** > > >+ * uses veritcal layout? > > >+ * Especially used for sliders and scrollbars > > >+ */ > > >+ const PRUint64 VERTICAL = 0x80000000000; > > > > why's ? here. > > I wasn't completely sure that was the case :) yeah, that's about vertical layout > > > >+ /** > > >+ * same as enabled for now??? > > >+ */ > > >+ const PRUint64 SENSITIVE = 0x400000000000; > > > > yes, perhaps we need it for specific toolkit, if so please file a bug for that > > my reading of the comment is that someone had some idea to change the meaning > to something different from sensitive at some point. However I can't think of > something it could mean other than the same as enabled so... well, we need to figure out that someone and something :) > > no end empty line? > > should there be one? other files don't seem to always have one. yes, it should be

(In reply to comment #15) > (In reply to comment #14) > > > > perhaps "has the focus" > > > > That's what I currently have and it seems reasonable, but fyi the msdn docs > > use the old wording word for word as it was in that comment. > > what does ATK say? it appears to be a copy and paste from msaa, refering to keyboard and typeing with characters entered. > > > > > > >+ /** > > > >+ * children "owned" not "contained" by parent, that is the object is not > > > >+ * clipped to the boundary of its parent object and does not move > > > >+ * automatically when the parent moves. > > > >+ */ > > > >+ const PRUint64 FLOATING = 0x1000; > > > > > > perhaps: the object is out of normal flow, may be outside of boundaries of its > > > parent > > > > Ok, added that to the comment, if you think the old comment is inaccurate I can > > remove it, but it seems to me like it may be useful. > > "owned" vs "contained" doesn't sound descriptive, contain word is used in > geometrical meaning, I'd like to have it clear (vs other contain word > meanings). This state is about CSS floats property > (http://www.w3.org/TR/CSS2/visuren.html#floats) and we should rely on it to > describe this state. ok, makes sense, I've removed the old description. > > > > >+ const PRUint64 ALERT = 0x8000000; > > > >+ /** > > > >+ * same as enabled for now??? > > > >+ */ > > > >+ const PRUint64 SENSITIVE = 0x400000000000; > > > > > > yes, perhaps we need it for specific toolkit, if so please file a bug for that > > > > my reading of the comment is that someone had some idea to change the meaning > > to something different from sensitive at some point. However I can't think of > > something it could mean other than the same as enabled so... > > well, we need to figure out that someone and something :) the atk docs claim therere is a difference in atk, giving an example of a "inconsistant togle button. See http://www.gtk.org/api/2.6/atk/atk-AtkState.html#AtkStateType

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > > > > perhaps "has the focus" > > > > > > That's what I currently have and it seems reasonable, but fyi the msdn docs > > > use the old wording word for word as it was in that comment. > > > > what does ATK say? > > it appears to be a copy and paste from msaa, refering to keyboard and typeing > with characters entered. the focused state is mapped to atk, so it should suitable both for msaa and atk and description should reflect that. > the atk docs claim therere is a difference in atk, giving an example of a > "inconsistant togle button. See > http://www.gtk.org/api/2.6/atk/atk-AtkState.html#AtkStateType file a bug please.

Comment on attachment 514338 [details] [diff] [review] add states.h defining accessible states >add states.h defining accessible states > >diff --git a/accessible/src/base/AccStates.h b/accessible/src/base/AccStates.h it should States.h and states.h. It's ok until we expose it outside an a11y, if we do then we should expose namespace inside mozilla::a11y namespace and file should be placed to include/mozilla/a11y folder. We shouldn't end up into ambiguity. >+ /** >+ * The object is designated read-only. >+ * This is the opposite of the editable state in Java / ATK. it doesn't make sense to mention platform mappings here. Talk about crossplatform behavior. Btw, this comment is true in crossplatform part. >+ /** >+ * Subitems of the control are displayed, opposite of collapsed. >+ */ >+ const PRUint64 EXPANDED = 0x200; >+ >+ /** >+ * Subitems of the control are not displayed. >+ */ >+ const PRUint64 COLLAPSED = 0x400; do we talk about subitems really? for example, comboboxes, flat trees. >+ /** >+ * The object can be checked. >+ * In msaa this is mapped to the marquee state. >+ */ >+ const PRUint64 CHECKABLE = 0x2000; move msaa comment into msaa part. >+ /** >+ * The objectcan have the focus and become focused. >+ */ >+ const PRUint64 FOCUSABLE = 0x100000; objectcan -> object can >+ /** >+ * This is used for links that have been traversed. >+ * The linked page has been visited. >+ */ >+ const PRUint64 TRAVERSED = 0x800000; combine two sentences? by i.e.? >+ /** >+ * The object is an alert, notifying the user of something. >+ */ >+ const PRUint64 ALERT = 0x8000000; notifying the user of something [important]? >+ /** >+ * Object no longer exists. >+ */ >+ const PRUint64 DEFUNCT = 0x100000000; technically speaking the object exists but can't be used to query any information. >+ /** >+ * The text is selectable, the object must implement nsIAccessibleText. >+ */ >+ const PRUint64 SELECTABLE_TEXT = 0x200000000; perhaps text interface instead of nsIAccessibleText? nsI interfaces are XPCOM part what should live separately. We shouldn't refer to it in internal part. >+ /** >+ * The text in this object can be edited. >+ */ >+ const PRUint64 EDITABLE = 0x400000000; >+ >+ /** >+ * This window is currently the active window. >+ */ >+ const PRUint64 ACTIVE = 0x800000000; >+ >+ /** >+ * Indicates that the object is modal. Modal objects have the behavior >+ * that something must be done with the object before the user can interact with an object in a different window. >+ */ >+ const PRUint64 MODAL = 0x1000000000; use 80 line restriction in comments. >+ >+/** >+ * Edit control that can take multiple lines. >+ */ >+ const PRUint64 MULTI_LINE = 0x2000000000; >+ >+/** >+ * Uses horizontal layout. >+ */ >+ const PRUint64 HORIZONTAL = 0x4000000000; >+ >+ /** >+ * Indicates this object paints every pixel within its rectangular region. >+ */ >+ const PRUint64 OPAQUE = 0x8000000000; >+ >+ /** >+ * This text object can only contain 1 line of text. >+ */ >+ const PRUint64 SINGLE_LINE = 0x10000000000; >+ >+ /** >+ * The parent object manages decendants, and this object may only exist decendants -> descendants, remove one exess space after "," >+ /** >+ * same as enabled for now??? >+ */ >+ const PRUint64 SENSITIVE = 0x200000000000; bug number please >+ >+ /** >+ * Can be expanded or collapsed. >+ */ >+ const PRUint64 EXPANDABLE = 0x400000000000; see expanded/collapsed states? r=me with nits addressed

Comment on attachment 513945 [details] [diff] [review] (wip) start implementing nativeState() >+ * Return the states of accessible, not taking into account ARIA states. >+ * if aExtraStates is FALSe extra states will not be calculated. > */ >- virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual PRUint64 NativeState(); there's no aExtraStates argument. >+PRUint64 >+nsApplicationAccessible::NativeState() > { >+ if(IsDefunct()) >+ return states::DEFUNCT; >+ return 0; you could do return IsDefunct() ? states::DEFUNCT : 0; >- NS_IMETHOD GetState(PRUint32 *aState , PRUint32 *aExtraState ); >+ virtual nsresult GetState(PRUint32 *aState, PRUint32 *aExtraState); GetState() comes from idl in this patch, don't change it. >- virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState); >+ PRUint64 NativeState(); virtual >+nsDocAccessible::NativeState() > { >+ PRUint64 states = 0; >+ // The root content of the document might be removed so that mContent is >+ // out of date. >+ states = (mContent->GetCurrentDoc() == mDocument) ? 0 : states::STALE; PRUint64 states = (mContent->GetCurrentDoc() == mDocument) ? 0 : states::STALE; > // Expose state busy until the document is loaded or tree is constructed. > if (!mIsLoaded || !mNotificationController->IsTreeConstructed()) { >+ states |= states::BUSY; >+ states |= states::STALE; > } fix indent > if (frame == nsnull || > !CheckVisibilityInParentChain(mDocument, frame->GetViewExternal())) { >- *aState |= nsIAccessibleStates::STATE_INVISIBLE | >- nsIAccessibleStates::STATE_OFFSCREEN; >+ states |= states::INVISIBLE | >+ states::OFFSCREEN; put it on the same line > nsCOMPtr<nsIEditor> editor; > GetAssociatedEditor(getter_AddRefs(editor)); > if (!editor) { >+ states |= states::READONLY; > } >+ else { >+ states |= states::EDITABLE; > } states |= editor ? states::EDITABLE : states::READONLY > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(this, nsIAccessibleStates::EXT_STATE_EDITABLE, >+ new AccStateChangeEvent(this, states::EDITABLE, > PR_TRUE, PR_TRUE); put arguments on the same line > nsRefPtr<AccEvent> enabledChangeEvent = > new AccStateChangeEvent(aContent, >- nsIAccessibleStates::EXT_STATE_ENABLED, >+ states::ENABLED, > PR_TRUE); the same > FireDelayedAccessibleEvent(enabledChangeEvent); > > nsRefPtr<AccEvent> sensitiveChangeEvent = > new AccStateChangeEvent(aContent, >- nsIAccessibleStates::EXT_STATE_SENSITIVE, >+ states::SENSITIVE, > PR_TRUE); the same > if (aAttribute == nsAccessibilityAtoms::contenteditable) { > nsRefPtr<AccEvent> editableChangeEvent = > new AccStateChangeEvent(aContent, >- nsIAccessibleStates::EXT_STATE_EDITABLE, >+ states::EDITABLE, > PR_TRUE); the same > if (aAttribute == nsAccessibilityAtoms::aria_required) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent, nsIAccessibleStates::STATE_REQUIRED, >+ new AccStateChangeEvent(aContent, states::REQUIRED, > PR_FALSE); the same > if (aAttribute == nsAccessibilityAtoms::aria_invalid) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent, nsIAccessibleStates::STATE_INVALID, >+ new AccStateChangeEvent(aContent, states::INVALID, > PR_FALSE); the same > if (aAttribute == nsAccessibilityAtoms::aria_expanded) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent, nsIAccessibleStates::STATE_EXPANDED, >+ new AccStateChangeEvent(aContent, states::EXPANDED, > PR_FALSE); the same > if (aAttribute == nsAccessibilityAtoms::aria_checked || > aAttribute == nsAccessibilityAtoms::aria_pressed) { > const PRUint32 kState = (aAttribute == nsAccessibilityAtoms::aria_checked) ? >- nsIAccessibleStates::STATE_CHECKED : >- nsIAccessibleStates::STATE_PRESSED; >+ states::CHECKED : >+ states::PRESSED; the same >- PRBool wasMixed = (gLastFocusedAccessiblesState & nsIAccessibleStates::STATE_MIXED) != 0; >+ PRBool wasMixed = (gLastFocusedAccessiblesState & states::MIXED) != 0; change gLastFocusedAccessiblesState to PRUint64 > if (wasMixed != isMixed) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent, nsIAccessibleStates::STATE_MIXED, >+ new AccStateChangeEvent(aContent, states::MIXED, > PR_FALSE, isMixed); put args on the same line if 80 chars restriction isn't broken > if (aAttribute == nsAccessibilityAtoms::aria_readonly) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent, nsIAccessibleStates::STATE_READONLY, >+ new AccStateChangeEvent(aContent, states::READONLY, > PR_FALSE); the same > if (aStateMask.HasState(NS_EVENT_STATE_INVALID)) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent1, nsIAccessibleStates::STATE_INVALID, >+ new AccStateChangeEvent(aContent1, states::INVALID, > PR_FALSE, PR_TRUE); the same >+#include<AccStates.h> while you change AccStates.h to States.h in first patch, please make sure "" are used and there's a space between include and ". >+PRUint64 >+nsOuterDocAccessible::NativeState() > { >+ PRUint64 states = nsAccessible::NativeState(); use state name vs states to avoid confusion with states namespace, please use everywhere >+ states &= ~states::FOCUSABLE; >+ return states; perhaps return nsAccessible::NativeState() & ~states::FOCUSABLE? >+PRUint64 >+nsHTMLCheckboxAccessible::NativeState() > { >+ PRUint64 states = nsFormControlAccessible::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_CHECKABLE; >+ states |= states::CHECKABLE; perhaps you should do PRUint64 states = nsFormControlAccessible::NativeState() |= states::CHECKABLE; if so then please fix it in other places >+PRUint64 >+nsHTMLTextFieldAccessible::NativeState() > { >- if (!(*aExtraState & nsIAccessibleStates::EXT_STATE_EDITABLE)) >+ if (!(states & states::EDITABLE)) > return NS_OK; be careful with return value! the diff doesn't allow to check all paths in methods. >+PRUint64 >+nsHTMLImageAccessible::NativeState() > { > // The state is a bitfield, get our inherited state, then logically OR it with >- // STATE_ANIMATED if this is an animated image. >+ // ANIMATED if this is an animated image. states::ANIMATED or ANIMATED state PRUint32 *aExtraState) >+PRUint64 >+nsHTMLSelectListAccessible::NativeState() > { >+ PRUint64 states = nsAccessibleWrap::NativeState(); > > // As a nsHTMLSelectListAccessible we can have the following states: >- // nsIAccessibleStates::STATE_MULTISELECTABLE >- // nsIAccessibleStates::STATE_EXTSELECTABLE >+ // states::MULTISELECTABLE >+ // states::EXTSELECTABLE make them on one line > PRUint32 state = 0; > nsCOMPtr<nsIContent> content = GetSelectState(&state); >- if (state & nsIAccessibleStates::STATE_COLLAPSED) { >+ if (state & states::COLLAPSED) { I hope GetSelectState is changed in some next patch and state is turned into PRUint64 > /** > * As a nsHTMLSelectOptionAccessible we can have the following states: >- * STATE_SELECTABLE >- * STATE_SELECTED >- * STATE_FOCUSED >- * STATE_FOCUSABLE >- * STATE_OFFSCREEN >+ * SELECTABLE >+ * SELECTED >+ * FOCUSED >+ * FOCUSABLE >+ * OFFSCREEN move this comment into method, make sure it doesnt take lot of lines :) >+PRUint64 >+nsHTMLSelectOptionAccessible::NativeState() > { > // Upcall to nsAccessible, but skip nsHyperTextAccessible impl >- // because we don't want EXT_STATE_EDITABLE or EXT_STATE_SELECTABLE_TEXT >+ // because we don't want EXT_EDITABLE or EXT_STATE_SELECTABLE_TEXT fix state names >- if (selectState & nsIAccessibleStates::STATE_INVISIBLE) { >+ if (selectState & states::INVISIBLE) { > return NS_OK; > } while you're here, you can remove braces around single statement ifs take care of return value > > NS_ENSURE_TRUE(selectContent, NS_ERROR_FAILURE); take care of return value! > > // Is disabled? >+ if (0 == (states & states::UNAVAILABLE)) { >+ states |= (states::FOCUSABLE | >+ states::SELECTABLE); inline them > // When the list is focused but no option is actually focused, > // Firefox draws a focus ring around the first non-disabled option. >- // We need to indicated STATE_FOCUSED in that case, because it >+ // We need to indicated FOCUSED in that case, because it states::FOCUSED or FOCUSED state > if ( isSelected ) >- *aState |= nsIAccessibleStates::STATE_SELECTED; >+ states |= states::SELECTED; feel free to fix style of that if while you're here >+ states &= ~states::OFFSCREEN; >+ states &= ~states::INVISIBLE; >+ states |= selectExtState & states::OPAQUE; fix indent, you may want to combine them into single statement or two statements >+PRUint64 >+nsHTMLSelectOptGroupAccessible::NativeState() > { >+ PRUint64 states = nsHTMLSelectOptionAccessible::NativeState(); > >- *aState &= ~(nsIAccessibleStates::STATE_FOCUSABLE | >- nsIAccessibleStates::STATE_SELECTABLE); >+ states &= ~(states::FOCUSABLE | >+ states::SELECTABLE); inline them > /** > * As a nsHTMLComboboxAccessible we can have the following states: >- * STATE_FOCUSED >- * STATE_FOCUSABLE >- * STATE_HASPOPUP >- * STATE_EXPANDED >- * STATE_COLLAPSED >+ * FOCUSED >+ * FOCUSABLE >+ * HASPOPUP >+ * EXPANDED >+ * COLLAPSED > */ >+PRUint64 >+nsHTMLComboboxAccessible::NativeState() please move comment inside the method > /** > * As a nsHTMLComboboxListAccessible we can have the following states: >- * STATE_FOCUSED >- * STATE_FOCUSABLE >- * STATE_INVISIBLE >- * STATE_FLOATING >+ * FOCUSED >+ * FOCUSABLE >+ * INVISIBLE >+ * FLOATING >+PRUint64 >+nsHTMLComboboxListAccessible::NativeState() the same >+PRUint64 >+nsHTMLTableCellAccessible::NativeState() > { >+ PRUint64 states= nsHyperTextAccessibleWrap::NativeState(); space before states variable > nsresult > nsHTMLTableAccessible::GetNameInternal(nsAString& aName) > { > if (docAccessible) { >+ PRUint64 state; >+ state = docAccessible->NativeState(); inline them, perhaps docState? > - (int)isChecked > { >- PRUint32 state = 0; >- mGeckoAccessible->GetStateInternal(&state, nsnull); >+ PRUint32 state = mGeckoAccessible->NativeState(); PRUint64 > PRBool HasPopup () { >- PRUint32 state = 0; >- GetStateInternal(&state, nsnull); >+ PRUint32 state = NativeState(); > return (state & nsIAccessibleStates::STATE_HASPOPUP); > } PRUInt64, but you don't need local variable here >+PRUint64 >+nsHTMLWin32ObjectOwnerAccessible::NativeState() > { >- nsresult rv = nsAccessibleWrap::GetStateInternal(aState, aExtraState); >- if (rv == NS_OK_DEFUNCT_OBJECT) >- return rv; >- >+ PRUint64 states = nsAccessibleWrap::NativeState(); > // XXX: No HWND means this is windowless plugin which is not accessible in > // the meantime. > if (!mHwnd) >- *aState = nsIAccessibleStates::STATE_UNAVAILABLE; >+ states = states::UNAVAILABLE; the code is not equivalent >+PRUint64 >+nsXFormsAccessible::NativeState() > { >+ if (IsDefunct()) >+ return states::DEFUNCT; > > NS_ENSURE_TRUE(sXFormsService, NS_ERROR_FAILURE); watch for return value > > nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent)); > > PRBool isRelevant = PR_FALSE; > nsresult rv = sXFormsService->IsRelevant(DOMNode, &isRelevant); > NS_ENSURE_SUCCESS(rv, rv); the same >@@ -180,32 +172,31 @@ nsXFormsAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState) > PRBool isRequired = PR_FALSE; > rv = sXFormsService->IsRequired(DOMNode, &isRequired); > NS_ENSURE_SUCCESS(rv, rv); the same > > PRBool isValid = PR_FALSE; > rv = sXFormsService->IsValid(DOMNode, &isValid); > NS_ENSURE_SUCCESS(rv, rv); the same >+PRUint64 >+nsXFormsEditableAccessible::NativeState() > { >- rv = sXFormsService->IsReadonly(DOMNode, &isReadonly); >+ nsresult rv = sXFormsService->IsReadonly(DOMNode, &isReadonly); > NS_ENSURE_SUCCESS(rv, rv); the same > > if (!isReadonly) { > PRBool isRelevant = PR_FALSE; > rv = sXFormsService->IsRelevant(DOMNode, &isRelevant); > NS_ENSURE_SUCCESS(rv, rv); the same > if (isRelevant) { >- *aExtraState |= nsIAccessibleStates::EXT_STATE_EDITABLE | >- nsIAccessibleStates::EXT_STATE_SELECTABLE_TEXT; >+ states |= states::EDITABLE | >+ states::SELECTABLE_TEXT; proper indent > } > } > > nsCOMPtr<nsIEditor> editor; > GetAssociatedEditor(getter_AddRefs(editor)); > NS_ENSURE_TRUE(editor, NS_ERROR_FAILURE); return value! > bool > nsXFormsSelectableAccessible::RemoveItemFromSelection(PRUint32 aIndex) > { > nsCOMPtr<nsIDOMNode> itemDOMNode(do_QueryInterface(GetItemByIndex(&aIndex))); > if (!itemDOMNode) > return false; > >- nsresult rv; unused? >+PRUint64 >+nsXFormsInputBooleanAccessible::NativeState() > { >+ nsresult rv = sXFormsService->GetValue(DOMNode, value); > NS_ENSURE_SUCCESS(rv, rv); retval! >+PRUint64 >+nsXFormsRangeAccessible::NativeState() > { >+ PRUint64 states = nsXFormsAccessible::NativeState(); > > PRUint32 isInRange = nsIXFormsUtilityService::STATE_NOT_A_RANGE; > nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent)); >- rv = sXFormsService->IsInRange(DOMNode, &isInRange); >+ nsresult rv = sXFormsService->IsInRange(DOMNode, &isInRange); > NS_ENSURE_SUCCESS(rv, rv); retval! >+PRUint64 >+nsXFormsSelectAccessible::NativeState() >+ nsresult rv = sXFormsService->IsInRange(DOMNode, &isInRange); > NS_ENSURE_SUCCESS(rv, rv); retval >+PRUint64 >+nsXFormsSelectComboboxAccessible::NativeState() > { >+ nsresult rv = sXFormsService->IsDropmarkerOpen(DOMNode, &isOpen); > NS_ENSURE_SUCCESS(rv, rv); retval > > if (isOpen) >- *aState = nsIAccessibleStates::STATE_EXPANDED; >+ states = states::EXPANDED; > else >- *aState = nsIAccessibleStates::STATE_COLLAPSED; >+ states = states::COLLAPSED; error in existing code, should be states |= >+PRUint64 >+nsXFormsDropmarkerWidgetAccessible::NativeState() > { >+ if (IsDefunct()) >+ return states::DEFUNCT; > > PRBool isOpen = PR_FALSE; > nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent)); > nsresult rv = sXFormsService->IsDropmarkerOpen(DOMNode, &isOpen); > NS_ENSURE_SUCCESS(rv, rv); retval > >- if (isOpen) >- *aState = nsIAccessibleStates::STATE_PRESSED; >- >- return NS_OK; >+ return isOpen? states::PRESSED: 0; space after isOen >+PRUint64 >+nsXFormsComboboxPopupWidgetAccessible::NativeState() > { >+ PRUint64 states = nsXFormsAccessible::NativeState(); > > PRBool isOpen = PR_FALSE; > nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent)); >- rv = sXFormsService->IsDropmarkerOpen(DOMNode, &isOpen); >+ nsresult rv = sXFormsService->IsDropmarkerOpen(DOMNode, &isOpen); > NS_ENSURE_SUCCESS(rv, rv); retval >+PRUint64 >+nsXULDropmarkerAccessible::NativeState() > { >+ if (IsDefunct()) >+ return states::DEFUNCT; > >+ PRUint64 states = 0; > if (DropmarkerOpen(PR_FALSE)) >- *aState = nsIAccessibleStates::STATE_PRESSED; >+ states = states::PRESSED; > >- return NS_OK; >+ return states; return DropmarkerOpen(PR_FALSE) ? states::PRESSED : 0; > NS_IMETHODIMP nsXULCheckboxAccessible::GetActionName(PRUint8 aIndex, nsAString& aName) > { >- nsresult rv = GetStateInternal(&state, nsnull); >- NS_ENSURE_SUCCESS(rv, rv); >+ PRUint64 states = NativeState(); > >- if (state & nsIAccessibleStates::STATE_CHECKED) >+ if (states & nsIAccessibleStates::STATE_CHECKED) if (NativeState() & states::CHECKED) > /** > * Possible states: focused, focusable, unavailable(disabled), checked > */ move comment inside the method >+PRUint64 >+nsXULCheckboxAccessible::NativeState() > { > if (xulCheckboxElement) { > PRBool checked = PR_FALSE; > xulCheckboxElement->GetChecked(&checked); > if (checked) { >- *aState |= nsIAccessibleStates::STATE_CHECKED; >+ states |= states::CHECKED; correct indent >+PRUint64 >+nsXULToolbarSeparatorAccessible::NativeState() > { >+ if (IsDefunct()) >+ return states::DEFUNCT; > >+ return 0; to one line > NS_IMETHODIMP nsXULTextFieldAccessible::GetValue(nsAString& aValue) > { >+PRUint64 >+nsXULTextFieldAccessible::NativeState() > { >- nsresult rv = nsHyperTextAccessibleWrap::GetStateInternal(aState, >- aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 states = nsHyperTextAccessibleWrap::NativeState(); > >- rv = tempAccessible->GetStateInternal(aState, nsnull); >- NS_ENSURE_SUCCESS(rv, rv); >+ states &= 0xffffffff; // for same behavior as GetStateInternal wrt extra states >+ states |= tempAccessible->NativeState() & (~0xffffffff); just apply all of them > // <xul:textbox> > if (mContent->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type, > nsAccessibilityAtoms::password, eIgnoreCase)) { >- *aState |= nsIAccessibleStates::STATE_PROTECTED; >+states |= states::PROTECTED; fix indent > class nsXULRadioButtonAccessible : public nsRadioButtonAccessible > { > > public: > nsXULRadioButtonAccessible(nsIContent *aContent, nsIWeakReference *aShell); > > // nsAccessible >- virtual nsresult GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState); > virtual void GetPositionAndSizeInternal(PRInt32 *aPosInSet, > PRInt32 *aSetSize); >+ PRUint64 NativeState(); virtual > NS_IMETHODIMP nsXULListitemAccessible::GetActionName(PRUint8 aIndex, nsAString& aName) > { >- if (state & nsIAccessibleStates::STATE_CHECKED) >+ if (states & nsIAccessibleStates::STATE_CHECKED) states::CHECKED >+PRUint64 >+nsXULMenuitemAccessible::NativeState() > { >- nsresult rv = nsAccessible::GetStateInternal(aState, aExtraState); > PRBool isComboboxOption = (Role() == nsIAccessibleRole::ROLE_COMBOBOX_OPTION); > if (isComboboxOption) { > // Is selected? > PRBool isSelected = PR_FALSE; > nsCOMPtr<nsIDOMXULSelectControlItemElement> > item(do_QueryInterface(mContent)); > NS_ENSURE_TRUE(item, NS_ERROR_FAILURE); retval > // Selected and collapsed? > if (isCollapsed) { > // Set selected option offscreen/invisible according to combobox state > nsAccessible* grandParentAcc = parentAcc->GetParent(); > NS_ENSURE_TRUE(grandParentAcc, NS_ERROR_FAILURE); retval > NS_ASSERTION(grandParentAcc->Role() == nsIAccessibleRole::ROLE_COMBOBOX, > "grandparent of combobox listitem is not combobox"); >- PRUint32 grandParentState, grandParentExtState; >- grandParentAcc->GetState(&grandParentState, &grandParentExtState); >+ PRUint64 grandParentState = grandParentAcc->NativeState(); NativeState is not GetState() >+ states &= ~(states::OFFSCREEN | >+ states::INVISIBLE); one line >+ states |= (grandParentState & states::OFFSCREEN) | >+ (grandParentState & states::INVISIBLE); >+ states |= grandParentState & states::OPAQUE; fix indent >- *aState|= (nsIAccessibleStates::STATE_FOCUSABLE | >- nsIAccessibleStates::STATE_SELECTABLE); >+ states|= (states::FOCUSABLE | states::SELECTABLE); space after states >+PRUint64 >+nsXULMenuSeparatorAccessible::NativeState() > { >- *aState &= (nsIAccessibleStates::STATE_OFFSCREEN | >- nsIAccessibleStates::STATE_INVISIBLE); >+ states &= (states::OFFSCREEN | >+ states::INVISIBLE); one line >+PRUint64 >+nsXULMenupopupAccessible::NativeState() > { >+ if (states & states::INVISIBLE) >+ states |= states::OFFSCREEN | states::COLLAPSED; indent! >+PRUint64 >+nsXULTabAccessible::NativeState() > { > // Check whether the tab is selected >- *aState |= nsIAccessibleStates::STATE_SELECTABLE; >- *aState &= ~nsIAccessibleStates::STATE_SELECTED; >+ states |= states::SELECTABLE; >+ states &= states::SELECTED; not equivalent! >+PRUint64 >+nsXULTreeAccessible::NativeState() > { > // multiselectable state. > nsCOMPtr<nsITreeSelection> selection; > mTreeView->GetSelection(getter_AddRefs(selection)); > NS_ENSURE_STATE(selection); retval > > PRBool isSingle = PR_FALSE; >- rv = selection->GetSingle(&isSingle); >+ nsresult rv = selection->GetSingle(&isSingle); > NS_ENSURE_SUCCESS(rv, rv); retval >+PRUint64 >+nsXULTreeItemAccessibleBase::NativeState() > { >+ states |= isContainerOpen ? states::EXPANDED: states::COLLAPSED; space after states::EXPANDED >+PRUint64 >+nsXULTreeGridCellAccessible::NativeState() > { >@@ -1232,17 +1224,17 @@ nsXULTreeGridCellAccessible::CellInvalidated() > if (type == nsITreeColumn::TYPE_CHECKBOX) { > mTreeView->GetCellValue(mRow, mColumn, textEquiv); > if (mCachedTextEquiv != textEquiv) { > PRBool isEnabled = textEquiv.EqualsLiteral("true"); > nsRefPtr<AccEvent> accEvent = >- new AccStateChangeEvent(this, nsIAccessibleStates::STATE_CHECKED, >+ new AccStateChangeEvent(this, states::CHECKED, > PR_FALSE, isEnabled); one line if possible

Comment on attachment 513946 [details] [diff] [review] change GetState() to State() >- PRUint32 state; >- aAccessible->GetState(&state, nsnull); >+ PRUint64 state = aAccessible->State(); > if (state & nsIAccessibleStates::STATE_HASPOPUP) { use states::HASPOPUP > // Map states >- PRUint32 accState = 0, accExtState = 0; >- nsresult rv = accWrap->GetState(&accState, &accExtState); >- NS_ENSURE_SUCCESS(rv, state_set); >+ PRUint64 accState = accWrap->State(); > > TranslateStates(accState, gAtkStateMap, state_set); > TranslateStates(accExtState, gAtkStateMapExt, state_set); I'm totally confused with where you're going to fix that or this. Do you plan to handle TranslateStates separately? >@@ -243,18 +243,17 @@ AccStateChangeEvent:: >- PRUint32 state = 0, extraState = 0; >- accessible->GetState(&state, mIsExtraState ? &extraState : nsnull); >+ PRUint64 state = accessible->state(); > mIsEnabled = ((mIsExtraState ? extraState : state) & mState) != 0; > } else { > mIsEnabled = PR_FALSE; do I get right the separate patch for AccStateChangeEvent is expected? >-NS_IMETHODIMP >-nsAccessible::GetState(PRUint32 *aState, PRUint32 *aExtraState) new patch to reintroduce GetState() for nsIAccessible implementation is planned? >+PRUint64 >+nsAccessible::State() > { > if (mRoleMapEntry && mRoleMapEntry->role == nsIAccessibleRole::ROLE_PAGETAB) { >- if (*aState & nsIAccessibleStates::STATE_FOCUSED) { >- *aState |= nsIAccessibleStates::STATE_SELECTED; >+ if (states & nsIAccessibleStates::STATE_FOCUSED) { >+ states |= nsIAccessibleStates::STATE_SELECTED; use states namespace, here and below. Or are you going to handle it separately? > /** > * Return the states of accessible, not taking into account ARIA states. > * if aExtraStates is FALSe extra states will not be calculated. > */ > virtual PRUint64 NativeState(); > > /** >+ * Calculate states including ARIA states >+ */ >+ virtual PRUint64 State(); >+ please define it before NativeState() > // nsIAccessible > NS_IMETHOD GetParent(nsIAccessible **aParent); > NS_IMETHOD GetNextSibling(nsIAccessible **aNextSibling); > NS_IMETHOD GetPreviousSibling(nsIAccessible **aPreviousSibling); > NS_IMETHOD GetName(nsAString &aName); > NS_IMETHOD GetValue(nsAString &aValue); > NS_IMETHOD GetDescription(nsAString &aDescription); > NS_IMETHOD GetKeyboardShortcut(nsAString &aKeyboardShortcut); >- virtual nsresult GetState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual PRUint64 State(); please move it nsAccessible part >@@ -576,17 +576,19 @@ nsIContent* nsHTMLSelectOptionAccessible::GetSelectState(PRUint32* aState, > nsIContent *content = mContent; > while (content && content->Tag() != nsAccessibilityAtoms::select) { > content = content->GetParent(); > } > > if (content) { > nsAccessible* selAcc = GetAccService()->GetAccessible(content); > if (selAcc) { >- selAcc->GetState(aState, aExtraState); >+ PRUint64 states = selAcc->State(); >+ *aState = states & 0xffffffff; >+ *aExtraState = states >> 32; > return content; new separate patch for GetSelectState, right? > - (BOOL)isFocused > { >- PRUint32 state = 0; >- mGeckoAccessible->GetState (&state, nsnull); >+ PRUint64 state = mGeckoAccessible->State(); > return (state & nsIAccessibleStates::STATE_FOCUSED) != 0; one line > } > > - (BOOL)canBeFocused > { >- PRUint32 state = 0; >- mGeckoAccessible->GetState (&state, nsnull); >- return (state & nsIAccessibleStates::STATE_FOCUSABLE) != 0; >-} >+ PRUint64 state = mGeckoAccessible->State(); too much of removals? > > - (BOOL)focus > { > nsresult rv = mGeckoAccessible->TakeFocus(); > return NS_SUCCEEDED(rv); > } > > - (BOOL)isEnabled > { >- PRUint32 state = 0; >- mGeckoAccessible->GetState (&state, nsnull); >+ PRUint64 state = mGeckoAccessible->State(); > return (state & nsIAccessibleStates::STATE_UNAVAILABLE) == 0; one line? > - (BOOL)isReadOnly > { > if (mGeckoEditableTextAccessible) { >- PRUint32 state = 0; >- mGeckoAccessible->GetState(&state, nsnull); >+ PRUint64 state = mGeckoAccessible->State(); > return (state & nsIAccessibleStates::STATE_READONLY) == 0; the same > } > > return NO; > > NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NO); > } > >diff --git a/accessible/src/msaa/CAccessibleComponent.cpp b/accessible/src/msaa/CAccessibleComponent.cpp >index 3e180c0..1319e25 100644 >--- a/accessible/src/msaa/CAccessibleComponent.cpp >+++ b/accessible/src/msaa/CAccessibleComponent.cpp >@@ -85,20 +85,17 @@ __try { > *aX = 0; > *aY = 0; > > nsCOMPtr<nsIAccessible> acc(do_QueryObject(this)); > if (!acc) > return E_FAIL; > > // If the object is not on any screen the returned position is (0,0). >- PRUint32 states = 0; >- nsresult rv = acc->GetState(&states, nsnull); >- if (NS_FAILED(rv)) >- return GetHRESULT(rv); >+ PRUint64 states = acc->State(); turn acc into nsAccessible, otherwise it doesn't work >- PRUint32 state = 0; >- if (NS_FAILED(xpAccessible->GetState(&state, nsnull))) >- return E_FAIL; >+ PRUint64 state = xpAccessible->State(); > > pvarState->lVal = state; some comments about assumptions please, why it's possible to do

Comment on attachment 513947 [details] [diff] [review] rename GetARIAState() to ApplyARIAState() I assume nsIAccessibleStates:: -> states:: will be handled in separate patch. >+void >+nsARIAGridCellAccessible::ApplyARIAState(PRUint64 *aState) PRUint64 *aState -> PRUint64* aState > { >- return NS_OK; >+ return; no return is needed > // nsAccessible >- virtual nsresult GetARIAState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual void ApplyARIAState(PRUint64 *aState); PRUint64 *aState -> PRUint64* aState >+void >+nsAccessible::ApplyARIAState(PRUint64 *aStates) PRUint64 *aStates -> PRUint64* aState >- virtual nsresult GetARIAState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual void ApplyARIAState(PRUint64 *aState); PRUint64 *aState -> PRUint64* aState >+void >+nsApplicationAccessible::ApplyARIAState(PRUint64 *aState) the same > // nsAccessible >- virtual nsresult GetARIAState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual void ApplyARIAState(PRUint64 *aState); the same >+void >+nsDocAccessible::ApplyARIAState(PRUint64 *aState) same >+ virtual void ApplyARIAState(PRUint64 *aState); ^ >+void >+nsXULTextFieldAccessible::ApplyARIAState(PRUint64 *aState) > { >- nsresult rv = nsHyperTextAccessibleWrap::GetARIAState(aState, aExtraState); >- NS_ENSURE_SUCCESS(rv, rv); >+ nsHyperTextAccessibleWrap::ApplyARIAState(aState); > > nsStateMapEntry::MapToStates(mContent, aState, aExtraState, eARIAAutoComplete); new patch for that, right? > >- return NS_OK; >+ return; no return please > // nsAccessible >- virtual nsresult GetARIAState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual void ApplyARIAState(PRUint64 *aState); ^ r=me with nits fixed

Comment on attachment 513948 [details] [diff] [review] replace nsAccUtils::State() and nsAccUtils::ExtendedState() with nsAccessible::State() >- PRUint32 state = nsAccUtils::State(this); >+ PRUint64 state = this->State(); this->State() is crazy :) fix it here and other places >- PRUint32 actionRule = GetActionRule(nsAccUtils::State(this)); >+ PRUint64 actionRule = GetActionRule(this->State()); I assume GetActionRule will be fixed in next patch. r=me with this->State() fixed

(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > > > > > perhaps "has the focus" > > > > > > > > That's what I currently have and it seems reasonable, but fyi the msdn docs > > > > use the old wording word for word as it was in that comment. > > > > > > what does ATK say? > > > > it appears to be a copy and paste from msaa, refering to keyboard and typeing > > with characters entered. > > the focused state is mapped to atk, so it should suitable both for msaa and atk ok, the old wording seems closer to what both msaa and atk use, so I went back to that.

(In reply to comment #20) > Comment on attachment 513945 [details] [diff] [review] > (wip) start implementing nativeState() > >+PRUint64 > >+nsHTMLCheckboxAccessible::NativeState() > > { > > >+ PRUint64 states = nsFormControlAccessible::NativeState(); > > > >- *aState |= nsIAccessibleStates::STATE_CHECKABLE; > >+ states |= states::CHECKABLE; > > perhaps you should do > PRUint64 states = nsFormControlAccessible::NativeState() |= states::CHECKABLE; > if so then please fix it in other places I'm not going to say its rational, but I'm not sure I'm a fan of the |= on the same line as the function call like that. However, I can live with it. > > PRUint32 state = 0; > > nsCOMPtr<nsIContent> content = GetSelectState(&state); > >- if (state & nsIAccessibleStates::STATE_COLLAPSED) { > >+ if (state & states::COLLAPSED) { > > I hope GetSelectState is changed in some next patch and state is turned into > PRUint64 yup, it will be, that's a fair gues in these cases. > > bool > > nsXFormsSelectableAccessible::RemoveItemFromSelection(PRUint32 aIndex) > > { > > nsCOMPtr<nsIDOMNode> itemDOMNode(do_QueryInterface(GetItemByIndex(&aIndex))); > > if (!itemDOMNode) > > return false; > > > >- nsresult rv; > > unused? yup

(In reply to comment #25) > (In reply to comment #20) > > Comment on attachment 513945 [details] [diff] [review] > > (wip) start implementing nativeState() > > > >+PRUint64 > > >+nsHTMLCheckboxAccessible::NativeState() > > > { > > > > >+ PRUint64 states = nsFormControlAccessible::NativeState(); > > > > > >- *aState |= nsIAccessibleStates::STATE_CHECKABLE; > > >+ states |= states::CHECKABLE; > > > > perhaps you should do > > PRUint64 states = nsFormControlAccessible::NativeState() |= states::CHECKABLE; > > if so then please fix it in other places > > I'm not going to say its rational, but I'm not sure I'm a fan of the |= on the > same line as the function call like that. However, I can live with it. (just for correctness) PRUint64 states = nsFormControlAccessible::NativeState() | states::CHECKABLE; it's not scary for me and I think this is nice actually, but if you prefer to keep them on separate lines then ok, up to you.

add states.h defining accessible states

(wip) start implementing nativeState()

change GetState() to State()

rename GetARIAState() to ApplyARIAState()

replace nsAccUtils::State() and nsAccUtils::ExtendedState() with nsAccessible::State()

Remove aExtraState from nsStateMapEntry

convert GetActionRule() to take a PRUint64 of states

change the atk state map and translateStates() to use 64 bitmaps.

change references to nsIAccessibleStates::{,EXT_}STATE_xxx to states::xxxx

remove extraState from AccEvents

Comment on attachment 516171 [details] [diff] [review] part 1 States.h >+ /** >+ * The object's children are displayed, the oposite of collapsed. >+ */ >+ const PRUint64 EXPANDED = 0x200; >+ /** >+ * The object's children are not displayed, the oposite of expanded. >+ */ >+ const PRUint64 COLLAPSED = 0x400; expanded collapsed aren't applied to any object, in other words, if children are displayed then it doesn't mean expanded/collapsed states. perhaps you should say 'the expandable object's children are displayed, applied to trees, lists and other controls', also add a reference to expandable state like @see EXPANDABLE state >+ /** >+ * The object can be checked. >+ */ >+ const PRUint64 CHECKABLE = 0x2000; perhaps in description of checked state you should say like 'The checkable object is checked, applied to checkbox controls, @see checkable, mixed states' >+ /** >+ * the editable area has some kind of autocompletion. the -> The >+ /** >+ * Indicates that the object is modal. Modal objects have the behavior excess space between sentences. >+/** >+ * Edit control that can take multiple lines. >+ */ >+ const PRUint64 MULTI_LINE = 0x2000000000; >+ >+/** >+ * Uses horizontal layout. >+ */ >+ const PRUint64 HORIZONTAL = 0x4000000000; fix indent of these comments >+ /** >+ * same as enabled for now see bug 636158 same -> Same, enabled -> ENABLED state >+ */ >+ const PRUint64 SENSITIVE = 0x200000000000; >+ >+ /** >+ * Can be expanded or collapsed, see the description of the exanded and >+ * collapsed states. exanded -> expanded just @see EXPANDED and COLLAPSED states.

Comment on attachment 516173 [details] [diff] [review] part 2 GetStateInternal() -> NativeState() >- * Return the state of accessible that doesn't take into account ARIA states. >- * Use nsIAccessible::state to get all states for accessible. If perhaps 'Use State() to get complete states'? > >+#include "States.h" > #include "nsAccessibilityService.h" > #include "nsAccUtils.h" in general you should add 'States.h' in alphabet order in a11y section of header files, here and in other files. Perhaps it's ok for now but keep this in mind for next time >--- a/accessible/src/base/nsApplicationAccessible.h >+++ b/accessible/src/base/nsApplicationAccessible.h >- NS_IMETHOD GetState(PRUint32 *aState , PRUint32 *aExtraState ); >+ NS_IMETHOD nsresult GetState(PRUint32 *aState, PRUint32 *aExtraState); revert this change please, NS_IMETHOD defines nsresult return value >+PRUint64 >+nsDocAccessible::NativeState() >+ PRUint64 state = (mContent->GetCurrentDoc() == mDocument) ? 0 : >+ states::STALE; fix indent, does it break 80 line restriction? >+PRUint64 >+nsHTMLTableAccessible::NativeState() > { >- nsresult rv= nsAccessible::GetStateInternal(aState, aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 state = nsAccessible::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_READONLY; >- return NS_OK; >+ state |= states::READONLY; >+ return state; > } I would prefer 'return nsAccessible::NativeState() | states::READONLY;' >@@ -1362,19 +1360,18 @@ nsHTMLTableAccessible::IsProbablyForLayout(PRBool *aIsProbablyForLayout) >- PRUint32 state, extState; >- docAccessible->GetState(&state, &extState); >- if (extState & nsIAccessibleStates::EXT_STATE_EDITABLE) { // Need to see all elements while document is being edited >+ PRUint64 docState = docAccessible->NativeState(); GetState() isn't equivalent to NativeState() in general, is it for editable state? >+PRUint64 >+nsHTMLLIAccessible::NativeState() > { >- nsresult rv = nsHyperTextAccessibleWrap::GetStateInternal(aState, >- aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 state = nsHyperTextAccessibleWrap::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_READONLY; >- return NS_OK; >+ state |= states::READONLY; >+ return state; inline return perhaps? >+PRUint64 >+nsHTMLListAccessible::NativeState() > { >- nsresult rv = nsHyperTextAccessibleWrap::GetStateInternal(aState, >- aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 state = nsHyperTextAccessibleWrap::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_READONLY; >- return NS_OK; >+ state |= states::READONLY; >+ return state; here too. >+PRUint64 >+nsHTMLWin32ObjectOwnerAccessible::NativeState() > { >- nsresult rv = nsAccessibleWrap::GetStateInternal(aState, aExtraState); >- if (rv == NS_OK_DEFUNCT_OBJECT) >- return rv; >- >+ PRUint64 state = nsAccessibleWrap::NativeState(); > // XXX: No HWND means this is windowless plugin which is not accessible in > // the meantime. > if (!mHwnd) >- *aState = nsIAccessibleStates::STATE_UNAVAILABLE; >+ state = states::UNAVAILABLE; > >- return rv; >+ return state; return mHWnd ? nsAccessibleWrap::NativeState() : states::UNAVAILABLE; >+PRUint64 >+nsXFormsSecretAccessible::NativeState() > { >- nsresult rv = nsXFormsInputAccessible::GetStateInternal(aState, aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 states = nsXFormsInputAccessible::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_PROTECTED; >- return NS_OK; >+ states |= states::PROTECTED; >+ return states; > } really, we don't need extra local variable here, do inline return. >+PRUint64 >+nsXFormsSelectComboboxAccessible::NativeState() > { >+ state |= states::HASPOPUP | states::FOCUSABLE; >+ return state; perhaps return state | states::HASPOPUP | states:FOCUSABLE; >+PRUint64 >+nsXULAlertAccessible::NativeState() > { >- nsresult rv = nsAccessible::GetStateInternal(aState, aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 states = nsAccessible::NativeState(); > > // XUL has no markup for low, medium or high >- *aState |= nsIAccessibleStates::STATE_ALERT_MEDIUM; >- return NS_OK; >+ states |= states::ALERT; >+ >+ return states; inline it please too >+PRUint64 >+nsXULColumnsAccessible::NativeState() > { >- NS_ENSURE_ARG_POINTER(aState); >- *aState = 0; >- >- if (IsDefunct()) { >- if (aExtraState) >- *aExtraState = nsIAccessibleStates::EXT_STATE_DEFUNCT; >- >- return NS_OK_DEFUNCT_OBJECT; >- } >- >- *aState = nsIAccessibleStates::STATE_READONLY; > >- if (aExtraState) >- *aExtraState = 0; >+ if (IsDefunct()) >+ return states::DEFUNCT; > >- return NS_OK; >+ return states::READONLY; inline it please return IsDefunct() ? ... >+PRUint64 >+nsXULColumnItemAccessible::NativeState() > { >+ if (IsDefunct()) >+ return states::DEFUNCT; > >+ return states::READONLY; the same >+PRUint64 >+nsXULMenuitemAccessible::NativeState() > { >+ PRUint64 grandParentState = grandParentAcc->GetState(); >+ state &= ~(states::OFFSCREEN | states::INVISIBLE); >+ state |= (grandParentState & states::OFFSCREEN) | >+ (grandParentState & states::INVISIBLE); correct indent please >+PRUint64 >+nsXULLinkAccessible::NativeState() > { >- nsresult rv = nsHyperTextAccessibleWrap::GetStateInternal(aState, >- aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 states = nsHyperTextAccessibleWrap::NativeState(); > >- *aState |= nsIAccessibleStates::STATE_LINKED; >- return NS_OK; >+ states |= states::LINKED; >+ return states; inline it please as well r=me with comments addressed

Comment on attachment 516174 [details] [diff] [review] part 3 GetState() -> State() > /** >+ * Calculate states including ARIA states >+ */ >+ virtual PRUint64 State(); Return states of the accessible? To make it similar to NativeState() description. >+ /** > * Return the states of accessible, not taking into account ARIA states. > */ > virtual PRUint64 NativeState(); >--- a/accessible/src/base/nsApplicationAccessible.h >+++ b/accessible/src/base/nsApplicationAccessible.h >@@ -90,16 +90,17 @@ public: > NS_IMETHOD GetParent(nsIAccessible **aParent); > NS_IMETHOD GetNextSibling(nsIAccessible **aNextSibling); > NS_IMETHOD GetPreviousSibling(nsIAccessible **aPreviousSibling); > NS_IMETHOD GetName(nsAString &aName); > NS_IMETHOD GetValue(nsAString &aValue); > NS_IMETHOD GetDescription(nsAString &aDescription); > NS_IMETHOD GetKeyboardShortcut(nsAString &aKeyboardShortcut); > NS_IMETHOD nsresult GetState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual PRUint64 State(); GetState should be removed

Comment on attachment 516175 [details] [diff] [review] part 4 GetARIASTATE() -> ApplyARIAState() >+void >+nsAccessible::ApplyARIAState(PRUint64* aStates) aStates -> aState

add states.h defining accessible states

Comment on attachment 516177 [details] [diff] [review] part 6 remove AExtraState from nsStateMap > PRBool > nsStateMapEntry::MapToStates(nsIContent *aContent, >- PRUint32 *aState, PRUint32 *aExtraState, >+ PRUint64 *aState, PRUint64 *aState -> PRUint64* aState >+ if (entry.mExtraState1) >+ *aState |= entry.mExtraState1; there's no mExtraState1 and other extra states

Comment on attachment 516179 [details] [diff] [review] part 8 atk state map >-static void TranslateStates(PRUint32 aState, const AtkStateMap *aStateMap, >+static void TranslateStates(PRUint64 aState, const AtkStateMap *aStateMap, > AtkStateSet *aStateSet) static void on separate line AtkStateMap *aStateMap -> AtkStateMap* aStateMap > { > NS_ASSERTION(aStateSet, "Can't pass in null state set"); > > // Convert every state to an entry in AtkStateMap > PRUint32 stateIndex = 0; >- PRUint32 bitMask = 1; >+ PRUint64 bitMask = 1; > while (aStateMap[stateIndex].stateMapEntryType != kNoSuchState) { > if (aStateMap[stateIndex].atkState) { // There's potentially an ATK state for this > PRBool isStateOn = (aState & bitMask) != 0; > if (aStateMap[stateIndex].stateMapEntryType == kMapOpposite) { > isStateOn = !isStateOn; > } > if (isStateOn) { > atk_state_set_add_state(aStateSet, aStateMap[stateIndex].atkState); remove '// Map extended state' comment below > // Map states > PRUint64 accState = accWrap->State(); > > TranslateStates(accState, gAtkStateMap, state_set); >- TranslateStates(accExtState, gAtkStateMapExt, state_set); TranslateStates(accWrap->State(), ...) >+ { ATK_STATE_EXPANDABLE, kMapDirectly }, // nsIAccessibleStates::EXT_STATE_EXPANDABLE = 0x400000000000 >+ { kNone, kNoSuchState }, // = 0x800000000000 >+ { kNone, kNoSuchState }, // = 0x1000000000000 >+ { kNone, kNoSuchState }, // = 0x2000000000000 >+ { kNone, kNoSuchState }, // = 0x4000000000000 >+ { kNone, kNoSuchState }, // = 0x8000000000000 >+ { kNone, kNoSuchState }, // = 0x10000000000000 >+ { kNone, kNoSuchState }, // = 0x20000000000000 >+ { kNone, kNoSuchState }, // = 0x40000000000000 >+ { kNone, kNoSuchState }, // = 0x80000000000000 >+ { kNone, kNoSuchState }, // = 0x100000000000000 >+ { kNone, kNoSuchState }, // = 0x200000000000000 >+ { kNone, kNoSuchState }, // = 0x400000000000000 >+ { kNone, kNoSuchState }, // = 0x800000000000000 >+ { kNone, kNoSuchState }, // = 0x1000000000000000 >+ { kNone, kNoSuchState }, // = 0x2000000000000000 >+ { kNone, kNoSuchState }, // = 0x4000000000000000 >+ { kNone, kNoSuchState }, // = 0x80000000000000 you need only one kNone item, alternatively you could use array size in TranslateState > };

Comment on attachment 516181 [details] [diff] [review] part 9 nsIAccessibleStates -> states >change references to nsIAccessibleStates::{,EXT_}STATE_xxx to states::xxxx > >diff --git a/accessible/src/atk/nsAccessibleWrap.cpp b/accessible/src/atk/nsAccessibleWrap.cpp >index e7702a3d..82e3b59 100644 >--- a/accessible/src/atk/nsAccessibleWrap.cpp >+++ b/accessible/src/atk/nsAccessibleWrap.cpp >@@ -800,17 +800,17 @@ AtkAttributeSet * > GetAttributeSet(nsIAccessible* aAccessible) > { > nsCOMPtr<nsIPersistentProperties> attributes; > aAccessible->GetAttributes(getter_AddRefs(attributes)); > > if (attributes) { > // Deal with attributes that we only need to expose in ATK > PRUint64 state = aAccessible->State(); >- if (state & nsIAccessibleStates::STATE_HASPOPUP) { >+ if (state & states::HASPOPUP) { > // There is no ATK state for haspopup, must use object attribute to expose the same info > nsAutoString oldValueUnused; > attributes->SetStringProperty(NS_LITERAL_CSTRING("haspopup"), NS_LITERAL_STRING("true"), > oldValueUnused); > } > > return ConvertToAtkAttributeSet(attributes); > } >@@ -931,18 +931,17 @@ static void TranslateStates(PRUint64 aState, const AtkStateMap *aStateMap, > AtkStateSet * > refStateSetCB(AtkObject *aAtkObj) > { > AtkStateSet *state_set = nsnull; > state_set = ATK_OBJECT_CLASS(parent_class)->ref_state_set(aAtkObj); > > nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj); > if (!accWrap) { >- TranslateStates(nsIAccessibleStates::EXT_STATE_DEFUNCT, >- gAtkStateMap, state_set); >+ TranslateStates(states::DEFUNCT, gAtkStateMap, state_set); > return state_set; > } > > // Map states > PRUint64 accState = accWrap->State(); > > TranslateStates(accState, gAtkStateMap, state_set); > >@@ -1077,18 +1076,17 @@ nsAccessibleWrap::FirePlatformEvent(AccEvent* aEvent) > case nsIAccessibleEvent::EVENT_FOCUS: > { > MAI_LOG_DEBUG(("\n\nReceived: EVENT_FOCUS\n")); > nsRefPtr<nsRootAccessible> rootAccWrap = accWrap->GetRootAccessible(); > if (rootAccWrap && rootAccWrap->mActivated) { > atk_focus_tracker_notify(atkObj); > // Fire state change event for focus > nsRefPtr<AccEvent> stateChangeEvent = >- new AccStateChangeEvent(accessible, >- nsIAccessibleStates::STATE_FOCUSED, >+ new AccStateChangeEvent(accessible, states::FOCUSED, > PR_FALSE, PR_TRUE); > return FireAtkStateChangeEvent(stateChangeEvent, atkObj); > } > } break; > > case nsIAccessibleEvent::EVENT_VALUE_CHANGE: > { > MAI_LOG_DEBUG(("\n\nReceived: EVENT_VALUE_CHANGE\n")); >diff --git a/accessible/src/base/AccGroupInfo.cpp b/accessible/src/base/AccGroupInfo.cpp >index 930d9d0..23f13ce 100644 >--- a/accessible/src/base/AccGroupInfo.cpp >+++ b/accessible/src/base/AccGroupInfo.cpp >@@ -55,17 +55,17 @@ AccGroupInfo::AccGroupInfo(nsAccessible* aItem, PRUint32 aRole) : > PRUint32 siblingRole = sibling->Role(); > > // If the sibling is separator then the group is ended. > if (siblingRole == nsIAccessibleRole::ROLE_SEPARATOR) > break; > > // If sibling is not visible and hasn't the same base role. > if (BaseRole(siblingRole) != aRole || >- sibling->State() & nsIAccessibleStates::STATE_INVISIBLE) >+ sibling->State() & states::INVISIBLE) > continue; > > // Check if it's hierarchical flatten structure, i.e. if the sibling > // level is lesser than this one then group is ended, if the sibling level > // is greater than this one then the group is split by some child elements > // (group will be continued). > PRInt32 siblingLevel = nsAccUtils::GetARIAOrDefaultLevel(sibling); > if (siblingLevel < level) { >@@ -99,17 +99,17 @@ AccGroupInfo::AccGroupInfo(nsAccessible* aItem, PRUint32 aRole) : > PRUint32 siblingRole = sibling->Role(); > > // If the sibling is separator then the group is ended. > if (siblingRole == nsIAccessibleRole::ROLE_SEPARATOR) > break; > > // If sibling is visible and has the same base role > if (BaseRole(siblingRole) != aRole || >- sibling->State() & nsIAccessibleStates::STATE_INVISIBLE) >+ sibling->State() & states::INVISIBLE) > continue; > > // and check if it's hierarchical flatten structure. > PRInt32 siblingLevel = nsAccUtils::GetARIAOrDefaultLevel(sibling); > if (siblingLevel < level) > break; > > // Skip subset. >diff --git a/accessible/src/base/filters.cpp b/accessible/src/base/filters.cpp >index 04dde11..962ca32 100644 >--- a/accessible/src/base/filters.cpp >+++ b/accessible/src/base/filters.cpp >@@ -38,23 +38,23 @@ > #include "filters.h" > > #include "nsAccessible.h" > #include "nsAccUtils.h" > > bool > filters::GetSelected(nsAccessible* aAccessible) > { >- return aAccessible->State() & nsIAccessibleStates::STATE_SELECTED; >+ return aAccessible->State() & states::SELECTED; > } > > bool > filters::GetSelectable(nsAccessible* aAccessible) > { >- return aAccessible->State() & nsIAccessibleStates::STATE_SELECTABLE; >+ return aAccessible->State() & states::SELECTABLE; > } > > bool > filters::GetRow(nsAccessible* aAccessible) > { > return aAccessible->Role() == nsIAccessibleRole::ROLE_ROW; > } > >diff --git a/accessible/src/base/nsARIAGridAccessible.cpp b/accessible/src/base/nsARIAGridAccessible.cpp >index 7a8be21..f46274f 100644 >--- a/accessible/src/base/nsARIAGridAccessible.cpp >+++ b/accessible/src/base/nsARIAGridAccessible.cpp >@@ -1110,32 +1110,31 @@ nsARIAGridCellAccessible::IsSelected(PRBool *aIsSelected) > // nsAccessible > > void > nsARIAGridCellAccessible::ApplyARIAState(PRUint64* aState) > { > nsHyperTextAccessibleWrap::ApplyARIAState(aState); > > // Return if the gridcell has aria-selected="true". >- if (*aState & nsIAccessibleStates::STATE_SELECTED) >+ if (*aState & states::SELECTED) > return; > > // Check aria-selected="true" on the row. > nsAccessible* row = GetParent(); > if (!row || row->Role() != nsIAccessibleRole::ROLE_ROW) > return; > > nsIContent *rowContent = row->GetContent(); > if (nsAccUtils::HasDefinedARIAToken(rowContent, > nsAccessibilityAtoms::aria_selected) && > !rowContent->AttrValueIs(kNameSpaceID_None, > nsAccessibilityAtoms::aria_selected, > nsAccessibilityAtoms::_false, eCaseMatters)) >- *aState |= nsIAccessibleStates::STATE_SELECTABLE | >- nsIAccessibleStates::STATE_SELECTED; >+ *aState |= states::SELECTABLE | states::SELECTED; > } > > nsresult > nsARIAGridCellAccessible::GetAttributesInternal(nsIPersistentProperties *aAttributes) > { > if (IsDefunct()) > return NS_ERROR_FAILURE; > >diff --git a/accessible/src/base/nsARIAMap.cpp b/accessible/src/base/nsARIAMap.cpp >index d6867cc..689677d 100644 >--- a/accessible/src/base/nsARIAMap.cpp >+++ b/accessible/src/base/nsARIAMap.cpp >@@ -35,17 +35,17 @@ > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsARIAMap.h" > > #include "nsIAccessibleRole.h" >-#include "nsIAccessibleStates.h" >+#include "States.h" > > #include "nsAccessibilityAtoms.h" > #include "nsIContent.h" > > /** > * This list of WAI-defined roles are currently hardcoded. > * Eventually we will most likely be loading an RDF resource that contains this information > * Using RDF will also allow for role extensibility. See bug 280138. >@@ -91,17 +91,17 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "article", > nsIAccessibleRole::ROLE_DOCUMENT, > kUseMapRole, > eNoValue, > eNoAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_READONLY >+ states::READONLY > }, > { > "button", > nsIAccessibleRole::ROLE_PUSHBUTTON, > kUseMapRole, > eNoValue, > eClickAction, > eNoLiveAttr, >@@ -132,17 +132,17 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "combobox", > nsIAccessibleRole::ROLE_COMBOBOX, > kUseMapRole, > eHasValueMinMax, > eOpenCloseAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_COLLAPSED | nsIAccessibleStates::STATE_HASPOPUP, >+ states::COLLAPSED | states::HASPOPUP, > eARIAAutoComplete, > eARIAReadonly > }, > { > "dialog", > nsIAccessibleRole::ROLE_DIALOG, > kUseMapRole, > eNoValue, >@@ -161,26 +161,26 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "document", > nsIAccessibleRole::ROLE_DOCUMENT, > kUseMapRole, > eNoValue, > eNoAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_READONLY >+ states::READONLY > }, > { > "grid", > nsIAccessibleRole::ROLE_TABLE, > kUseMapRole, > eNoValue, > eNoAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_FOCUSABLE, >+ states::FOCUSABLE, > eARIAMultiSelectable, > eARIAReadonly > }, > { > "gridcell", > nsIAccessibleRole::ROLE_GRID_CELL, > kUseMapRole, > eNoValue, >@@ -228,26 +228,26 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "link", > nsIAccessibleRole::ROLE_LINK, > kUseMapRole, > eNoValue, > eJumpAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_LINKED >+ states::LINKED > }, > { > "list", > nsIAccessibleRole::ROLE_LIST, > kUseMapRole, > eNoValue, > eNoAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_READONLY, >+ states::READONLY, > eARIAMultiSelectable > }, > { > "listbox", > nsIAccessibleRole::ROLE_LISTBOX, > kUseMapRole, > eNoValue, > eNoAction, >@@ -258,17 +258,17 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "listitem", > nsIAccessibleRole::ROLE_LISTITEM, > kUseMapRole, > eNoValue, > eNoAction, // XXX: should depend on state, parent accessible > eNoLiveAttr, >- nsIAccessibleStates::STATE_READONLY, >+ states::READONLY, > eARIASelectable, > eARIACheckedMixed > }, > { > "log", > nsIAccessibleRole::ROLE_NOTHING, > kUseNativeRole, > eNoValue, >@@ -365,17 +365,17 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] = > }, > { > "progressbar", > nsIAccessibleRole::ROLE_PROGRESSBAR, > kUseMapRole, > eHasValueMinMax, > eNoAction, > eNoLiveAttr, >- nsIAccessibleStates::STATE_READONLY >+ states::READONLY > }, > { > "radio", > nsIAccessibleRole::ROLE_RADIOBUTTON, > kUseMapRole, > eNoValue, > eSelectAction, > eNoLiveAttr, >@@ -595,96 +595,87 @@ nsRoleMapEntry nsARIAMap::gEmptyRoleMap = { > }; > > nsStateMapEntry nsARIAMap::gWAIStateMap[] = { > // eARIANone > nsStateMapEntry(), > > // eARIAAutoComplete > nsStateMapEntry(&nsAccessibilityAtoms::aria_autocomplete, >- "inline", 0, nsIAccessibleStates::EXT_STATE_SUPPORTS_AUTOCOMPLETION, >- "list", nsIAccessibleStates::STATE_HASPOPUP, nsIAccessibleStates::EXT_STATE_SUPPORTS_AUTOCOMPLETION, >- "both", nsIAccessibleStates::STATE_HASPOPUP, nsIAccessibleStates::EXT_STATE_SUPPORTS_AUTOCOMPLETION), >+ "inline", 0, states::SUPPORTS_AUTOCOMPLETION, >+ "list", states::HASPOPUP, states::SUPPORTS_AUTOCOMPLETION, >+ "both", states::HASPOPUP, states::SUPPORTS_AUTOCOMPLETION), > > // eARIABusy > nsStateMapEntry(&nsAccessibilityAtoms::aria_busy, >- "true", nsIAccessibleStates::STATE_BUSY, 0, >- "error", nsIAccessibleStates::STATE_INVALID, 0), >+ "true", states::BUSY, 0, >+ "error", states::INVALID, 0), > > // eARIACheckableBool > nsStateMapEntry(&nsAccessibilityAtoms::aria_checked, kBoolType, >- nsIAccessibleStates::STATE_CHECKABLE, >- nsIAccessibleStates::STATE_CHECKED, 0, >- 0, 0, PR_TRUE), >+ states::CHECKABLE, states::CHECKED, 0, 0, 0, PR_TRUE), > > // eARIACheckableMixed > nsStateMapEntry(&nsAccessibilityAtoms::aria_checked, kMixedType, >- nsIAccessibleStates::STATE_CHECKABLE, >- nsIAccessibleStates::STATE_CHECKED, 0, >- 0, 0, PR_TRUE), >+ states::CHECKABLE, states::CHECKED, 0, 0, 0, PR_TRUE), > > // eARIACheckedMixed > nsStateMapEntry(&nsAccessibilityAtoms::aria_checked, kMixedType, >- nsIAccessibleStates::STATE_CHECKABLE, >- nsIAccessibleStates::STATE_CHECKED, 0), >+ states::CHECKABLE, states::CHECKED, 0), > > // eARIADisabled > nsStateMapEntry(&nsAccessibilityAtoms::aria_disabled, kBoolType, 0, >- nsIAccessibleStates::STATE_UNAVAILABLE, 0), >+ states::UNAVAILABLE, 0), > > // eARIAExpanded > nsStateMapEntry(&nsAccessibilityAtoms::aria_expanded, kBoolType, 0, >- nsIAccessibleStates::STATE_EXPANDED, 0, >- nsIAccessibleStates::STATE_COLLAPSED, 0), >+ states::EXPANDED, 0, states::COLLAPSED, 0), > > // eARIAHasPopup > nsStateMapEntry(&nsAccessibilityAtoms::aria_haspopup, kBoolType, 0, >- nsIAccessibleStates::STATE_HASPOPUP, 0), >+ states::HASPOPUP, 0), > > // eARIAInvalid > nsStateMapEntry(&nsAccessibilityAtoms::aria_invalid, kBoolType, 0, >- nsIAccessibleStates::STATE_INVALID, 0), >+ states::INVALID, 0), > > // eARIAMultiline > nsStateMapEntry(&nsAccessibilityAtoms::aria_multiline, kBoolType, 0, >- 0, nsIAccessibleStates::EXT_STATE_MULTI_LINE, >- 0, nsIAccessibleStates::EXT_STATE_SINGLE_LINE, PR_TRUE), >+ 0, states::MULTI_LINE, >+ 0, states::SINGLE_LINE, PR_TRUE), > > // eARIAMultiSelectable > nsStateMapEntry(&nsAccessibilityAtoms::aria_multiselectable, kBoolType, 0, >- nsIAccessibleStates::STATE_MULTISELECTABLE | nsIAccessibleStates::STATE_EXTSELECTABLE, 0), >+ states::MULTISELECTABLE | states::EXTSELECTABLE, 0), > > // eARIAOrientation > nsStateMapEntry(&nsAccessibilityAtoms::aria_orientation, eUseFirstState, >- "vertical", 0, nsIAccessibleStates::EXT_STATE_VERTICAL, >- "horizontal", 0, nsIAccessibleStates::EXT_STATE_HORIZONTAL), >+ "vertical", 0, states::VERTICAL, >+ "horizontal", 0, states::HORIZONTAL), > > // eARIAPressed > nsStateMapEntry(&nsAccessibilityAtoms::aria_pressed, kMixedType, >- nsIAccessibleStates::STATE_CHECKABLE, >- nsIAccessibleStates::STATE_PRESSED, 0), >+ states::CHECKABLE, >+ states::PRESSED, 0), > > // eARIAReadonly > nsStateMapEntry(&nsAccessibilityAtoms::aria_readonly, kBoolType, 0, >- nsIAccessibleStates::STATE_READONLY, 0), >+ states::READONLY, 0), > > // eARIAReadonlyOrEditable > nsStateMapEntry(&nsAccessibilityAtoms::aria_readonly, kBoolType, 0, >- nsIAccessibleStates::STATE_READONLY, 0, >- 0, nsIAccessibleStates::EXT_STATE_EDITABLE, PR_TRUE), >+ states::READONLY, 0, 0, states::EDITABLE, PR_TRUE), > > // eARIARequired > nsStateMapEntry(&nsAccessibilityAtoms::aria_required, kBoolType, 0, >- nsIAccessibleStates::STATE_REQUIRED, 0), >+ states::REQUIRED, 0), > > // eARIASelectable > nsStateMapEntry(&nsAccessibilityAtoms::aria_selected, kBoolType, >- nsIAccessibleStates::STATE_SELECTABLE, >- nsIAccessibleStates::STATE_SELECTED, 0, >- 0, 0, PR_TRUE) >+ states::SELECTABLE, states::SELECTED, 0, 0, 0, PR_TRUE) > }; > > /** > * Universal (Global) states: > * The following state rules are applied to any accessible element, > * whether there is an ARIA role or not: > */ > eStateMapEntryID nsARIAMap::gWAIUnivStateMap[] = { >@@ -774,17 +765,17 @@ nsStateMapEntry::nsStateMapEntry(nsIAtom **aAttrName, eStateValueType aType, > mState2(0), > mValue3(nsnull), > mState3(0), > mDefaultState(aTrueState), > mDefinedIfAbsent(aDefinedIfAbsent) > { > if (aType == kMixedType) { > mValue2 = "mixed"; >- mState2 = nsIAccessibleStates::STATE_MIXED; >+ mState2 = states::MIXED; > } > } > > nsStateMapEntry::nsStateMapEntry(nsIAtom **aAttrName, > const char *aValue1, > PRUint64 aState1, > const char *aValue2, > PRUint64 aState2, >diff --git a/accessible/src/base/nsAccDocManager.cpp b/accessible/src/base/nsAccDocManager.cpp >index ee61e16..f66fdbb 100644 >--- a/accessible/src/base/nsAccDocManager.cpp >+++ b/accessible/src/base/nsAccDocManager.cpp >@@ -216,18 +216,17 @@ nsAccDocManager::OnStateChange(nsIWebProgress *aWebProgress, > > // Mark the document accessible as loading, if it stays alive then we'll mark > // it as loaded when we receive proper notification. > docAcc->MarkAsLoading(); > > // Fire state busy change event. Use delayed event since we don't care > // actually if event isn't delivered when the document goes away like a shot. > nsRefPtr<AccEvent> stateEvent = >- new AccStateChangeEvent(document, nsIAccessibleStates::STATE_BUSY, >- PR_FALSE, PR_TRUE); >+ new AccStateChangeEvent(document, states::BUSY, PR_FALSE, PR_TRUE); > docAcc->FireDelayedAccessibleEvent(stateEvent); > > return NS_OK; > } > > NS_IMETHODIMP > nsAccDocManager::OnProgressChange(nsIWebProgress *aWebProgress, > nsIRequest *aRequest, >@@ -351,18 +350,17 @@ nsAccDocManager::HandleDOMDocumentLoad(nsIDocument *aDocument, > // Fire complete/load stopped if the load event type is given. > if (aLoadEventType) { > nsRefPtr<AccEvent> loadEvent = new AccEvent(aLoadEventType, aDocument); > docAcc->FireDelayedAccessibleEvent(loadEvent); > } > > // Fire busy state change event. > nsRefPtr<AccEvent> stateEvent = >- new AccStateChangeEvent(aDocument, nsIAccessibleStates::STATE_BUSY, >- PR_FALSE, PR_FALSE); >+ new AccStateChangeEvent(aDocument, states::BUSY, PR_FALSE, PR_FALSE); > docAcc->FireDelayedAccessibleEvent(stateEvent); > } > > PRBool > nsAccDocManager::IsEventTargetDocument(nsIDocument *aDocument) const > { > nsCOMPtr<nsISupports> container = aDocument->GetContainer(); > nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem = >diff --git a/accessible/src/base/nsAccDocManager.h b/accessible/src/base/nsAccDocManager.h >index c1f67fd..b75bdc7 100644 >--- a/accessible/src/base/nsAccDocManager.h >+++ b/accessible/src/base/nsAccDocManager.h >@@ -365,17 +365,17 @@ private: > if (type == nsIAccessibleEvent::EVENT_DOCUMENT_LOAD_STOPPED) { \ > strEventType.AssignLiteral("load stopped"); \ > } else if (type == nsIAccessibleEvent::EVENT_DOCUMENT_LOAD_COMPLETE) { \ > strEventType.AssignLiteral("load complete"); \ > } else if (type == nsIAccessibleEvent::EVENT_DOCUMENT_RELOAD) { \ > strEventType.AssignLiteral("reload"); \ > } else if (type == nsIAccessibleEvent::EVENT_STATE_CHANGE) { \ > AccStateChangeEvent* event = downcast_accEvent(aEvent); \ >- if (event->GetState() == nsIAccessibleStates::STATE_BUSY) { \ >+ if (event->GetState() == states::BUSY) { \ align '\' with others > strEventType.AssignLiteral("busy "); \ > if (!itemAcc || >- State(itemAcc) & nsIAccessibleStates::STATE_INVISIBLE) { >+ State(itemAcc) & states::INVISIBLE) { it should fit into one line > nsAccessibilityService::GetStringStates(PRUint32 aStates, PRUint32 aExtraStates, > nsIDOMDOMStringList **aStringStates) >- if (aStates & nsIAccessibleStates::STATE_IMPORTANT) >+ if (aStates & states::IMPORTANT) > stringStates->Add(NS_LITERAL_STRING("important")); there's no important state, there's alert > //extraStates >- if (aExtraStates & nsIAccessibleStates::EXT_STATE_SUPPORTS_AUTOCOMPLETION) >+ if (aExtraStates & states::SUPPORTS_AUTOCOMPLETION) new patch to get rid extra state from this method is planned? >@@ -1243,52 +1243,52 @@ nsAccessibleWrap::get_states(AccessibleStates *aStates) > { > >- if (extraStates & nsIAccessibleStates::EXT_STATE_ACTIVE) >+ if (extraStates & states::ACTIVE) > *aStates |= IA2_STATE_ACTIVE; new patch for this too?

Comment on attachment 516183 [details] [diff] [review] part 10 AccEvents > if (aStateMask.HasState(NS_EVENT_STATE_INVALID)) { > nsRefPtr<AccEvent> event = >- new AccStateChangeEvent(aContent1, states::INVALID, PR_FALSE, PR_TRUE); >+ new AccStateChangeEvent(aContent1, states::INVALID, PR_FALSE); PR_TRUE!

Comment on attachment 516209 [details] [diff] [review] part 1 States.h >+ /** >+ * The expandable object's children are displayed, the oposite of collapsed oposite -> opposite >+ * The expandable object's children are not displayed, the oposite of extra space between object's and children, fix ;oposite' >+ * expanded, applied to tree lists and other controls, @see EXPANDED state. @see on new line >+ /** >+ * @see the description of the EXPANDED and COLLAPSED states. >+ */ >+ const PRUint64 EXPANDABLE = 0x400000000000; The object is expandable, provide UI to expand/collapse its children? @see EXPANDED and COLLAPSED states.

Comment on attachment 516173 [details] [diff] [review] part 2 GetStateInternal() -> NativeState() >-nsresult >-nsApplicationAccessible::GetStateInternal(PRUint32 *aState, >- PRUint32 *aExtraState) >+PRUint64 >+nsApplicationAccessible::NativeState() > { >- *aState = 0; >- >- if (IsDefunct()) { >- if (aExtraState) >- *aExtraState = nsIAccessibleStates::EXT_STATE_DEFUNCT; >- >- return NS_OK_DEFUNCT_OBJECT; >- } >- >- if (aExtraState) >- *aExtraState = 0; >- >- return NS_OK; >+ return ISDefunct() ? states::DEFUNCT : 0; > } "IS"? You're making sure everything compiles, right?

(In reply to comment #47) > > "IS"? You're making sure everything compiles, right? not at this point, lot of changes splitted to different patches to make review easier, when all wanted changes are done, I expect to get the last patch that makes all working. I will push all these patches as a single changeset so it doesn't matter where and what was fixed.

add states.h defining accessible states

(wip) start implementing nativeState()

change GetState() to State()

rename GetARIAState() to ApplyARIAState()

Remove aExtraState from nsStateMapEntry

change the atk state map and translateStates() to use 64 bitmaps.

change references to nsIAccessibleStates::{,EXT_}STATE_xxx to states::xxxx

remove extraState from AccEvents

Comment on attachment 516765 [details] [diff] [review] part 2 >-nsresult >-nsDocAccessible::GetStateInternal(PRUint32 *aState, PRUint32 *aExtraState) >+PRUint64 >+nsDocAccessible::NativeState() > { >+ // The root content of the document might be removed so that mContent is >+ // out of date. >+ PRUint64 state = (mContent->GetCurrentDoc() == mDocument) ? 0 : >+ states::STALE; correct indent please! either (if no more than 80 chars) PRUint64 state = (mContent->GetCurrentDoc() == mDocument) ? 0 : states::STALE; or PRUint64 state = (mContent->GetCurrentDoc() == mDocument) ? 0 : states::STALE; >+PRUint64 >+nsHTMLTextAccessible::NativeState() > { > nsDocAccessible *docAccessible = GetDocAccessible(); > if (docAccessible) { >- PRUint32 state, extState; >- docAccessible->GetState(&state, &extState); >- if (0 == (extState & nsIAccessibleStates::EXT_STATE_EDITABLE)) { >- *aState |= nsIAccessibleStates::STATE_READONLY; // Links not focusable in editor >+ PRUint64 docState = docAccessible->NativeState(); NativeState() is not GetState() >+PRUint64 >+nsHTMLWin32ObjectOwnerAccessible::NativeState() > { >- nsresult rv = nsAccessibleWrap::GetStateInternal(aState, aExtraState); >- if (rv == NS_OK_DEFUNCT_OBJECT) >- return rv; >- > // XXX: No HWND means this is windowless plugin which is not accessible in > // the meantime. >- if (!mHwnd) >- *aState = nsIAccessibleStates::STATE_UNAVAILABLE; >- >- return rv; >+ return mHwnd ? nsAccessibleWrap::NativeState() : states::UNAVAILABLE; hm, I suggested that earlier but it's better to change if (mHwnd) return nsAccessibleWrap::NativeState(); return IsDefunct() ? states::DEFUNCT : states::UNAVAILABLE; >+PRUint64 >+nsXULRadioGroupAccessible::NativeState() > { >- nsresult rv = nsAccessible::GetStateInternal(aState, aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 state = NativeState(); infinite recursion > > // The radio group is not focusable. Sometimes the focus controller will > // report that it is focused. That means that the actual selected radio button > // should be considered focused. >- *aState &= ~(nsIAccessibleStates::STATE_FOCUSABLE | >- nsIAccessibleStates::STATE_FOCUSED); >+ state &= ~(states::FOCUSABLE | states::FOCUSED); > >- return NS_OK; >+ return state; I'd prefer return nsAccessible::NativeState() & ~(states::FOCUSABLE | states::FOCUSED); (make sure it fits to 80 char line) >+PRUint64 >+nsXULMenuSeparatorAccessible::NativeState() > { > // Isn't focusable, but can be offscreen/invisible -- only copy those states >- nsresult rv = nsXULMenuitemAccessible::GetStateInternal(aState, aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 state = nsXULMenuitemAccessible::NativeState(); > >- *aState &= (nsIAccessibleStates::STATE_OFFSCREEN | >- nsIAccessibleStates::STATE_INVISIBLE); >+ state &= (states::OFFSCREEN | states::INVISIBLE); > >- return NS_OK; >+ return state; I'd prefer return nsXULMenuitemAccessible::NativeState() & (states::OFFSCREEN | states::INVISIBLE); >+PRUint64 >+nsXULTextAccessible::NativeState() > { >- nsresult rv = nsHyperTextAccessibleWrap::GetStateInternal(aState, >- aExtraState); >- NS_ENSURE_A11Y_SUCCESS(rv, rv); >+ PRUint64 states = nsHyperTextAccessibleWrap::NativeState(); > > // Labels and description have read only state > // They are not focusable or selectable >- *aState |= nsIAccessibleStates::STATE_READONLY; >- return NS_OK; >+ states |= states::READONLY; >+ return states; return nsHyperTextAccessibleWrap::NativeState() | states::READONLY; >+PRUint64 >+nsXULTreeItemAccessibleBase::NativeState() > { > // expanded/collapsed state > if (IsExpandable()) { > PRBool isContainerOpen; > mTreeView->IsContainerOpen(mRow, &isContainerOpen); >- *aState |= isContainerOpen ? >- static_cast<PRUint32>(nsIAccessibleStates::STATE_EXPANDED) : >- static_cast<PRUint32>(nsIAccessibleStates::STATE_COLLAPSED); >+ state |= isContainerOpen ? states::EXPANDEj : states::COLLAPSED states::EXPANDEj

(In reply to comment #57) > Comment on attachment 516765 [details] [diff] [review] > part 2 btw, in several places you use GetState() instead of State().

Comment on attachment 516766 [details] [diff] [review] part 3 State() > /** >+ * return states including ARIA states >+ */ >+ virtual PRUint64 State(); return -> Return perhaps "Return all states of accessible (including ARIA states)"? > // nsIAccessible >- NS_IMETHOD GetState(PRUint32 *aState, PRUint32 *aExtraState); >+ virtual PRUint64 State(); move new method to nsAccessible section in header file > STDMETHODIMP > CAccessibleComponent::get_locationInParent(long *aX, long *aY) > { > // If the object is not on any screen the returned position is (0,0). >- PRUint32 states = 0; >- nsresult rv = acc->GetState(&states, nsnull); >- if (NS_FAILED(rv)) >- return GetHRESULT(rv); >+ PRUint64 states = acc->State(); rename states to state while you're here please >+ // msaa only has 31 states and the lowest 31 bits of our state bit mask msaa -> MSAA >+ // are the same states as msaa. >+ // This means we can get our state bit mask, mask off the upper 33 bits >+ // and return the lowest 31 states. please add comment that you're mapping INVALID and etc states to certain MSAA states (what's a Gecko specific) > nsAccessibleWrap::get_states(AccessibleStates *aStates) > { > __try { > *aStates = 0; > > // XXX: bug 344674 should come with better approach that we have here. > >- PRUint32 states = 0, extraStates = 0; >- nsresult rv = GetState(&states, &extraStates); >- if (NS_FAILED(rv)) >- return GetHRESULT(rv); >+ PRUint64 states = State(); states -> state while you're here.

Comment on attachment 516772 [details] [diff] [review] part 8 atk state map >+static void >+TranslateStates(PRUint64 aState, AtkStateSet *aStateSet) as you suggested on irc use global variable

Comment on attachment 516778 [details] [diff] [review] part 9 rename states you don't need to reask review for trivial changes, just upload new patch

(In reply to comment #60) > Comment on attachment 516772 [details] [diff] [review] > part 8 atk state map > > > >+static void > >+TranslateStates(PRUint64 aState, AtkStateSet *aStateSet) > > as you suggested on irc use global variable I'm not sure I understand what you saying here, I made the change I was thinking of, TranslateStates() now take 2 args instead of three, where you thinking of something else?

(wip) start implementing nativeState()

change GetState() to State()

(wip) start implementing nativeState()

change GetState() to State()

remove extraState from AccEvents

modify GetStringState() to take a PRUint64

remove aExtraState from GetSelectState()

remove extra state code from nsAccessibleEvent and nsIAccessibleEvent

remove atk code dealing with extra states

Some places where nsAccUtils::State() was used the call to State() wasn't changed from State(foo) to foo->State()

fix a bunch of invocations of nsStateMapEntry() that were in static initialization.

various trivial build fixes.

(In reply to comment #70) > Created attachment 517359 [details] [diff] [review] > part 13 NsIAccessibleEvent > > remove extra state code from nsAccessibleEvent and nsIAccessibleEvent Alexander, thoughtss? I'm not sure if we want to change the xpcom interfaces for states or not

(In reply to comment #62) > > >+static void > > >+TranslateStates(PRUint64 aState, AtkStateSet *aStateSet) > > > > as you suggested on irc use global variable > > I'm not sure I understand what you saying here, I made the change I was > thinking of, TranslateStates() now take 2 args instead of three, where you > thinking of something else? I must misread it, ignore.

(In reply to comment #75) > (In reply to comment #70) > > Created attachment 517359 [details] [diff] [review] > > part 13 NsIAccessibleEvent > > > > remove extra state code from nsAccessibleEvent and nsIAccessibleEvent > > Alexander, thoughtss? I'm not sure if we want to change the xpcom interfaces > for states or not it's worth to change it, to clean it up

Comment on attachment 517356 [details] [diff] [review] part 10 AccEvents > PRUint64 state = accessible->state(); >- mIsEnabled = ((mIsExtraState ? extraState : state) & mState) != 0; >+ mIsEnabled = (state & mState) != 0; > } else { > mIsEnabled = PR_FALSE; > } mIsEnabled = accessible && (accessible->State() & mState != 0);

Comment on attachment 517357 [details] [diff] [review] part 11 GetStringState() actually we don't need this patch since it's XPCOM method and we should deal with states and extrastates (be compatible to nsIAccessible::GetState() method).

Comment on attachment 517358 [details] [diff] [review] part 12 GetSelectState() > PRUint32 selectState = 0, selectExtState = 0; >- nsCOMPtr<nsIContent> selectContent = GetSelectState(&selectState, >- &selectExtState); >+ nsCOMPtr<nsIContent> selectContent = GetSelectState(&selectState); you don't need nsCOMPtr here, just do nsIContent* selectContent = GetSelectState(&selectState);

Comment on attachment 517359 [details] [diff] [review] part 13 NsIAccessibleEvent we don't need this (must be compatible with nsIAccessible::GetState().

Comment on attachment 517363 [details] [diff] [review] part 17 >- nsIDOMDOMStringList getStringStates(in unsigned long aStates, >- in unsigned long aExtraStates); >+ nsIDOMDOMStringList getStringStates(in unsigned long long aStates); no needed. > AtkAttributeSet * > GetAttributeSet(nsIAccessible* aAccessible) > { > nsCOMPtr<nsIPersistentProperties> attributes; > aAccessible->GetAttributes(getter_AddRefs(attributes)); > > if (attributes) { > // Deal with attributes that we only need to expose in ATK >- PRUint64 state = aAccessible->State(); >+ nsRefPtr<nsAccessible> acc(do_QueryObject(aAccessible)); >+ PRUint64 state = acc->State(); turn it to take nsAccessible* as an argument > AccStateChangeEvent(nsAccessible* aAccessible, PRUint64 aState, > PRBool aIsEnabled, EIsFromUserInput aIsFromUserInput): > AccEvent(nsIAccessibleEvent::EVENT_STATE_CHANGE, aAccessible, > aIsFromUserInput, eAllowDupes), >- mState(aState), mIsEnabled(aIsEnabled) >+ mState(aState), mIsEnabled(mIsEnabled) wrong > } > > AccStateChangeEvent:: > AccStateChangeEvent(nsINode* aNode, PRUint64 aState, PRBool aIsEnabled): > AccEvent(::nsIAccessibleEvent::EVENT_STATE_CHANGE, aNode), >- mState(aState), IsEnabled(aIsEnabled) >+ mState(aState), mIsEnabled(mIsEnabled) wrong > void > nsDocAccessible::ApplyARIAState(PRUint64* aState) > { > // Combine with states from outer doc >- NS_ENSURE_ARG_POINTER(aState); >+ // >+ if (!aState) >+ return; >+ You don't need this check > nsIFrame* nsHTMLSelectOptionAccessible::GetBoundsFrame() > { >- PRUint32 state = 0; >+ PRUint64 state = 0; > nsCOMPtr<nsIContent> content = GetSelectState(&state); please: nsIContent* content =

(In reply to comment #80) > Comment on attachment 517358 [details] [diff] [review] > part 12 GetSelectState() > > > > PRUint32 selectState = 0, selectExtState = 0; > >- nsCOMPtr<nsIContent> selectContent = GetSelectState(&selectState, > >- &selectExtState); > >+ nsCOMPtr<nsIContent> selectContent = GetSelectState(&selectState); > > you don't need nsCOMPtr here, just do > nsIContent* selectContent = GetSelectState(&selectState); just that call site, or the other one too?

never mind comment 83 I see you metnioned the other call site in comment 82 :)

add states.h defining accessible states

(wip) start implementing nativeState()

change GetState() to State()

rename GetARIAState() to ApplyARIAState()

replace nsAccUtils::State() and nsAccUtils::ExtendedState() with nsAccessible::State()

Remove aExtraState from nsStateMapEntry

convert GetActionRule() to take a PRUint64 of states

change the atk state map and translateStates() to use 64 bitmaps.

change references to nsIAccessibleStates::{,EXT_}STATE_xxx to states::xxxx

remove extraState from AccEvents

remove aExtraState from GetSelectState()

remove atk code dealing with extra states

Some places where nsAccUtils::State() was used the call to State() wasn't changed from State(foo) to foo->State()

fix a bunch of invocations of nsStateMapEntry() that were in static initialization.

various trivial build fixes.

provide xpcom methods This is the new implementation of nsAccStateChangeEvent and GetState()

(In reply to comment #100) > Created attachment 518650 [details] [diff] [review] > part 16 xpcom methods > > provide xpcom methods > > This is the new implementation of nsAccStateChangeEvent and GetState() per irc: please replace tabs on whitespaces

bug 634218 - dexpcom accessible state methods

bug 634218 - dexpcom accessible state methods

nsARIAMap should store states in PRUint64s not PRUint32s

fix some nsStateMapEntry constructors

(In reply to comment #106) > Created attachment 523335 [details] [diff] [review] > patch hm, this doesn't seem to apply for me, either on trunk, or on trunk with 646368 and 646369 applied. The errors all seem to be in files in src/xul/

(In reply to comment #107) > (In reply to comment #106) > > Created attachment 523335 [details] [diff] [review] > > patch > > hm, this doesn't seem to apply for me, either on trunk, or on trunk with 646368 > and 646369 applied. The errors all seem to be in files in src/xul/ one more is in my queue is bug 630486

pass mochitests when patch from bug 648235 is applied

landed on cedar - http://hg.mozilla.org/projects/cedar/rev/f15bfa1cb14b

Backed out of Aurora because it was causing too many instabilities: http://hg.mozilla.org/mozilla-aurora/rev/25feb8bd6294

Sorry for the spam, the correct link for the backout changeset on Aurora is: http://hg.mozilla.org/mozilla-aurora/rev/bcc0d10b5d4b

Build: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0 All the proposed, reviewed and landed changes in m-c are visible in hg, under beta (i.e. the States.h file found here http://hg.mozilla.org/releases/mozilla-beta/file/f3e82fad65b2/accessible/src/base/States.h) Setting this as Verified for Firefox 6 Beta.