Reports the position of the caret and current selection in content editables.

Reports the position of the caret and current selection in content editables.

Implements storing of per-frame selection offsets in the accessibility tree and adds Windows specific code for reporting these offsets across embedded objects according to the IA2 Spec.
BUG=491027
R=dmazzoni@chromium.org

Committed: https://crrev.com/8ff878a865f9810d9338a077680b706c2a07f265
Cr-Commit-Position: refs/heads/master@{#342368}

[dmazzoni, 2015-06-22 19:23:22]

https://codereview.chromium.org/1195223006/diff/1/components/test_runner/web_...
File components/test_runner/web_ax_object_proxy.cc (right):

https://codereview.chromium.org/1195223006/diff/1/components/test_runner/web_...
components/test_runner/web_ax_object_proxy.cc:747: blink::WebAxObject*
anchorObject = nullptr;
Typo: WebAxObject -> WebAXObject

https://codereview.chromium.org/1195223006/diff/1/components/test_runner/web_...
File components/test_runner/web_ax_object_proxy.h (right):

https://codereview.chromium.org/1195223006/diff/1/components/test_runner/web_...
components/test_runner/web_ax_object_proxy.h:79: // The following selection
functions return text offsets calculated starting
Did you mean "starting at this object".

https://codereview.chromium.org/1195223006/diff/1/components/test_runner/web_...
components/test_runner/web_ax_object_proxy.h:81: // object or on any of its
descendants.
Wouldn't it be more accurate to say, they report on a selection of an input or
textarea?

https://codereview.chromium.org/1195223006/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/1195223006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2012: int
selection_start = GetSelectionStart();
nit: indentation

https://codereview.chromium.org/1195223006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:3667: if
(!GetIntAttribute(ui::AX_ATTR_TEXT_SEL_START, &selection_start))
Did you mean this to be negated?

https://codereview.chromium.org/1195223006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:3672: if (!root ||
!GetIntAttribute(ui::AX_ATTR_OBJ_SEL_START, &focus_id))
Did you mean to call GetIntAttribute on root?

https://codereview.chromium.org/1195223006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:3688: auto
focus_parent = focus_object->GetParent();
Stick this part of the code in a helper function to be shared between
GetSelectionStart and GetSelectionEnd.

https://codereview.chromium.org/1195223006/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1195223006/diff/1/content/renderer/accessibil...
content/renderer/accessibility/blink_ax_tree_source.cc:511: WebAXObject
anchorObject, focusObject;
You don't want this code inside this inner "if" block (if
(node_to_frame_routing_id_map_ && !src.equals(GetRoot())) {) - it's unrelated to
those conditions.

Move it up next to the code like AX_ATTR_DOC_TITLE

https://codereview.chromium.org/1195223006/diff/1/content/renderer/accessibil...
content/renderer/accessibility/blink_ax_tree_source.cc:519:
dst->AddIntAttribute(ui::AX_ATTR_TEXT_SEL_START, anchorOffset);
I think it's confusing to reuse TEXT_SEL_START.

Add new attributes that more closely match the blink APIs - like
AX_ATTR_FOCUS_OBJ_ID, AX_ATTR_FOCUS_OFFSET, etc. - and then delete the old
attributes when we no longer need them.

https://codereview.chromium.org/1195223006/diff/1/ui/accessibility/ax_enums.idl
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1195223006/diff/1/ui/accessibility/ax_enums.i...
ui/accessibility/ax_enums.idl:272: obj_sel_start,
How about doc_sel_start instead?

[chromium-reviews, 2015-06-24 19:26:24]

All fixed.

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

[dmazzoni, 2015-06-26 06:22:16]

Great, just needs a unit test, ideally something very much like
BrowserAccessibilityTest::TestCaretAndTextSelection. I'd be happiest with a
simple small example that demonstrates the basic concept of the selection index
returning the hyperlink index.

Either David or Alice can review the test when you're finished with that, but
aside from the test I'm happy. lgtm

[chromium-reviews, 2015-07-15 18:52:27]

A) Wrote extensive unit tests for:
1. get_caretOffset, get_nSelections and get_selection on all objects 
inside a content editable.
2. Moving the focus and ensuring that results remain unaffected since 
this is a global selection.
3. Ensuring that standard form fields, text field and combo-box, remain 
unaffected.
4. Results match the IAcccessibleText examples in the IA2 Spec at
http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_...
5. If the object on which the caret or the selection endpoints are 
present is an ancestor of the current object, ensures that correct 
offsets are returned.

B) I had to fix the constness of IsDescendantOf(...) in 
BrowserAccessibility.
C) Browser tests will be sent out in another CL.

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

[dmazzoni, 2015-07-16 19:50:23]

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:9: #include
<algorithm>
Nit: don't group this with the Microsoft includes, above - put one more blank
line before it.

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:2009:
GetSelectionOffsets(&selection_start, &selection_end);
Are you 100% sure that this change isn't going to regress any existing behavior,
like for <input> and <textarea> elements? Please check carefully.

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3654: for (auto&
offset_index : offsets_map)
Braces around the body of the for loop since there's more than one line inside

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3750:
std::swap(*selection_start, *selection_end);
Are you sure this is correct? I think IA2 might support the selection start and
end being reversed indicating that the selection focus is at the beginning
rather than the end. Please verify and manually test with selections that go
both directions.

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.h:779: // First they
checks for a local selection found on the current control, e.g.
Nit: First they checks -> First they check

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win_unittest.cc
(right):

https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win_unittest.cc:985:
div_editable.state |= 1 << ui::AX_STATE_FOCUSED;
I don't think this line is doing anything, you're not using div_editable again.

https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:499: WebAXObject
anchorObject, focusObject;
This code doesn't make sense here - it's in a block of code that specifically
excludes the root of the tree. I think you accidentally broke this when merging
because it changed recently.

https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:504: int anchorId =
anchorObject.axID();
nit: anchor_id, etc.

[chromium-reviews, 2015-07-23 22:05:58]

All recommendations finished but nothing will be checked in until my 
browser test is ready. Also, tested Blink CL on its own without this CL.
I want to test set_selection as well so there is     still more testing 
to do.

On 7/16/2015 3:50 PM, dmazzoni@chromium.org wrote:
>
>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win.cc:9: #include
> <algorithm>
> Nit: don't group this with the Microsoft includes, above - put one more
> blank line before it.
>
Done.

>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win.cc:2009:
> GetSelectionOffsets(&selection_start, &selection_end);
> Are you 100% sure that this change isn't going to regress any existing
> behavior, like for <input> and <textarea> elements? Please check
> carefully.
>
I'll not check in anything until fully tested.

>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win.cc:3654: for
> (auto& offset_index : offsets_map)
> Braces around the body of the for loop since there's more than one line
> inside
>
Done.

>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win.cc:3750:
> std::swap(*selection_start, *selection_end);
> Are you sure this is correct? I think IA2 might support the selection
> start and end being reversed indicating that the selection focus is at
> the beginning rather than the end. Please verify and manually test with
> selections that go both directions.
>
Tested with Firefox. get_caretOffset always tells you where the focus 
(endpoint) of the selection is. get_selection always return start_offset 
to be <= end_offset.

>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> File content/browser/accessibility/browser_accessibility_win.h (right):
>
>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win.h:779: // First
> they checks for a local selection found on the current control, e.g.
> Nit: First they checks -> First they check
>
Fixed.

>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> File content/browser/accessibility/browser_accessibility_win_unittest.cc
> (right):
>
>
https://codereview.chromium.org/1195223006/diff/60001/content/browser/accessi...

>
> content/browser/accessibility/browser_accessibility_win_unittest.cc:985:
> div_editable.state |= 1 << ui::AX_STATE_FOCUSED;
> I don't think this line is doing anything, you're not using div_editable
> again.
>
Correct but it's better whenever there is a state transition to always 
update internal data structures to reflect the change in case I add more 
code in the future and then spend a long time trying to understand why 
the state doesn't update.

>
https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...

>
> File content/renderer/accessibility/blink_ax_tree_source.cc (right):
>
>
https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...

>
> content/renderer/accessibility/blink_ax_tree_source.cc:499: WebAXObject
> anchorObject, focusObject;
> This code doesn't make sense here - it's in a block of code that
> specifically excludes the root of the tree. I think you accidentally
> broke this when merging because it changed recently.
>
Fixed.

>
https://codereview.chromium.org/1195223006/diff/60001/content/renderer/access...

>
> content/renderer/accessibility/blink_ax_tree_source.cc:504: int anchorId
> = anchorObject.axID();
> nit: anchor_id, etc.
>
Fixed.

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

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

[dmazzoni, 2015-07-27 19:22:36]

I don't see a new upload

[chromium-reviews, 2015-07-27 22:02:58]

I guess I forgot to upload. Sorry. Can you see it now?

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

[dmazzoni, 2015-07-27 23:24:15]

lgtm

https://codereview.chromium.org/1195223006/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/1195223006/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3738: if
(!selection_start || !selection_end)
This could maybe be a CHECK instead, unless there's a good reason to pass null
here.

[nektarios, 2015-08-05 19:02:44]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-05 19:02:44]

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/1195223006/#ps100001
(title: "Added a CHECK for nullptr function arguments.")

[commit-bot: I haz the power, 2015-08-05 19:03:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/100001

[commit-bot: I haz the power, 2015-08-05 20:05:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 20:05:44]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[nektarios, 2015-08-05 23:15:30]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-05 23:15:31]

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/1195223006/#ps120001
(title: "Fixed DCHECK check.")

[commit-bot: I haz the power, 2015-08-05 23:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/120001

[commit-bot: I haz the power, 2015-08-06 00:27:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 00:27:24]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nektarios, 2015-08-06 15:20:53]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2015-08-06 15:21:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/120001

[commit-bot: I haz the power, 2015-08-06 16:19:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 16:19:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nektarios, 2015-08-06 16:26:30]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2015-08-06 16:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/120001

[commit-bot: I haz the power, 2015-08-06 16:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 16:58:38]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[nektarios, 2015-08-06 20:57:31]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-06 20:57:32]

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/1195223006/#ps140001
(title: "Fixed one affected test.")

[commit-bot: I haz the power, 2015-08-06 20:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/140001

[commit-bot: I haz the power, 2015-08-06 22:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 22:05:34]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nektarios, 2015-08-07 14:36:14]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-07 14:36:15]

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/1195223006/#ps160001
(title: "Disabled tests that require CL that fixes IA2 Hypertext to be submitted
before they would start fun…")

[commit-bot: I haz the power, 2015-08-07 14:36:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/160001

[commit-bot: I haz the power, 2015-08-07 14:38:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:38:01]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[nektarios, 2015-08-07 14:46:01]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:46:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/160001

[commit-bot: I haz the power, 2015-08-07 14:47:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:47:41]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nektarios, 2015-08-07 14:52:56]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-07 14:52:56]

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/1195223006/#ps180001
(title: "Added back DCHECKs.")

[commit-bot: I haz the power, 2015-08-07 14:53:20]

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

[commit-bot: I haz the power, 2015-08-07 14:54:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 14:54:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nektarios, 2015-08-07 15:46:35]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2015-08-07 15:46:36]

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/1195223006/#ps200001
(title: "Rebased with master.")

[commit-bot: I haz the power, 2015-08-07 15:46:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195223006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1195223006/200001

[commit-bot: I haz the power, 2015-08-07 16:57:06]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-07 16:57:47]

Patchset 11 (id:??) landed as
https://crrev.com/8ff878a865f9810d9338a077680b706c2a07f265
Cr-Commit-Position: refs/heads/master@{#342368}