Add remaining text/caret navigation commands to ChromeVox Next.

Add remaining text/caret navigation commands to ChromeVox Next.

This adds the character, and word navigation commands with tentitive key bindings.
There is placeholder code to compute the utterance to be spokenwhich is not suitable for braille.

Committed: https://crrev.com/eb85cd11fa9087049cb5008272f7b11c45b24ade
Cr-Commit-Position: refs/heads/master@{#302114}

[David Tseng, 2014-10-24 22:53:50]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org, plundblad@chromium.org

[David Tseng, 2014-10-24 22:53:51]

Ready for an initial look.

[dmazzoni, 2014-10-27 07:06:20]

https://codereview.chromium.org/674263003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/674263003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:334:
if (range.getStart().getNode() === range.getEnd().getNode()) {
Suppose I'm moving by words and I move from the last word of one paragraph to
the first word of the next element, which happens to be a heading. This code
would have it read just the first word of the heading, and I think it'd drop the
role. Do you think it should still output "heading", or something like "entering
heading"?

As usual, you don't have to solve this issue completely, but I'd like to figure
out what the intended behavior is and how the code should be structured to
support it.

Just looking at the code, it feels wrong to compute the output and then replace
it in this special case here.

Oh - I'm also wondering if it makes sense for this code to trigger if you have a
range that spans from the first to the last character of a node, as opposed to a
range that just spans that node's element - is there any way we could make it so
that those two cases behave identically?

[David Tseng, 2014-10-27 15:53:55]

On Mon 27 Oct 2014 12:06:20 AM PDT, dmazzoni@chromium.org wrote:

 > 
 >
https://codereview.chromium.org/674263003/diff/20001/chrome/browser/resources...
 > File
 > chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
 > (right):
 > 
 >
https://codereview.chromium.org/674263003/diff/20001/chrome/browser/resources...
 >
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:334:
 > if (range.getStart().getNode() === range.getEnd().getNode()) {
 > Suppose I'm moving by words and I move from the last word of one
 > paragraph to the first word of the next element, which happens to be a
 > heading. This code would have it read just the first word of the
 > heading, and I think it'd drop the role. Do you think it should still
 > output "heading", or something like "entering heading"?
 > 
 > As usual, you don't have to solve this issue completely, but I'd like to
 > figure out what the intended behavior is and how the code should be
 > structured to support it.
 > 
 > Just looking at the code, it feels wrong to compute the output and then
 > replace it in this special case here.

This (as the TODO above hopefully conveys) the temporary nature of this
code. We, I think, want to develop a pretty flexible output system that
is capable of formatting both speech and braille. See the design doc for
more details.
 > 
 > Oh - I'm also wondering if it makes sense for this code to trigger if
 > you have a range that spans from the first to the last character of a
 > node, as opposed to a range that just spans that node's element - is
 > there any way we could make it so that those two cases behave
 > identically?
 > 

Do we want to? In line or sub node navigation, we may want it to be less
verbose. However, as mentioned above, this should now be pretty neatly
decoupled form the way navigation works. Navigation just maintains the
range and the output module will be responsible for computing speech and
braille based on a range and a set of format templates.

 > https://codereview.chromium.org/674263003/
 > 
 > To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
 > 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-10-27 21:31:44]

dtseng@chromium.org changed reviewers:
+ finnur@chromium.org

[David Tseng, 2014-10-27 21:31:45]

+ finnur; specifically, commands.cc and the restriction of an accelerator to
have only 3 keys. Is this limitation intended and if so, is whitelisting your
preferred solution?

@dmazzoni, added output class that should clean things up a little.

[dmazzoni, 2014-10-27 21:41:11]

In ChromeVox classic, output is normally based on both a previous node and a
current node, so we can compute the unique ancestors rather than all of them.
Should the interface here be something similar?

[David Tseng, 2014-10-27 21:55:03]

On Mon, Oct 27, 2014 at 2:41 PM, <dmazzoni@chromium.org> wrote:

> In ChromeVox classic, output is normally based on both a previous node and
> a
> current node, so we can compute the unique ancestors rather than all of
> them.
> Should the interface here be something similar?
>
> The Output class now tracks the previous range; I'm ok either way.

>
> https://codereview.chromium.org/674263003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-10-27 21:56:32]

lgtm

I'm not sure prevRange_ should belong to output, but we can deal with that
later. No other concerns from me.

[Finnur, 2014-10-28 10:16:12]

>  Is this limitation intended and if so, is whitelisting your preferred
solution?

This limitation was intended, because there was no shortcut possible on any
platform with four or more tokens. ChromeOS adds the fourth modifier (Search),
so I guess it would be possible to go above 3 now. I don't think we'd need to
whitelist this, but probably should make the limit depend on the platform
(ChromeOS > 4, others > 3).

But this got me thinking... Is there a way for the user to manually set Search
as a modifier through chrome://extensions/configureCommands? We should not leave
out from that UI keys that are settable through the manifest. See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

But that's a topic for another CL.

[David Tseng, 2014-10-28 14:23:20]

Actually, the limits are +1 I think:
Linux/Win: Ctrl+Alt+Shift+X
ChromeOS/Mac: Search or MacCommand +Ctrl+Alt+Shift+X

I'll go ahead and define a constant MAX_KEY_TOKENS ifdef'ed on win/linux to 4
and mac/chromeos to 5 if the above is correct.

Will followup on the second issue in a separate thread.

On 2014/10/28 10:16:12, Finnur wrote:
> >  Is this limitation intended and if so, is whitelisting your preferred
> solution?
> 
> This limitation was intended, because there was no shortcut possible on any
> platform with four or more tokens. ChromeOS adds the fourth modifier (Search),
> so I guess it would be possible to go above 3 now. I don't think we'd need to
> whitelist this, but probably should make the limit depend on the platform
> (ChromeOS > 4, others > 3).
> 
> But this got me thinking... Is there a way for the user to manually set Search
> as a modifier through chrome://extensions/configureCommands? We should not
leave
> out from that UI keys that are settable through the manifest. See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> But that's a topic for another CL.

[Finnur, 2014-10-28 14:49:49]

Not quite. It is:

Linux/Win: Ctrl and Alt are mutually exclusive, so the limit is still 3 there.
ChromeOS now has Search, which means it gets bumped to the 4 category.

I forget what the situation with Mac is. It may have been wrong to set a limit
of 3 there. Not sure.

But keep in mind the JavaScript link I pointed at, there's similar logic there
to make sure the user inputs the shortcut correctly and that has to change as
well, I believe.

[David Tseng, 2014-10-29 19:59:22]

PTAL
I filed crbug.com/428447 to track the Search key issue on the keyboard settings
UI.

[Finnur, 2014-10-30 10:04:06]

LGTM, two nits.

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
chrome/common/extensions/command.cc:32: static const int kMaxTokenSize = 4;
Nit: I'd comment this with:
// ChromeOS supports an additional modifier 'Search', which can
// result in longer sequences.

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
chrome/common/extensions/command.cc:49: ui::Accelerator ParseImpl(const
std::string& accelerator,
Can you comment this function with something like:

// Parse an |accelerator| for a given platform (specified by |platform_key|) and
// return the result as a ui::Accelerator if successful, or VKEY_UNKNOWN if not.
// |index| is used when constructing an |error| messages to show which command
// in the manifest is failing and |should_parse_media_keys| specifies whether
// media keys are to be considered for parsing.
// Note: If the parsing rules here are changed, make sure to update the
// corresponding extension_command_list.js validation, which validates the user
// input for chrome://extensions/configureCommands.

[David Tseng, 2014-10-30 17:00:12]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/674263003/180001

[David Tseng, 2014-10-30 17:01:47]

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
chrome/common/extensions/command.cc:32: static const int kMaxTokenSize = 4;
On 2014/10/30 10:04:06, Finnur wrote:
> Nit: I'd comment this with:
> // ChromeOS supports an additional modifier 'Search', which can
> // result in longer sequences.

Done.

https://codereview.chromium.org/674263003/diff/160001/chrome/common/extension...
chrome/common/extensions/command.cc:49: ui::Accelerator ParseImpl(const
std::string& accelerator,
On 2014/10/30 10:04:06, Finnur wrote:
> Can you comment this function with something like:
> 
> // Parse an |accelerator| for a given platform (specified by |platform_key|)
and
> // return the result as a ui::Accelerator if successful, or VKEY_UNKNOWN if
not.
> // |index| is used when constructing an |error| messages to show which command
> // in the manifest is failing and |should_parse_media_keys| specifies whether
> // media keys are to be considered for parsing.
> // Note: If the parsing rules here are changed, make sure to update the
> // corresponding extension_command_list.js validation, which validates the
user
> // input for chrome://extensions/configureCommands.

Done.

[commit-bot: I haz the power, 2014-10-30 18:33:47]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-10-30 18:34:28]

Patchset 10 (id:??) landed as
https://crrev.com/eb85cd11fa9087049cb5008272f7b11c45b24ade
Cr-Commit-Position: refs/heads/master@{#302114}