Last Comment Bug 748069 - [AccessFu] Document UtteranceGenerator
: [AccessFu] Document UtteranceGenerator
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 12:52 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-07 17:57 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Document UtteranceGenerator. (4.83 KB, patch)
2012-04-23 12:55 PDT, Eitan Isaacson [:eeejay]
surkov.alexander: review+
Details | Diff | Splinter Review
Bug 748069 - Document UtteranceGenerator. r=surkov (5.00 KB, patch)
2012-05-04 13:26 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Document UtteranceGenerator. r=surkov (4.98 KB, patch)
2012-05-07 09:43 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-04-23 12:52:40 PDT
Add some much needed documentation to UtteranceGenerator.
Comment 1 Eitan Isaacson [:eeejay] 2012-04-23 12:55:08 PDT
Created attachment 617604 [details] [diff] [review]
Document UtteranceGenerator.
Comment 2 alexander :surkov 2012-04-24 01:31:27 PDT
Comment on attachment 617604 [details] [diff] [review]
Document UtteranceGenerator.

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

thank you for doing this

::: accessible/src/jsat/AccessFu.jsm
@@ +52,5 @@
>  
>    /**
> +   * Start the special AccessFu mode, this primarily means controlling the
> +   * virtual cursor with arrow keys. Currently, on platforms other than
> +   * Android this needs to be called explicitly.

it makes sense to keep that "Currently, ..." under @note section

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +27,5 @@
> + * An utterance is an array of strings.
> + *
> + * It should not be assumed that flattening an utterance array would create a
> + * gramatically correct sentence. For example:
> + * ['graphic', 'Welcome to my home page'].

makes sense to say what "objects, actions or state change" results in these strings

@@ +29,5 @@
> + * It should not be assumed that flattening an utterance array would create a
> + * gramatically correct sentence. For example:
> + * ['graphic', 'Welcome to my home page'].
> + * Each string element in an utterance should be gramatically correct in itself.
> + * For example: ['list item 2 of 5', 'Alabama'].

from this example, it is not clear how to use it. you might want to show an example in the end

@@ +32,5 @@
> + * Each string element in an utterance should be gramatically correct in itself.
> + * For example: ['list item 2 of 5', 'Alabama'].
> + *
> + * An utterance is ordered from the least to the most important. Speaking the
> + * last string usually makes sense, but speaking the first often won't.

sort of strange that last is more important than the first one, sort of rtl? :)

@@ +34,5 @@
> + *
> + * An utterance is ordered from the least to the most important. Speaking the
> + * last string usually makes sense, but speaking the first often won't.
> + * For example ['button', 'clicked'] for a clicked event. Speaking only
> + * 'clicked' makes sense. Speaking 'button' does not.

if it doesn't make sense to speak something then what sense does it make to expose it. It's not clear how the caller should decide what to speak and what to not.

@@ +59,5 @@
> +   * Generates an utterance for an object.
> +   * @param {nsIAccessible} aAccessible accessible object to generate utterance
> +   *    for.
> +   * @param {boolean} aForceName include the object's name in the utterance
> +   *    even if this object type does not usually have it's name uttered.

a ref to object that doesn't have uttered name would be nice, because I don't have any clue when I should force name and when I shouldn't do that

@@ +83,5 @@
> +  /**
> +   * Generates an utterance for an action performed.
> +   * TODO: May become more verbose in the future.
> +   * @param {nsIAccessible} aAccessible accessible object that was the target
> +   *    of the action.

the accessible object that the action was invoked on

@@ +86,5 @@
> +   * @param {nsIAccessible} aAccessible accessible object that was the target
> +   *    of the action.
> +   * @param {string} aActionName the name of the action, one of the keys in
> +   *    {@link gActionMap}.
> +   * @return {Array} A one string array with the action.

btw, it's still not clear why you need an array for one item

@@ +98,5 @@
> +   * @param {nsIAccessible} aAccessible accessible object of the tab's attached
> +   *    document.
> +   * @param {string} aTabState the tab state name, see
> +   *    {@link Presenter.tabStateChanged}.
> +   * @return {Array} The tab state utterace.

same here
Comment 3 Eitan Isaacson [:eeejay] 2012-05-04 13:23:06 PDT
(In reply to alexander :surkov from comment #2)
> Comment on attachment 617604 [details] [diff] [review]
> Document UtteranceGenerator.
> 
> Review of attachment 617604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thank you for doing this
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +52,5 @@
> >  
> >    /**
> > +   * Start the special AccessFu mode, this primarily means controlling the
> > +   * virtual cursor with arrow keys. Currently, on platforms other than
> > +   * Android this needs to be called explicitly.
> 
> it makes sense to keep that "Currently, ..." under @note section
> 

I'm throwing that sentence away since it is no longer true.

> 
> @@ +29,5 @@
> > + * It should not be assumed that flattening an utterance array would create a
> > + * gramatically correct sentence. For example:
> > + * ['graphic', 'Welcome to my home page'].
> > + * Each string element in an utterance should be gramatically correct in itself.
> > + * For example: ['list item 2 of 5', 'Alabama'].
> 
> from this example, it is not clear how to use it. you might want to show an
> example in the end
> 

It is not an example of how to use it, just an example of what to expect (grammar that is internal to the elements, but not in between elements).

> @@ +32,5 @@
> > + * Each string element in an utterance should be gramatically correct in itself.
> > + * For example: ['list item 2 of 5', 'Alabama'].
> > + *
> > + * An utterance is ordered from the least to the most important. Speaking the
> > + * last string usually makes sense, but speaking the first often won't.
> 
> sort of strange that last is more important than the first one, sort of rtl?
> :)
> 

true. that might change in the future if we decide to put the description after the object name, or at least make that a preference. i have seen it done in both ways in different platforms and screen readers.

this also has to do with the fact that the "read everything under this container accessible" feature has not yet been implemented here, although i prototyped it in the past. it uses preorder traversal for the subtree (because that is how nsIAccessiblePivot works). So preorder dictates description (parent) first, name second.

anyway, we will cross that bridge when we reach it.

> @@ +34,5 @@
> > + *
> > + * An utterance is ordered from the least to the most important. Speaking the
> > + * last string usually makes sense, but speaking the first often won't.
> > + * For example ['button', 'clicked'] for a clicked event. Speaking only
> > + * 'clicked' makes sense. Speaking 'button' does not.
> 
> if it doesn't make sense to speak something then what sense does it make to
> expose it. It's not clear how the caller should decide what to speak and
> what to not.
> 

it is on a case by case bases, each generator uses the rule differently.

> @@ +59,5 @@
> > +   * Generates an utterance for an object.
> > +   * @param {nsIAccessible} aAccessible accessible object to generate utterance
> > +   *    for.
> > +   * @param {boolean} aForceName include the object's name in the utterance
> > +   *    even if this object type does not usually have it's name uttered.
> 
> a ref to object that doesn't have uttered name would be nice, because I
> don't have any clue when I should force name and when I shouldn't do that
> 

I changed the return description to:
   * @return {Array} Two string array. The first string describes the object
   *    and its states. The second string is the object's name. Some object
   *    types may have the description or name omitted, instead an empty string
   *    is returned as a placeholder. Whether the object's description or it's role
   *    is included is determined by {@link verbosityRoleMap}.

> @@ +86,5 @@
> > +   * @param {nsIAccessible} aAccessible accessible object that was the target
> > +   *    of the action.
> > +   * @param {string} aActionName the name of the action, one of the keys in
> > +   *    {@link gActionMap}.
> > +   * @return {Array} A one string array with the action.
> 
> btw, it's still not clear why you need an array for one item
> 

besides the TODO above it. i gave a lengthy description defining an utterance as an array.
Comment 4 Eitan Isaacson [:eeejay] 2012-05-04 13:26:27 PDT
Created attachment 621155 [details] [diff] [review]
Bug 748069 - Document UtteranceGenerator. r=surkov
Comment 5 alexander :surkov 2012-05-07 02:09:04 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #3)

> > 
> > @@ +29,5 @@
> > > + * It should not be assumed that flattening an utterance array would create a
> > > + * gramatically correct sentence. For example:
> > > + * ['graphic', 'Welcome to my home page'].
> > > + * Each string element in an utterance should be gramatically correct in itself.
> > > + * For example: ['list item 2 of 5', 'Alabama'].
> > 
> > from this example, it is not clear how to use it. you might want to show an
> > example in the end
> > 
> 
> It is not an example of how to use it, just an example of what to expect
> (grammar that is internal to the elements, but not in between elements).

I meant the information ordering. I'd say that
#1 is Generates speech utterances from objects, actions and state changes. An utterance is an array of strings.
#2 is An utterance is ordered from the least to the most important...
#3 is It should not be assumed that flattening an utterance array would create a gramatically correct sentence...

> > sort of strange that last is more important than the first one, sort of rtl?
> > :)
> > 
> 
> true. that might change in the future if we decide to put the description
> after the object name, or at least make that a preference. i have seen it
> done in both ways in different platforms and screen readers.

I'd say it should depend on how easy to use it.
#1. get most important announcement: 
var array = generator.genForObject(acc);
return array[array.length - 1];
vs
generator.getForObject(acc)[0];

#2. speak everything:
return generator.getForObject(acc).reverse().join();
vs
return generator.getForObject(acc).join();

> this also has to do with the fact that the "read everything under this
> container accessible" feature has not yet been implemented here, although i
> prototyped it in the past. it uses preorder traversal for the subtree
> (because that is how nsIAccessiblePivot works). So preorder dictates
> description (parent) first, name second.

so you traverse objects from top to left-bottom, what is announcement order?

> > if it doesn't make sense to speak something then what sense does it make to
> > expose it. It's not clear how the caller should decide what to speak and
> > what to not.

> it is on a case by case bases, each generator uses the rule differently.

ok, you might want to say about this

> I changed the return description to:
>    * @return {Array} Two string array. The first string describes the object
>    *    and its states. The second string is the object's name. Some object
>    *    types may have the description or name omitted, instead an empty
> string
>    *    is returned as a placeholder. 

it's not clear what is returned as a placeholder.
Comment 6 alexander :surkov 2012-05-07 02:09:32 PDT
Comment on attachment 621155 [details] [diff] [review]
Bug 748069 - Document UtteranceGenerator. r=surkov

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

::: accessible/src/jsat/AccessFu.jsm
@@ +53,5 @@
>    },
>  
>    /**
> +   * Start the special AccessFu mode, this primarily means controlling the
> +   * virtual cursor with arrow keys.

btw, the comment makes me think there's 'not special AccessFu mode' or at least AccessFu have different modes
Comment 7 Eitan Isaacson [:eeejay] 2012-05-07 09:36:17 PDT
(In reply to alexander :surkov from comment #6)
> Comment on attachment 621155 [details] [diff] [review]
> Bug 748069 - Document UtteranceGenerator. r=surkov
> 
> Review of attachment 621155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +53,5 @@
> >    },
> >  
> >    /**
> > +   * Start the special AccessFu mode, this primarily means controlling the
> > +   * virtual cursor with arrow keys.
> 
> btw, the comment makes me think there's 'not special AccessFu mode' or at
> least AccessFu have different modes

Removed "the special".
Comment 8 Eitan Isaacson [:eeejay] 2012-05-07 09:43:31 PDT
Created attachment 621636 [details] [diff] [review]
Document UtteranceGenerator. r=surkov
Comment 9 Eitan Isaacson [:eeejay] 2012-05-07 09:54:47 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bc7f3a9deba8
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-05-07 17:57:58 PDT
http://hg.mozilla.org/mozilla-central/rev/bc7f3a9deba8

Note You need to log in before you can comment on or make changes to this bug.