Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726433002:
Complete initial role and state mappings for ARC++ accessibility (Closed)
|
Created:
3 years, 9 months ago by David Tseng Modified:
3 years, 9 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionComplete initial role and state mappings for ARC++ accessibility
Android -> chrome classname mappings taken partially from
Android utils (Role.java) and
BrowserAccessibilityAndroid (inverse mapping).
BUG=683396
Review-Url: https://codereview.chromium.org/2726433002
Cr-Commit-Position: refs/heads/master@{#455508}
Committed: https://chromium.googlesource.com/chromium/src/+/37adeac7017da79020cfaf91b07f1f8d8c371b2e
Patch Set 1 : Provide shared constants file for Android accessibility related classnames. #Patch Set 2 : Clear focused node id only on blur. #
Total comments: 10
Patch Set 3 : Address feedback. #Patch Set 4 : Pager fix. #Patch Set 5 : Export. #
Messages
Total messages: 36 (26 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Provide shared constants file for Android accessibility related classnames. BUG= ========== to ========== Complete initial role and state mappings for ARC++ accessibility Android -> chrome classname mappings taken partially from Android utils (Role.java) and BrowserAccessibilityAndroid (inverse mapping). BUG=683396 ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org, yawano@chromium.org
Patchset #1 (id:1) has been deleted
lgtm https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:106: #define MAP_ROLE(android_class_name, chrome_role) \ Perhaps these should be a hash_map from string to ui::AXRole, at least as much as possible? https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:141: if (!text.empty()) How about only if it's a leaf node. The rest of the code will be very confused if a static text node has children (other than inline text boxes)
https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:104: GetStringProperty(node, arc::mojom::AccessibilityStringProperty::TEXT, &text); nit: We can get this later. We are not using the text until no role matches in the list. We can get text after we matched with all roles. https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:132: MAP_ROLE(ui::kAXSpinnerClassname, ui::AX_ROLE_SPIN_BUTTON); question: We have different mapping for AX_ROLE_SPIN_BUTTON in browser_accessibility_android.cc. Is this intentional? In browser_accessibility_android.cc, ui::AX_ROLE_SPIN_BUTTON is mapped to ui::kAxEditTextClassname. https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:152: state |= 1 << chrome_state; Can we change GetAxState to be passed AxNodeData and use AxNodeData::AddStateFlag here? It would be better if we could capsule the implementation details (i.e. 1 << chrome_state) into AxNodeData.
https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:132: MAP_ROLE(ui::kAXSpinnerClassname, ui::AX_ROLE_SPIN_BUTTON); On 2017/02/28 05:36:07, yawano wrote: > question: We have different mapping for AX_ROLE_SPIN_BUTTON in > browser_accessibility_android.cc. Is this intentional? In > browser_accessibility_android.cc, ui::AX_ROLE_SPIN_BUTTON is mapped to > ui::kAxEditTextClassname. I think the more appropriate mapping isto a pop_up_button. BAA maps a few things to spinner (one which is popup button). Role.java maps spinner to a drop down list. https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:141: if (!text.empty()) On 2017/02/28 04:31:25, dmazzoni wrote: > How about only if it's a leaf node. The rest of the code will be > very confused if a static text node has children (other than > inline text boxes) This is the fallback case (in case we can't assign any role to the node). From what I've seen and what BrowserAccessibilityAndroid does, this seems to be reasonable behavior (i.e. the chlidren are text nodes themselves) so ChromeVox would stop descending here. Let me know if you have any specific cases where you think this would break. https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:152: state |= 1 << chrome_state; On 2017/02/28 05:36:07, yawano wrote: > Can we change GetAxState to be passed AxNodeData and use > AxNodeData::AddStateFlag here? It would be better if we could capsule the > implementation details (i.e. 1 << chrome_state) into AxNodeData. Done
https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:106: #define MAP_ROLE(android_class_name, chrome_role) \ On 2017/02/28 04:31:25, dmazzoni wrote: > Perhaps these should be a hash_map from string to ui::AXRole, at least as much > as possible? It's not one to one and will probably be less so as time goes on, so let's keep it this way. Happy to make this cleaner in any other ways you can think of.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
lgtm for c/b/chromeos/arc/accessibility. Thank you! https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc (right): https://codereview.chromium.org/2726433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc:132: MAP_ROLE(ui::kAXSpinnerClassname, ui::AX_ROLE_SPIN_BUTTON); sgtm.
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 Link to the patchset: https://codereview.chromium.org/2726433002/#ps80001 (title: "Pager fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2726433002/#ps100001 (title: "Export.")
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": 1488999389954460,
"parent_rev": "4e4eae4cbe6136b538a3ea8297b07ef365d06337", "commit_rev":
"37adeac7017da79020cfaf91b07f1f8d8c371b2e"}
Message was sent while issue was closed.
Description was changed from ========== Complete initial role and state mappings for ARC++ accessibility Android -> chrome classname mappings taken partially from Android utils (Role.java) and BrowserAccessibilityAndroid (inverse mapping). BUG=683396 ========== to ========== Complete initial role and state mappings for ARC++ accessibility Android -> chrome classname mappings taken partially from Android utils (Role.java) and BrowserAccessibilityAndroid (inverse mapping). BUG=683396 Review-Url: https://codereview.chromium.org/2726433002 Cr-Commit-Position: refs/heads/master@{#455508} Committed: https://chromium.googlesource.com/chromium/src/+/37adeac7017da79020cfaf91b07f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/37adeac7017da79020cfaf91b07f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
