Bug 65900

Summary:

Correctly report selected text range for accessibility APIs for role=textbox

Product:

WebKit

Reporter:

Alice Boxhall <aboxhall>

Component:

Accessibility

Assignee:

Alice Boxhall <aboxhall>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cfleizach, davidbarr, dmazzoni, mikelawther, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

66638

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Alice Boxhall

Reported 2011-08-08 23:16:28 PDT

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.

Attachments
Patch (9.89 KB, patch) 2011-08-08 23:22 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.12 KB, patch) 2011-08-09 17:11 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.23 KB, patch) 2011-08-14 18:58 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.26 KB, patch) 2011-08-22 16:42 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.27 KB, patch) 2011-08-22 21:58 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.38 KB, patch) 2011-08-23 23:15 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (8.19 KB, patch) 2011-08-25 17:31 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Patch (9.16 KB, patch) 2011-08-29 22:04 PDT, Alice Boxhall	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Alice Boxhall

Comment 1 2011-08-08 23:22:58 PDT

Created attachment 103332 [details] Patch

chris fleizach

Comment 2 2011-08-09 00:01:13 PDT

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,

Alice Boxhall

Comment 3 2011-08-09 02:07:16 PDT

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

Alice Boxhall

Comment 4 2011-08-09 02:19:11 PDT

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

Alice Boxhall

Comment 5 2011-08-09 17:11:04 PDT

Created attachment 103421 [details] Patch

chris fleizach

Comment 6 2011-08-10 08:39:20 PDT

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

Alice Boxhall

Comment 7 2011-08-14 18:58:14 PDT

Created attachment 103890 [details] Patch

Alice Boxhall

Comment 8 2011-08-14 18:59:27 PDT

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.

chris fleizach

Comment 9 2011-08-16 23:19:04 PDT

Comment on attachment 103890 [details] Patch r=me

WebKit Review Bot

Comment 10 2011-08-17 03:53:23 PDT

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

Ryosuke Niwa

Comment 11 2011-08-22 09:45:24 PDT

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.

Ryosuke Niwa

Comment 12 2011-08-22 09:48:06 PDT

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.

Ryosuke Niwa

Comment 13 2011-08-22 10:15:21 PDT

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.

Alice Boxhall

Comment 14 2011-08-22 16:42:13 PDT

Created attachment 104764 [details] Patch

Alice Boxhall

Comment 15 2011-08-22 21:58:15 PDT

Created attachment 104788 [details] Patch

Alice Boxhall

Comment 16 2011-08-23 16:08:25 PDT

Comment on attachment 104788 [details] Patch Chris, would you mind reviewing this again?

chris fleizach

Comment 17 2011-08-23 17:12:46 PDT

(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

Alice Boxhall

Comment 18 2011-08-23 17:18:35 PDT

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

Alice Boxhall

Comment 19 2011-08-23 23:15:17 PDT

Created attachment 104966 [details] Patch

Alice Boxhall

Comment 20 2011-08-23 23:17:07 PDT

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?

Darin Adler

Comment 21 2011-08-24 09:27:42 PDT

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?

Alice Boxhall

Comment 22 2011-08-25 17:31:55 PDT

Created attachment 105282 [details] Patch

Alice Boxhall

Comment 23 2011-08-25 17:34:03 PDT

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

Alice Boxhall

Comment 24 2011-08-29 16:38:43 PDT

Comment on attachment 105282 [details] Patch Can anyone take a look at this?

chris fleizach

Comment 25 2011-08-29 16:44:56 PDT

(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

Alice Boxhall

Comment 26 2011-08-29 22:04:33 PDT

Created attachment 105576 [details] Patch

Alice Boxhall

Comment 27 2011-08-29 22:06:25 PDT

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.

Alice Boxhall

Comment 28 2011-08-30 16:27:20 PDT

Comment on attachment 105576 [details] Patch Is this ok?

chris fleizach

Comment 29 2011-08-30 16:36:40 PDT

Comment on attachment 105576 [details] Patch r=me. thanks

WebKit Review Bot

Comment 30 2011-08-30 17:20:23 PDT

Comment on attachment 105576 [details] Patch Clearing flags on attachment: 105576 Committed r94134: <http://trac.webkit.org/changeset/94134>

WebKit Review Bot

Comment 31 2011-08-30 17:20:28 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.