Issue 2271893002: Creates AXPosition to uniquely identify a position in the accessibility tree

nektarios

4 years, 4 months ago (2016-08-23 23:18:45 UTC) #1

dmazzoni

This is looking like it's on the right track. Consider trying to implement Mac TextMarkers ...

4 years, 4 months ago (2016-08-24 05:46:59 UTC) #2

dmazzoni

https://codereview.chromium.org/2271893002/diff/20001/content/browser/accessibility/ax_platform_position.h File content/browser/accessibility/ax_platform_position.h (right): https://codereview.chromium.org/2271893002/diff/20001/content/browser/accessibility/ax_platform_position.h#newcode22 content/browser/accessibility/ax_platform_position.h:22: ui::AXTextAffinity get_affinity() { return affinity_; } I think this ...

4 years, 2 months ago (2016-10-03 18:22:51 UTC) #3

dmazzoni

https://codereview.chromium.org/2271893002/diff/40001/content/browser/accessibility/ax_platform_position.h File content/browser/accessibility/ax_platform_position.h (right): https://codereview.chromium.org/2271893002/diff/40001/content/browser/accessibility/ax_platform_position.h#newcode33 content/browser/accessibility/ax_platform_position.h:33: std::unique_ptr<ui::AXPositionFactory> factory = It's not necessarily a bad idea ...

4 years, 2 months ago (2016-10-04 20:08:24 UTC) #4

nektarios

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

4 years, 2 months ago (2016-10-05 15:09:32 UTC) #5

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-05 15:10:06 UTC) #6

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-05 15:20:51 UTC) #7

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-05 15:20:52 UTC) #8

dmazzoni

https://codereview.chromium.org/2271893002/diff/120001/content/browser/accessibility/browser_accessibility_cocoa.mm File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2271893002/diff/120001/content/browser/accessibility/browser_accessibility_cocoa.mm#newcode142 content/browser/accessibility/browser_accessibility_cocoa.mm:142: const AXPlatformPositionFactory factory; I'm assuming this needs to be ...

4 years, 2 months ago (2016-10-05 18:15:30 UTC) #9

nektarios

nektar@chromium.org changed reviewers: + ellyjones@chromium.org

4 years, 2 months ago (2016-10-05 19:02:27 UTC) #10

dmazzoni

I'm a bit confused now because it seems like you have both templatization AND subclassing, ...

4 years, 2 months ago (2016-10-05 19:10:35 UTC) #12

nektarios

The idea was, as ingeniously suggested by Elly, to have: template<AXNodeType> static typename AXNodeType::AXPosition AXPosition<AXNodeType>::Create...(...) ...

4 years, 2 months ago (2016-10-06 16:34:25 UTC) #13

Elly Fong-Jones

On 2016/10/06 16:34:25, nektarios wrote: > The idea was, as ingeniously suggested by Elly, to ...

4 years, 2 months ago (2016-10-07 15:08:36 UTC) #14

nektarios

Hi Elly, The template pattern seems to work but I have a compilation error I ...

4 years, 2 months ago (2016-10-07 20:47:34 UTC) #15

David Tseng

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

4 years, 2 months ago (2016-10-07 21:02:40 UTC) #16

David Tseng

Good start! Driving by as a potential customer/bug fixer. I am wondering, is AXPosition/AXRange defined ...

4 years, 2 months ago (2016-10-07 21:02:42 UTC) #17

nektarios

Description was changed from ========== Initial design of AXPosition class. R=dmazzoni@chromium.org ========== to ========== Initial ...

4 years, 2 months ago (2016-10-10 16:56:18 UTC) #18

nektarios

nektar@chromium.org changed reviewers: + avi@chromium.org

4 years, 2 months ago (2016-10-10 16:56:18 UTC) #19

dmazzoni

> > However, with the help of Avi we found a better design: > Curiously ...

4 years, 2 months ago (2016-10-10 17:22:04 UTC) #21

chromium-reviews

On 10/10/2016 1:21 PM, Dominic Mazzoni wrote: > > However, with the help of Avi ...

4 years, 2 months ago (2016-10-10 18:14:07 UTC) #22


On 10/10/2016 1:21 PM, Dominic Mazzoni wrote:
>
>     However, with the help of Avi we found a better design:
>     Curiously recurring template pattern
>     https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
>
>
> I'm not sure that's the pattern we're using. If we were using that 
> pattern, BrowserAccessibility would inherit from 
> AXPosition<BrowserAccessibility>.
>
I think we are using this pattern because
AXPlatformPosition inherits from AXPosition<AXPlatformPosition, 
BrowserAccessibility>
That's why it's recursive.

> However, having a clone() method instead of a factory sounds fine. 
> Perhaps we should just call it create(...) and have it take the same 
> arguments as the constructor?
>
I have three Create methods now, one per kind of position, text 
position, tree position and null position.
I prefer if I hide the position's kind from the users of this class, so 
that we could add conversions between tree position and text positions 
that look more natural.
I mean, instead of modifying the kind, we would call a method such as 
ToTextPosition().

> After seeing David's comments I keep coming back to this idea that 
> we're making it way more complicated than it needs to be. Did you 
> consider the idea of having AXPosition operate on an AXTree and AXNode 
> directly and only allow it to be allocated on the stack so we can use 
> it temporarily?
>
How is that different from AXTextMarker we already have on the Mac? It's 
just a struct without any logic. I thought we wanted something smarter.
I spoke to David on the phone and explained and I think he agrees with 
this design safe some details.
However, we'll work together when he is in New York on Thursday, to make 
this work across ChromeVox, in addition to Mac and Win.

> I do think there are some advantages of a class over a functional 
> interface, but I think that all of the templatizing and subclassing 
> and converting between IDs and nodes is a lot of complication, and I'm 
> not convinced we couldn't do something a lot simpler.
>
Essentially, a functional interface is what we already have and it's 
messy, isn't that so?
I think that there would be multiple functions and it would be even more 
complicated, because every function needs to get at least a node and an 
offset and then validate them. Then, we'd need functions for word, 
character and line for example, and different overrides for Windows and 
Mac, to handle text positions vs. tree positions.
And all those functions would go on BrowserAccessibilityManager? What 
about ChromeVox and Automation, don't we want to share logic with that 
part of the code?
All these functions are nothing less than some methods that operate on 
the same data, i.e. a node, some offsets and a tree. This is a class.
Plus, we do need to keep state, isn't it? What if I want all the lines 
instead of only the next one. I'll call GetNextLinePosition and keep 
calling it until I get the Null position.
Unless we'd have another function for getting all the lines?
Another example is on Windows there are cases we want to get both the 
next boundary (e.g. word) and the previous one, hence we'd need to 
compute state for the current position.
Then, there is the idea of caching. If we have a class we'd have a 
better chance optimizing performance down the line.
Also, if we get lines and words and characters etc. working, I was 
thinking of making selections and bounding rectangles be part of 
AXRange, so that you get a range and then call a method to get the 
selection or the bounding rectangle. This would allow us to put 
validation and kanonicalization code in one place.
However, I do agree it's somewhat complicated,.

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

dmazzoni

On Mon, Oct 10, 2016 at 11:14 AM 'Nektarios Paisios' via Chromium-reviews < chromium-reviews@chromium.org> wrote: ...

4 years, 2 months ago (2016-10-10 18:33:54 UTC) #23

On Mon, Oct 10, 2016 at 11:14 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> However, having a clone() method instead of a factory sounds fine. Perhaps
> we should just call it create(...) and have it take the same arguments as
> the constructor?
>
> I have three Create methods now, one per kind of position, text position,
> tree position and null position.
> I prefer if I hide the position's kind from the users of this class, so
> that we could add conversions between tree position and text positions that
> look more natural.
> I mean, instead of modifying the kind, we would call a method such as
> ToTextPosition().
>

Wouldn't clone() be protected? I was imagining that clone would just be
used so that implementations in the base AXPosition class could return an
appropriate AXPosition subclass as a return value.

Do you think it'd be better to call clone() and then modify the resulting
object?

> After seeing David's comments I keep coming back to this idea that we're
> making it way more complicated than it needs to be. Did you consider the
> idea of having AXPosition operate on an AXTree and AXNode directly and only
> allow it to be allocated on the stack so we can use it temporarily?
>
> How is that different from AXTextMarker we already have on the Mac? It's
> just a struct without any logic. I thought we wanted something smarter.
>

I think we're all in agreement that we want functionality like converting
between deep positions and shallow positions, and finding the next line
break, and stuff like that.

Essentially, a functional interface is what we already have and it's messy,
> isn't that so?
>

I feel like what we have right now is that every time we need to walk the
tree for text positions we write custom code to do so. The advantage of
AXPosition is that it would consolidate a lot of the tree walking code for
text positions into a single class.

I'm not saying I want a functional interface, I'm saying I want an object
that's used only on the stack to do temporary manipulations of positions,
not a persistent object that stores a position for a long time.

Then, there is the idea of caching. If we have a class we'd have a better
> chance optimizing performance down the line.
>

I'm not sure where we could cache these. Think about all of the Windows API
calls. In lots of the IAccessibleText functions we're given a position and
asked to return some other position. There's nothing to cache.

> Also, if we get lines and words and characters etc. working, I was
> thinking of making selections and bounding rectangles be part of AXRange,
> so that you get a range and then call a method to get the selection or the
> bounding rectangle. This would allow us to put validation and
> kanonicalization code in one place.
>

That sounds good, I'm not disagreeing with any of that. However I think
this is another example of why we just want a temporary object, not a
persistent one.

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

chromium-reviews

Addressed some of David's comments. On 10/10/2016 2:27 PM, Dominic Mazzoni wrote: > On Mon, ...

4 years, 2 months ago (2016-10-10 19:40:17 UTC) #24

Addressed some of David's comments.

On 10/10/2016 2:27 PM, Dominic Mazzoni wrote:
> On Mon, Oct 10, 2016 at 11:14 AM 'Nektarios Paisios' via 
> Chromium-reviews <chromium-reviews@chromium.org 
> <mailto:chromium-reviews@chromium.org>> wrote:
>
>>     However, having a clone() method instead of a factory sounds
>>     fine. Perhaps we should just call it create(...) and have it take
>>     the same arguments as the constructor?
>>
>     I have three Create methods now, one per kind of position, text
>     position, tree position and null position.
>     I prefer if I hide the position's kind from the users of this
>     class, so that we could add conversions between tree position and
>     text positions that look more natural.
>     I mean, instead of modifying the kind, we would call a method such
>     as ToTextPosition().
>
>
> Wouldn't clone() be protected? I was imagining that clone would just 
> be used so that implementations in the base AXPosition class could 
> return an appropriate AXPosition subclass as a return value.
>
> Do you think it'd be better to call clone() and then modify the 
> resulting object?

With my current implementation Create... methods could be called from 
both inside the base class and its sub-classes, as well as from outside 
the class with the same effect.
I used "new AXNodeType()" and then Initialize(...).
I don't feel that comfortable cloning and then modifying even though I 
know it will work, because it might be tricky to maintain in the future.
>
> I'm not saying I want a functional interface, I'm saying I want an 
> object that's used only on the stack to do temporary manipulations of 
> positions, not a persistent object that stores a position for a long time.
>
Okay, but the complexity doesn't arise from looking up node IDs, does it?
We had an issue with object construction from sub-classes. Now it's 
solved. This is what created the complexity and took time to design.
Using temporary vs. persistent nodes is one more virtual method override 
|GetNodeInTree|. That's not that bad, isn't it?
I could change it back to get pointers to nodes instead of IDs, but how 
much of the complexity will go away?

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

dmazzoni

OK, I see how it works now. I was confused because in your comment it ...

4 years, 2 months ago (2016-10-10 20:00:56 UTC) #25

dmazzoni

https://codereview.chromium.org/2271893002/diff/240001/ui/accessibility/ax_position.h File ui/accessibility/ax_position.h (right): https://codereview.chromium.org/2271893002/diff/240001/ui/accessibility/ax_position.h#newcode341 ui/accessibility/ax_position.h:341: virtual AXPosition<AXPositionType, AXNodeType>* GetChildPositionAt( Would it be simpler to ...

4 years, 2 months ago (2016-10-10 20:05:15 UTC) #26

nektarios

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

4 years, 2 months ago (2016-10-10 20:12:06 UTC) #27

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 20:12:25 UTC) #28

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 20:25:50 UTC) #29

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/213965) linux_chromium_chromeos_ozone_rel_ng on ...

4 years, 2 months ago (2016-10-10 20:25:51 UTC) #30

nektarios

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

4 years, 2 months ago (2016-10-10 21:25:32 UTC) #31

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 21:25:57 UTC) #32

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 21:38:31 UTC) #33

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/142841)

4 years, 2 months ago (2016-10-10 21:38:33 UTC) #34

nektarios

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

4 years, 2 months ago (2016-10-10 22:11:03 UTC) #35

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 22:11:25 UTC) #36

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-10 22:33:34 UTC) #37

commit-bot: I haz the power

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_ng/builds/311898)

4 years, 2 months ago (2016-10-10 22:33:35 UTC) #38

dmazzoni

https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_node_position.h File ui/accessibility/ax_node_position.h (right): https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_node_position.h#newcode17 ui/accessibility/ax_node_position.h:17: class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> { Is ...

4 years, 2 months ago (2016-10-12 20:45:41 UTC) #39

nektarios

On 2016/10/12 at 20:45:41, dmazzoni wrote: > https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_node_position.h > File ui/accessibility/ax_node_position.h (right): > > https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_node_position.h#newcode17 ...

4 years, 2 months ago (2016-10-18 00:05:43 UTC) #40

On 2016/10/12 at 20:45:41, dmazzoni wrote:
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_no...
> File ui/accessibility/ax_node_position.h (right):
> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_no...
> ui/accessibility/ax_node_position.h:17: class AX_EXPORT AXNodePosition :
public AXPosition<AXNodePosition, AXNode> {
> Is this intended only for testing? It can't really
> work without SetTreeForTesting.

For now yes, but my intention is to use it for the Automation Tree. Automation
uses an AXNode internally and so we could make use of this class there. That
doesn't mean that I might not need to tweak it a bit to add the logic for AXTree
management, but let's do this in another patch.



> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_no...
> File ui/accessibility/ax_node_position_unittest.cc (right):
> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_no...
> ui/accessibility/ax_node_position_unittest.cc:151:
std::unique_ptr<AXPosition<AXNodePosition, AXNode>> null_position(
> How about a typedef for AXPosition<AXNodePosition, AXNode>?

Done. Called TestPositionType.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> File ui/accessibility/ax_position.h (right):
> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:231: // TODO(nektar): Not yet implemented for
tree positions.
> As you walk the tree won't you go between tree positions
> and text positions? It seems like this class needs another
> virtual method that returns whether or not a node is a
> text node or not.

I don't know if my latest patch fixed this or if it was correct from before, but
basically I chose to skip having something like  |IsTextOnlyNode| and simply
check if the node has any children. If it doesn't then I assume it is a text
node because all inline text boxes are leaves. Also, in the cases when a leaf
node is not an inline text box, we would want to traverse its text anyway, e.g.
when an image has alt text.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:317: virtual AXPosition<AXPositionType,
AXNodeType>* GetPreviousAnchorPosition()
> Shouldn't this return the end of the previous anchor? I don't see where it
does that.

Fixed.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:336: leaf->GetChildPositionAt(0);
> It doesn't look like you're doing anything with the return value here,
> this could loop forever

Fixed.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:342: virtual AXPosition<AXPositionType,
AXNodeType>* GetChildPositionAt(
> As I said in a previous comment, I think it would be better
> to have this be GetChildAt() and return an AXNodeType, because then
> you could implement GetChildPositionAt in terms of that and not
> need to duplicate all of the logic in two subclasses.

Done but with a tweak. Since sometimes I need an AXNodeType and at other times I
need an ID, so I implemented |AnchorChildID| and |AnchorParentID| to get the IDs
and I can easily convert those to nodes using |AXNodeInTree|.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:344: virtual AXPosition<AXPositionType,
AXNodeType>* GetParentPosition() const = 0;
> Same, this should return an AXNodeType.

Done but see above.

> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_ra...
> File ui/accessibility/ax_range.h (right):
> 
>
https://codereview.chromium.org/2271893002/diff/320001/ui/accessibility/ax_ra...
> ui/accessibility/ax_range.h:12: struct AXRange {
> I suggest you delete it from this change since you're
> not using it or testing it anywhere

I'll delete it before checking in and you could have a final look then. I'll
also delete all operators and CommonAncestor. I wanted to make sure that I don't
introduce a breaking change to that file or functions with all these rewrites.

nektarios

BUG and TESTED lines added. https://codereview.chromium.org/2271893002/diff/160001/content/browser/accessibility/ax_platform_position.h > File content/browser/accessibility/ax_platform_position.h (right): > > https://codereview.chromium.org/2271893002/diff/160001/content/browser/accessibility/ax_platform_position.h#newcode26 > content/browser/accessibility/ax_platform_position.h:26: ...

4 years, 2 months ago (2016-10-18 00:36:22 UTC) #41

BUG and TESTED lines added.

https://codereview.chromium.org/2271893002/diff/160001/content/browser/access...
> File content/browser/accessibility/ax_platform_position.h (right):
> 
>
https://codereview.chromium.org/2271893002/diff/160001/content/browser/access...
> content/browser/accessibility/ax_platform_position.h:26: int text_offset,
> Can you think of a way to combine text_offset and child_index? One can be
derived from the other.

Currently it might be a good idea to have both so that we can quickly switch
between the two kinds of positions, but I am open to combining them after I try
to use this on Windows and see if there are any performance benefits in having
both or not.
Also, I am not sure if I'll eventually keep the distinction between tree and
text position internally in the class. It might be better for a position to have
both a child_index and a text_offset, e.g. when you want to point to a character
inside a child of the anchor.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/content/browser/access...
> content/browser/accessibility/ax_platform_position.h:29: int
AnchorChildCount() const override {
> AXPosition<BrowserAccessibility> overrides
> 
>
https://codereview.chromium.org/2271893002/diff/160001/content/browser/access...
> content/browser/accessibility/ax_platform_position.h:30: return GetAnchor() ?
static_cast<int>(GetAnchor()->PlatformChildCount())
> This isn't a simple getter; put in .cc

I wish I could, but the style guide says that all the code of a templated class
should go in a header file.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/content/browser/access...
> content/browser/accessibility/ax_platform_position.h:31: : -1;
> Name this special constant

All special constants have been named.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> File ui/accessibility/ax_position.h (right):
> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:16: enum class AXPositionType { NullPosition,
TreePosition, TextPosition };
> Please expand a little on these. What exactly is a null position?

Wrote a comment.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:20: // text node and a character offset.
> What is a text node?

Wrote a comment.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:34: int get_tree_id() const { return tree_id_;
}
> Remove the get_ here
> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:35: int32_t get_anchor_id() const { return
anchor_id_; }
> Remove the get_ here

Simple geters should be preceded with a "get_" isn't it? In Blink they are not,
but in Chromium they are.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:38: int get_text_offset() const { return
text_offset_; }
> I think we should re-think the design here.
> 
> either:
> - don't explicitly include tree vs text positions and simply have one offset
(i.e. yuou could have two methods, IsTextPosition/IsTreePosition which simply
checks the node is/isn't a text node).
> - create subclasses AXTextPosition/AXTreePosition.

I understand the subclasses idea, but it would become too complicated, because
then I would need two subclasses per platform, i.e. two subclasses for
Automation and two for Win / Mac.
Essentially, I think I would need to create two sub-templates as it were,
|AXTextPosition<AXTextPositionType, AXNodeType>| and
|AXTreePosition<AXTreePositionType, AXNodeType>|, that would inherit from a
single base template. That sounds hard.
A better idea is to drop the distinction and have the position class include
both a child index and a text offset. Clients could simply use either or both,
depending on the use case.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:40: void set_affinity(ui::AXTextAffinity
affinity) { affinity_ = affinity; }
> I think it would make more sense to make all properties read-only (i.e. set on
construction).

I agree but I want to get rid of affinity and so I don't want to promote it
unnecessarily to something you need to provide at construction. Affinity should
go away eventually.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:49: return GetAnchor() && type_ ==
AXPositionType::TextPosition;
> So, in theory, I could construct an AXPosition (w char offset) on a non-text
node? I thought that wasn't allowed:
> +// It could either indicate a non-textual node in the accessibility tree, or
a
> +// text node and a character offset.

Yes, the text offset would refer to the text inside the object, either directly
or from all its children.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:53: bool AtEndOfAnchor() const;
> In Blink, the position types are:
> offsetInAnchor, beforeAnchor, afterAnchor, beforeChildren, afterChildren.
> 
> Every time we deviate from what Blink does, it will make it that much harder
to map a position back to the DOM.

Eventually I could implement more types.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:57: bool operator<(const AXPosition& position)
const;
> Please add some comments for all of these and above.

I thought operators were self explanatory.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:78: // Uses depth-first pre-order traversal.
> What does this mean? The next tree or character position?

No, constructs a new position using the next anchor in the tree found using
depth-first traversal. The kind of position is not changed.

> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:83: virtual AXPosition* GetChildPositionAt(int
child_index) const = 0;
> The more I read this, the more I thnk we should separate into AXTextPosition
and AXTreePosition.

See above. It would be too complicated.
> 
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> ui/accessibility/ax_position.h:88: virtual int MaxTextOffset() const = 0;
> Definitely needs comment.

Done.

nektarios

Description was changed from ========== Initial design of AXPosition class. R=dmazzoni@chromium.org, dtseng@chromium.org, avi@chromium.org, ellyjones@chromium.org ========== ...

4 years, 2 months ago (2016-10-18 00:41:48 UTC) #42

dmazzoni

> > > https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_position.h#newcode35 > > ui/accessibility/ax_position.h:35: int32_t get_anchor_id() const { return > anchor_id_; } ...

4 years, 2 months ago (2016-10-18 05:53:30 UTC) #43

>
>
>
https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> > ui/accessibility/ax_position.h:35: int32_t get_anchor_id() const { return
> anchor_id_; }
> > Remove the get_ here
>
> Simple geters should be preceded with a "get_" isn't it? In Blink they are
> not,
> but in Chromium they are.
>

Chromium uses the Google C++ style guide, if it's a simple getter with no
side effects, no "get_foo" is needed, just "foo". If it's a virtual method
or something that computes a value, use GetFoo() with camel case.

I'm pretty sure that cases in Chromium and Blink where you've seen Get
being added is because they're virtual.

> - don't explicitly include tree vs text positions and simply have one
> offset
> (i.e. yuou could have two methods, IsTextPosition/IsTreePosition which
> simply
> checks the node is/isn't a text node).
> > - create subclasses AXTextPosition/AXTreePosition.
>

I agree with Nektarios, subclasses for text position and tree position
would be overkill.

Eventually we might only need one offset, but I think the advantage of this
approach is that it adds a bit of extra type safety and forces you to
acknowledge what type of position you're getting.

I vote that we stick with this for now.

https://codereview.chromium.org/2271893002/diff/160001/ui/accessibility/ax_po...
> > ui/accessibility/ax_position.h:40: void set_affinity(ui::AXTextAffinity
> affinity) { affinity_ = affinity; }
> > I think it would make more sense to make all properties read-only (i.e.
> set on
> construction).
>
> I agree but I want to get rid of affinity and so I don't want to promote it
> unnecessarily to something you need to provide at construction. Affinity
> should
> go away eventually.
>

I think I agree with David here that until affinity goes away, I'd rather
not make it a weird special case. Just put it in the constructor. It won't
be that hard to refactor it later. Until affinity goes away I'd rather try
to force us to deal with affinity correctly.

> Every time we deviate from what Blink does, it will make it that much
> harder
> to map a position back to the DOM.
>

I don't think the primary goal of this class is to represent Blink
selection, this class is to represent positions and selection as exposed to
platform accessibility APIs. Those APIs tend to be a lot simpler than
Blink's model of selection.

I suspect we'll need to keep modifying this code as we rewrite the rest of
the Mac and Windows APIs in terms of AXPosition, so I'm okay with just
keeping this for now.

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

dmazzoni

Tentative looks good, but would prefer to see you remove the unused code and upload ...

4 years, 2 months ago (2016-10-18 05:55:03 UTC) #44

dmazzoni

Tentative looks good, but would prefer to see you remove the unused code and upload ...

4 years, 2 months ago (2016-10-18 05:55:05 UTC) #45

nektarios

1. I removed all irrelevant code. 2. I changed all "get_..." methods and dropped the ...

4 years, 2 months ago (2016-10-18 23:03:19 UTC) #46

dmazzoni

https://codereview.chromium.org/2271893002/diff/420001/content/browser/accessibility/ax_platform_position.cc File content/browser/accessibility/ax_platform_position.cc (right): https://codereview.chromium.org/2271893002/diff/420001/content/browser/accessibility/ax_platform_position.cc#newcode15 content/browser/accessibility/ax_platform_position.cc:15: int32_t AXPlatformPosition::AnchorChildID(int child_index) const { I just realized that ...

4 years, 2 months ago (2016-10-18 23:59:12 UTC) #47

chromium-reviews

https://codereview.chromium.org/2271893002/diff/420001/content/browser/accessibility/ax_platform_position.cc > File content/browser/accessibility/ax_platform_position.cc (right): > > https://codereview.chromium.org/2271893002/diff/420001/content/browser/accessibility/ax_platform_position.cc#newcode15 > content/browser/accessibility/ax_platform_position.cc:15: int32_t > AXPlatformPosition::AnchorChildID(int child_index) const ...

4 years, 2 months ago (2016-10-20 00:21:54 UTC) #48

nektarios

The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the ...

4 years, 2 months ago (2016-10-20 22:45:46 UTC) #51

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 22:46:32 UTC) #52

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-20 23:20:13 UTC) #53

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/286446)

4 years, 2 months ago (2016-10-20 23:20:14 UTC) #54

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-21 20:40:11 UTC) #57

commit-bot: I haz the power

Description was changed from ========== Creates AXPosition to uniquely identify a position in the accessibility ...

4 years, 2 months ago (2016-10-21 22:02:49 UTC) #59

commit-bot: I haz the power

4 years, 2 months ago (2016-10-21 22:02:51 UTC) #60

Message was sent while issue was closed.

Patchset 26 (id:??) landed as
https://crrev.com/9de71d4067f24c4e333b5bef4f1210c9395ba847
Cr-Commit-Position: refs/heads/master@{#426906}

Issue 2271893002: Creates AXPosition to uniquely identify a position in the accessibility tree (Closed)

Description

Patch Set 1 #

Patch Set 2 : Re-wrote implementation to include tree_id and platform specific AXPosition. #

Patch Set 3 : Created a factory. #

Patch Set 4 : Switched to using templates. #

Patch Set 5 : Switched to using templates. #

Patch Set 6 : Added more overrides. #

Patch Set 7 : Added test skeleton. #

Patch Set 8 : Implemented AXNodePosition. #

Patch Set 9 : Added overrides for parent and childAtIndex for BrowserAccessibility. #

Patch Set 10 : Implemented curiously recurring template pattern. #

Patch Set 11 : Added comment explaining curiously recursive template and fixed base types. #

Patch Set 12 : Added static casts where needed. #

Patch Set 13 : Addressed some of the comments by David and Dominic. #

Patch Set 14 : Fixed compilation error. #

Patch Set 15 : Fixed compilation error. #

Patch Set 16 : Fixed includes on the Mac. #

Patch Set 17 : Added a number of unit tests. #

Patch Set 18 : Made some improvements to tests. #

Patch Set 19 : Fixed most tests except CommonAncestor. #

Patch Set 20 : Added a few more tests. #

Patch Set 21 : Fixed all memory leaks. #

Patch Set 22 : Removed AXRange class. #

Patch Set 23 : Removed AXRange from build. #

Patch Set 24 : Made platform positions treat platform leaves correctly. #

Patch Set 25 : Strengthened unit tests for next/previous character a bit. #

Patch Set 26 : Fixed some compilation errors on the Mac. #

Messages