[Webkit-unassigned] [Bug 25673] [GTK] ATs should be able to select/unselect text

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25673


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66735|0                           |1
        is obsolete|                            |
  Attachment #67206|                            |review?, commit-queue?
               Flag|                            |




--- Comment #10 from Mario Sanchez Prada <msanchez at igalia.com>  2010-09-10 11:04:13 PST ---
Created an attachment (id=67206)
 --> (https://bugs.webkit.org/attachment.cgi?id=67206)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.