212157 - Get ready to start freezing some accessibility interfaces

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

By starting to freeze some COM and XPCOM interfaces in Gecko, we will allow
in-process and Windows accessibility clients to work safely.

Comment 1

[Aaron Leventhal]

Attachment: Needs work. Add comments, move constants from nsIAccessibleEventReceiver to nsIAccessibleEvent. Make GetAccessibleCaret return nsIAccessible.

Comment 2

[Aaron Leventhal]

Still to do:
Get rid of nsIAccessibleEventReceiver. Do everything in Init().
Comment nsIAccessible
Comment ISimpleDOM* interfaces
Add documentation URL's to files

Comment 3

[Aaron Leventhal]

Attachment: Add comments, remove unnecessary interfaces, use nsIArray, freeze interfaces

Comment 4

[Aaron Leventhal]

Add comments, move constants to nsIAccessibleEvent, don't use internal
interfaces in public places, remove unnecessary interfaces, change to nsIArray,
add to SDK_XPIDLSRCS, add @status FROZEN

Comment 5

[Aaron Leventhal]

Attachment: Add comments, move constants to nsIAccessibleEvent, don't use internal interfaces in public places, remove unnecessary interfaces, change to nsIArray, add to SDK_XPIDLSRCS, add @status FROZEN

Comment 6

[Aaron Leventhal]

Doug, what if I want to include my COM ISimpleDOM*.idl files in the SDK. Does
that make sense?
What would I add to the makefiles
Or should I only include the .h files for those?

Comment 7

[Aaron Leventhal]

Kyle, at some point I may want to move a couple of useful things like
nsIAccessibleTable into the SDK, because in-process accessibility clients can
make use of them.

Also, instead of the current nsAccessibleEventData structs I'd like to move
torward an XPCOM approach, something like:
GetStateChangeEvent(nsIAccessibleEvent *aEvent)
{
nsCOMPtr<nsIAccessibleStateChangeEvent>
stateChangeEvent(do_QueryInterface(accessibleEvent));
// Also can try nsIAccessibleTableChangeEvent, nsIAccessibleTextChangeEvent,
etc. One interface per event struct type in nsAccessibleEventData.h
}

We can file a different bug later for those things.

Comment 8

[timeless]

Comment on attachment 127452 [details] [diff] [review]
Add comments, move constants to nsIAccessibleEvent, don't use internal interfaces in public places, remove unnecessary interfaces, change to nsIArray, add to SDK_XPIDLSRCS, add @status FROZEN

> When accessibility is active in Gecko,
> + * every DOM node can have one nsIAccessNode, for each
I don't think you want a comma before for each
> + * pres shell the DOM node is rendered in.

> [scriptable, uuid(46820F9B-3088-4046-AB0F-56FDACDC7A82)]
> interface nsIAccessNode : nsISupports

+   * Get the nth of this node
nth _child_ :)
+   * @param childNum Zero-based child index
+   * @return The nth nsIAccessNode child
the nth child of this nsIAccessNode
+   */
   nsIAccessNode getChildAt(in long childNum);

   readonly attribute DOMString innerHTML;
*freezing warning*
innerText might be added to some dom. Would that make nsIAccessNode out of sync
and cause problems?

> +    * purposes of caching and referring to this object.
purposes of caching and referencing this object.

> +   * Note: the meaning of width, height and other size measurements depends
meanings/depend?
> +   * on the version of CSS being used.
end user question: how do i find out what version of css is being used?

> +   * @param pseudoElt The psuedo element to retrieve style for, or NULL
pseudo

- general nit:
ending fragments with a preposition.

> + * A cross-platform interface that contains supports platform-specific
-contains supports- => bad, fixme.


> + * Can also be used by in-process accessibility clients to get information
> + * about objects in the accessible tree, which represents a subset of
> + * nodes in the DOM tree -- such as documents, focusable elements and text.
the commas bother me. is the which designed to restrict accessible tree (it
appears to) or just be a random thought?

> + * The implementations of nsIAccessible are created on demand.
you mean mozilla writes code to implement nsIAccessible on demand? wow.

perhaps 'nsIAccessible instances are created on demand.'

> + * See http://www.mozilla.org/projects/ui/accessibility for more information.
I think there's some way to do @see such that idl knows to try to hyperlink
urls...


> +   * Number of accessible children
i read this to mean that there are uncounted number of inaccessible children. i
think nsIAccessible children might be better.
> +   */
>   readonly attribute long accChildCount;

> +   * Accessible name -- the main text equivalent for this node
Is 'this' the nsIAccessible or the nsIDOMNode? 
> +   */
>    attribute AString accName;


> +   * Underlined accesskey for this node.
so 'a'?
> +   * Usually alt+letter, or just the letter alone for menu items
or 'alt+a', or	'strg-a'?
is this text going to include localized text and be affected by user
preference?
if some other thing will trap the keyshortcut, how does this interface express
that to its consumer so that the client doesn't mislead the user?
> +   */
>    readonly attribute AString accKeyboardShortcut;

> +   * Enumerated accessible role,
> +   * values depend on platform because of variations in accessibility toolkits
> +   */
>    readonly attribute unsigned long accRole;

> +   * Accessible states -- bit field which describes boolean properties of node
based on the preceding i'm presuming that these values don't depend on
toolkits, so what do they mean?
> +   */
>    readonly attribute unsigned long accState;

> +   * Extended accessible states -- second bit field describing node
> +   */
>    readonly attribute unsigned long accExtState;

> +   * Accessible child which contains the coordinate at x,y
what happens if there are multiple nodes which contian this coordinate?
> +   */
>    nsIAccessible accGetAt(in long x, in long y);

> +   * Accessible node geometrically right of this one
i'm waiting for 'geometrically wrong'. i think you want 'geometrically _to the_
right of this one'
>    nsIAccessible accNavigateRight();
To me navigate is much more active than the comment would indicate.

> +   * Add this accessible to the current selection
Can this fail?
> +   */
>    void accAddSelection();

> +   * Remove this accessible from the current selection
What happens if this accessible isn't in the current selection?
> +   */
>    void accRemoveSelection();

> +   * Focus this accessible node, if it is focusable
what happens if it isn't focusable?
> +   * The state STATE_FOCUSABLE indicates whether this node is focusable.
> +   */
>    void accTakeFocus();

>    AString getAccActionName(in PRUint8 index);
Why is this PRUint8? (I know 257 actions would be pretty excessive, but i
wonder if it could hurt performance)

> + * there is an nsIAccessibleDocument for each document
How do I reach it?
> + * whether it is XUL, HTML or whatever.
whatever doesn't seem very documentation friendly :)
> + * You can QueryInterface to nsIAccessibleDocument from
> + * the nsIAccessible or nsIAccessNode for the root node
> + * of a document.


> +   * True if the document is live in an editor.
> +   * False if the document is being displayed but not edited.
> +   */
>    readonly attribute boolean isEditable;
what if a <div> is contentEditable?

>    [noscript] nsIAccessible getAccessibleInParentChain(in nsIDOMNode aDOMNode);
why is this noscript?

> + * to find out to get accessibility and DOM interfaces for
out _how_ to ...

> + * To listen for accessibility events, make your in-process
> + * accessibility component an nsIObserver, and use
> + * nsIObserverService::AddObserver to observe "accessible-event"
where does this sentence end?
i think you probably should specify the contract for which you're suggesting
the consumer call AddObserver.
If there's @timeless/silly-observerservice;1 and
@mozilla.org/observer-service;1 and they both implement nsIObserverService,
then you wouldn't want the consumer to be confused.
> + * For more info, see http://www.mozilla.org/projects/ui/accessibility


> + * An interface for in-process accessibility clients
> + * wishing to get an nsIAccessible or nsIAccessNode for
> + * a given DOM node
where does this sentence end? (i won't nit this anymore)
> + * More documentation at:

> +  /**
> +    * Return an nsIAccessible for an DOM node in pres shell 0.
first and only instance for this nit, the style is:
/**
 *
 */
not:
/**
  *
  */
(you're inconsistent although i think most of the time you do it the the first
way)

> +    * Return an nsIAccessible for an DOM node in pres shell 0.
isn't it a DOM node? (first and only instance for this nit)

> + * for dealing with selection and deselection of accessible nodes.
deselection? yuck

> +         * @return The nth selected accessible child
tabs? (first and only instance for this nit)
>       */
>     nsIAccessible refSelection(in long index);


te("// Both methods get_clippedSubstringBounds and get_unclippedSubstringBounds
return the screen pixel")
te("// coordinates if the given text substring. The in parameters for start and
end indices refer")
should 'if' be 'of'?

--
Personally I don't like it when people try to change and freeze an interface in
the same step. please use @status UNDER_REVIEW instead.

It'd be really nice if we could collect a few comments from real [potential]
consumers indicating that they don't forsee any problems with the interfaces
that you're freezing (preferably including potential consumers who haven't
lived with this api as it developed and would therefore be jaded or otherwise
ignore problems to which they've already found alternatives).

Note: I didn't run the entire patch through a spellchecker, but your employer
makes a mozilla derivative which includes one, so it'd be nice if you ran the
apis through it. If you aren't interested in doing that please inform me and
allow a week to do it myself.

Comment 9

[Aaron Leventhal]

Comment on attachment 127452 [details] [diff] [review]
Add comments, move constants to nsIAccessibleEvent, don't use internal interfaces in public places, remove unnecessary interfaces, change to nsIArray, add to SDK_XPIDLSRCS, add @status FROZEN

Will put up a new patch next week, addressing Timeless' comments.

Comment 10

[Doug Turner (:dougt)]

aaron, i guess the idls would be useful since you can convert them to typelibs.

Comment 11

[Aaron Leventhal]

Doug, what do I put in the Makefiles to get the COM idl included in the SDK? If
I put them in SDK_XPIDLSRCS then I think xpidl will try to compile them, and
will thus break the build.

Comment 12

[Doug Turner (:dougt)]

we might have to add a new SDK var.

Comment 13

[Aaron Leventhal]

Doug, I don't know how to do that. Do you have any ideas?

Comment 14

[Doug Turner (:dougt)]

take a look at how SDK_BINARY works.

Comment 15

[Aaron Leventhal]

Attachment: New patch -- folds in Timeless' comments

Timeless, I don't know how to determine what version of CSS is being used.
However, I added a comment to nsIAccessNode suggesting that
nsIAccessible::accGetBounds be used for width and height info, to get around
the problem.

Also, I don't see much use in changing accDoAction(PRUint8) to PRUint16 or
PRUint32, since there should never be that many actions. However, if you or
another reviewer insists, I'll change it.

Comment 16

[Aaron Leventhal]

Kyle, I'm still interested in moving this and other accessibilit bugs forward.

Comment 17

[Kyle Yuan]

Comment on attachment 127871 [details] [diff] [review]
New patch -- folds in Timeless' comments

>Index: accessible/public/nsIAccessible.idl
>===================================================================

>+   * will still set focus on that node, although mormally that will not
						   ~~~~~~~~
						   normally?

>+ * The following nsIAccessible roles are traslated to ATK_ROLE_UNKNOWN
					   ~~~~~~~~~
					   translated

>+ *    the currrent item.
	    ~~~~~~~~
	    current

>Index: accessible/public/nsIAccessibleSelectable.idl
>===================================================================
>+ * An interface for the accessibility module and in-process accessibity
							      ~~~~~~~~~~~
					      accessibility or accessible? 

>Index: accessible/public/msaa/ISimpleDOMDocument.idl
>===================================================================
>+cpp_quote("// get_childAt        (/* [in] */ unsigned childIndex, /* [ou] 
									~~
									out

>Index: accessible/src/msaa/nsAccessibleWrap.cpp
>===================================================================
>+        if ( tempAccess ) {
please remove the spaces around |tempAccess|.

r=kyle with above changed.

Comment 18

[Aaron Leventhal]

Comment on attachment 127871 [details] [diff] [review]
New patch -- folds in Timeless' comments

Alec, are you still around for super reviews? This one is 99% comment changes.

Comment 19

[Alec Flett]

Comment on attachment 127871 [details] [diff] [review]
New patch -- folds in Timeless' comments

so far this looks pretty good .. a few comments:

1) I think its "UNDER_REVIEW" not "UNDER REVIEW" (note the underscore)
2) why do all the nsIAccessible APIs start with "acc" - isn't that redundant,
given the interface name?

I'm not sure I understand the need for the %{C++ stuff with #ifdef
MOZ_ACCESSIBILITY_ATK - if these are C++-only then perhaps those particular
constants belong in a seperate header (nsAccessibleAtk.h?) - but if not they
should be idl constants inside nsIAccessible.

its too bad there's no obvious scriptable method  to do the uniqueid stuff - I
guess there's no opaque token or anything.. is there ANY reason we might
someday need (or at least want) scriptable access to aUniqueID? Could the
underlying object point to an nsISupports, or would that be overkill?

Except for the under_review stuff, none of these comments should block checking
in of THIS patch - but we should address the rest before actually freezing
this. 

sr=alecf with the under_review stuff fixed.

Comment 20

[Aaron Leventhal]

Attachment: Final patch that is going into tree, contains Kyle's spelling fixes and changes UNDER REVIEW to UNDER_REVIEW with underscore

Comment 21

[Aaron Leventhal]

checked into trunk 7/22/03