Implement the nsIAccessibleText interface for HTML text elements. The nsTextAccessible class will inherit from this interface.

Jessie will be in charge of this bug.

need r=.

nsTextAccessible::AddSelection(PRInt32 aSelectionNum, PRInt32 aStartOffset, PRInt32 aEndOffset) Why have the parameter aSelectionNum here? It is not needed or used. Use nsITextContent->GetText(const nsTextFragment**) where possible. It doesn't need to copy the string. For example: +NS_IMETHODIMP nsTextAccessible::GetCharacterAtOffset(PRInt32 aOffset, PRUnichar *_retval) +{ + nsAutoString text, subtext; + mDOMNode->GetNodeValue(text); + subtext = Substring(text, aOffset, 1); + *_retval = subtext[0]; + return NS_OK; +} I think you should do something like: nsCOMPtr<nsITextContent> textContent(do_QueryInterface(mDOMNode)); NS_ENSURE.... const nsTextFragment textFrag; textContent->GetText(&textFrag); *_retval = textFrag.CharAt(aOffset); return NS_OK; } You can also improve some of your other methods in the same way. I realize that you are not the person who originally put nsTextAccessible in nsFormControlAccessible.cpp. However, it seems like it should be in a new file nsTextAccessible.cpp, because we have nsXULTextAccessible and nsHTMLTextAccessible. - Aaron

1) remove PRInt32 aSelectionNum from AddSelection method; 2) create two new files: nsTextAccessible.cpp and nsTextAccessible.h; 3) use nsTextFragment to avoid extra string copy work; 4) add new data member selectionCount;

Thank you for the changes. I have delayed reviewing the GetTextHelper() method for now, because I'm not quite sure I understand how it works. Could you add some comments to it?

Aaron, please forgive me for my bad habit :( There is the commented version of GetTextHelper(). I'll add comment for other code as much as I can. It will come with next patch. /* Gets the specified text. aBoundaryType means: ATK_TEXT_BOUNDARY_CHAR The character before/at/after the offset is returned. ATK_TEXT_BOUNDARY_WORD_START The returned string is from the word start before/at/after the offset to the next word start. ATK_TEXT_BOUNDARY_WORD_END The returned string is from the word end before/at/after the offset to the next work end. ATK_TEXT_BOUNDARY_SENTENCE_START The returned string is from the sentence start before/at/after the offset to the next sentence start. ATK_TEXT_BOUNDARY_SENTENCE_END The returned string is from the sentence end before/at/after the offset to the next sentence end. ATK_TEXT_BOUNDARY_LINE_START The returned string is from the line start before/at/after the offset to the next line start. ATK_TEXT_BOUNDARY_LINE_END The returned string is from the line end before/at/after the offset to the next line start. */ nsresult nsTextAccessible::GetTextHelper(PRInt32 aType, nsAccessibleTextBoundary aBoundaryType, PRInt32 aOffset, PRInt32 *aStartOffset, PRInt32 *aEndOffset, nsAString & _retval) { nsCOMPtr<nsISelectionController> selCon; nsCOMPtr<nsISelection> domSel; if (NS_FAILED(GetSelections(getter_AddRefs(selCon), getter_AddRefs(domSel)))) return NS_ERROR_FAILURE; nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID)); if (!range) return NS_ERROR_OUT_OF_MEMORY; *aStartOffset = *aEndOffset = aOffset; // Move caret position to offset range->SetStart(mDOMNode, aOffset); range->SetEnd(mDOMNode, aOffset); domSel->RemoveAllRanges(); domSel->AddRange(range); // Step1: move caret to an appropriate start position // Step2: move caret to end postion and select the text PRBool step1Forward, step2Forward; // Moving directions for two steps switch (aType) { case -1: //before step1Forward = PR_FALSE; step2Forward = PR_FALSE; break; case 0: //at step1Forward = PR_FALSE; step2Forward = PR_TRUE; break; case 1: //after step1Forward = PR_TRUE; step2Forward = PR_TRUE; break; default: return NS_ERROR_INVALID_ARG; } switch (aBoundaryType) { case BOUNDARY_CHAR: if (aType == 1) { // We need the character next to current position selCon->CharacterMove(step1Forward, PR_FALSE); domSel->GetFocusOffset(aStartOffset); } selCon->CharacterMove(step2Forward, PR_TRUE); domSel->GetFocusOffset(aEndOffset); break; case BOUNDARY_WORD_START: selCon->WordMove(step1Forward, PR_FALSE); // Move caret to previous/next word start boundary domSel->GetFocusOffset(aStartOffset); selCon->WordMove(step2Forward, PR_TRUE); // Select previous/next word domSel->GetFocusOffset(aEndOffset); break; case BOUNDARY_LINE_START: if (aType != 0) { // We need adjust the caret position to previous/next line selCon->LineMove(step1Forward, PR_TRUE); domSel->GetFocusOffset(aEndOffset); } selCon->IntraLineMove(PR_FALSE, PR_FALSE); // Move caret to the line start domSel->GetFocusOffset(aStartOffset); selCon->IntraLineMove(PR_TRUE, PR_TRUE); // Move caret to the line end and select the whole line domSel->GetFocusOffset(aEndOffset); break; case BOUNDARY_WORD_END: case BOUNDARY_LINE_END: case BOUNDARY_SENTENCE_START: case BOUNDARY_SENTENCE_END: case BOUNDARY_ATTRIBUTE_RANGE: return NS_ERROR_NOT_IMPLEMENTED; default: return NS_ERROR_INVALID_ARG; } nsXPIDLString text; // Get text from selection domSel->ToString(getter_Copies(text)); domSel->RemoveAllRanges(); _retval = text; // Ensure aStartOffset <= aEndOffset if (*aStartOffset > *aEndOffset) { PRInt32 tmp = *aStartOffset; *aStartOffset = *aEndOffset; *aEndOffset = tmp; } return NS_OK; }

Okay, thanks for the comments. Perhaps it would be easier to read if PRInt32 aType used enum. Also, perhaps change the names of step1Forward and step2Forward to isStep1Forward and isStep2Forward. Is there a way to avoid the extra temporary variable here? Let's ask alecf. nsXPIDLString text; // Get text from selection domSel->ToString(getter_Copies(text)); domSel->RemoveAllRanges(); _retval = text;

nsXPIDLString text; // Get text from selection domSel->ToString(getter_Copies(text)); domSel->RemoveAllRanges(); _retval = text; Alec said there is no good way to avoid the extra temporary variable, except changing nsISelection::ToString to take a nsAString. That's not necessary.

Comment on attachment 88408 [details] [diff] [review] add enum for GetTextHelper, add more comments r=aaronl

Comment on attachment 88408 [details] [diff] [review] add enum for GetTextHelper, add more comments - In nsIAccessibleText.idl readonly attribute long caretOffset; - /** - * This needs a return value for success/failure - * Not all text fields may support this - */ - boolean setCaretOffset (in long offset); + + void setCaretOffset (in long offset); Why not make the "caretOffset" attribute writable in stead of having a separate method for setting the caretOffset? - In nsTextAccessible::GetSelections(): +{ + nsCOMPtr<nsIPresShell> shell(do_QueryReferent(mPresShell)); + if (!shell) + return NS_ERROR_FAILURE; + ... + + if (*aSelCon && *aDomSel) + return NS_OK; + + return NS_ERROR_FAILURE; +} Seems wrong to me for this to return an error just because there's no presshell, or just because there's no selection at the moment. How about just passing back a null selection and selection controller and returning NS_OK in those cases since they are not really errors? Answer those questions and I'll have one more look...

jst, thanks for your review. 1) that's ok, I'll change. 2) nsTextAccessible::GetSelections() is a help function. Other functions, such as GetTextHelper, GetCaretOffset, etc., are depend on selection and selection controller, they shouldn't be null. or you mean, + if (NS_FAILED(GetSelections(getter_AddRefs(selCon), getter_AddRefs(domSel)))) there we should return NS_OK rather than NS_ERROR_FAILURE?

Ok, if it's an internal helper then returning an error code is acceptable...

Comment on attachment 88408 [details] [diff] [review] add enum for GetTextHelper, add more comments sr=jst

checked in! revised per jst's suggestions.