Bug 25677

Summary: [GTK] Implement support for get_character_extents and get_range_extents
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, cfleizach, commit-queue, mario, walker.willie, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 30883    
Bug Blocks: 25531    
Attachments:
Description Flags
work in progress (lacks test)
none
proposed fix, includes unit test
none
Patch proposal + unit tests
cfleizach: review-
Patch proposal + unit tests
none
Patch proposal + unit tests none

Joanmarie Diggs
Reported 2009-05-10 15:14:37 PDT
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).
Attachments
work in progress (lacks test) (6.72 KB, patch)
2009-12-06 22:34 PST, Joanmarie Diggs
no flags
proposed fix, includes unit test (12.78 KB, patch)
2009-12-27 14:35 PST, Joanmarie Diggs
no flags
Patch proposal + unit tests (16.80 KB, patch)
2010-07-27 15:21 PDT, Mario Sanchez Prada
cfleizach: review-
Patch proposal + unit tests (18.52 KB, patch)
2010-08-02 04:30 PDT, Mario Sanchez Prada
no flags
Patch proposal + unit tests (18.44 KB, patch)
2010-08-02 05:40 PDT, Mario Sanchez Prada
no flags
Joanmarie Diggs
Comment 1 2009-12-06 22:34:38 PST
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
Comment 2 2009-12-27 14:35:28 PST
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
Comment 3 2009-12-27 14:40:16 PST
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 4 2009-12-30 19:52:22 PST
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
Comment 5 2010-07-27 15:21:33 PDT
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
Comment 6 2010-07-27 15:24:11 PDT
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
Comment 7 2010-07-27 15:26:04 PDT
(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 8 2010-07-29 10:57:56 PDT
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
Comment 9 2010-07-29 10:58:33 PDT
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
Comment 10 2010-08-02 04:30:06 PDT
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
Comment 11 2010-08-02 04:32:23 PDT
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
Comment 12 2010-08-02 04:36:29 PDT
Mario Sanchez Prada
Comment 13 2010-08-02 05:40:51 PDT
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
Comment 14 2010-08-02 05:44:18 PDT
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 15 2010-08-02 10:00:05 PDT
Comment on attachment 63202 [details] Patch proposal + unit tests Thanks r=me
WebKit Commit Bot
Comment 16 2010-08-02 10:53:19 PDT
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
Comment 17 2010-08-02 10:53:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.