Closed Bug 1472274 Opened 6 years ago Closed 6 years ago

Support text selection direction

Categories

(Core :: Disability Access APIs, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: eeejay, Assigned: eeejay, NeedInfo)

References

Details

(Whiteboard: [geckoview:klar:p2])

Attachments

(1 file, 1 obsolete file)

Our current text selection support has no concept of direction. For example, if I have a caret in a block of ltr text at offset 10 and hold shift and press right i am expanding the selection forward, and the caret is at the tip of the selection at offset 11. On the other hand, if I press shift+left, the caret is at the beginning of the selection at offset 9.

This is important because the caret placement determines what the next interaction would do. If the caret is at the end of the selection and i press shift+right, the selection grows. If it is at the beginning, and I press shift+right, it shrinks.

Our current implementation of nsIAccessible.setSelectionBounds always puts the caret at the end of the selection.
nsIAccessibleText.setSelectionBounds should accept a start offset that is larger than an end offset. This is an indica tion that it is a reverse selection, and the caret should be at the start of the selection.

This should also be consistent with how multiple range selections work interactively - the caret should re
main at the last offset reached.
Attachment #8988848 - Flags: review?(surkov.alexander)
Comment on attachment 8988848 [details] [diff] [review]
Alter selection direction according to the last change. r?surkov

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

it looks good with me, but it makes sense to check with Jamie if reverse offsets may be a problem for platform APIs. I think it shouldn't be, but it's better stay of a safe side.

::: accessible/generic/HyperTextAccessible.cpp
@@ +1651,5 @@
>  {
>    index_t startOffset = ConvertMagicOffset(aStartOffset);
>    index_t endOffset = ConvertMagicOffset(aEndOffset);
>    if (!startOffset.IsValid() || !endOffset.IsValid() ||
> +      startOffset > CharacterCount() || endOffset > CharacterCount()) {

nit: it'd better to keep CharacterCount() in a local variable, or you could keep min and max offsets.

@@ +1676,3 @@
>      return false;
>  
> +  // If this is not a new range,  notify selection listeners that the existing

nit: extra space after ','

::: accessible/tests/mochitest/textselection/test_general.html
@@ +61,5 @@
>          return "nsIAccessibleText::addSelection test for " + aID;
>        };
>      }
>  
> +    function changeSelection(aID, aIndex, aStartOffset, aEndOffset) {

maybe it'd be more readable to keep [startOffset, endOffsets) as array, makes it closer to addSelections function

@@ +100,5 @@
>        ];
>  
>        this.invoke = function removeSelection_invoke() {
> +        let selectionCount = this.hyperText.selectionCount;
> +        for (let i=0; i < selectionCount; i++) {

nit: spaces around =
Attachment #8988848 - Flags: review?(surkov.alexander)
Attachment #8988848 - Flags: review+
Attachment #8988848 - Flags: feedback?(jteh)
(In reply to alexander :surkov from comment #2)
> it looks good with me, but it makes sense to check with Jamie if reverse
> offsets may be a problem for platform APIs.

I checked NVDA's code and it seems we handle both start < end and end < start gracefully. However, I then read this patch, and if I'm not mistaken (according to the tests), what gets returned for selection to platform APIs hasn't changed here; i.e. start is always < end. What *has* changed is that caretOffset could be either start or end. is this correct?

I think it might be good to have Marco test this with both NVDA and JAWS anyway, just to be safe. Also, copying Joanie so she's aware of this.
If this is a problem in Windows or Linux we can make start greater than end arguments illegal in their respective wrappers.
I played around with a GTK text widget in accerciser, and it seems like backwards selections are actually supported in ATK, so this should be a win on Linux! Flagging Joanie so she sees this.
Flags: needinfo?(jdiggs)
(In reply to Eitan Isaacson [:eeejay] from comment #4)
> If this is a problem in Windows or Linux we can make start greater than end
> arguments illegal in their respective wrappers.

I'm not worried about start > end for the setter. I got the impression from Alex that the getter might start returning start > end, which would be more of a concern. However, my understanding from reading the tests is that the getter is not affected by this. Is that correct? If that's true, we have nothing to worry about here.
(In reply to James Teh [:Jamie] from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #4)
> > If this is a problem in Windows or Linux we can make start greater than end
> > arguments illegal in their respective wrappers.
> 
> I'm not worried about start > end for the setter. I got the impression from
> Alex that the getter might start returning start > end, which would be more
> of a concern. However, my understanding from reading the tests is that the
> getter is not affected by this. Is that correct? If that's true, we have
> nothing to worry about here.

Correct, the getter is not affected.
P1 because bug 1471951 depends on this and that is p1.
Priority: -- → P1
nsIAccessibleText.setSelectionBounds should accept a start offset that is larger than an end offset. This is an indica tion that it is a reverse selection, and the caret should be at the start of the selection.

This should also be consistent with how multiple range selections work interactively - the caret should re
main at the last offset reached.
Attachment #8988848 - Attachment is obsolete: true
Attachment #8988848 - Flags: feedback?(jteh)
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/178261758dc9
Alter selection direction according to the last change. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/178261758dc9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
[geckoview:klar] so this bug will be triaged as a potential Focus+GV blocker.
Whiteboard: [geckoview:klar]
Whiteboard: [geckoview:klar] → [geckoview:klar:p2]
You need to log in before you can comment on or make changes to this bug.