Closed
Bug 1472274
Opened 6 years ago
Closed 6 years ago
Support text selection direction
Categories
(Core :: Disability Access APIs, defect, P1)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: eeejay, Assigned: eeejay, NeedInfo)
References
Details
(Whiteboard: [geckoview:klar:p2])
Attachments
(1 file, 1 obsolete file)
11.11 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
(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.
Assignee | ||
Comment 4•6 years ago
|
||
If this is a problem in Windows or Linux we can make start greater than end arguments illegal in their respective wrappers.
Assignee | ||
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
(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.
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Attachment #8988848 -
Attachment is obsolete: true
Attachment #8988848 -
Flags: feedback?(jteh)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/178261758dc9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 12•6 years ago
|
||
[geckoview:klar] so this bug will be triaged as a potential Focus+GV blocker.
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Whiteboard: [geckoview:klar]
Updated•6 years ago
|
Whiteboard: [geckoview:klar] → [geckoview:klar:p2]
You need to log in
before you can comment on or make changes to this bug.
Description
•