Bug 25609

Summary: [GTK] Implement support for get_selection and get_n_selections
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: VERIFIED FIXED    
Severity: Normal CC: apinheiro, walker.willie, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
getselection.patch
none
getnselections.patch
gustavo: review+
getselectionfix.patch gustavo: review+

Joanmarie Diggs
Reported 2009-05-06 23:41:46 PDT
The accessible text interface includes a method to get a specified selection. For more information please see http://library.gnome.org/devel/atk/unstable/AtkText.html#atk-text-get-selection. Usage/need example: ATs are now aware that text has been selected/unselected thanks to the newly-implemented text-selection-changed events. A screen reader such as Orca should in turn present the text that just became selected/unselected. In order to do that, it needs to know the start and end offset of the selected text (which it can get via get_selection). (Note that the other selection-related methods of the accessible text interface should be implemented too. However, get_selection is the most critical of the bunch because without it, we cannot do anything in response to a text-selection-changed event.)
Attachments
getselection.patch (2.53 KB, patch)
2009-06-10 10:00 PDT, Xan Lopez 		no flags
DetailsFormatted DiffDiff
getnselections.patch (2.19 KB, patch)
2009-06-10 13:50 PDT, Xan Lopez 		gustavo: review+
DetailsFormatted DiffDiff
getselectionfix.patch (3.84 KB, patch)
2009-06-11 00:07 PDT, Xan Lopez 		gustavo: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joanmarie Diggs
Comment 1 2009-05-10 12:27:41 PDT
I'm going to add my request for support for get_n_selections to this bug (see http://library.gnome.org/devel/atk/unstable/AtkText.html#atk-text-get-n-selections for more info). Rationale: 1. It's more closely related to this bug than the bug I'm going to open up for the remaining selection-related methods which need to be implemented. 2. I'm assuming it won't take much (more) work to implement, so why open a separate bug for it? :-) If I'm wrong w.r.t. #2 or you would prefer a separate bug be opened for this, please let me know. Thanks!
Xan Lopez
Comment 2 2009-06-10 10:00:43 PDT
Created attachment 31132 [details] getselection.patch Implement get_selection.
Gustavo Noronha (kov)
Comment 3 2009-06-10 10:13:09 PDT
Comment on attachment 31132 [details] getselection.patch Ok, makes sense.
Brent Fulgham
Comment 4 2009-06-10 10:34:49 PDT
Landed in @r44567.
Xan Lopez
Comment 5 2009-06-10 10:38:56 PDT
We are still missing the get_n_selections part to close this bug, reopening.
Xan Lopez
Comment 6 2009-06-10 10:39:17 PDT
Comment on attachment 31132 [details] getselection.patch Clearing review flag.
Xan Lopez
Comment 7 2009-06-10 13:50:36 PDT
Created attachment 31140 [details] getnselections.patch And get_n_selections.
Gustavo Noronha (kov)
Comment 8 2009-06-10 13:52:30 PDT
Comment on attachment 31140 [details] getnselections.patch r=me
Xan Lopez
Comment 9 2009-06-10 13:57:42 PDT
(In reply to comment #8) > (From update of attachment 31140 [details] [review]) > r=me > Landed as r44592, thanks!
Joanmarie Diggs
Comment 10 2009-06-10 16:12:29 PDT
I'm afraid I'm reopening this one. In testing I'm seeing the following unexpected/undesirable behavior: * get_n_selections returns 1 even when no text is selected. * get_selection returns the offsets of the selected text on the page; NOT the selected text from the current accessible. For instance: 1. View the page for this bug. 2. Select "Kit" in "WebKit Bugzilla" 3. In Accerciser, select the accessible text object associated with, WebKit Bugzilla. In the iPython console, try: acc.queryText().getNSelections() result: 1 (as expected) acc.queryText().getSelection(0) result: (3, 6) (as expected) 4. In Accerciser, choose any other text object (e.g. « Prev) and repeat the above commands. The expected result is that getNSelections would return 0 because *in the newly-chosen accessible*, no text is selected. Similarly, getSelection(0) should not indicate that text is selected. The actual result is 1 and (3, 6) respectively. 5. Deselect the text in WebKit Bugzilla, leaving the caret between the "b" and the "K". Choose any text object and repeat the above commands. Expected results: getNSelections would return 0 because no text is selected. Actual results: getNSelections continues to return 1. In addition, the offsets returned by getSelection(0) indicate the current caret position (3, 3) even if the accessible being examined doesn't have the caret.
Xan Lopez
Comment 11 2009-06-11 00:07:36 PDT
Created attachment 31153 [details] getselectionfix.patch You are totally right! The API managed to confuse me, because object->selection() actually return the global selection, wherever it is, not the selection for the object you are using. This workaround seems to fix things.
Joanmarie Diggs
Comment 12 2009-06-11 12:15:45 PDT
(In reply to comment #11) > This workaround seems to fix > things. Confirmed. Thanks!
Gustavo Noronha (kov)
Comment 13 2009-06-11 17:45:24 PDT
Comment on attachment 31153 [details] getselectionfix.patch > This is pretty hacky-ish, but I can't seem to find a direct API to > get the VisibleSelection for a given object, only the global one. This should go as a comment in the code (as well?), IMO. > + > + if (selection_num != 0 || !selectionBelongsToObject(coreObject, selection)) { > // WebCore does not support multiple selection, so anything but 0 does not make sense for now. Probably here, extending this comment. I'd also move this comment to before the if, so that it is clear that you are talking about the whole conditional. Other than that, despite being hacky, this looks good.
Xan Lopez
Comment 14 2009-06-11 23:47:07 PDT
Thanks, landed with the modifications suggested as r44622. Closing as FIXED.
Joanmarie Diggs
Comment 15 2009-07-27 14:28:45 PDT
Verifying. Thanks!
Note You need to log in before you can comment on or make changes to this bug.