Last Comment Bug 688126 - nsIAccessibleText::setSelectionBounds doesn't fire text selection changed events in some cases
: nsIAccessibleText::setSelectionBounds doesn't fire text selection changed eve...
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: caretsela11y 693934 546068
  Show dependency treegraph
 
Reported: 2011-09-21 03:27 PDT by alexander :surkov
Modified: 2011-10-13 07:31 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (29.87 KB, patch)
2011-09-21 03:45 PDT, alexander :surkov
mzehe: review+
bugs: superreview+
Details | Diff | Review

Description alexander :surkov 2011-09-21 03:27:31 PDT
When existing range is changed then we don't fire text selection changed events.
Comment 1 alexander :surkov 2011-09-21 03:45:08 PDT
Created attachment 561426 [details] [diff] [review]
patch

Olli, some questions for future cleaning-ups:

1) Should we operate with nsIRange rather than nsIDOMRange?
2) Can nsTypedSelection be exposed rather than nsISelection (for example, change existing nsFrameSelection::GetSelection or add new method)? nsTypedSelection allows to deal with nsIRange directly.
3) Can I use nsIFrame::GetConstFrameSelection over nsIFrame::GetFrameSeletion? I need is to get selection and change it. Is it safe to keep frame selection and selection objects not addreffed in this case?
4) We need to avoid selection listeners for in-the-middle operations, for example, one selection notification for add range and remove others. Should selection object gain new helper methods for these operations or is it better to make public batching mechanism (similar to nsSelectionBatcher)?
5) When we change existing range then it is required to be removed and then reinserted, otherwise there's no selection notifications. What the way is preferable to fix that?
Comment 2 Marco Zehe (:MarcoZ) 2011-09-21 04:44:23 PDT
Comment on attachment 561426 [details] [diff] [review]
patch

> PRInt32 nsHyperTextAccessible::GetCaretLineNumber()

Don't we usually keep the return type on a separate line now? Since you're touching this method anyway, you might as well clean this up to conform to most of the other methods in this file.

> /*
>  * Gets the number of selected regions.
>  */
> NS_IMETHODIMP nsHyperTextAccessible::GetSelectionCount(PRInt32 *aSelectionCount)

The same.

> /*
>  * Gets the start and end offset of the specified selection.
>  */
> NS_IMETHODIMP nsHyperTextAccessible::GetSelectionBounds(PRInt32 aSelectionNum, PRInt32 *aStartOffset, PRInt32 *aEndOffset)

The same.

> @@ -1863,17 +1819,18 @@ NS_IMETHODIMP nsHyperTextAccessible::Get
...
>   PRInt32 endOffset;
>   range->GetEndOffset(&endOffset);

Nit/Question: While you're here, could you please add a meaningful initialization to endOffset so it doesn't point to any random value?

>   PRInt16 rangeCompareResult;
>-  rv = range->CompareBoundaryPoints(nsIDOMRange::START_TO_END, range, &rangeCompareResult);
> +  nsresult rv = range->CompareBoundaryPoints(nsIDOMRange::START_TO_END, range,
> +                                             &rangeCompareResult);

The same for rangeCompareResult so we don't operate with random values here.

In nsHyperTextAccessible::SetSelectionBound

>   PRInt32 rangeCount;
>   domSel->GetRangeCount(&rangeCount);

Again make sure we don't deal with random numbers, unless GetRangecount *always* returns a valid value and we can make sure it's not random.

In the same method:
>   PRInt32 startOffset, endOffset;

The same.

> /*
>  * Adds a selection bounded by the specified offsets.
>  */
> NS_IMETHODIMP nsHyperTextAccessible::AddSelection(PRInt32 aStartOffset, PRInt32 aEndOffset)

Cleanup/code style.

>   PRInt32 rangeCount;
>   domSel->GetRangeCount(&rangeCount);

Same question as above.

> /*
>  * Removes the specified selection.
>  */
> NS_IMETHODIMP nsHyperTextAccessible::RemoveSelection(PRInt32 aSelectionNum)

Code style.

>   PRInt32 rangeCount;
>   domSel->GetRangeCount(&rangeCount);

The same as above.

>+      //gQueue.push(new removeSelection("paragraph"));

I assume this is commented out because of the questions you asked Smaug above?

In the tests, I believe we can rely on the fact that we're testing the text offsets in a different place so don't need to test for the actual strings being selected here, right?
Comment 3 alexander :surkov 2011-09-21 04:55:50 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #2)

> > PRInt32 nsHyperTextAccessible::GetCaretLineNumber()
> 
> Don't we usually keep the return type on a separate line now? Since you're
> touching this method anyway, you might as well clean this up to conform to
> most of the other methods in this file.

we do, but usually when method definition is changed. But anyway I will change.

> >+      //gQueue.push(new removeSelection("paragraph"));
> 
> I assume this is commented out because of the questions you asked Smaug
> above?

because of bug 688124, I forgot to mention it here.

> In the tests, I believe we can rely on the fact that we're testing the text
> offsets in a different place so don't need to test for the actual strings
> being selected here, right?

I think so.
Comment 4 alexander :surkov 2011-09-29 04:04:34 PDT
Olli, could you answer comment #1 and look at the patch to check what I'm doing is correct things.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-10 13:47:19 PDT
(In reply to alexander surkov from comment #1)
> Created attachment 561426 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Olli, some questions for future cleaning-ups:
> 
> 1) Should we operate with nsIRange rather than nsIDOMRange?
Possibly. May not matter much

> 2) Can nsTypedSelection be exposed rather than nsISelection (for example,
> change existing nsFrameSelection::GetSelection or add new method)?
> nsTypedSelection allows to deal with nsIRange directly.
Sounds ok to me.


> 3) Can I use nsIFrame::GetConstFrameSelection over
> nsIFrame::GetFrameSeletion? I need is to get selection and change it. Is it
> safe to keep frame selection and selection objects not addreffed in this
> case?
I don't quite understand this.
GetConstFrameSelection() returns an object you shouldn't change.
It is after all a const object.



> 4) We need to avoid selection listeners for in-the-middle operations, for
> example, one selection notification for add range and remove others. Should
> selection object gain new helper methods for these operations or is it
> better to make public batching mechanism (similar to nsSelectionBatcher)?
Could you use scriptrunners? And add scriptblockers when needed.


> 5) When we change existing range then it is required to be removed and then
> reinserted, otherwise there's no selection notifications. What the way is
> preferable to fix that?
You may want to ask Mats about this.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-10 14:06:33 PDT
Comment on attachment 561426 [details] [diff] [review]
patch

>+  if (!startNode)
>+    return;
if (expr) {
  stmt;
}



>+  // Remove collapsed ranges
>+  PRInt32 numRanges = aRanges->Count();
>+  for (PRInt32 count = 0; count < numRanges; count ++) {
>+    PRBool isCollapsed;
>+    (*aRanges)[count]->GetCollapsed(&isCollapsed);
>+    if (isCollapsed) {
>+      aRanges->RemoveObjectAt(count);
>+      -- numRanges;
>+      -- count;
Strange spaces after --


>@@ -1919,59 +1878,67 @@ nsHyperTextAccessible::SetSelectionBound
>   else {
>     domSel->GetRangeAt(aSelectionNum, getter_AddRefs(range));
>     NS_ENSURE_TRUE(range, NS_ERROR_FAILURE);
>   }
> 
>   PRInt32 startOffset, endOffset;
>   nsCOMPtr<nsIDOMNode> startNode, endNode;
> 
>-  rv = HypertextOffsetsToDOMRange(aStartOffset, aEndOffset,
>-                                  getter_AddRefs(startNode), &startOffset,
>-                                  getter_AddRefs(endNode), &endOffset);
>+  nsresult rv = HypertextOffsetsToDOMRange(aStartOffset, aEndOffset,
>+                                           getter_AddRefs(startNode), &startOffset,
>+                                           getter_AddRefs(endNode), &endOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = range->SetStart(startNode, startOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = isOnlyCaret ? range->Collapse(PR_TRUE) :
>                      range->SetEnd(endNode, endOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (aSelectionNum == rangeCount) { // Add successfully created new range
>+  // If new range was created then add it, otherwise notify selection listeners
>+  // that existing selection range was changed.
>+  if (aSelectionNum == rangeCount) {
>     return domSel->AddRange(range);
>+  } else {
>+    domSel->RemoveRange(range);
>+    domSel->AddRange(range);
>   }
So, this is probably ok for now, but you may want to ask Mats if we could add some new way to do this.


> NS_IMETHODIMP nsHyperTextAccessible::RemoveSelection(PRInt32 aSelectionNum)
> {
>-  nsCOMPtr<nsISelection> domSel;
>-  nsresult rv = GetSelections(nsISelectionController::SELECTION_NORMAL,
>-                              nsnull, getter_AddRefs(domSel));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsRefPtr<nsFrameSelection> frameSelection = FrameSelection();
>+  NS_ENSURE_STATE(frameSelection);
>+
>+  nsISelection* domSel =
>+    frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
>+  NS_ENSURE_STATE(domSel);
Do you modify domSel? If so, use nsCOMPtr
(There isn't enough context in the patch to see if domSel or frameSelection is modified, and
domSel used after that. Check also carefully other places where you use weak nsISelection*)


>+  void GetSelectionDOMRanges(PRInt16 aType, nsCOMArray<nsIDOMRange>* aRanges);
Maybe nsCOMArray<nsIDOMRange>& aRanges.


>+already_AddRefed<nsFrameSelection>
>+nsXULTextFieldAccessible::FrameSelection()
>+{
>+  nsCOMPtr<nsIContent> inputContent(GetInputField());
>+  if (inputContent) {
>+    nsIFrame* frame = inputContent->GetPrimaryFrame();
>+    return frame ? frame->GetFrameSelection() : nsnull;
>+  }
>+}
>+
Does this even compile? If inputContent is null, this doesn't return anything.
And, since this can return null, the method should be called GetFrameSelection()
(In general in Gecko GetFoo() can return null, Foo() can't.)
Comment 7 alexander :surkov 2011-10-10 19:00:54 PDT
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 561426 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >+  if (!startNode)
> >+    return;
> if (expr) {
>   stmt;
> }

no braces is usual style in accessibility module

> >+    domSel->RemoveRange(range);
> >+    domSel->AddRange(range);
> >   }
> So, this is probably ok for now, but you may want to ask Mats if we could
> add some new way to do this.

I'll file followup.

> > NS_IMETHODIMP nsHyperTextAccessible::RemoveSelection(PRInt32 aSelectionNum)
> > {
> >-  nsCOMPtr<nsISelection> domSel;
> >-  nsresult rv = GetSelections(nsISelectionController::SELECTION_NORMAL,
> >-                              nsnull, getter_AddRefs(domSel));
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >+  nsRefPtr<nsFrameSelection> frameSelection = FrameSelection();
> >+  NS_ENSURE_STATE(frameSelection);
> >+
> >+  nsISelection* domSel =
> >+    frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
> >+  NS_ENSURE_STATE(domSel);
> Do you modify domSel? If so, use nsCOMPtr
> (There isn't enough context in the patch to see if domSel or frameSelection
> is modified, and
> domSel used after that. Check also carefully other places where you use weak
> nsISelection*)

I do I think. This method removes ranges from DOM selection. This operation may destroy selection object, right?

How can frameSelection be changed? Does DOM selection selection make frameSelection changed?

> >+  void GetSelectionDOMRanges(PRInt16 aType, nsCOMArray<nsIDOMRange>* aRanges);
> Maybe nsCOMArray<nsIDOMRange>& aRanges.

why is it preferred?

> >+already_AddRefed<nsFrameSelection>
> >+nsXULTextFieldAccessible::FrameSelection()
> >+{
> >+  nsCOMPtr<nsIContent> inputContent(GetInputField());
> >+  if (inputContent) {
> >+    nsIFrame* frame = inputContent->GetPrimaryFrame();
> >+    return frame ? frame->GetFrameSelection() : nsnull;
> >+  }
> >+}
> >+
> Does this even compile? If inputContent is null, this doesn't return
> anything.

right, I fixed that locally.

> And, since this can return null, the method should be called
> GetFrameSelection()
> (In general in Gecko GetFoo() can return null, Foo() can't.)

it shouldn't be ever null, I'll add assertions for these 'if' conditions. But accessibility might be odd here since we don't follow the Gecko convention.
Comment 8 alexander :surkov 2011-10-10 19:04:41 PDT
(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to alexander surkov from comment #1)
> > Created attachment 561426 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch
> > 
> > Olli, some questions for future cleaning-ups:
> > 
> > 1) Should we operate with nsIRange rather than nsIDOMRange?
> Possibly. May not matter much

yep, just odd query interfaces calls

> > 2) Can nsTypedSelection be exposed rather than nsISelection (for example,
> > change existing nsFrameSelection::GetSelection or add new method)?
> > nsTypedSelection allows to deal with nsIRange directly.
> Sounds ok to me.

I'll file a bug.

> > 3) Can I use nsIFrame::GetConstFrameSelection over
> > nsIFrame::GetFrameSeletion? I need is to get selection and change it. Is it
> > safe to keep frame selection and selection objects not addreffed in this
> > case?
> I don't quite understand this.
> GetConstFrameSelection() returns an object you shouldn't change.
> It is after all a const object.

I meant that GetFrameSelection() returns addrefed pointer while GetConstFrameSelection() returns raw pointer. So the question was is can I operate on non adderefed frameSelection object? For example, can it be destroyed if I add/remove/change DOM selection ranges?

> > 4) We need to avoid selection listeners for in-the-middle operations, for
> > example, one selection notification for add range and remove others. Should
> > selection object gain new helper methods for these operations or is it
> > better to make public batching mechanism (similar to nsSelectionBatcher)?
> Could you use scriptrunners? And add scriptblockers when needed.

could you give me a link in the code and code snippet where I can get learn more about this?
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-11 02:57:55 PDT
(In reply to alexander surkov from comment #7)
> 
> no braces is usual style in accessibility module
Ok


> I do I think. This method removes ranges from DOM selection. This operation
> may destroy selection object, right?
So you're modifying selection. Use nsCOMPtr.

> 
> > >+  void GetSelectionDOMRanges(PRInt16 aType, nsCOMArray<nsIDOMRange>* aRanges);
> > Maybe nsCOMArray<nsIDOMRange>& aRanges.
> 
> why is it preferred?
GetSelectionDOMRanges knows that the parameter can't be null, ever.


> 
> right, I fixed that locally.
> 
> > And, since this can return null, the method should be called
> > GetFrameSelection()
> > (In general in Gecko GetFoo() can return null, Foo() can't.)
> 
> it shouldn't be ever null, I'll add assertions for these 'if' conditions.
But if the method implementation may return null, it should be named GetFoo.
So either don't add any assertions but just do
already_AddRefed<nsFrameSelection>
nsXULTextFieldAccessible::FrameSelection()
{
  nsCOMPtr<nsIContent> inputContent = GetInputField();
  nsIFrame* frame = inputContent->GetPrimaryFrame();
  return frame->GetFrameSelection();
}

or call the method GetFrameSelection() and add the null checks.
Comment 10 alexander :surkov 2011-10-11 05:58:07 PDT
(In reply to Olli Pettay [:smaug] from comment #9)

> > > >+  void GetSelectionDOMRanges(PRInt16 aType, nsCOMArray<nsIDOMRange>* aRanges);
> > > Maybe nsCOMArray<nsIDOMRange>& aRanges.
> > 
> > why is it preferred?
> GetSelectionDOMRanges knows that the parameter can't be null, ever.

I can see the point but isn't it general practice to use const references to pass object by reference if the object is not supposed for change, otherwise use pointers?
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-11 07:05:36 PDT
What has |const| to do with this all?

You can pass reference if you want to make it clear that the parameter cannot ever never be null.
Comment 12 alexander :surkov 2011-10-11 07:20:20 PDT
(In reply to Olli Pettay [:smaug] from comment #11)
> What has |const| to do with this all?

that was about hints I'm trying to follow usually (like http://www2.research.att.com/~bs/bs_faq2.html#call-by-reference)
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-11 08:40:39 PDT
That link says
"If you want to change the object passed, call by reference or use a pointer; e.g. void f(X&); or void f(X*);"

And I'd say & is usually better when dealing things like nsCOMArray.
But, doesn't really matter. Whatever works for you.
Comment 14 alexander :surkov 2011-10-11 08:47:35 PDT
(In reply to Olli Pettay [:smaug] from comment #13)
> That link says
> "If you want to change the object passed, call by reference or use a
> pointer; e.g. void f(X&); or void f(X*);"

also it says: "My personal style is to use a pointer when I want to modify an object because in some contexts that makes it easier to spot that a modification is possible. "

> And I'd say & is usually better when dealing things like nsCOMArray.
> But, doesn't really matter. Whatever works for you.

ok
Comment 15 alexander :surkov 2011-10-12 00:41:41 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/197cf8c60edc
Comment 16 Marco Bonardo [::mak] 2011-10-13 07:12:26 PDT
https://hg.mozilla.org/mozilla-central/rev/197cf8c60edc

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