[Webkit-unassigned] [Bug 25677] [GTK] Implement support for get_character_extents and get_range_extents

[bugzilla-daemon at webkit.org]

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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62755|0                           |1
        is obsolete|                            |
  Attachment #63189|                            |review?
               Flag|                            |




--- Comment #10 from Mario Sanchez Prada <msanchez at igalia.com>  2010-08-02 04:30:06 PST ---
Created an attachment (id=63189)
 --> (https://bugs.webkit.org/attachment.cgi?id=63189)
Patch proposal + unit tests

(In reply to comment #8)
> (From update of attachment 62755 [details])
> WebCore/accessibility/AccessibilityObject.cpp:380
>  +  
> you could do here
> if (!textLength) {
>    Node* node = node();
>    if (node) {
>       RenderText* renderText = toRenderText(node->renderer());
>   }

True. Done.

> WebCore/accessibility/AccessibilityObject.cpp:381
>  +          // Plain text a11y object first
> don't use made up words like a11y. make sure to write full sentences for your comments

Fixed, sorry.

> WebCore/accessibility/AccessibilityObject.cpp:386
>  +          if (isWebArea() || isGroup() || isLink() || isHeading()) {
> this should be else if, no?

Not really, because I want to enter even if I entered the previous if branch, when textLength keeps being zero.

However it's true something was missing there, an extra check for !textLengh:

   [...]
   if (renderText)
       textLength = renderText->textLength();

   // Get the text length from the elements under the
   // accessibility object if not a RenderText object.
   if (!textLength && allowsTextRanges())
       textLength = textUnderElement().length();
   [...]

> WebCore/accessibility/AccessibilityObject.cpp:389
>  +              textLength = textValue.length();
> can't you do textLength = textUnderElement().length();

Done, as you can see in the snipped above.

> WebCore/accessibility/AccessibilityObject.cpp:387
>  +              // Check composite objects just in case
> this comment should go above the if and should be a full sentence. you should also enumerate what "just in case" means

Done, as you can see in the snipped above.

> WebCore/accessibility/AccessibilityRenderObject.cpp:2414
>  +  #else
> you also check isWebArea in the other locations. do you not want to do it here?

You were right. Fixed in the new AccessibilityObject::allowsTextRange() function

> WebCore/accessibility/AccessibilityObject.cpp:387
>  +              // Check composite objects just in case
> make sure to end comments with a period

Done

> WebCore/accessibility/AccessibilityRenderObject.cpp:2676
>  +      bool isTextObject =  isTextControl() || m_renderer->isText()
> too much white space

Fixed

> WebCore/accessibility/AccessibilityRenderObject.cpp:2680
>  +  #endif
> instead of doing this PLATFORM stuff and enumerating in each instance, i think you should have a platform specific method on AccessibilityObject, like
> 
> AccessibilityObject::allowsTextRanges() or something appropriate
> 
> then in your AccessibilityObjectGTK.cpp you can override. for the other platforms you can have the default impl live in AccessibilityObject and do what it does now

Fixed, by creating such a function as you suggessted (I agree it's better that way).

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1270
>  +          rangeLength = textLength;
> if the rangelength == 0 are you sure you want to reset to the text length?

Technically, that never will happen (as code inside ATK library won't allow that), but I agree with you it's strange to do it so, so I've changed that <= to just <.

> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1276
>  +  
> i don't see renderObject being used anywhere

True, looks like it belongs to some previous version of the patch and I've just overlooked it. Sorry for the noise

Now attaching a new version of the patch addressing all these issues. Thanks for the feedback, Chris

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