Switched to using |AXPosition| for calculating word and line boundaries on Windows.

Switched to using |AXPosition| for calculating word and line boundaries on Windows.

These are enough changes for one patch but followup patches need to:
1. Switch to using |AXPosition| for character boundaries and get rid of ui::FindAccessibleTextBoundary.
2. Fix all bugs with SetSelection in Blink and get rid of Windows specific workarounds.
BUG=672969
R=dmazzoni@chromium.org
TESTED=manual testing, browser tests

Review-Url: https://codereview.chromium.org/2806773002
Cr-Commit-Position: refs/heads/master@{#463920}
Committed: https://chromium.googlesource.com/chromium/src/+/dfe0ed4a9362cae34fe04555791f34b05cb21457

[nektarios, 2017-04-10 21:15:33]

The CQ bit was checked by nektar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 21:16:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nektarios, 2017-04-10 21:18:59]

Description was changed from

==========
Switched to using |AXPosition| for calculating word and line boundaries on
Windows.

There are still some corner cases that fail, but it's mostly done.
These are enough changes for one patch but followup patches need to:
1. Switch to using |AXPosition| for character boundaries and get rid of
ui::FindAccessibleTextBoundary.
2. Fix all bugs with SetSelection in Blink and get rid of Windows specific
workarounds.
BUG=672969
R=dmazzoni@chromium.org
TESTED=manual testing, browser tests
==========

to

==========
Switched to using |AXPosition| for calculating word and line boundaries on
Windows.

These are enough changes for one patch but followup patches need to:
1. Switch to using |AXPosition| for character boundaries and get rid of
ui::FindAccessibleTextBoundary.
2. Fix all bugs with SetSelection in Blink and get rid of Windows specific
workarounds.
BUG=672969
R=dmazzoni@chromium.org
TESTED=manual testing, browser tests
==========

[commit-bot: I haz the power, 2017-04-10 21:30:04]

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

[commit-bot: I haz the power, 2017-04-10 21:30:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dmazzoni, 2017-04-10 22:39:50]

lgtm

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
File content/browser/accessibility/ax_platform_position.cc (right):

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
content/browser/accessibility/ax_platform_position.cc:112: // Objects in the
shadow DOM are not visible to platform APIs and thus should
Shadow DOM isn't necessarily the correct term here. Sometimes that's indeed
what's going on, for example if this is a <textarea> then the inner content is
the shadow DOM. But if it's just a static text object that's a leaf then the
inline text box is not really shadow DOM.

How about just saying something about: not all nodes in the internal
accessibility
tree are exposed to the platform.

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

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:5007: previous_word =
position->CreatePreviousWordStartPosition();
Just curious, should CreatePreviousWordStartPosition and
CreateNextWordStartPosition always return the start or end of anchor if there's
no previous or next word?

I'm just thinking that might be better default behavior. But if another platform
needs this current behavior that's fine.

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:5015: }
Maybe add a NOTREACHED() after the switch since there shouldn't be
anything other than FORWARDS or BACKWARDS

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:5042: }
NOTREACHED

https://codereview.chromium.org/2806773002/diff/40001/ui/accessibility/ax_pos...
File ui/accessibility/ax_position.h (right):

https://codereview.chromium.org/2806773002/diff/40001/ui/accessibility/ax_pos...
ui/accessibility/ax_position.h:833: }
nit: no brackets needed here?

[chromium-reviews, 2017-04-11 16:36:05]

https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> File content/browser/accessibility/ax_platform_position.cc (right):
>
>
https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> content/browser/accessibility/ax_platform_position.cc:112: // Objects in
> the shadow DOM are not visible to platform APIs and thus should
> Shadow DOM isn't necessarily the correct term here. Sometimes that's
> indeed
> what's going on, for example if this is a <textarea> then the inner
> content is
> the shadow DOM. But if it's just a static text object that's a leaf then
> the
> inline text box is not really shadow DOM.
>
> How about just saying something about: not all nodes in the internal
> accessibility
> tree are exposed to the platform.
>
Done.

>
https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> content/browser/accessibility/browser_accessibility_win.cc:5007:
> previous_word = position->CreatePreviousWordStartPosition();
> Just curious, should CreatePreviousWordStartPosition and
> CreateNextWordStartPosition always return the start or end of anchor if
> there's no previous or next word?
>
> I'm just thinking that might be better default behavior. But if another
> platform needs this current behavior that's fine.
>

I think you might have found a potential bug.
The issue is that next / previous word will go through the whole page 
before returning NullPosition so it wouldn't help to return 
AtStart/EndOfAnchor all the time.
I'll submit this patch and then test if we'd need to bound AXPosition to 
the current edit field.


>
https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> content/browser/accessibility/browser_accessibility_win.cc:5015: }
> Maybe add a NOTREACHED() after the switch since there shouldn't be
> anything other than FORWARDS or BACKWARDS
>
Since this is an enumerated value I don't think it's needed. According 
to the style guide:
> If not conditional on an enumerated value, switch statements should 
> always have a default case (in the case of an enumerated value, the 
> compiler will warn you if any values are not handled).


>
https://codereview.chromium.org/2806773002/diff/40001/content/browser/accessi...
> content/browser/accessibility/browser_accessibility_win.cc:5042: }
> NOTREACHED
>
Same as above.

>
https://codereview.chromium.org/2806773002/diff/40001/ui/accessibility/ax_pos...
> File ui/accessibility/ax_position.h (right):
>
>
https://codereview.chromium.org/2806773002/diff/40001/ui/accessibility/ax_pos...
> ui/accessibility/ax_position.h:833: }
> nit: no brackets needed here?
>
Done.

-- 
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.

[nektarios, 2017-04-11 16:55:22]

The CQ bit was checked by nektar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:56:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 18:58:08]

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

[commit-bot: I haz the power, 2017-04-11 18:58:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nektarios, 2017-04-12 02:39:03]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-04-12 02:39:03]

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/2806773002/#ps80001
(title: "Fixed unit tests.")

[commit-bot: I haz the power, 2017-04-12 02:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 03:33:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491964743519440,
"parent_rev": "8bc11efe0ad6275747cb848e083051409b5144b2", "commit_rev":
"dfe0ed4a9362cae34fe04555791f34b05cb21457"}

[commit-bot: I haz the power, 2017-04-12 03:34:22]

Description was changed from

==========
Switched to using |AXPosition| for calculating word and line boundaries on
Windows.

These are enough changes for one patch but followup patches need to:
1. Switch to using |AXPosition| for character boundaries and get rid of
ui::FindAccessibleTextBoundary.
2. Fix all bugs with SetSelection in Blink and get rid of Windows specific
workarounds.
BUG=672969
R=dmazzoni@chromium.org
TESTED=manual testing, browser tests
==========

to

==========
Switched to using |AXPosition| for calculating word and line boundaries on
Windows.

These are enough changes for one patch but followup patches need to:
1. Switch to using |AXPosition| for character boundaries and get rid of
ui::FindAccessibleTextBoundary.
2. Fix all bugs with SetSelection in Blink and get rid of Windows specific
workarounds.
BUG=672969
R=dmazzoni@chromium.org
TESTED=manual testing, browser tests

Review-Url: https://codereview.chromium.org/2806773002
Cr-Commit-Position: refs/heads/master@{#463920}
Committed:
https://chromium.googlesource.com/chromium/src/+/dfe0ed4a9362cae34fe04555791f...
==========

[commit-bot: I haz the power, 2017-04-12 03:34:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/dfe0ed4a9362cae34fe04555791f...