Bug 25677 – [GTK] Implement support for get_character_extents and get_range_extents

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

The accessible text interface includes methods to get the extents of a given character (get_character_extents) and a range of characters (get_range_extents). For more information please see http://library.gnome.org/devel/atk/unstable/AtkText.html.

There are various and sundry reasons why an AT might need to know the extents of a character or range of characters. The most immediate needs in terms of Orca are:

1. Determining if two characters from different objects happen to be on the same line or not for the purpose of accurately presenting the line contents to a user. 

2. Identifying the extents of a character, word, or line for the purpose of drawing a rectangle around the block of text being "reviewed" in Orca's Flat Review mode.

Currently the two methods in question always return extents of (0, 0, 0, 0).

[Joanmarie Diggs]

Created attachment 44386 [details]
work in progress (lacks test)

Presumably I will be asked to create a test for this. I'm still mulling over what that test should be, as the nature of the functionality is highly dependent upon font, window size, and window position. I'll come up with something.... Anyhoo, I'm not bothering to ask for a review yet due to the testlessness.

That said.... I wouldn't mind a reality check/informal review of this to make sure I'm not out in left field. Thanks!

[Joanmarie Diggs]

Created attachment 45535 [details]
proposed fix, includes unit test

Essentially [1] the same patch as before, but with a unit test added.

Notes:

1. The change to the code proper between the previous version and this version is the addition of some sanity checking for two possible conditions:

a. A call to atk_text_get_range_extents() is made with an end offset of -1 (to represent the last character). This is extremely likely to occur. I wasn't handling it before. That having been said, now that it's being handled here, it turns out that we're claiming some pretty bogus extents for end offset of -1. After much hair pulling, I started examining the values from other applications (e.g. Gedit, Firefox) under the same conditions. The results were equally (and in some cases more) bogus. So this is not a WebKit bug. Once I track down the true source of this bug (gail perhaps?) and it gets fixed, I believe we will automatically start doing the right thing here.

b. A call to atk_text_get_range_extents() is made with an end offset that is greater than the last character. Our choices are (0, 0, 0, 0) and to assume that the caller erred but wants the extents for the range of text beginning with the start offset provided through the final character.  Personally, I think we should go with the latter, and have done so in this version of the patch. If anyone feels strongly otherwise, I can rip that bit out.

2. Since unit tests are designed (as I understand it) to test platform-specific API calls, this new functionality seemed to call out for a unit test rather than a layout test. I believe you'll find that the unit test I wrote is quite thorough and that it avoids (I hope) the potential for inconsistent results based on the environment of the individual (or bot) running the test.

The disadvantage seems to be that we can't test the coordinate type of ATK_XY_SCREEN; just ATK_XY_WINDOW. When I attempt to add ATK_XY_SCREEN to the unit test, the test aborts with the following error:

~~~~~
Gdk-CRITICAL **: gdk_window_get_origin: assertion `GDK_IS_WINDOW (window)' failed
aborting...
~~~~~

I presume this is because unit tests don't cause an actual window to be rendered. :-)

a. Is it possible to write a unit test which forces an actual window to be rendered?

b. Do we care?

On the latter front, I'm not convinced it's a big deal given that we will be thoroughly testing the window-based extents via the included unit test. And I think that it might be more trouble than it's worth to generate a layout test which presumably has the potential for inconsistent results (i.e. failures) based on the environment of the individual (or bot) running the test. But, I'll of course do whatever y'all suggest.

Finally, I just added my test to testatk.c. I'm not sure at what point it might be worth splitting the Atk unit tests up. If now is the time, please let me know. If now is not the time.... I'm guessing that the style queue bot is going to spit up on this patch due to the new test adhering to the style guidelines for C++ and not the style guidelines for C. Last time this issue came up, Xan said to not change the test and to disregard the style bot in this particular instance. I'm sticking with that advice until I hear otherwise. :-)

Please review. Thanks!

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/tests/testatk.c:369:  test_webkit_atk_get_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:371:  Declaration has space between * and variable name in WebKitWebView* webView  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:372:  Declaration has space between * and variable name in AtkObject* obj  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:373:  Declaration has space between * and variable name in GMainLoop* loop  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:390:  Declaration has space between * and variable name in AtkText* text_obj_line1  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:390:  text_obj_line1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:392:  Declaration has space between * and variable name in AtkText* text_obj_line2  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:392:  text_obj_line2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:394:  Declaration has space between * and variable name in AtkText* text_obj_line3  [whitespace/declaration] [3]
WebKit/gtk/tests/testatk.c:394:  text_obj_line3 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:398:  rect1_window is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:398:  rect2_window is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/gtk/tests/testatk.c:398:  rect3_window is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1002:  webkit_accessible_text_get_character_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1011:  webkit_accessible_text_get_range_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 15

[Joanmarie Diggs]

Comment on attachment 45535 [details]
proposed fix, includes unit test

Clearing the review flag as the test -- and possibly the fix itself -- may be impacted by the fix for bug 30883.

[Mario Sanchez Prada]

Created attachment 62755 [details]
Patch proposal + unit tests

Attaching a new patch to address this issue, based on the original one provided by Joanmarie + addressing the following issues:

  - Adapted code to work with all the changes that have happened since the original one was written, that is, as Joanmarie calls it... The Great Flattening (I love this name) :-)
  - Adapted unit tests in the testatk.c file to test the get_character_extents() and get_range_extents() functions for the AtkText interface, after all the changes that were done
  - Checked how it works with non "pure text" node, such as links, headings, groups or webareas

Eager to read some feedback on it, but now time to go to bed.

Greetings from GUADEC!

[WebKit Review Bot]

Attachment 62755 [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:1290:  webkit_accessible_text_get_character_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1299:  webkit_accessible_text_get_range_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 6 files


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

[Mario Sanchez Prada]

(In reply to comment #6)
> Attachment 62755 [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:1290:  webkit_accessible_text_get_character_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1299:  webkit_accessible_text_get_range_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 2 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Same problem always... can't fix this because that's the common naming style policy in that file, sorry.

[chris fleizach]

Comment on attachment 62755 [details]
Patch proposal + unit tests

WebCore/accessibility/AccessibilityObject.cpp:380
 +  
you could do here
if (!textLength) {
   Node* node = node();
   if (node) {
      RenderText* renderText = toRenderText(node->renderer());
  }


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

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

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

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

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

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

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

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

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

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

[chris fleizach]

formatting of this review seems messed up. sorry bout that

(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());
>   }
> 
> 
> 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
> 
> WebCore/accessibility/AccessibilityObject.cpp:386
>  +          if (isWebArea() || isGroup() || isLink() || isHeading()) {
> this should be else if, no?
> 
> WebCore/accessibility/AccessibilityObject.cpp:389
>  +              textLength = textValue.length();
> can't you do textLength = textUnderElement().length();
> 
> 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
> 
> WebCore/accessibility/AccessibilityRenderObject.cpp:2414
>  +  #else
> you also check isWebArea in the other locations. do you not want to do it here?
> 
> WebCore/accessibility/AccessibilityObject.cpp:387
>  +              // Check composite objects just in case
> make sure to end comments with a period
> 
> WebCore/accessibility/AccessibilityRenderObject.cpp:2676
>  +      bool isTextObject =  isTextControl() || m_renderer->isText()
> too much white space
> 
> 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
> 
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1270
>  +          rangeLength = textLength;
> if the rangelength == 0 are you sure you want to reset to the text length?
> 
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1276
>  +  
> i don't see renderObject being used anywhere

[Mario Sanchez Prada]

Created attachment 63189 [details]
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

[WebKit Review Bot]

Attachment 63189 [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:1286:  webkit_accessible_text_get_character_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1295:  webkit_accessible_text_get_range_extents 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.

[Early Warning System Bot]

Attachment 63189 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3590797

[Mario Sanchez Prada]

Created attachment 63202 [details]
Patch proposal + unit tests

(In reply to comment #12)
> Attachment 63189 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/3590797

Attached new version of this patch fixing this issue.

[WebKit Review Bot]

Attachment 63202 [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:1286:  webkit_accessible_text_get_character_extents is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1295:  webkit_accessible_text_get_range_extents 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.

[chris fleizach]

Comment on attachment 63202 [details]
Patch proposal + unit tests

Thanks
r=me

[WebKit Commit Bot]

Comment on attachment 63202 [details]
Patch proposal + unit tests

Clearing flags on attachment: 63202

Committed r64475: <http://trac.webkit.org/changeset/64475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.