Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339093003:
Support child-based node offsets when setting accessible selections (Closed)
|
Created:
4 years, 3 months ago by David Tseng Modified:
4 years, 3 months ago CC:
dmazzoni, aboxhall+watch_chromium.org, aboxhall, blink-reviews, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, haraken, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport child-based node offsets when setting accessible selections
https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k
RTnH4qt3fd-hXqfjvs/edit#
Committed: https://crrev.com/f1dfcce84beb802d628d217044a5ceb046d18f05
Cr-Commit-Position: refs/heads/master@{#419325}
Patch Set 1 #Patch Set 2 : Fix ChromeVox test. #Patch Set 3 : More tests. #
Total comments: 10
Patch Set 4 : Make test changes. #Patch Set 5 : Be more friendly. #Patch Set 6 : Fix 0-childCount child node index case. #
Total comments: 2
Patch Set 7 : Decide on 0 childCount. #Patch Set 8 : Correct the 0-child case. #
Messages
Total messages: 48 (33 generated)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix set selection. -UU-:**--F1 cl_descriptionjlIhxl All L2 (Fundamental) ------------------ C-c- ========== to ========== Implement child index offsets for AXLayoutObject::setSelection https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ==========
dtseng@chromium.org changed reviewers: + nektar@chromium.org
Still somewhat wip (pending tests). Have a look at the proposed impl for the setting-side of selection via child offsets.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I'll wait for the tests and then approve.
static VisiblePosition toVisiblePosition(AXObject* obj, int offset)
I know it was written as a static function, but does it really need to be
static? If it's just an instance method, we'll get rid of all the obj->this and
obj->that.
if (!isValidSelectionBound(anchorObject)
- || !isValidSelectionBound(focusObject)) {
I thought the C++ Style Guide didn't allow braces to be omitted when the
conditional or its body spans more than one line.
I read it again. It's not perfectly clear, but this is what it implies I think.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
dtseng@chromium.org changed reviewers: + aboxhall@chromium.org
PTAL. Added back curlies (re your comment) and added a test which shows child index selections at work (including being able to select whitespace present in the DOM). + aboxhall for OWNERS.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html (right): https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:19: var axGroup = accessibilityController.accessibleElementById("link").parentElement(); What element does this correspond to? Is it not span1? Is it an anonymous block? I'd like to get rid of anonymous blocks and make spans part of the accessibility tree. What happens if you wrap the spans in a div and use the div here? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:52: // A selection demonstrating ax can use child indicies to select whitespace only in the DOM (second child). What do you mean by whitespace only in the DOM? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:62: assert_equals(sel.endContainer.previousSibling.lastChild.lastChild, sel.startContainer); I'm worried that this is really fragile and will be a pain to maintain if we do any refactoring. Is there a more relaxed assertion you could make that asserts what you really care about? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1824: if (!node->isTextNode()) { It's possible to call AXObject::setSelection on an object that doesn't even have a node, right? How about if (!node || !node->isTextNode()) to handle that case too? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1832: static_cast<unsigned>(offset) > (obj->children().size() - 1) ? offset - 1 : offset; Clearer using std::min? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1856: return createVisiblePosition(Position::editingPositionOf(childNode->parentNode(), adjustedOffset)); Instead of computing the index in parent in the DOM tree, could you just call Position::beforeNode(childNode)?
https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html (right): https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:19: var axGroup = accessibilityController.accessibleElementById("link").parentElement(); On 2016/09/15 16:32:36, dmazzoni wrote: > What element does this correspond to? Is it not span1? Is it an anonymous block? > > I'd like to get rid of anonymous blocks and make spans part of the accessibility > tree. What happens if you wrap the spans in a div and use the div here? The idea was to use an example where the ax tree re-promoted children (the nested link) up the tree. I wanted an example that could cover many cases. Isn't it useful to have this and see it break once you make your change? https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:52: // A selection demonstrating ax can use child indicies to select whitespace only in the DOM (second child). On 2016/09/15 16:32:36, dmazzoni wrote: > What do you mean by whitespace only in the DOM? Were you expecting a line break (either a node or within ax name text) in the ax tree? There isn't one. https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:62: assert_equals(sel.endContainer.previousSibling.lastChild.lastChild, sel.startContainer); On 2016/09/15 16:32:36, dmazzoni wrote: > I'm worried that this is really fragile and will be a pain to > maintain if we do any refactoring. > > Is there a more relaxed assertion you could make that asserts > what you really care about? This is the DOM tree; what type of refactoring are you considering?
https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html (right): https://codereview.chromium.org/2339093003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/set-selection-child-offset.html:62: assert_equals(sel.endContainer.previousSibling.lastChild.lastChild, sel.startContainer); On 2016/09/15 at 18:34:28, David Tseng wrote: > On 2016/09/15 16:32:36, dmazzoni wrote: > > I'm worried that this is really fragile and will be a pain to > > maintain if we do any refactoring. > > > > Is there a more relaxed assertion you could make that asserts > > what you really care about? > > This is the DOM tree; what type of refactoring are you considering? How about this: add comments next to the two types of assertions distinguishing between (1) things that really shouldn't break, and (2) things that could be modified if a future refactoring breaks them without changing the spirit of the test
Changed axGroup to an axRegion, removed whitespace comment, and used compareDocumentPosition in place of explicit tree expectation.
Description was changed from ========== Implement child index offsets for AXLayoutObject::setSelection https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ========== to ========== Support child-based node offsets when setting accessible selections https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ==========
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2339093003/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2339093003/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1828: // Figure out what it means when |childCount| is 0. Is this a TODO/FIXME? Maybe it should just select this node? It seems silly to me that in order to select a node you have to call setSelection on its parent, why not just allow selecting a node itself and then if it has no children, the start and end offsets don't matter? You don't have to change that, but please update the comment to indicate what it does now and what you may want to do later
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2339093003/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2339093003/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1828: // Figure out what it means when |childCount| is 0. On 2016/09/16 18:10:56, dmazzoni wrote: > Is this a TODO/FIXME? > > Maybe it should just select this node? > > It seems silly to me that in order to select a node you have to call > setSelection on its parent, why not just allow selecting a node itself and then > if it has no children, the start and end offsets don't matter? > > You don't have to change that, but please update the comment to indicate what it > does now and what you may want to do later > > This isn't a selection but a position. It's not quite that simple. We have to go to the axobj's *parent* and use the obj's index in parent to point to the child. That still leaves us with deciding whether to point to *before* or *after* the obj. I picked before, but have no strong preference. In general though, it breaks the contract with the client regarding non-text nodes having child node offsets. I uploaded the change, but we might want to reconsider at some point if it causes confusion/problems. For example, I could do: setDocumentSelection({anchorObject: divWithNoChildren, focusObject: divWithNoChildren, anchorOffset: 1000000, focusOffset: 2000000000}); and it would select the div by setting anchor/focus to divWithNoChildren.parent and offsets to divWithNoChildren.indexInParent (for both anchor and focus). So, that's an insertion point which makes as much sense as selecting the whole object I guess.
On Fri, Sep 16, 2016 at 1:50 PM <dtseng@chromium.org> wrote: > This isn't a selection but a position. > It could be an additional argument to toVisiblePosition, though, since it's called on either the focus or the anchor of the selection - basically a hint to err on the before side or the after side. OK to leave for later. In general though, it breaks the contract with the client regarding > non-text nodes having child node offsets. I uploaded the change, but we > might want to reconsider at some point if it causes confusion/problems. > For example, I could do: > setDocumentSelection({anchorObject: divWithNoChildren, focusObject: > divWithNoChildren, anchorOffset: 1000000, focusOffset: 2000000000}); > > and it would select the div by setting anchor/focus to > divWithNoChildren.parent and offsets to divWithNoChildren.indexInParent > (for both anchor and focus). So, that's an insertion point which makes > as much sense as selecting the whole object I guess. > I guess my inclination is that selecting the whole object is preferable. If the goal of selecting is to copy text, it's better to get a bit more than to get nothing. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Fri, Sep 16, 2016 at 1:50 PM <dtseng@chromium.org> wrote: > This isn't a selection but a position. > It could be an additional argument to toVisiblePosition, though, since it's called on either the focus or the anchor of the selection - basically a hint to err on the before side or the after side. OK to leave for later. In general though, it breaks the contract with the client regarding > non-text nodes having child node offsets. I uploaded the change, but we > might want to reconsider at some point if it causes confusion/problems. > For example, I could do: > setDocumentSelection({anchorObject: divWithNoChildren, focusObject: > divWithNoChildren, anchorOffset: 1000000, focusOffset: 2000000000}); > > and it would select the div by setting anchor/focus to > divWithNoChildren.parent and offsets to divWithNoChildren.indexInParent > (for both anchor and focus). So, that's an insertion point which makes > as much sense as selecting the whole object I guess. > I guess my inclination is that selecting the whole object is preferable. If the goal of selecting is to copy text, it's better to get a bit more than to get nothing. -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Let's hash it out on the doc. The position is a tree node offset not a text offset so In the 0 child count case, that still leaves bad child offsets as a misunderstanding of the api by the caller. Furthermore, I would like to stay as in sync with how Range works as possible to ensure we work with popular usage patterns by complex webapps.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2339093003/#ps150001 (title: "Correct the 0-child case.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support child-based node offsets when setting accessible selections https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ========== to ========== Support child-based node offsets when setting accessible selections https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ==========
Message was sent while issue was closed.
Committed patchset #8 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Support child-based node offsets when setting accessible selections https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# ========== to ========== Support child-based node offsets when setting accessible selections https://docs.google.com/document/d/1IZ7HtM0BZlMgNyYo6AUHGL84k RTnH4qt3fd-hXqfjvs/edit# Committed: https://crrev.com/f1dfcce84beb802d628d217044a5ceb046d18f05 Cr-Commit-Position: refs/heads/master@{#419325} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f1dfcce84beb802d628d217044a5ceb046d18f05 Cr-Commit-Position: refs/heads/master@{#419325} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
