Summary: | [GTK] Implement support for get_character_extents and get_range_extents | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||||||
Component: | Accessibility | Assignee: | 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
Joanmarie Diggs
2009-05-10 15:14:37 PDT
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!
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!
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
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. 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!
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.
(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. 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
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 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 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.
Attachment 63189 [details] did not build on qt: Build output: http://queues.webkit.org/results/3590797 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. 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.
Comment on attachment 63202 [details]
Patch proposal + unit tests
Thanks
r=me
Comment on attachment 63202 [details] Patch proposal + unit tests Clearing flags on attachment: 63202 Committed r64475: <http://trac.webkit.org/changeset/64475> All reviewed patches have been landed. Closing bug. |