Bug 25673 – [GTK] ATs should be able to select/unselect text

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

As part of bug 16135 support was added for text selection via the keyboard. (Thanks!!)

In addition to this capability, users of refreshable braille displays expect to be able to select text using their display instead of the keyboard. In order to provide this support, assistive technologies such as Orca need to be able to select and unselect text. This can be accomplished via set_selection and remove_selection. 

(If multiple text selections can or will be possible via keyboard or mouse, add_selection should also be implemented.)

Marking the priority as P3 because the other Gtk+/Atk bugs are more critical than this one.

[Mario Sanchez Prada]

Created attachment 64235 [details]
Patch proposal

Asking for review on this patch. More details in the ChangeLog entry inside the patch.

Note: Unit tests for this new stuff are provided by the patch for bug 43919 (which was filed just with the purpose of providing those unittests) because they couldn't be properly implemented without the fix for bug 26991, which is closely related to this one. That's why bug 43919 was set as dependent on these other two bugs (so patches get applied in order once reviewed+). See comments on bug 26991 and bug 43919 for more details

[WebKit Review Bot]

Attachment 64235 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3762092

[Mario Sanchez Prada]

(In reply to comment #2)
> Attachment 64235 [details] did not build on gtk:
> Build output: http://queues.webkit.org/results/3762092

Hmm... true, but that is because it uses a new static function defined while writing the patch for bug 26991.

It would be great if we could set this bug as depending on bug 26991 then, but I can't do it. Perhaps Joanmarie could (as she's the reporter). If not possible, I can write a new patch not assuming the function defined in the other patch and later on we could do the merge, or just provide a new -updated- patch in the bug being fixed in second place.

Opinions?

[Mario Sanchez Prada]

(In reply to comment #3)
> [...]
> It would be great if we could set this bug as depending on bug 26991 then, but 
> I can't do it. Perhaps Joanmarie could (as she's the reporter). 

Thanks Joanie! Now let's wait for 26991 to get reviewed+ :-)

[Mario Sanchez Prada]

Created attachment 66040 [details]
Patch proposal

Updating patch after coming back from holidays, based on comment #5 for bug 26991 (guess the same rationale applies here).
(Remember that this patch depends on patch bug 26991 to work)

[WebKit Review Bot]

Attachment 66040 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3855184

[Mario Sanchez Prada]

Created attachment 66735 [details]
Path proposal + Unit tests

Attaching new up-to-date version now we (Martin and I) decided to bundle unit tests for this stuff along with this patch (instead of using a the new bug 43919 for that, which should be closed/invalidated now).

Hope this helps to get this stuff in soon O:-)

PS: Still, note this patch won't work if you don't apply patch for bug 26991 first, as it uses a new function defined there

[WebKit Review Bot]

Attachment 66735 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3961245

[Martin Robinson]

Comment on attachment 66735 [details]
Path proposal + Unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=66735&action=prettypatch

Looks good, but I have a few concerns.

> WebCore/accessibility/AccessibilityObject.cpp:-377
> -#if PLATFORM(GTK)
Nice!

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1419
>  static gboolean webkit_accessible_text_remove_selection(AtkText* text, gint selection_num)
Please convert non-WebKit-style variable names as you implement these methods.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1435
> +    if (startOffset != endOffset) {
Please use early returns.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1440
> +        VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
Why not just use core(text)->setSelectedTextRange(PlainTextRange(caretOffset, 0)); ?

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1449
>  static gboolean webkit_accessible_text_set_selection(AtkText* text, gint selection_num, gint start_offset, gint end_offset)
Please convert these variable names to WebKit-style as you implement these methods.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1455
> +    if (start_offset != end_offset) {
Please use an early return here instead.
if (start_offset == end_offset)
    return FALSE;

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1458
> +        VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
Why not just use core(text)->setSelectedTextRange(PlainTextRange(startOffset, endOffset - startOffset)); ?

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1472
> +    VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
Why not just use core(text)->setSelectedTextRange(PlainTextRange(offset, 0)); ?

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:357
> +    if (gtk_widget_has_screen(GTK_WIDGET(m_webView))) {
This change seems sane, but since we're getting into a condition of depth three (if you count the #if), perhaps we should move this logic to a new method called something like setSelectionPrimaryClipboardIfNeeded and use early returns.

[Mario Sanchez Prada]

Created attachment 67206 [details]
Path proposal + Unit tests

(In reply to comment #9)
> (From update of attachment 66735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66735&action=prettypatch
> 
> Looks good, but I have a few concerns.

Attaching a new patch, hopefully better (still it depends on patch for 26991 so don't expect this to compile without applying the other one first).

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1419
> >  static gboolean webkit_accessible_text_remove_selection(AtkText* text, gint selection_num)
> Please convert non-WebKit-style variable names as you implement these methods.

Done
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1435
> > +    if (startOffset != endOffset) {
> Please use early returns.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1440
> > +        VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(caretOffset, 0)); ?

I removed this part now, and make removeSelection delegate on setSelection, so no more a problem.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1449
> >  static gboolean webkit_accessible_text_set_selection(AtkText* text, gint selection_num, gint start_offset, gint end_offset)
> Please convert these variable names to WebKit-style as you implement these methods.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1455
> > +    if (start_offset != end_offset) {
> Please use an early return here instead.
> if (start_offset == end_offset)
>     return FALSE;
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1458
> > +        VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(startOffset, endOffset - startOffset)); ?

I tried that but didn't work fine, probably (wild guess) because it's not the same than setting a VisibleSelection. Not sure, though.
Hence I kept this part as it is.
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1472
> > +    VisiblePositionRange range = coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(offset, 0)); ?

I kept this part as it is, as that change didn't work (see rationale above)
 
> > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:357
> > +    if (gtk_widget_has_screen(GTK_WIDGET(m_webView))) {
> This change seems sane, but since we're getting into a condition of depth three (if you count the #if), perhaps we should move this logic to a new method called something like setSelectionPrimaryClipboardIfNeeded and use early returns.

Done.

[Martin Robinson]

Comment on attachment 67206 [details]
Path proposal + Unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=67206&action=prettypatch

> WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:112
> +    if (!textLength) {
This should probably be an early return.

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:346
> +{
I like this a lot better, but I'd rather see early returns. :/

Looks good! r=me, but please switch to using early returns before landing.

[Mario Sanchez Prada]

Created attachment 67208 [details]
Path proposal + Unit tests

(In reply to comment #11)
> (From update of attachment 67206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67206&action=prettypatch
> 
> > WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:112
> > +    if (!textLength) {
> This should probably be an early return.
> 
> > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:346
> > +{
> I like this a lot better, but I'd rather see early returns. :/
> 
> Looks good! r=me, but please switch to using early returns before landing.

Here you have a new version.

BTw, I'm not asking for cq+ for now because we must make sure patch for 26991 are landed first (hence, I'll ask for cq+ later)

[Martin Robinson]

Comment on attachment 67208 [details]
Path proposal + Unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=67208&action=prettypatch

Thanks! LGTM.

[Mario Sanchez Prada]

Created attachment 67214 [details]
Path proposal + Unit tests

(In reply to comment #13)
> (From update of attachment 67208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67208&action=prettypatch
> 
> Thanks! LGTM.

This is *the right patch*, sorry for the noise. Guess you'll need to review+ it again (but do not cq+ it yet, please)

[WebKit Review Bot]

Attachment 67214 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1390:  webkit_accessible_text_set_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1411:  webkit_accessible_text_remove_selection is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 67214 [details]
Path proposal + Unit tests

Ok.

[Eric Seidel (no email)]

Comment on attachment 67206 [details]
Path proposal + Unit tests

Cleared Martin Robinson's review+ from obsolete attachment 67206 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 67208 [details]
Path proposal + Unit tests

Cleared Martin Robinson's review+ from obsolete attachment 67208 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mario Sanchez Prada]

Comment on attachment 67214 [details]
Path proposal + Unit tests

Now that the patch for bug 26991 has already landed in the WK repository, it's time to ask for the cq+ for this patch.

Thanks!

[Xan Lopez]

Comment on attachment 67214 [details]
Path proposal + Unit tests

Bang.

[WebKit Commit Bot]

Comment on attachment 67214 [details]
Path proposal + Unit tests

Clearing flags on attachment: 67214

Committed r67383: <http://trac.webkit.org/changeset/67383>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.