Last Comment Bug 758112 - use nsTypedSelection in a11y
: use nsTypedSelection in a11y
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-05-23 21:57 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-10-03 07:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (11.45 KB, patch)
2012-05-23 22:00 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (10.99 KB, patch)
2012-06-01 20:39 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-05-23 21:57:51 PDT

    
Comment 1 Trevor Saunders (:tbsaunde) 2012-05-23 22:00:04 PDT
Created attachment 626694 [details] [diff] [review]
wip

I've been meaning to finish this up, but haven't had time to touch it in a while...
Comment 2 Mark Capella [:capella] 2012-05-27 22:08:26 PDT
Can you define the scope of this change a little bit more? The title of the bug says "use nsTypedSelection in a11y" ...

Do you want someone to just complete the patch you've attached and its associated changes to nsHTMLTextAccessible and nsHyperTextAccessible?

Or is this concept to be expanded to other a11y classes that use "nsISelectionPrivate.h" ie: nsCaretAccessible, / nsCoreUtils?

If the the goal to discontinue use of that header file?
Comment 3 Trevor Saunders (:tbsaunde) 2012-05-27 22:31:26 PDT
(In reply to Mark Capella [:capella] from comment #2)
> Can you define the scope of this change a little bit more? The title of the
> bug says "use nsTypedSelection in a11y" ...
> 
> Do you want someone to just complete the patch you've attached and its
> associated changes to nsHTMLTextAccessible and nsHyperTextAccessible?

want my be to strong, but if someone decided to do that because say it made there work easier, or they were about to touch some of that I would not object.  Or if they just wanted to make that stuff nicer :)

> Or is this concept to be expanded to other a11y classes that use
> "nsISelectionPrivate.h" ie: nsCaretAccessible, / nsCoreUtils?
> 
> If the the goal to discontinue use of that header file?

no, just use nicer and faster functions where we can. For atleast some of nsCaretAccessible we don't have a way to get a nsTypedSelection, so that will have to stay as is for now, not sure bout nsCoreUtils.
Comment 4 Trevor Saunders (:tbsaunde) 2012-06-01 20:39:13 PDT
Created attachment 629433 [details] [diff] [review]
patch
Comment 5 alexander :surkov 2012-06-01 21:05:46 PDT
Comment on attachment 629433 [details] [diff] [review]
patch

Review of attachment 629433 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1557,4 @@
>      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
>    NS_ENSURE_STATE(domSel);
>  
> +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)

what if no ranges? it would be a long cycle? :)

@@ +1557,5 @@
>      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
>    NS_ENSURE_STATE(domSel);
>  
> +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> +    domSel->RemoveRange(domSel->GetRangeAt(idx));

I don't see a RemoveRange version that takes nsRange

@@ +1557,5 @@
>      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
>    NS_ENSURE_STATE(domSel);
>  
> +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> +    domSel->RemoveRange(domSel->GetRangeAt(idx));

btw, any particular reason you changed it to remove ranges from the start?

@@ +1659,1 @@
>      return -1;

you ignore text nodes, nothing prevents them to be a focus node of the selection

@@ +1846,5 @@
>    bool isOnlyCaret = (aStartOffset == aEndOffset);
>  
> +  PRUint32 rangeCount = domSel->GetRangeCount();
> +  if (rangeCount > static_cast<PRUint32>(aSelectionNum))
> +    return NS_ERROR_INVALID_ARG;

move this check before bool isOnlyCaret and shouldn't it be opposite condition?

@@ +1897,2 @@
>  
>    return SetSelectionBounds(rangeCount, aStartOffset, aEndOffset);

you don't need local variable to store the value of this inline

@@ +1918,1 @@
>    return domSel->RemoveRange(range);

again about RemoveRange
Comment 6 Trevor Saunders (:tbsaunde) 2012-06-01 21:17:54 PDT
> ::: accessible/src/generic/HyperTextAccessible.cpp
> @@ +1557,4 @@
> >      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
> >    NS_ENSURE_STATE(domSel);
> >  
> > +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> 
> what if no ranges? it would be a long cycle? :)

actually, SetSelectionBounds() makes there always be atleast one range.

> @@ +1557,5 @@
> >      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
> >    NS_ENSURE_STATE(domSel);
> >  
> > +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> > +    domSel->RemoveRange(domSel->GetRangeAt(idx));
> 
> I don't see a RemoveRange version that takes nsRange

nsRange inherits nsIDOMRange

> @@ +1557,5 @@
> >      frameSelection->GetSelection(nsISelectionController::SELECTION_NORMAL);
> >    NS_ENSURE_STATE(domSel);
> >  
> > +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> > +    domSel->RemoveRange(domSel->GetRangeAt(idx));
> 
> btw, any particular reason you changed it to remove ranges from the start?

make it faster, since if you do it the other way you have to shift the elements of the array down each time.

> @@ +1659,1 @@
> >      return -1;
> 
> you ignore text nodes, nothing prevents them to be a focus node of the
> selection

yeah, you caught me before I took of the clown suit :)

> @@ +1846,5 @@
> >    bool isOnlyCaret = (aStartOffset == aEndOffset);
> >  
> > +  PRUint32 rangeCount = domSel->GetRangeCount();
> > +  if (rangeCount > static_cast<PRUint32>(aSelectionNum))
> > +    return NS_ERROR_INVALID_ARG;
> 
> move this check before bool isOnlyCaret and shouldn't it be opposite
> condition?

same :)

> @@ +1897,2 @@
> >  
> >    return SetSelectionBounds(rangeCount, aStartOffset, aEndOffset);
> 
> you don't need local variable to store the value of this inline

ok, sure

> @@ +1918,1 @@
> >    return domSel->RemoveRange(range);
> 
> again about RemoveRange

same answer
Comment 7 alexander :surkov 2012-06-01 21:23:35 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> > > +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> > 
> > what if no ranges? it would be a long cycle? :)
> 
> actually, SetSelectionBounds() makes there always be atleast one range.

ok, I see but I would feel safer if you go without this assumption.

> > I don't see a RemoveRange version that takes nsRange
> 
> nsRange inherits nsIDOMRange

here's a trick :) Btw, RemoveRange doesn't seem optimal since it's supposed to work with arbitrary range but we are guaranteed we work with selection ranges. Maybe it's worth to file a bug on this?

> > btw, any particular reason you changed it to remove ranges from the start?
> 
> make it faster, since if you do it the other way you have to shift the
> elements of the array down each time.

ok, cool
Comment 8 alexander :surkov 2012-06-01 21:23:56 PDT
Comment on attachment 629433 [details] [diff] [review]
patch

r=me with comments addressed
Comment 9 Trevor Saunders (:tbsaunde) 2012-06-01 21:31:43 PDT
(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > > > +  for (PRUint32 idx = domSel->GetRangeCount() - 1; idx > 0; idx--)
> > > 
> > > what if no ranges? it would be a long cycle? :)
> > 
> > actually, SetSelectionBounds() makes there always be atleast one range.
> 
> ok, I see but I would feel safer if you go without this assumption.

would an assert work for you, or would you prefer I put the whole thing in an if?

> > > I don't see a RemoveRange version that takes nsRange
> > 
> > nsRange inherits nsIDOMRange
> 
> here's a trick :) Btw, RemoveRange doesn't seem optimal since it's supposed
> to work with arbitrary range but we are guaranteed we work with selection
> ranges. Maybe it's worth to file a bug on this?

not sure I follow, do you mean the issue where we already know the index, but nsTypedSelection has to figure it out again based on the range? that seems like a reasonable bug.
Comment 10 alexander :surkov 2012-06-01 22:05:19 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> > ok, I see but I would feel safer if you go without this assumption.
> 
> would an assert work for you, or would you prefer I put the whole thing in
> an if?

can you change the index to PRInt32?

> > here's a trick :) Btw, RemoveRange doesn't seem optimal since it's supposed
> > to work with arbitrary range but we are guaranteed we work with selection
> > ranges. Maybe it's worth to file a bug on this?
> 
> not sure I follow, do you mean the issue where we already know the index,
> but nsTypedSelection has to figure it out again based on the range? that
> seems like a reasonable bug.

yes but not only, it seems it checks if the given range overlaps with ranges of the selection
Comment 11 Trevor Saunders (:tbsaunde) 2012-06-02 18:35:14 PDT
(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > ok, I see but I would feel safer if you go without this assumption.
> > 
> > would an assert work for you, or would you prefer I put the whole thing in
> > an if?
> 
> can you change the index to PRInt32?

yeah, I think that works, so long as we don't cre about huge numbers of selections

> > > here's a trick :) Btw, RemoveRange doesn't seem optimal since it's supposed
> > > to work with arbitrary range but we are guaranteed we work with selection
> > > ranges. Maybe it's worth to file a bug on this?
> > 
> > not sure I follow, do you mean the issue where we already know the index,
> > but nsTypedSelection has to figure it out again based on the range? that
> > seems like a reasonable bug.
> 
> yes but not only, it seems it checks if the given range overlaps with ranges
> of the selection

ok, seems you might understand how to describe better than me :)
Comment 12 alexander :surkov 2012-06-03 21:08:21 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > yes but not only, it seems it checks if the given range overlaps with ranges
> > of the selection
> 
> ok, seems you might understand how to describe better than me :)

ignore this, they update frame selection bits, I misread the code.
Comment 13 Trevor Saunders (:tbsaunde) 2012-07-01 23:17:05 PDT
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/20137a4f2187
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-07-02 15:30:52 PDT
https://hg.mozilla.org/mozilla-central/rev/20137a4f2187

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