Bug 25673 - [GTK] ATs should be able to select/unselect text
Summary: [GTK] ATs should be able to select/unselect text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 26991
Blocks: 25531 43919
  Show dependency treegraph
 
Reported: 2009-05-10 12:43 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2010-09-13 05:30 PDT (History)
8 users (show)

See Also:


Attachments
Patch proposal (9.77 KB, patch)
2010-08-12 10:15 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (9.61 KB, patch)
2010-08-31 05:19 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Path proposal + Unit tests (19.52 KB, patch)
2010-09-07 10:13 PDT, Mario Sanchez Prada
mrobinson: review-
Details | Formatted Diff | Diff
Path proposal + Unit tests (22.62 KB, patch)
2010-09-10 11:04 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Path proposal + Unit tests (22.55 KB, patch)
2010-09-10 11:27 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Path proposal + Unit tests (21.73 KB, patch)
2010-09-10 11:55 PDT, Mario Sanchez Prada
no flags 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-10 12:43:00 PDT
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.
Comment 1 Mario Sanchez Prada 2010-08-12 10:15:21 PDT
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
Comment 2 WebKit Review Bot 2010-08-12 12:15:31 PDT
Attachment 64235 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3762092
Comment 3 Mario Sanchez Prada 2010-08-12 12:43:46 PDT
(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?
Comment 4 Mario Sanchez Prada 2010-08-12 12:49:41 PDT
(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+ :-)
Comment 5 Mario Sanchez Prada 2010-08-31 05:19:00 PDT
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)
Comment 6 WebKit Review Bot 2010-08-31 05:34:07 PDT
Attachment 66040 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3855184
Comment 7 Mario Sanchez Prada 2010-09-07 10:13:40 PDT
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
Comment 8 WebKit Review Bot 2010-09-07 11:15:07 PDT
Attachment 66735 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3961245
Comment 9 Martin Robinson 2010-09-07 15:06:32 PDT
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.
Comment 10 Mario Sanchez Prada 2010-09-10 11:04:13 PDT
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.
Comment 11 Martin Robinson 2010-09-10 11:12:16 PDT
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.
Comment 12 Mario Sanchez Prada 2010-09-10 11:27:39 PDT
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)
Comment 13 Martin Robinson 2010-09-10 11:35:26 PDT
Comment on attachment 67208 [details]
Path proposal + Unit tests

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

Thanks! LGTM.
Comment 14 Mario Sanchez Prada 2010-09-10 11:55:00 PDT
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)
Comment 15 WebKit Review Bot 2010-09-10 11:56:40 PDT
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.
Comment 16 Martin Robinson 2010-09-10 12:59:17 PDT
Comment on attachment 67214 [details]
Path proposal + Unit tests

Ok.
Comment 17 Eric Seidel (no email) 2010-09-10 23:03:58 PDT
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.
Comment 18 Eric Seidel (no email) 2010-09-10 23:04:02 PDT
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.
Comment 19 Mario Sanchez Prada 2010-09-13 04:58:09 PDT
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!
Comment 20 Xan Lopez 2010-09-13 05:08:01 PDT
Comment on attachment 67214 [details]
Path proposal + Unit tests

Bang.
Comment 21 WebKit Commit Bot 2010-09-13 05:30:08 PDT
Comment on attachment 67214 [details]
Path proposal + Unit tests

Clearing flags on attachment: 67214

Committed r67383: <http://trac.webkit.org/changeset/67383>
Comment 22 WebKit Commit Bot 2010-09-13 05:30:17 PDT
All reviewed patches have been landed.  Closing bug.