The selected text range is not currently reported correctly for elements with role=textbox, as the relevant code assumes selection will only be reported for browser-based editable elements.
Created attachment 103332 [details] Patch
Comment on attachment 103332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103332&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567 > + I think for this method you could just return return ariaRoleAttribute() == TextFieldRole || ariaRoleAttribute() == TextAreaRole; > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3037 > + I think generally doing things like "cleanup" and renaming should not be mixed with behavioral changes. it makes it harder to review. The changes in the lines above this comment look to be only superficial, but it's hard to know,
Comment on attachment 103332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103332&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2567 >> + > > I think for this method you could just return > > return ariaRoleAttribute() == TextFieldRole || ariaRoleAttribute() == TextAreaRole; I don't think so, because the node in question is not necessarily the node relating to this render object (which is why this method is necessary). I should make this method static to make that clearer. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3037 >> + > > I think generally doing things like "cleanup" and renaming should not be mixed with behavioral changes. it makes it harder to review. The changes in the lines above this comment look to be only superficial, but it's hard to know, This is not actually cleanup: I'm factoring a method out so that I can call it from nodeIsTextControl() above. Since it returns an AccessibilityRole but takes a string, I thought it would be confusing to leave the parameter named "ariaRole".
> >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3037 > >> + > > > > I think generally doing things like "cleanup" and renaming should not be mixed with behavioral changes. it makes it harder to review. The changes in the lines above this comment look to be only superficial, but it's hard to know, > > This is not actually cleanup: I'm factoring a method out so that I can call it from nodeIsTextControl() above. Since it returns an AccessibilityRole but takes a string, I thought it would be confusing to leave the parameter named "ariaRole". Ah -- after a closer look I see how much state is used in the original determineAriaRoleAttribute() method, so I'll have to have another think about this. Will have a look at it tomorrow.
Created attachment 103421 [details] Patch
Comment on attachment 103421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103421&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2541 > + why does this method ask for the root editable element and then seemingly doing the same thing by iterating up the parent chain? can you add a comment why this is necessary. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2542 > + for (const Node* n = node; n != rootEditableElement; n = n->parentNode()) { seems like you might also want to check for n != NULL in case something does not have the correct hierarchy > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2546 > + break; if someone did (incorrectly) <div role="textbox"><div><div><body><div> would this fail? should you just > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2561 > + if (!axObjectForNode) is there a reason you don't want getOrCreate() here
Created attachment 103890 [details] Patch
Comment on attachment 103421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103421&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2541 >> + > > why does this method ask for the root editable element and then seemingly doing the same thing by iterating up the parent chain? can you add a comment why this is necessary. Ok, I've added a comment explaining that it's looking for the root editable or pseudo-editable element. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2542 >> + for (const Node* n = node; n != rootEditableElement; n = n->parentNode()) { > > seems like you might also want to check for n != NULL in case something does not have the correct hierarchy Good catch, thanks. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2546 >> + break; > > if someone did (incorrectly) > > <div role="textbox"><div><div><body><div> > > would this fail? > > should you just I think if someone did that the problems with rendering the page would extend far beyond correctly determining the relative cursor offset for an element; regardless, this is the same logic used in the Node::rootEditableElement() http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp#L1637) method, so it'll at least be consistent. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2561 >> + if (!axObjectForNode) > > is there a reason you don't want getOrCreate() here No, getOrCreate() is correct, thanks.
Comment on attachment 103890 [details] Patch r=me
Comment on attachment 103890 [details] Patch Rejecting attachment 103890 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: cessibilityRenderObject.cpp Hunk #1 FAILED at 2517. Hunk #2 succeeded at 2535 (offset 1 line). Hunk #3 succeeded at 3025 (offset -5 lines). Hunk #4 succeeded at 3049 (offset -5 lines). 1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityRenderObject.cpp.rej patching file Source/WebCore/accessibility/AccessibilityRenderObject.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Chris Fleizach', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/9404821
Comment on attachment 103890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103890&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2520 > - if (!indexPosition.anchorNode() || indexPosition.anchorNode()->rootEditableElement() != node) > + if (!indexPosition.anchorNode() || rootEditableElementForNode(indexPosition.anchorNode()) != node) Why are we calling anchorNode here? I know you didn't write this code but calling anchorNode is almost always wrong. See https://rniwa.com/2011-06-26/position-and-anchor-types/ > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2542 > + Element* rootEditableElement = node->rootEditableElement(); What if rootEditableElement was null here? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2546 > + if (nodeIsTextControl(n)) > + result = static_cast<const Element*>(n); Do we really need to check this case? The only way this can happen is if position's anchor node had RenderTextControl because parentNode never returns the shadow root.
Comment on attachment 103890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103890&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2546 >> + result = static_cast<const Element*>(n); > > Do we really need to check this case? The only way this can happen is if position's anchor node had RenderTextControl because parentNode never returns the shadow root. Also note that if the anchor node had a RenderTextControl, then the caret is outside of that text control so this seems wrong in that case as well.
Comment on attachment 103890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103890&action=review >>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2546 >> >> Do we really need to check this case? The only way this can happen is if position's anchor node had RenderTextControl because parentNode never returns the shadow root. > > Also note that if the anchor node had a RenderTextControl, then the caret is outside of that text control so this seems wrong in that case as well. Oh, never mind. IsTextControl apparently means a different thing in accessibility code. I still think we shouldn't be using anchorNode though.
Created attachment 104764 [details] Patch
Created attachment 104788 [details] Patch
Comment on attachment 104788 [details] Patch Chris, would you mind reviewing this again?
(In reply to comment #16) > (From update of attachment 104788 [details]) > Chris, would you mind reviewing this again? Alice do you have an answer for Ryosuke as to why we need to use anchorNode()? i didn't write this code either so i don't know
(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 104788 [details] [details]) > > Chris, would you mind reviewing this again? > > Alice do you have an answer for Ryosuke as to why we need to use anchorNode()? i didn't write this code either so i don't know Oh, thanks for the reminder; I was going to look into rewriting that section of the code and then forgot overnight. Will hopefully get a chance later today.
Created attachment 104966 [details] Patch
Comment on attachment 104966 [details] Patch Ryosuke, I modified the rootEditableElement method on AccessibilityRenderObject to take a Position rather than a Node, and used position.containerNode() to mimic the Position::rootEditableElement method. Is that ok?
Comment on attachment 104966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104966&action=review review- because of the missing null check. There are other smaller mistakes. > LayoutTests/accessibility/textbox-role-reports-selection-expected.txt:8 > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS It’s much better to have tests that log some data rather than just the word PASS. See the style of the shouldBe macros, which log what is being tested, not just "pass". Something to keep in mind when working on a new test. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2538 > +const Element* AccessibilityRenderObject::rootEditableElementForPosition(const Position& position) const The use of const here in const Element* doesn’t make much sense to me. Why is the element returned a const*? Does the caller not have permission to make changes to that element? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2545 > + for (const Node* n = position.containerNode(); n && n != rootEditableElement; n = n->parentNode()) { It would be better to write this loop using parentElement. That way we could achieve type safety without at typecast. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2563 > + const AccessibilityObject* axObjectForNode = axObjectCache()->getOrCreate(node->renderer()); What if node->renderer() is 0?
Created attachment 105282 [details] Patch
(In reply to comment #21) > (From update of attachment 104966 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104966&action=review > > review- because of the missing null check. There are other smaller mistakes. > > > LayoutTests/accessibility/textbox-role-reports-selection-expected.txt:8 > > +PASS > > +PASS > > +PASS > > +PASS > > +PASS > > +PASS > > It’s much better to have tests that log some data rather than just the word PASS. See the style of the shouldBe macros, which log what is being tested, not just "pass". Something to keep in mind when working on a new test. Thanks for the explanation. I've received conflicting advice on tests; will keep this in mind for future work. (It does log more information for the fail case, at least!) > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2538 > > +const Element* AccessibilityRenderObject::rootEditableElementForPosition(const Position& position) const > > The use of const here in const Element* doesn’t make much sense to me. Why is the element returned a const*? Does the caller not have permission to make changes to that element? I was sure I had a reason, but it looks like I didn't. Fixed. > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2545 > > + for (const Node* n = position.containerNode(); n && n != rootEditableElement; n = n->parentNode()) { > > It would be better to write this loop using parentElement. That way we could achieve type safety without at typecast. > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2563 > > + const AccessibilityObject* axObjectForNode = axObjectCache()->getOrCreate(node->renderer()); > > What if node->renderer() is 0? The first line of AXObjectCache::getOrCreate() does a null check.
Comment on attachment 105282 [details] Patch Can anyone take a look at this?
(In reply to comment #24) > (From update of attachment 105282 [details]) > Can anyone take a look at this? I think Darren's comments about the tests are the most important actually. The rest of the patch seems fine. I think your tests should follow the style of the other accessibility tests, which use the testing framework and the shouldBe... functions to get better output. Can you fix that? otherwise seems ok
Created attachment 105576 [details] Patch
Comment on attachment 105576 [details] Patch Ok, I rewrote the test to use a shouldBe style assertion. I couldn't use shouldBe directly as I wanted to include some extra information. I hope this is sufficient.
Comment on attachment 105576 [details] Patch Is this ok?
Comment on attachment 105576 [details] Patch r=me. thanks
Comment on attachment 105576 [details] Patch Clearing flags on attachment: 105576 Committed r94134: <http://trac.webkit.org/changeset/94134>
All reviewed patches have been landed. Closing bug.