Bug 25609 - [GTK] Implement support for get_selection and get_n_selections
Summary: [GTK] Implement support for get_selection and get_n_selections
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-06 23:41 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2009-07-27 14:28 PDT (History)
3 users (show)

See Also:


Attachments
getselection.patch (2.53 KB, patch)
2009-06-10 10:00 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
getnselections.patch (2.19 KB, patch)
2009-06-10 13:50 PDT, Xan Lopez
gns: review+
Details | Formatted Diff | Diff
getselectionfix.patch (3.84 KB, patch)
2009-06-11 00:07 PDT, Xan Lopez
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 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.)
Comment 1 Joanmarie Diggs (irc: joanie) 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!
Comment 2 Xan Lopez 2009-06-10 10:00:43 PDT
Created attachment 31132 [details]
getselection.patch

Implement get_selection.
Comment 3 Gustavo Noronha (kov) 2009-06-10 10:13:09 PDT
Comment on attachment 31132 [details]
getselection.patch

Ok, makes sense.
Comment 4 Brent Fulgham 2009-06-10 10:34:49 PDT
Landed in @r44567.
Comment 5 Xan Lopez 2009-06-10 10:38:56 PDT
We are still missing the get_n_selections part to close this bug, reopening.
Comment 6 Xan Lopez 2009-06-10 10:39:17 PDT
Comment on attachment 31132 [details]
getselection.patch

Clearing review flag.
Comment 7 Xan Lopez 2009-06-10 13:50:36 PDT
Created attachment 31140 [details]
getnselections.patch

And get_n_selections.
Comment 8 Gustavo Noronha (kov) 2009-06-10 13:52:30 PDT
Comment on attachment 31140 [details]
getnselections.patch

r=me
Comment 9 Xan Lopez 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!
Comment 10 Joanmarie Diggs (irc: joanie) 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.
Comment 11 Xan Lopez 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.
Comment 12 Joanmarie Diggs (irc: joanie) 2009-06-11 12:15:45 PDT
(In reply to comment #11)

> This workaround seems to fix
> things.

Confirmed. Thanks! 

Comment 13 Gustavo Noronha (kov) 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.
Comment 14 Xan Lopez 2009-06-11 23:47:07 PDT
Thanks, landed with the modifications suggested as r44622. Closing as FIXED.
Comment 15 Joanmarie Diggs (irc: joanie) 2009-07-27 14:28:45 PDT
Verifying. Thanks!