use nsTypedSelection in a11y

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
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...
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?
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 629433 [details] [diff] [review]
patch
Attachment #629433 - Flags: review?(surkov.alexander)

Comment 5

5 years ago
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
Attachment #629433 - Flags: review?(surkov.alexander)
(Assignee)

Comment 6

5 years ago
> ::: 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

5 years ago
(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

5 years ago
Comment on attachment 629433 [details] [diff] [review]
patch

r=me with comments addressed
Attachment #629433 - Flags: review+

Updated

5 years ago
Attachment #626694 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
(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

5 years ago
(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
(Assignee)

Comment 11

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/20137a4f2187
https://hg.mozilla.org/mozilla-central/rev/20137a4f2187
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Updated

5 years ago
Assignee: nobody → trev.saunders
You need to log in before you can comment on or make changes to this bug.