Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1185343003:
Implements the ability to get and set the caret position and the current selection from anywhere in… (Closed)
DescriptionImplements the ability to get and set the caret position and the current selection from anywhere in the accessibility tree.
This is required in order to support contenteditables on some platforms.
BUG=491027
Committed: https://crrev.com/e97cb85ced628844abb138653971aee2eb1c5c99
git-svn-id: svn://svn.chromium.org/blink/trunk@199415 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #Patch Set 3 : Fixed some compiler errors. #
Total comments: 8
Patch Set 4 : Addressed more comments from reviewer. #
Total comments: 5
Patch Set 5 : Addressed more comments from reviewer. #Patch Set 6 : Fixed retrieving the selection in text controls. #
Total comments: 6
Patch Set 7 : Adopted final round of recommendations and improved fixed layout test #Patch Set 8 : Moved layout tests to another CL. #Depends on Patchset: Messages
Total messages: 24 (5 generated)
domenic@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... File Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.cpp:1842: AXSelection textSelection = textControlSelection(); This only seems to work if *this* is a text control. Shouldn't it try to handle the case where you call selection() on the root of the tree but a text control has focus? https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.cpp:1856: AXObject* anchorObject = axObjectCache()->get(anchorNode); I think you need to handle the case where this object is ignored. As an example, if the selection start is inside an object that has aria-hidden=true, I think we should walk up to the parent of that object and consider the parent to be the start of the selection. https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.cpp:1877: AXObject::AXSelection AXLayoutObject::selectionUnderObject() const Can you explain what this does? https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.cpp:1970: if (!anchorNode || !focusNode) Some objects in the tree have a layoutObject but not a node - you should maybe walk up the parents until you find an ancestor with a node. One example is an anonymous block, another example is CSS-generated text. https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.cpp:1979: frame->selection().setSelection(VisibleSelection(Position(anchorNode, I think you need to check that both the anchorNode and focusNode are part of the same Document or maybe the same TreeScope, since Blink doesn't let a selection span frames. https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... File Source/modules/accessibility/AXLayoutObject.h (right): https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.h:185: virtual AXSelection selection() const override; These should all be in their own section with comments indicating that they apply to the whole document, or the subtree, etc. https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXLayoutObject.h:226: AXObject::AXSelection textControlSelection() const; The AXObject:: qualifier shouldn't be needed here https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... File Source/modules/accessibility/AXObject.h (right): https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... Source/modules/accessibility/AXObject.h:751: virtual void setSelection(const AXSelection&) { } Why is setSelection implemented here but not exposed in WebAXObject? https://codereview.chromium.org/1185343003/diff/1/Source/web/WebAXObject.cpp File Source/web/WebAXObject.cpp (right): https://codereview.chromium.org/1185343003/diff/1/Source/web/WebAXObject.cpp#... Source/web/WebAXObject.cpp:1537: Vector<AXObject::AXSelection> wordBoundaries; Switching this to use AXSelection doesn't make sense to me, because anchorNode and focusNode are just ignored. Also, word boundaries are not selections and they don't have "anchor" and "focus", they just have a start and end. Trying to reuse the same data structure here just seems confusing. https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h File public/web/WebAXObject.h (right): https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h#ne... public/web/WebAXObject.h:160: BLINK_EXPORT void selection(unsigned* anchorID, unsigned* anchorOffset, unsigned* focusId, unsigned* focusOffset) const; Put this in its own block with a comment like "Can be called on any object, applies to the whole document. Later maybe we should have a separate AX interface for a document." To be consistent with the rest of the interface, return WebAXObjects for the anchor and focus, not IDs. To be consistent with Blink style, use references, not pointers.
On 6/16/2015 1:24 PM, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > File Source/modules/accessibility/AXLayoutObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.cpp:1842: AXSelection > textSelection = textControlSelection(); > This only seems to work if *this* is a text control. Shouldn't it try to > handle the case where you call selection() on the root of the tree but a > text control has focus? > Yeah, I thought of that but you see there might be multiple text boxes each one with its own selection and then which one would I return? Done. I made it return the selection of the one that has the focus, if any. > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.cpp:1856: AXObject* > anchorObject = axObjectCache()->get(anchorNode); > I think you need to handle the case where this object is ignored. > > As an example, if the selection start is inside an object that has > aria-hidden=true, I think we should walk up to the parent of that object > and consider the parent to be the start of the selection. > Wouldn't the anchorOffset be wrong in that case? Isn't it better to simply ignore such selections? > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.cpp:1877: > AXObject::AXSelection AXLayoutObject::selectionUnderObject() const > Can you explain what this does? > Yeah, I'll add a comment. It returns a simple selection where the offsets are calculated from the current object only. > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.cpp:1970: if (!anchorNode || > !focusNode) > Some objects in the tree have a layoutObject but not a node - you should > maybe walk up the parents until you find an ancestor with a node. > > One example is an anonymous block, another example is CSS-generated > text. > OK. Done. > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.cpp:1979: > frame->selection().setSelection(VisibleSelection(Position(anchorNode, > I think you need to check that both the anchorNode and focusNode are > part of the same Document or maybe the same TreeScope, since Blink > doesn't let a selection span frames. > I checked that the objects are in the AX cache. Is that sufficient? > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > File Source/modules/accessibility/AXLayoutObject.h (right): > > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.h:185: virtual AXSelection > selection() const override; > These should all be in their own section with comments indicating that > they apply to the whole document, or the subtree, etc. > Done in the base class and comments added. > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXLayoutObject.h:226: AXObject::AXSelection > textControlSelection() const; > The AXObject:: qualifier shouldn't be needed here > Done. > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > File Source/modules/accessibility/AXObject.h (right): > > https://codereview.chromium.org/1185343003/diff/1/Source/modules/accessibilit... > > Source/modules/accessibility/AXObject.h:751: virtual void > setSelection(const AXSelection&) { } > Why is setSelection implemented here but not exposed in WebAXObject? > Done. > https://codereview.chromium.org/1185343003/diff/1/Source/web/WebAXObject.cpp > > File Source/web/WebAXObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/1/Source/web/WebAXObject.cpp#... > > Source/web/WebAXObject.cpp:1537: Vector<AXObject::AXSelection> > wordBoundaries; > Switching this to use AXSelection doesn't make sense to me, because > anchorNode and focusNode are just ignored. Also, word boundaries are not > selections and they don't have "anchor" and "focus", they just have a > start and end. Trying to reuse the same data structure here just seems > confusing. > Yes, AXSelection should be renamed to AXRange. I want to have anchorObject and focusObject because what if in a contenteditable has a link in the middle of a word? Wouldn't that make simple start/end offsets not sufficient because they will be in different objects? https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h > File public/web/WebAXObject.h (right): > > https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h#ne... > > public/web/WebAXObject.h:160: BLINK_EXPORT void selection(unsigned* > anchorID, unsigned* anchorOffset, unsigned* focusId, unsigned* > focusOffset) const; > Put this in its own block with a comment like "Can be called on any > object, applies to the whole document. Later maybe we should have a > separate AX interface for a document." > > To be consistent with the rest of the interface, return WebAXObjects for > the anchor and focus, not IDs. > > To be consistent with Blink style, use references, not pointers. > Done. > https://codereview.chromium.org/1185343003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/06/22 14:48:32, blink-reviews wrote: > > As an example, if the selection start is inside an object that has > > aria-hidden=true, I think we should walk up to the parent of that object > > and consider the parent to be the start of the selection. > > > Wouldn't the anchorOffset be wrong in that case? Isn't it better to > simply ignore such selections? We need to update the anchorOffset when walking up the tree, of course. Why would you ignore the entire selection just because part of it - possibly even just a trivially small part - isn't in the accessibility tree? I see it as just rounding the selection to the nearest nodes in the accessibility tree so it makes sense. > > Source/modules/accessibility/AXLayoutObject.cpp:1979: > > frame->selection().setSelection(VisibleSelection(Position(anchorNode, > > I think you need to check that both the anchorNode and focusNode are > > part of the same Document or maybe the same TreeScope, since Blink > > doesn't let a selection span frames. > > > I checked that the objects are in the AX cache. Is that sufficient? No, because the AX cache spans all frames on the page, but a selection can only be within a single document. > > Source/web/WebAXObject.cpp:1537: Vector<AXObject::AXSelection> > > wordBoundaries; > > Switching this to use AXSelection doesn't make sense to me, because > > anchorNode and focusNode are just ignored. Also, word boundaries are not > > selections and they don't have "anchor" and "focus", they just have a > > start and end. Trying to reuse the same data structure here just seems > > confusing. > > > Yes, AXSelection should be renamed to AXRange. > I want to have anchorObject and focusObject because what if in a > contenteditable has a link in the middle of a word? Wouldn't that make > simple start/end offsets not sufficient because they will be in > different objects? But this API is for words within an inline text box, not for words within a contenteditable. If you want to later add an API for walking word boundaries in rich text, go ahead, but I don't see the need to change this API. > https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h > > File public/web/WebAXObject.h (right): > > > > > https://codereview.chromium.org/1185343003/diff/1/public/web/WebAXObject.h#ne... > > > > > public/web/WebAXObject.h:160: BLINK_EXPORT void selection(unsigned* > > anchorID, unsigned* anchorOffset, unsigned* focusId, unsigned* > > focusOffset) const; > > Put this in its own block with a comment like "Can be called on any > > object, applies to the whole document. Later maybe we should have a > > separate AX interface for a document." > > > > To be consistent with the rest of the interface, return WebAXObjects for > > the anchor and focus, not IDs. > > > > To be consistent with Blink style, use references, not pointers. > > > Done. > > > https://codereview.chromium.org/1185343003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... File Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1846: // Find the closest parent that has a node. I don't think you need to find a parent with a node here. If layoutObject() is null, exit early - and otherwise just get visibleSelection(). https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1867: if (!anchorObject || !focusObject) Here's where you need to walk up the tree. anchorNode and focusNode might both be reasonable but might not have AX objects for some reason. Walk up until you find one that does. https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1895: || !selectionRange->intersectsNode(node(), IGNORE_EXCEPTION)) It looks like selectionUnderObject returns a selection that intersects this node - but that might be one that starts before this node and ends after this node. Is that what you want? https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1925: VisibleSelection AXLayoutObject::visibleSelection() const I'd just delete this helper, because any function that calls it should already early-out if layoutObject() is null, and it doesn't do anything other than return layoutObject()->frame()->selection().selection(). https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1964: && axObjectCache()->isIDinUse(selection.anchorObject->axObjectID())) Rather than isIDinUse, just check selection.anchorObject->isDetached to see if it's valid. Maybe also check that selection.anchorObject->axObjectCache() == axObjectCache(), and same for focusObject. Checking isIDinUse could be subtly wrong if you're passed an AXObject from a totally different cache that has its own ID mapping! https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1981: Node* anchorNode = anchorObject->node(); Need to walk up the tree here - someone might want to select a range where the start or end of the range is a valid object in the tree that doesn't have a node. https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... File Source/modules/accessibility/AXLayoutObject.h (right): https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.h:185: virtual AXRange selection() const override; Put this in its own section, not grouped next to these methods that aren't related. https://codereview.chromium.org/1185343003/diff/40001/public/web/WebAXObject.h File public/web/WebAXObject.h (right): https://codereview.chromium.org/1185343003/diff/40001/public/web/WebAXObject.... public/web/WebAXObject.h:158: // The following selection functions get or set the global document Put these in their own section in the file
On 6/22/2015 12:58 PM, dmazzoni@chromium.org wrote: > On 2015/06/22 14:48:32, blink-reviews wrote: >> > As an example, if the selection start is inside an object that has >> > aria-hidden=true, I think we should walk up to the parent of that >> object >> > and consider the parent to be the start of the selection. >> > >> Wouldn't the anchorOffset be wrong in that case? Isn't it better to >> simply ignore such selections? > > We need to update the anchorOffset when walking up the tree, of course. > > Why would you ignore the entire selection just because part of it - > possibly > even just a trivially small part - isn't in the accessibility tree? I > see it as > just rounding the selection to the nearest nodes in the accessibility > tree so it > makes sense. > Done via accessibilityIsIgnored which also checkeds for aria-hidden. But I didn't update the offsets because if something is ignored it shouldn't affect the offset because the ignored object will provide no text. I don't know how to implement this in a more safe manner. >> > Source/modules/accessibility/AXLayoutObject.cpp:1979: >> > frame->selection().setSelection(VisibleSelection(Position(anchorNode, >> > I think you need to check that both the anchorNode and focusNode are >> > part of the same Document or maybe the same TreeScope, since Blink >> > doesn't let a selection span frames. >> > >> I checked that the objects are in the AX cache. Is that sufficient? > > No, because the AX cache spans all frames on the page, but a selection > can only > be within a single document. > Fixed. I now compare the frame pointers. >> > Source/web/WebAXObject.cpp:1537: Vector<AXObject::AXSelection> >> > wordBoundaries; >> > Switching this to use AXSelection doesn't make sense to me, because >> > anchorNode and focusNode are just ignored. Also, word boundaries >> are not >> > selections and they don't have "anchor" and "focus", they just have a >> > start and end. Trying to reuse the same data structure here just seems >> > confusing. >> > >> Yes, AXSelection should be renamed to AXRange. >> I want to have anchorObject and focusObject because what if in a >> contenteditable has a link in the middle of a word? Wouldn't that make >> simple start/end offsets not sufficient because they will be in >> different objects? > > But this API is for words within an inline text box, not for words > within a > contenteditable. If you want to later add an API for walking word > boundaries in > rich text, go ahead, but I don't see the need to change this API. I am not changing the API only the Blink internal structure for returning text ranges. I don't think we should have two structs for representing a range. I added isSimpleRange to the AXRange struct to distinguish between the two uses if needed, but I would not like to have both AXRange and PlainTextRange. The public Blink API has remained the same. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > File Source/modules/accessibility/AXLayoutObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1846: // Find the > closest parent that has a node. > I don't think you need to find a parent with a node here. If > layoutObject() is null, exit early - and otherwise just get > visibleSelection(). > Removed. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1867: if (!anchorObject > || !focusObject) > Here's where you need to walk up the tree. anchorNode and focusNode > might both be reasonable but might not have AX objects for some reason. > Walk up until you find one that does. > Done. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1895: || > !selectionRange->intersectsNode(node(), IGNORE_EXCEPTION)) > It looks like selectionUnderObject returns a selection that intersects > this node - but that might be one that starts before this node and ends > after this node. Is that what you want? > Sorry wrong function. Switched to Range::compareNode should be NODE_BEFORE_AND_AFTER. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1925: VisibleSelection > AXLayoutObject::visibleSelection() const > I'd just delete this helper, because any function that calls it should > already early-out if layoutObject() is null, and it doesn't do anything > other than return layoutObject()->frame()->selection().selection(). > I agree. Done. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1964: && > axObjectCache()->isIDinUse(selection.anchorObject->axObjectID())) > Rather than isIDinUse, just check selection.anchorObject->isDetached to > see if it's valid. > Done. > Maybe also check that selection.anchorObject->axObjectCache() == > axObjectCache(), and same for focusObject. > Done. > Checking isIDinUse could be subtly wrong if you're passed an AXObject > from a totally different cache that has its own ID mapping! > Scary. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1981: Node* anchorNode = > anchorObject->node(); > Need to walk up the tree here - someone might want to select a range > where the start or end of the range is a valid object in the tree that > doesn't have a node. > Done. > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > File Source/modules/accessibility/AXLayoutObject.h (right): > > https://codereview.chromium.org/1185343003/diff/40001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.h:185: virtual AXRange > selection() const override; > Put this in its own section, not grouped next to these methods that > aren't related. > Done. > https://codereview.chromium.org/1185343003/diff/40001/public/web/WebAXObject.h > > File public/web/WebAXObject.h (right): > > https://codereview.chromium.org/1185343003/diff/40001/public/web/WebAXObject.... > > public/web/WebAXObject.h:158: // The following selection functions get > or set the global document > Put these in their own section in the file > They are in a their own section already aren't they? I didn't move them anywhere else because functions appear to be in alphabetical order. > https://codereview.chromium.org/1185343003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
> > > public/web/WebAXObject.h:158: // The following selection functions get > > or set the global document > > Put these in their own section in the file > > > They are in a their own section already aren't they? I didn't move them > anywhere else because functions appear to be in alphabetical order. There are a few other sections of grouped functions, then a big alphabetical list of functions without any other comments. I'm asking you to put these in their own section so that it's more clear that the comments apply to those four but not the others. A comment that applies to only the following line in an alphabetical list is fine. A comment that applies to the following four lines in an alphabetical list is potentially ambiguous so we shouldn't do that. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Getting closer! https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... File Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1859: while (anchorObject = axObjectCache()->get(anchorNode) This loop doesn't seem right because if axObjectCache()->get(anchorNode) returns null, you'll exit out of the loop immediately rather than walking the tree. Also, every place you have axObjectCache()->get(), it should be axObjectCache()->getOrCreate(), because AX objects are created lazily and you want to create it if it exists. An example of a time when we call get() is when firing a change notification - there's no point in firing a notification on an object that hasn't been created yet. https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... Source/modules/accessibility/AXLayoutObject.cpp:1861: || anchorObject->accessibilityIsIgnored())) { Blink has no 80 column line limit - you should wrap sometimes for readability but in this particular case I think it'd be more readable to put the three OR'd contitions inside parens on one line. https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... File Source/modules/accessibility/AXObject.h (right): https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... Source/modules/accessibility/AXObject.h:410: return anchorObject == focusObject || !anchorObject || !focusObject; I don't think this should return null if just one of anchorObject or focusObject is nullptr, only if both of them are. https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.cpp File Source/web/WebAXObject.cpp (right): https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.... Source/web/WebAXObject.cpp:855: AXObject::AXRange axSelection = m_private->selectionUnderObject(); It seems like this changes the behavior. Previously, selectionEnd only returned something for an input or textarea. Now, it will return the selection whenever there's a document selection or contenteditable selection within the subtree rooted at this element. That's going to expose the selection start and end in the accessibility tree and may cause strange behavior. It's not entirely clear to me if we really need selectionUnderObject the way you implemented it. Perhaps what's actually needed is something like plainTextSelectionRange() which returns the start and end offset of the selection for an editable text element - that could include a contentEditable where it collapses all of the text. https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.... Source/web/WebAXObject.cpp:1531: wordStartOffsets[i] = wordBoundaries[i].anchorOffset; How about: ASSERT(wordBoundaries[i].isSimple());
On 6/23/2015 8:11 PM, dmazzoni@chromium.org wrote: > Getting closer! > > > https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... > > File Source/modules/accessibility/AXLayoutObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1859: while > (anchorObject = axObjectCache()->get(anchorNode) > This loop doesn't seem right because if axObjectCache()->get(anchorNode) > returns null, you'll exit out of the loop immediately rather than > walking the tree. > Fixed. Nice catch. > Also, every place you have axObjectCache()->get(), it should be > axObjectCache()->getOrCreate(), because AX objects are created lazily > and you want to create it if it exists. An example of a time when we > call get() is when firing a change notification - there's no point in > firing a notification on an object that hasn't been created yet. > Done. > https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... > > Source/modules/accessibility/AXLayoutObject.cpp:1861: || > anchorObject->accessibilityIsIgnored())) { > Blink has no 80 column line limit - you should wrap sometimes for > readability but in this particular case I think it'd be more readable to > put the three OR'd contitions inside parens on one line. > Done. Around 113 chars. I hope that's not too long. > https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... > > File Source/modules/accessibility/AXObject.h (right): > > https://codereview.chromium.org/1185343003/diff/60001/Source/modules/accessib... > > Source/modules/accessibility/AXObject.h:410: return anchorObject == > focusObject || !anchorObject || !focusObject; > I don't think this should return null if just one of anchorObject or > focusObject is nullptr, only if both of them are. > I am not sure. If one of them is nullptr and the other one is not shouldn't that be an invalid range? I kept isSimple as is and modified isNull to make this clear. > https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.cpp > > File Source/web/WebAXObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.... > > Source/web/WebAXObject.cpp:855: AXObject::AXRange axSelection = > m_private->selectionUnderObject(); > It seems like this changes the behavior. > > Previously, selectionEnd only returned something for an input or > textarea. > > Now, it will return the selection whenever there's a document selection > or contenteditable selection within the subtree rooted at this element. > That's going to expose the selection start and end in the accessibility > tree and may cause strange behavior. > > It's not entirely clear to me if we really need selectionUnderObject the > way you implemented it. Perhaps what's actually needed is something like > plainTextSelectionRange() which returns the start and end offset of the > selection for an editable text element - that could include a > contentEditable where it collapses all of the text. > I think we would need the offsets for the collapsed text for content editable on the Mac, but I am not sure about it yet. However, it's not going to add any new selection start/end offsets in the tree because on the Chromium side we do: if (!src.isReadOnly() || dst->role == ui::AX_ROLE_TEXT_FIELD) { } I think that this covers only text fields and content editables. I don't think that Blink should be the one that decides for what types of objects it should return the selection under object. The way that I have it now Chromium decides for what objects to call what function. Also, I don't want to limit selectionUnderObject() to text fields and content editables, because I think there are cases where you can designate a part of the webpage to have a selection without it being editable. We might need to handle such cases in the future and this design would at least make it possible. > https://codereview.chromium.org/1185343003/diff/60001/Source/web/WebAXObject.... > > Source/web/WebAXObject.cpp:1531: wordStartOffsets[i] = > wordBoundaries[i].anchorOffset; > How about: ASSERT(wordBoundaries[i].isSimple()); > Done. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 6/23/2015 7:55 PM, Dominic Mazzoni wrote: > > > public/web/WebAXObject.h:158: // The following selection > functions get > > or set the global document > > Put these in their own section in the file > > > They are in a their own section already aren't they? I didn't move > them > anywhere else because functions appear to be in alphabetical order. > > > There are a few other sections of grouped functions, then a big > alphabetical list of functions without any other comments. > > I'm asking you to put these in their own section so that it's more > clear that the comments apply to those four but not the others. A > comment that applies to only the following line in an alphabetical > list is fine. A comment that applies to the following four lines in an > alphabetical list is potentially ambiguous so we shouldn't do that. > Oh sorry, didn't see the grouped functions. Done. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/06/24 19:11:05, blink-reviews wrote:
> > Source/modules/accessibility/AXObject.h:410: return anchorObject ==
> > focusObject || !anchorObject || !focusObject;
> > I don't think this should return null if just one of anchorObject or
> > focusObject is nullptr, only if both of them are.
> >
> I am not sure. If one of them is nullptr and the other one is not
> shouldn't that be an invalid range?
> I kept isSimple as is and modified isNull to make this clear.
OK, that works for me.
> However, it's not going to add any new selection start/end offsets in
> the tree because on the Chromium side we do:
> if (!src.isReadOnly() || dst->role == ui::AX_ROLE_TEXT_FIELD) {
OK, in that case this seems safe. I agree with the general direction,
I just didn't want to have temporary side effects while this code is
still under development.
> Also, I don't want to limit selectionUnderObject() to text fields and
> content editables, because I think there are cases where you can
> designate a part of the webpage to have a selection without it being
> editable. We might need to handle such cases in the future and this
> design would at least make it possible.
Yes, it'd be great if we could support caret browsing using this too!
dmazzoni@chromium.org changed reviewers: + mkwst@chromium.org
lgtm +mkwst for Source/web and public/web I only have some style suggestions left and one corner case, so lgtm and feel free to commit when ready. Be sure to run try jobs that include Chromium tests (not just Blink tests), like win_chromium_rel_ng and mac_chromium_rel_ng. If you have more questions or want another look after making changes, ask Alice to continue the review next week. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... File Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXLayoutObject.cpp:1863: if (anchorNode->nextSibling()) I think you should update anchorOffset here. To take a completely contrived but simple example, suppose you have <div aria-hidden="true">Hello World</div> and the word "World" is selected, so anchorOffset is 6. As you go to nextSibling or parentNode from here, I think you should reset anchorOffset to 0. Anyway, these are corner cases and I'm okay with deferring them for a follow-up change that adds more unit tests for those types of cases. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXLayoutObject.cpp:1981: if (selection.anchorObject Nit: this could be a helper function like IsValidSelectionBound(AXObject* obj) that you could call on anchorObject and focusObject. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXLayoutObject.cpp:1995: AXObject* anchorObject = selection.anchorObject ? You could maybe combine this too - have the helper return the valid bound or |this|. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXLayoutObject.cpp:2003: layoutObject())->textFormControlElement(); When the indentation is 4 spaces, I think the line continuation should be 8 spaces. I think it should already be part of a presubmit check, but if not try running Tools/Scripts/check-webkit-style and fix any issues it raises. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXLayoutObject.cpp:2009: Node* anchorNode = nullptr; Possibly a helper here, like nearestNodeFromAXObject? Note that if a helper doesn't need to be a member function, i.e. if it doesn't need |this|, then just declare it as a static function immediately above this one. You'll see several other similar examples in this source file, a static non-member helper function. https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... File Source/modules/accessibility/AXObject.h (right): https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... Source/modules/accessibility/AXObject.h:406: bool isNull() const Maybe this should be isValid? isNull is usually taken to be much more literal, like for a wrapper object like WebAXObject that has only one member variable and it's a pointer that can be null. So not a big deal but isValid might be more clear since it checks more than just null.
LGTM, once you're convinced that Dominic/Alice will be happy. :)
Most recommendations adopted, but still writing extensive tests which will be moved to another CL. I'll fully test with Jaws and NVDA without the Chromium CL and if everything works fine, including browser tests, I'll submit. Removing the Chromium CL will ensure that the old interface still works with no bugs. On 6/26/2015 2:14 AM, dmazzoni@chromium.org wrote: > lgtm > > +mkwst for Source/web and public/web > > I only have some style suggestions left and one corner case, so lgtm > and feel > free to commit when ready. > > Be sure to run try jobs that include Chromium tests (not just Blink > tests), like > win_chromium_rel_ng and mac_chromium_rel_ng. > > If you have more questions or want another look after making changes, > ask Alice > to continue the review next week. > > > > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > File Source/modules/accessibility/AXLayoutObject.cpp (right): > > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXLayoutObject.cpp:1863: if > (anchorNode->nextSibling()) > I think you should update anchorOffset here. > > To take a completely contrived but simple example, suppose you have <div > aria-hidden="true">Hello World</div> and the word "World" is selected, > so anchorOffset is 6. As you go to nextSibling or parentNode from here, > I think you should reset anchorOffset to 0. > > Anyway, these are corner cases and I'm okay with deferring them for a > follow-up change that adds more unit tests for those types of cases. > Done. > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXLayoutObject.cpp:1981: if > (selection.anchorObject > Nit: this could be a helper function like > IsValidSelectionBound(AXObject* obj) that you could call on anchorObject > and focusObject. > Done. > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXLayoutObject.cpp:1995: AXObject* > anchorObject = selection.anchorObject ? > You could maybe combine this too - have the helper return the valid > bound or |this|. > I don't like to do this because then the helper would do two things not one. It might become less reusable. > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXLayoutObject.cpp:2003: > layoutObject())->textFormControlElement(); > When the indentation is 4 spaces, I think the line continuation should > be 8 spaces. I think it should already be part of a presubmit check, but > if not try running Tools/Scripts/check-webkit-style and fix any issues > it raises. > I'll do that just before submitting. > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXLayoutObject.cpp:2009: Node* anchorNode = > nullptr; > Possibly a helper here, like nearestNodeFromAXObject? > > Note that if a helper doesn't need to be a member function, i.e. if it > doesn't need |this|, then just declare it as a static function > immediately above this one. You'll see several other similar examples in > this source file, a static non-member helper function. > Not the same code in both cases. I don't think I could do that. > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > File Source/modules/accessibility/AXObject.h (right): > > https://codereview.chromium.org/1185343003/diff/100001/Source/modules/accessi... > > Source/modules/accessibility/AXObject.h:406: bool isNull() const > Maybe this should be isValid? > Done. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Tested with NVDA and Jaws. Tested with textarea and <input type="text"> and <input type="password">. Tested cursor navigation, selection, editing and deleting.
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1185343003/#ps140001 (title: "Moved layout tests to another CL.")
The CQ bit was checked by nektar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185343003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1185343003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199415
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e97cb85ced628844abb138653971aee2eb1c5c99 |
