[Webkit-unassigned] [Bug 26991] [Gtk] get_n_selections and get_selection fail when selecting text across object boundaries

[bugzilla-daemon at webkit.org]

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


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

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




--- Comment #11 from Mario Sanchez Prada <msanchez at igalia.com>  2010-09-10 10:56:42 PST ---
Created an attachment (id=67204)
 --> (https://bugs.webkit.org/attachment.cgi?id=67204)
Patch proposal

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

Thanks for the review, I've been working on this addressing these issues and I have to say I learnt quite a lot of interesting things in the way, so here you have a new patch, hopefully better :-)

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1336
> > +        return false;
> Any reason you aren't using selection.firstRange() or selection.toNormalizedRange() here?

I didn't know of those :-). selection.toNormalizedRange() works fine in this case so I used that one in this new patch

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1341
> > +    for (TextIterator it(range.get()); !it.atEnd(); it.advance()) {
> Would it be possible to replace this loop with Range::intersectsNode?

In this case intersectsNode() *almost* works ok but it fails when the selection is right touching one of the boundaries of the selected node so that wouldn't be enough. Anyway, I still could replace the loop with a combination of intersectsNode() + 2 extra checks to make sure we don't return TRUE if the situation with the boundaries happened. Thanks for the tip.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1360
> > +    RefPtr<Range> range = makeRange(selection.visibleStart(), selection.visibleEnd());
> Again it seems like this should be selection.firstRange() or selection.toNormalizedRange().

Changed to selection.toNormalizedRange().

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1376
> > +            break;
> Tricky loop logic. :/ I admit I'm not sure exactly what the range return value is supposed to represent, but could you take a non-loop approach, such as the one used in AccessibilityRenderObject::ariaSelectedTextDOMRange in WebCore/accessibility/AccessibilityRenderObject.cpp?

It is not exactly the same thing because we need to return the offsets of the selection relative to the selected object, so some manual calculation, at least for the endoffset is still needed.

However, I was able to improve the code (I think) making it more readable (easier to understand) and efficient (still a loop but with far less iterations), allowing to determine starOffset in a much more logical fashion, and keeping a small loop just to compute the length of the intermediate nodes to build the "effective" value for endOffset

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1397
> > +    if (selection_num)
> There are lots of non-WebKit style variable names in this file, but we should be converting them gradually to WebKit style. Please do this for all other arguments of this method as well.

I fixed those as well in this new patch, compiled and tested against the very vest version of trunk :-)

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