Bug 132527 – AX: [ATK] [PATCH] add text-caret-moved signal to accessibilityController

• Component: Accessibility
• Status: RESOLVED FIXED

[Jarek Czekalski]

Mario's suggestions (in "[webkit-gtk] testing text-caret-moved events" thread) [1] made it easy to prepare the patch.

This patch could potentially break tests which use logAccessibilityEvents feature, but all 2 tests using it in gtk are anyway marked with Failure. Maybe this gets explained in thread "[webkit-dev] why these tests are still marked Failure? r148002" [2].

There are several possibilities to show the signal for logAccessibilityEvents() mode. I chose the way similar to what was in testatk.c [3], line 87. It displays for example:
Accessibility object emitted "text-caret-moved = paragraph|4" / Name: "(No name)" / Role: 71

[1] https://lists.webkit.org/pipermail/webkit-gtk/2014-May/001888.html
[2] https://lists.webkit.org/pipermail/webkit-dev/2014-May/026509.html
[3] https://trac.webkit.org/browser/releases/WebKitGTK/webkit-2.4.1/Tools/TestWebKitAPI/Tests/WebKitGtk/testatk.c

[Radar WebKit Bug Importer]

<rdar://problem/16805879>

[Jarek Czekalski]

Created attachment 230759 [details]
the patch, version 1.00

[Mario Sanchez Prada]

Comment on attachment 230759 [details]
the patch, version 1.00

View in context: https://bugs.webkit.org/attachment.cgi?id=230759&action=review

Thanks for the patch. The idea looks good to me, although I think it would be nice if we could include a new layout test using this new feature together with the patch (e.g. migrating one of the unit tests in the old testatk.c file), to confirm it works and have it properly justified.

Also, now that the WebKitGTK+ port removed the WebKit1 API from trunk, I'm not sure whether it makes sense to keep mirroring the changes from WKTR here.

The only reason would be the EFL port, but I think they pretty much use WebKit2 only these days (yet I'm not sure). It would be nice if someone from EFL could answer that question (e.g. Krzysztof?)

See my comments below (setting r- for now)...

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:93
> +    AtkRole objectRole = atk_object_get_role(accessible);
> +    const gchar* roleName = atk_role_get_name(objectRole);

Why do you need the role and the name of the object here? This variables are used only to set the value of signalValue, which will be used only to pass it to printAccessibilityEvent(), where the role will be retrieved and printed anyway

(Same comment apply to DRT implementation)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:122
> +        signalValue.reset(g_strdup_printf("%s|%d",
> +            roleName, g_value_get_int(&paramValues[1])));

you don't need to emit the roleName explicitly here, just the value for the offset (the "%d")

(Same comment apply to DRT implementation)

[Mario Sanchez Prada]

(In reply to comment #3)
> [...]
> Also, now that the WebKitGTK+ port removed the WebKit1 API from trunk, I'm 
> not sure whether it makes sense to keep mirroring the changes from WKTR here.
> 
> The only reason would be the EFL port, but I think they pretty much use 
> WebKit2 only these days (yet I'm not sure). It would be nice if someone
> from EFL could answer that question (e.g. Krzysztof?)

Krzysztof, would you mind commenting on this?

Thanks!

[Jarek Czekalski]

Hi Mario,

Thank you for the review.

(In reply to comment #3)
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:93
> > +    AtkRole objectRole = atk_object_get_role(accessible);
> > +    const gchar* roleName = atk_role_get_name(objectRole);
> 
> Why do you need the role and the name of the object here? This variables are used only to set the value of signalValue, which will be used only to pass it to printAccessibilityEvent(), where the role will be retrieved and printed anyway

Good catch. My intention was to pass signalValue as the 3rd parameter to event listener. But there is no 3rd parameter, I missed it.

In testatk.c we verify not only whether the signal was delivered, but if the reported offset is correct as well. I will try to provide a modification allowing to pass additional parameter to the handler.

[Krzysztof Czech]

(In reply to comment #4)
> (In reply to comment #3)
> > [...]
> > Also, now that the WebKitGTK+ port removed the WebKit1 API from trunk, I'm 
> > not sure whether it makes sense to keep mirroring the changes from WKTR here.
> > 
> > The only reason would be the EFL port, but I think they pretty much use 
> > WebKit2 only these days (yet I'm not sure). It would be nice if someone
> > from EFL could answer that question (e.g. Krzysztof?)
> 
> Krzysztof, would you mind commenting on this?
> 
> Thanks!

I agree, we do not have to keep mirroring WKTR - DRT in terms of accessibility and I guess other things as well, but I'm rather far from decide that we definitely should stop doing this.

Regarding accessibility, we concentrate on WebKit2.

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
> I agree, we do not have to keep mirroring WKTR - DRT in terms of accessibility 
> and I guess other things as well, but I'm rather far from decide that we 
> definitely should stop doing this.
> 
> Regarding accessibility, we concentrate on WebKit2.

Then I think we should focus on WKTR and forget about DRT from now on for ATK accessibility related purposes, at least on trunk.

[Jarek Czekalski]

Created attachment 231399 [details]
caret-moved signal and caret offset tests

Hi Mario. This is bigger, as you suggested. Thanks in advance for your review!

[Mario Sanchez Prada]

(In reply to comment #8)
> Created an attachment (id=231399) [details]
> caret-moved signal and caret offset tests
> 
> Hi Mario. This is bigger, as you suggested. Thanks in advance for your review!

Thanks for the patch Jarek, it definitely looks like a better approach after a quick glance over it!

Still, before I do a proper review, would you mind rebasing your patch against trunk and uploading it again? Asking that because the patch does not currently apply on trunk, which is preventing the style and EWS bots from running, which would be helpful if they did.

Once you do that, I promise I'll find time during this crazy week to review it properly :)

Thanks again!

PS: I've tried to apply it locally to make sure it did not apply and this is the rejection that I got:

--- Source/WebCore/page/ViewportConfiguration.cpp       (revision 0)
+++ Source/WebCore/page/ViewportConfiguration.cpp       (working copy)
@@ -26,8 +26,10 @@
 #include "config.h"
 #include "ViewportConfiguration.h"

+#include <WebCore/TextStream.h>
 #include <wtf/Assertions.h>
 #include <wtf/MathExtras.h>
+#include <wtf/text/CString.h>

 #if PLATFORM(IOS)
 #include "WebCoreSystemInterface.h"

[Jarek Czekalski]

Created attachment 231455 [details]
caret-moved signal and caret offset tests

Mario,

Sorry for posting an outdated patch. I thought I was in non-changeable area. But AccessibilityUIElement.idl file is alive and it produced the conflict. Now it's clear. My box will finish build tomorrow, but I submit now to let bots check other ports, before you start working on that.

[Mario Sanchez Prada]

Comment on attachment 231455 [details]
caret-moved signal and caret offset tests

View in context: https://bugs.webkit.org/attachment.cgi?id=231455&action=review

Sorry for the delay, Jarek. This week is being crazy here, but I'd rather review this now because next week will be even more complex :)

This patch is awesome, I'm placing the r- flag now because there are some things that need fixing, but it seems to be indeed in the right direction. See my comments below...

> LayoutTests/ChangeLog:8
> +        * platform/gtk/accessibility/js-test-atk.js: Added, to place

I think it's ok to have a new file to place this helpers (and probably others that are used in different ATK tests) in a common place, but I'd rather put it in a platform/gtk/resources directory, for consistency with what's done everywhere else. Also (and sorry for the bike shedding), I'd rather name it something like atk-helpers.js or the like, not need to use a "js-" prefix in a .js file :)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:5
> +<script src="js-test-atk.js"></script>

Remember to update this path if you do the change suggested above

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:9
> +<script>
> +descriptionText = "This test is replicated from old file testatk.c and tests various scenarios<br>" +
> +                  "of caret movement: setting caret offset and receiving text-caret-moved signal.";
> +</script>

We normally use the description() JS function for this purpose (see for instance accessibility/button-title-uses-inner-img-alt.html)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:44
> +if (window.layoutController) {

s/layoutController/testRunner (layoutController was the old name of the testRunner)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:47
> +if (window.layoutController) {
> +    testRunner.overridePreference("WebKitEnableCaretBrowsing", true);
> +    testRunner.dumpAsText();
> +}

Could you move this "if { ... }" block after the functions? (to keep the functions at the beginning)?

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:53
> +        caretMovedData += role + '|' + offset;

You are using caretMovedData as a global variable, so you should declare it with "var caretMovedData" outside every block and every function (not needed but good practice)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:57
> +function setCaretOffsetHelper(object, offset)

Perhaps naming this 'resetCaretOffsetForObject()' would be clearer

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:81
> +    /* Following tests are replicated from testatk.c file, functions
> +    testWebkitAtkCaretOffsets()
> +    testWebkitAtkCaretOffsetsAndExtranousWhiteSpaces()
> +    that was present in webkit1. Removed from
> +    trunk in r166977, so the last version was
> +    https://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKitGtk/testatk.c?rev=166976
> +    What could not be verified in the same way:
> +    1. Whether element implements ATK_TEXT. If setCaretOffset succeeds, it confirms
> +       that element is text as well. But in cases when setCaretOffset must fail
> +       I see no way to confirm it is text element.
> +    2. Whether position is correct through a call to atk_text_get_caret_offset.
> +       text-caret-moved signal value is tested instead.
> +    Anyway these cases don't look crucial, so they are skipped.
> +    */

I think this is a great comment to put in the LayoutTests/ChangeLog, but I'm not sure you need to put it here because, as you mentioned, those cases are not crucial anyway :)

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:62
> +// Up to 2014 it was obligatory to mirror the changes from
> +// WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp,
> +// but the habit has been dropped: https://bugs.webkit.org/show_bug.cgi?id=132527#c6

Again, I think this is a nice comment for the ChangeLog, but I wouldn't put it here since the reason is quite simple anyway: The WebKit1 versions of the GTK and EFL ports (the only ones using ATK) are no longer maintained upstream

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:261
> -    
> +

Don't include this change (it's not related)

> Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl:-203
> -    

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:43
> +WTF::Vector<guint> listenerIds;

Loved this change, but please keep the unsigned type instead of guint (we only use GLib types for public interfaces)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:143
> +        int lastArgument = 1;
> +        for (int i = 0; i < extraArgs.size(); i++)
> +            arguments[lastArgument += 1] = extraArgs[i];

Instead of doing this to keep the value of lastArgument outside the loop and use it later, you could use a while loop with only one iterator (e.g. "currentArgument") and then use it later to know the last argument position (e.g. currentArgument+1).

If, however, you still prefer to use the for loop, please split that statement in too so you don't use the 'lastArgument += 1' as the index for the array (that's not clear)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:-139
> -

I think you can keep this line :)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:269
> +    for (const char** signalName = signalNames; *signalName; signalName++) {

Thanks for doing this refactoring :)

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:273
> +            String message = String::format("atk_add_global_event_listener failed for signal %s\n",
> +                *signalName);

One line only

[Jarek Czekalski]

Hi Mario. Thank you for your comments. I have only one thing to discuss before I proceed with applying your suggestions. 

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:62
>> +// Up to 2014 it was obligatory to mirror the changes from
>> +// WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp,
>> +// but the habit has been dropped: https://bugs.webkit.org/show_bug.cgi?id=132527#c6

>Again, I think this is a nice comment for the ChangeLog, but I wouldn't put it >here since the reason is quite simple anyway: The WebKit1 versions of the GTK >and EFL ports (the only ones using ATK) are no longer maintained upstream

Someone may notice similar things in these 2 places and I would like to help him by leaving a notice here. Now the files will differ significantly, but the argument still holds. And if we remove this comment, we will have no reference from this file's history (svn log) to the comment in Changelog.

Have a nice weekend!

[Jarek Czekalski]

Comment on attachment 231455 [details]
caret-moved signal and caret offset tests

View in context: https://bugs.webkit.org/attachment.cgi?id=231455&action=review

Mario, except the thing I put yesterday there is only one more item that was not fully done along your suggestions. The rest of the hints is applied thoroughly.

Are there any additional changes needed to land it in 2.4? Because all I do is to fix the bugs in the branch containing webkit1.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:143
>> +            arguments[lastArgument += 1] = extraArgs[i];
> 
> Instead of doing this to keep the value of lastArgument outside the loop and use it later, you could use a while loop with only one iterator (e.g. "currentArgument") and then use it later to know the last argument position (e.g. currentArgument+1).
> 
> If, however, you still prefer to use the for loop, please split that statement in too so you don't use the 'lastArgument += 1' as the index for the array (that's not clear)

I find it clearer with 'i', so I only did the splitting.

[Jarek Czekalski]

Created attachment 231629 [details]
 caret-moved signal and caret offset tests v2.04

[Mario Sanchez Prada]

Comment on attachment 231629 [details]
 caret-moved signal and caret offset tests v2.04

View in context: https://bugs.webkit.org/attachment.cgi?id=231629&action=review

Hi Jarek, apologies for the delayed response, but last week I've been quite busy (and mostly offline) most of the time and could not devote time to this at all. Now I'm back, and reviewing your patch again, which I think it's almost there. I just have a couple of comments/suggestions more.

See them below...

> LayoutTests/ChangeLog:29
> +        * platform/gtk/accessibility/caret-offsets.html: Added.

Sorry I haven't realized about this before, but I think that this file is actually too big (as it includes tests from both testWebkitAtkCaretOffsets() and testWebkitAtkCaretOffsetsAndExtraneousWhiteSpaces()). Could you better split it into two different .html files? Something like this: 

  - platform/gtk/accessibility/caret-offsets.html
  - platform/gtk/accessibility/caret-offsets-and-extraneous-white-spaces.html

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:147
>              // Listener for one element just gets one argument, the notification name.

This comment is obsolete now

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:150
> +                0, lastArgument + 1 - 1, arguments + 1, 0);

lastArgument + 1 - 1? That looks like a strange idiom, even if it's correct. I personally think something like this would be less cryptic:

 [...]

 size_t numOfExtraArgs = extraArgs.size();
 for (int i = 0; i < numOfExtraArgs; i++)
     arguments[i + 2] = extraArgs[i];

 if (elementNotificationHandler != notificationHandlers.end()) {
     // Listener for one element. As arguments, it gets the notification name
     // plus any extra argument that must be needed to pass to the callback
     JSObjectCallAsFunction(jsContext,
         const_cast<JSObjectRef>(elementNotificationHandler->value->notificationFunctionCallback()),
         0, numOfExtraArgs + 1, arguments + 1, 0);

 [...]


What do you think?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:157
> +                0, lastArgument + 1, arguments, 0);

If you agree with the proposed change above, then you'd need to use numberOfExtraArgs + 2 here

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:273
> +        guint id = atk_add_global_event_listener(axObjectEventListener, *signalName);

Use non-glib types for basic data (e.g. unsigned instead of guint) for this kind of variables in private files

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:278
> +        if (!id) {
> +            String message = String::format("atk_add_global_event_listener failed for signal %s\n", *signalName);
> +            InjectedBundle::shared().outputText(message);
> +        } else
> +            listenerIds.append(id);

Could you make this just an "if" with an "early continue"?

  for (...) {
   [...]
   if (!id) {
       String message = String::format("atk_add_global_event_listener failed for signal %s\n", *signalName);
       InjectedBundle::shared().outputText(message);
       continue;
   }

   listenerIds.append(id);
  }

[Jarek Czekalski]

Comment on attachment 231629 [details]
 caret-moved signal and caret offset tests v2.04

View in context: https://bugs.webkit.org/attachment.cgi?id=231629&action=review

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:150
>> +                0, lastArgument + 1 - 1, arguments + 1, 0);
> 
> lastArgument + 1 - 1? That looks like a strange idiom, even if it's correct. I personally think something like this would be less cryptic:
> 
>  [...]
> 
>  size_t numOfExtraArgs = extraArgs.size();
>  for (int i = 0; i < numOfExtraArgs; i++)
>      arguments[i + 2] = extraArgs[i];
> 
>  if (elementNotificationHandler != notificationHandlers.end()) {
>      // Listener for one element. As arguments, it gets the notification name
>      // plus any extra argument that must be needed to pass to the callback
>      JSObjectCallAsFunction(jsContext,
>          const_cast<JSObjectRef>(elementNotificationHandler->value->notificationFunctionCallback()),
>          0, numOfExtraArgs + 1, arguments + 1, 0);
> 
>  [...]
> 
> 
> What do you think?

I think this is a good change. The comments were outdated indeed. Have a look at them once more before committing, because I rewrote them.

[Jarek Czekalski]

Created attachment 232251 [details]
caret-moved signal and caret offset tests v2.05

Hi Mario. No problem with the delay. I'm attaching another version, incorporating your suggestions.

[Mario Sanchez Prada]

Comment on attachment 232251 [details]
caret-moved signal and caret offset tests v2.05

View in context: https://bugs.webkit.org/attachment.cgi?id=232251&action=review

Thanks for the new patch, it's almost there. Only a minor suggestion (in the ChangeLog, sorry!) and I think we are ready to give this a try.

(In reply to comment #16)
> (From update of attachment 231629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231629&action=review
> [...]
> The comments were outdated indeed. Have a look at them once more before committing, because I rewrote them.

I think the new comments are fine

> LayoutTests/ChangeLog:30
> +        * platform/gtk/accessibility/caret-offsets.html: Added.
> +        * platform/gtk/accessibility/caret-offsets-expected.txt: Added.

You forgot to mention the new test caret-offsets-and-extraneous-white-spaces.html here. But that's the only thing I see in this patch at the moment so iy fyou could update this changelog and submit a new patch just with that change, I'll r+ and cq+ rightaway.

By the way, thanks a lot for splitting the original test in two anyway.

[Jarek Czekalski]

Created attachment 232286 [details]
caret-moved signal and caret offset tests v2.06

Mario, thank you for immediate and positive review!

How about pushing this to 2.4 branch?

[Mario Sanchez Prada]

Comment on attachment 232286 [details]
caret-moved signal and caret offset tests v2.06

(In reply to comment #19)
> Created an attachment (id=232286) [details]
> caret-moved signal and caret offset tests v2.06
> 
> Mario, thank you for immediate and positive review!

No problem. Thanks for the quick response
 
> How about pushing this to 2.4 branch?

Normally, only critical bugfixes are merged into the stable branch, and this changeset is related to improving test coverage (and testing tools!), so I don't think we should port it there.

[WebKit Commit Bot]

Comment on attachment 232286 [details]
caret-moved signal and caret offset tests v2.06

Clearing flags on attachment: 232286

Committed r169483: <http://trac.webkit.org/changeset/169483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jarek Czekalski]

(In reply to comment #20)

> > How about pushing this to 2.4 branch?
> 
> Normally, only critical bugfixes are merged into the stable branch, and this changeset is related to improving test coverage (and testing tools!), so I don't think we should port it there.

Accessibility fixes will contain tests using this patch. How are we going to apply these fixes to 2.4, if we don't apply this patch?

[Mario Sanchez Prada]

(In reply to comment #23)
> (In reply to comment #20)
> 
> > > How about pushing this to 2.4 branch?
> > 
> > Normally, only critical bugfixes are merged into the stable branch, and this changeset is related to improving test coverage (and testing tools!), so I don't think we should port it there.
> 
> Accessibility fixes will contain tests using this patch. How are we going to apply these fixes to 2.4, if we don't apply this patch?

I see your point, but still this patch is not a bugfix per se, so I don't see why we should request to merge it into the 2.4 stable branch *now*.

It would be different if we had a bugfix that we wanted to merge in 2.4. If that's the case, and such a bugfix depends on this other patch, then we could request the inclusion of both the two patches (assuming they apply cleanly on top).

For merge requests in the 2.4 branch, see this wiki page:
http://trac.webkit.org/wiki/WebKitGTK/2.4.x

[Jarek Czekalski]

(In reply to comment #24)
>It would be different if we had a bugfix that we wanted to merge in 2.4.

Mario, could we make it clear: do we want the fixes for bug #130941 and bug #132349 (not yet ready) to be merged in 2.4?

[Mario Sanchez Prada]

(In reply to comment #25)
> (In reply to comment #24)
> >It would be different if we had a bugfix that we wanted to merge in 2.4.
> 
> Mario, could we make it clear: do we want the fixes for bug #130941 and bug #132349 (not yet ready) to be merged in 2.4?

I'd say "yes", but we need to make sure that the proposed list of patches apply cleanly on top of that branch.

[Mario Sanchez Prada]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > >It would be different if we had a bugfix that we wanted to merge in 2.4.
> > 
> > Mario, could we make it clear: do we want the fixes for bug #130941 and bug #132349 (not yet ready) to be merged in 2.4?
> 
> I'd say "yes", but we need to make sure that the proposed list of patches apply cleanly on top of that branch.

Damm... I just found out that I missed two things while reviewing this patch, that caused two tests to get broken:

  accessibility/notification-listeners.html [ Failure ]
  accessibility/textarea-selected-text-range.html [ Failure ]

The first test is broken because of this:

+    const char* signalNames[] = {
+        "ATK:AtkObject:state-change",
+        "ATK:AtkObject:state-change",  // DUPLICATED
+        "ATK:AtkObject:focus-event",
+        "ATK:AtkObject:active-descendant-changed",
+        "ATK:AtkObject:children-changed",
+        "ATK:AtkObject:property-change",
+        "ATK:AtkObject:visible-data-changed",
+        "ATK:AtkDocument:load-complete",
+        "ATK:AtkText:text-caret-moved",
+        0
+    };

The second test is broken because of this:

-void AccessibilityUIElement::setSelectedTextRange(unsigned location, unsigned length)
+bool AccessibilityUIElement::setSelectedTextRange(unsigned location, unsigned length)
 {
     if (!ATK_IS_TEXT(m_element.get()))
-        return;
+        return false;
+
+    if (length)
+        return atk_text_set_caret_offset(ATK_TEXT(m_element.get()), location);
 
-    atk_text_set_selection(ATK_TEXT(m_element.get()), 0, location, location + length);
+    return atk_text_set_selection(ATK_TEXT(m_element.get()), 0, location, location + length);
 }


It should be "if (!length)" not, "if (length)". I'll propose a quick patch now...

[Mario Sanchez Prada]

FWIW, the following patch fixes the wrong bits:
http://pastebin.com/ejzW7viS

I've asked for a "rubber stamp" in #webkitgtk+ (as it's a simple thing) but if we don't get it approved today I'm afraid the best thing to do would be to roll out the original patch and provide a new one with those bits corrected.

Still, the accessibility/textarea-selected-text-range.html test is giving a different output, but I believe that's because we are now using atk_text_set_caret_offset() for zero-length selections and so in that case a rebaseline would probably be fine.

[Mario Sanchez Prada]

(In reply to comment #28)
> FWIW, the following patch fixes the wrong bits:
> http://pastebin.com/ejzW7viS
> 
> I've asked for a "rubber stamp" in #webkitgtk+ (as it's a simple thing) but if
> we don't get it approved today I'm afraid the best thing to do would be to roll 
> out the original patch and provide a new one with those bits corrected.

Rubber stamped patch landed here:
http://trac.webkit.org/changeset/169485

[Jarek Czekalski]

(In reply to comment #27)
> Damm... I just found out that I missed two things while reviewing this patch, that caused two tests to get broken:
> 
>   accessibility/notification-listeners.html [ Failure ]
>   accessibility/textarea-selected-text-range.html [ Failure ]
> [...]
> The second test is broken because of this:
> [...]
> +    if (length)
> +        return atk_text_set_caret_offset(ATK_TEXT(m_element.get()), 
[...]
> It should be "if (!length)" not, "if (length)". I'll propose a quick patch now...

I did that mistake during style fix. length == 0 would be more readable here (as in 2.00), but such expressions are banned. It's funny that it didn't affect my tests at all.

My fault, I'm sorry. But please note that I was looking for a way to perform all tests: https://bugs.webkit.org/show_bug.cgi?id=131373#c3

[Jarek Czekalski]

(In reply to comment #26)
> > Mario, could we make it clear: do we want the fixes for bug #130941 and bug #132349 (not yet ready) to be merged in 2.4?
> 
> I'd say "yes", but we need to make sure that the proposed list of patches apply cleanly on top of that branch.

Do I need to rebase those patches for 2.4? This patch does not apply cleanly. There were changes in Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl

[Mario Sanchez Prada]

(In reply to comment #30)
> (In reply to comment #27)
> > Damm... I just found out that I missed two things while reviewing this patch, that caused two tests to get broken:
> > 
> >   accessibility/notification-listeners.html [ Failure ]
> >   accessibility/textarea-selected-text-range.html [ Failure ]
> > [...]
> > The second test is broken because of this:
> > [...]
> > +    if (length)
> > +        return atk_text_set_caret_offset(ATK_TEXT(m_element.get()), 
> [...]
> > It should be "if (!length)" not, "if (length)". I'll propose a quick patch now...
> 
> I did that mistake during style fix. length == 0 would be more readable here (as in 2.00), but such expressions are banned. It's funny that it didn't affect my tests at all.
> 
> My fault, I'm sorry. But please note that I was looking for a way to perform all tests: https://bugs.webkit.org/show_bug.cgi?id=131373#c3

No problem, it's fixed now. Only thing is that we need to remember to ask for merging that follow up patch when asking for integrating this into the stable branch.

[Mario Sanchez Prada]

(In reply to comment #31)
> (In reply to comment #26)
> > > Mario, could we make it clear: do we want the fixes for bug #130941 and bug #132349 (not yet ready) to be merged in 2.4?
> > 
> > I'd say "yes", but we need to make sure that the proposed list of patches apply cleanly on top of that branch.
> 
> Do I need to rebase those patches for 2.4? This patch does not apply cleanly.
> There were changes in Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl

If it's a simply rebase, I think Carlos can do it while merging, as he's the one usually taking care of this tasks.

I'm adding him to CC now in case he wants to comment on this, although as I said, I would not propose to merge this patch before having the bugfixes depending on it ready.

[Jarek Czekalski]

(In reply to comment #29)
> Rubber stamped patch landed here:
> http://trac.webkit.org/changeset/169485

A note for merge time: the log message header in this revision comes from a different issue, it could be a mistake.

[Mario Sanchez Prada]

(In reply to comment #34)
> (In reply to comment #29)
> > Rubber stamped patch landed here:
> > http://trac.webkit.org/changeset/169485
> 
> A note for merge time: the log message header in this revision comes from a different issue, it could be a mistake.

Indeed it was a mistake, my mistake actually :/