Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2757623003:
Initial support for accessible text fields and focus tracking in ARC++ (Closed)
|
Created:
3 years, 9 months ago by David Tseng Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, dtseng+watch_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, aboxhall+watch_chromium.org, hidehiko+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, lhchavez+watch_chromium.org, dmazzoni+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial support for accessible text fields and focus tracking in ARC++
In order to support text fields, full support for focus tracking is needed.
On Android, focus events can and do get fired on ancestors of the focused node. As a result, the accessibility service needs to manually call |findFocus| to get the deepest focused node.
TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted.
Additionally, navigate through the app with plain arrow keys. Verify output and focus movement gets reflected both from js and in ChromeVox.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2757623003
Cr-Commit-Position: refs/heads/master@{#458465}
Committed: https://chromium.googlesource.com/chromium/src/+/81caea92aaa24baa082bd4be9e07cc09839be419
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix focus. #Patch Set 3 : Simpler fix. #
Total comments: 9
Patch Set 4 : Address nits. #
Messages
Total messages: 33 (17 generated)
Description was changed from ========== Initial support for accessible text fields in ARC++ TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. BUG= ========== to ========== Initial support for accessible text fields in ARC++ TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Initial support for accessible text fields in ARC++ TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Initial support for accessible text fields in ARC++ TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org, y.davila0427@gmail.com
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yawano: arc dmazzoni: cvox2 There are some strange edge cases such as when arrowing to the beginning of the text field, the node appears to be invalidated on the Android end. Still looking into that; however, this cl gives basic text entry feedback pretty smoothly.
dtseng@chromium.org changed reviewers: + yawano@chromium.org - y.davila0427@gmail.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Don't forget to associate with a bug https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:87: arc::mojom::AccessibilityIntProperty prop, nit: indentation https://codereview.chromium.org/2757623003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js (right): https://codereview.chromium.org/2757623003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:402: // Only process editable text events if either system or ChromeVox focus is I think this might break contenteditable if the selection start and end are both children of the contenteditable root. Maybe walk up to the focused ancestor or something?
https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:350: if (GetIntProperty(node, AXIntProperty::TEXT_SELECTION_START, Node can have both TEXT_SELECTION_START and TEXT_SELECTION_END. It would be if (TEXT_SELECTION_START) { // Set start value } if (TEXT_SELECTION_END) { // Set end value } Not like, if (A) {} else if (B) {} https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:353: out_data->AddIntAttribute(ui::AX_ATTR_TEXT_SEL_END, val); This line of setting AX_ATTR_TEXT_SEL_END would be unnecessary.
https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:87: arc::mojom::AccessibilityIntProperty prop, On 2017/03/17 03:22:37, dmazzoni wrote: > nit: indentation Done. https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:350: if (GetIntProperty(node, AXIntProperty::TEXT_SELECTION_START, On 2017/03/17 08:38:23, yawano wrote: > Node can have both TEXT_SELECTION_START and TEXT_SELECTION_END. It would be > > if (TEXT_SELECTION_START) { > // Set start value > } > > if (TEXT_SELECTION_END) { > // Set end value > } > > Not like, > > if (A) {} else if (B) {} Done. https://codereview.chromium.org/2757623003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:353: out_data->AddIntAttribute(ui::AX_ATTR_TEXT_SEL_END, val); On 2017/03/17 08:38:23, yawano wrote: > This line of setting AX_ATTR_TEXT_SEL_END would be unnecessary. Yeah totally; was wip while I was trying to figure out why the text field gets detached from its parent. https://codereview.chromium.org/2757623003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js (right): https://codereview.chromium.org/2757623003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:402: // Only process editable text events if either system or ChromeVox focus is On 2017/03/17 03:22:37, dmazzoni wrote: > I think this might break contenteditable if the selection start and end are both > children of the contenteditable root. Maybe walk up to the focused ancestor or > something? Yeah, I think you're right. The issue is that on Android, the text field doesn't actually get focus as we're currently computing it. I might think about explicitly tracking focus on the Android end to help us from not doing this. I'll try to do something there before this change since comparing ranges is fragile.
PTAL. I removed the mappings for accessibility focus / clear accessibility focus. We now keep only the mapping for view focus which is input focus. This resolves the tracking of focused node and we now get the proper focused state computed. Note for Yuki. If you get the chance, try typing and arrowing around in a text field. If you arrow to the beginning and once more to the left, I'm seeing a focus event come through on the back button and then the entire window gets cleared. I'm guessing there's probably something else going on perhaps with an ime. I think though that this change is ready to go.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Initial support for accessible text fields in ARC++ TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Initial support for accessible text fields and focus tracking in ARC++ In order to support text fields, full support for focus tracking is needed. On Android, focus events can and do get fired on ancestors of the focused node. As a result, the accessibility service needs to manually call |findFocus| to get the deepest focused node. TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. Additionally, navigate through the app with plain arrow keys. Verify output and focus movement gets reflected both from js and in ChromeVox. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Ansewring my own questions, as FYI. dtseng@chromium.org writes: > PTAL. > > I removed the mappings for accessibility focus / clear accessibility focus. > We > now keep only the mapping for view focus which is input focus. This > resolves the > tracking of focused node and we now get the proper focused state > computed. Not doing this anymore. We can re-target focus events on the Android end to the proper focused node. > > Note for Yuki. If you get the chance, try typing and arrowing around in a > text > field. If you arrow to the beginning and once more to the left, I'm seeing a > focus event come through on the back button and then the entire window gets > cleared. I'm guessing there's probably something else going on perhaps with > an > ime. This is because we're not doing the right thing Android side. Focus events can have their source as the focused root. It is then up to the service to call |findFocus| to grab the deepest focused node, if possible. > > I think though that this change is ready to go. > > > https://codereview.chromium.org/2757623003/ -- -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:350: val >= 0) nit: add braces since the condition spans two lines.
lgtm. I'll play with this CL once I find a time. Thank you.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by: https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:21: return ui::AX_EVENT_FOCUS; nit: unnecessary change. https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/common/chromevox_json.js (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/common/chromevox_json.js:58: Boolean.prototype.toJSON = nit: ditto. Unnecessary change? https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:145: firstValidNode.root != undefined) { nit: looks unnecessary change? (Or any reasons to relax the condition...?)
https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:21: return ui::AX_EVENT_FOCUS; On 2017/03/21 03:43:23, hidehiko wrote: > nit: unnecessary change. Done. https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:350: val >= 0) On 2017/03/17 22:13:20, Luis Héctor Chávez wrote: > nit: add braces since the condition spans two lines. STyle guide says "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always always have an accompanying brace." and I've always omitted braces for single statement if's. https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/common/chromevox_json.js (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/common/chromevox_json.js:58: Boolean.prototype.toJSON = On 2017/03/21 03:43:23, hidehiko wrote: > nit: ditto. Unnecessary change? This is actually needed. It fixes a presubmit error. These usually happen because of an upgrade to Closure or someone changed an extern. In this case, we're basically just getting the presubmit to pass. This file will be deleted as part of moving everything to ChromeVox Next. https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:145: firstValidNode.root != undefined) { On 2017/03/21 03:43:23, hidehiko wrote: > nit: looks unnecessary change? (Or any reasons to relax the condition...?) It actually is. There are some cases, particularly in text fields, where the text field node is detached and its parent is set to null (not undefined).
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, yawano@chromium.org Link to the patchset: https://codereview.chromium.org/2757623003/#ps100001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490114587620350,
"parent_rev": "8fe2ec01a7bf1d14ff9ae0372b65da3df9b0e2e2", "commit_rev":
"81caea92aaa24baa082bd4be9e07cc09839be419"}
Message was sent while issue was closed.
Description was changed from ========== Initial support for accessible text fields and focus tracking in ARC++ In order to support text fields, full support for focus tracking is needed. On Android, focus events can and do get fired on ancestors of the focused node. As a result, the accessibility service needs to manually call |findFocus| to get the deepest focused node. TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. Additionally, navigate through the app with plain arrow keys. Verify output and focus movement gets reflected both from js and in ChromeVox. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Initial support for accessible text fields and focus tracking in ARC++ In order to support text fields, full support for focus tracking is needed. On Android, focus events can and do get fired on ancestors of the focused node. As a result, the accessibility service needs to manually call |findFocus| to get the deepest focused node. TEST=Play Store->Search Play Store->type. Verify characters and words are echoed when inserted, echoed in lower pitch when deleted. Additionally, navigate through the app with plain arrow keys. Verify output and focus movement gets reflected both from js and in ChromeVox. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2757623003 Cr-Commit-Position: refs/heads/master@{#458465} Committed: https://chromium.googlesource.com/chromium/src/+/81caea92aaa24baa082bd4be9e07... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/81caea92aaa24baa082bd4be9e07...
Message was sent while issue was closed.
FYI (after commit :-)). https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js (right): https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:145: firstValidNode.root != undefined) { On 2017/03/21 16:41:16, David Tseng wrote: > On 2017/03/21 03:43:23, hidehiko wrote: > > nit: looks unnecessary change? (Or any reasons to relax the condition...?) > > It actually is. There are some cases, particularly in text fields, where the > text field node is detached and its parent is set to null (not undefined). firstValidNode.root can be null, I think it's nice to comment (or explicitly check either null or undefined), for future maintenance.
Message was sent while issue was closed.
On 2017/03/21 22:00:43, hidehiko wrote: > FYI (after commit :-)). > > https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js > (right): > > https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:145: > firstValidNode.root != undefined) { > On 2017/03/21 16:41:16, David Tseng wrote: > > On 2017/03/21 03:43:23, hidehiko wrote: > > > nit: looks unnecessary change? (Or any reasons to relax the condition...?) > > > > It actually is. There are some cases, particularly in text fields, where the > > text field node is detached and its parent is set to null (not undefined). > > firstValidNode.root can be null, I think it's nice to comment (or explicitly > check either null or undefined), for future maintenance. Oops typo... "if firstValidNode.root can be null", I meant.
Message was sent while issue was closed.
hidehiko@chromium.org writes: > On 2017/03/21 22:00:43, hidehiko wrote: >> FYI (after commit :-)). > > > https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... >> File >> chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js >> (right): > > > https://codereview.chromium.org/2757623003/diff/80001/chrome/browser/resource... >> chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors.js:145: >> firstValidNode.root != undefined) { >> On 2017/03/21 16:41:16, David Tseng wrote: >> > On 2017/03/21 03:43:23, hidehiko wrote: >> > > nit: looks unnecessary change? (Or any reasons to relax the >> condition...?) >> > >> > It actually is. There are some cases, particularly in text fields, >> where the >> > text field node is detached and its parent is set to null (not >> undefined). > >> firstValidNode.root can be null, I think it's nice to comment (or >> explicitly >> check either null or undefined), for future maintenance. > > Oops typo... > > "if firstValidNode.root can be null", I meant. > In principle, I agree better readability clarity is important. However, anyone writing js should understand the difference between != and !==. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
