Bug 39997 – [Gtk] Make sure DRT return the right AXTitle for controls

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

The accessibility/aria-checkbox-text.html test is failing for the GTK port, both because it has no *-expected file and because the required implementation to get the title from WebKitGtk through the DRT is buggy. We'd need to fix those issues to remove it from the Skipped file then.

[Mario Sanchez Prada]

Created attachment 57544 [details]
Proposed patch

Here's a patch that would solve this issue. I've tested everything with and this patch and the only difference is that one more test gets fixed, so I'd say it's right :-)

By the way, perhaps we should think about being more coherent and, from now on, to always use this kind of "AXWhatever :" prefixes in the GTK's DRT, in the same way that mac port does it. But that would be something for another bug,,,

[Martin Robinson]

Comment on attachment 57544 [details]
Proposed patch


> -    return JSStringCreateWithUTF8CString(name);
> +    gchar* title = g_strdup_printf("AXTitle: %s", name);
> +    JSStringRef jsTitle = JSStringCreateWithUTF8CString(title);
> +
> +    g_free(title);
> +    return jsTitle;

You can use GOwnptr here for title.

[Martin Robinson]

Comment on attachment 57544 [details]
Proposed patch


> -    return JSStringCreateWithUTF8CString(name);
> +    gchar* title = g_strdup_printf("AXTitle: %s", name);
> +    JSStringRef jsTitle = JSStringCreateWithUTF8CString(title);
> +
> +    g_free(title);
> +    return jsTitle;

You can use GOwnptr here for title.

[Mario Sanchez Prada]

Created attachment 57570 [details]
Proposed patch

(In reply to comment #3)
> (From update of attachment 57544 [details])
> 
> > -    return JSStringCreateWithUTF8CString(name);
> > +    gchar* title = g_strdup_printf("AXTitle: %s", name);
> > +    JSStringRef jsTitle = JSStringCreateWithUTF8CString(title);
> > +
> > +    g_free(title);
> > +    return jsTitle;
> 
> You can use GOwnptr here for title.

Thanks Martin for your comments, which have been addressed in the patch proposed for bug 40009 as at the end I thought it would be better to follow a different approach with this problem and split it in two parts:

 1. Make GTK's DRT more coherent with other ports (bug 40009)
 2. Actually make sure the AXTitle for controls is properly calculated and returned, to unskip the proper test

So here we are the patch for (2), patch for (1) already attached along with bug 40009

[Xan Lopez]

Comment on attachment 57570 [details]
Proposed patch

Looks good. It would make sense to try to just have one expected file now that both ports have identical results...

[Xan Lopez]

Comment on attachment 57570 [details]
Proposed patch

Mario thinks this won't apply anymore to trunk, so removing cq+

[Mario Sanchez Prada]

(In reply to comment #6)
> (From update of attachment 57570 [details])
> Mario thinks this won't apply anymore to trunk, so removing cq+

Yeah, I need to recheck this, and maybe come with an up-to-date patch since it's possible the modifications in AccessibilityobjectWrapperAtk.cpp are no longer needed after fixing bug 36128.

PS: Shouldn't you remove the review+`flag as well, btw?

[Mario Sanchez Prada]

Created attachment 60026 [details]
Proposed patch

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 57570 [details] [details])
> > Mario thinks this won't apply anymore to trunk, so removing cq+
> 
> Yeah, I need to recheck this, and maybe come with an up-to-date patch since
> it's possible the modifications in AccessibilityobjectWrapperAtk.cpp are no
> longer needed after fixing bug 36128.

Indeed, the previous patch wouldn't apply so this new one makes a lot of sense :-). In the other hand, I was wrong in my guessing and no code can be removed just because of bug 36128, so the new patch is basically the same one than before, but properly applying against latest trunk.

[Eric Seidel (no email)]

Comment on attachment 57570 [details]
Proposed patch

Cleared Xan Lopez's review+ from obsolete attachment 57570 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Xan Lopez]

Comment on attachment 60026 [details]
Proposed patch

OK.

[Jeremy Orlow]

Comment on attachment 60026 [details]
Proposed patch

The review queue has been stuck on this patch for some time.  I suspect it just needs to be landed by hand, though I'm not sure why the queue keeps spinning rather than explicitly rejecting it.

[Jeremy Orlow]

Sorry, meant to say commit queue.

[Mario Sanchez Prada]

(In reply to comment #11)
> (From update of attachment 60026 [details])
> The commit queue has been stuck on this patch for some time.  I suspect it just 
> needs to be landed by hand, though I'm not sure why the queue keeps spinning 
> rather than explicitly rejecting it.

Perhaps it's a matter of the patch being too old and not applying correctly?

If so, I can provide a new patch rebased against master

[Jeremy Orlow]

It's worth a shot.  Normally the queue outright rejects it in such a case, but I suspect this was hitting on some bug in the queue.

[Mario Sanchez Prada]

Created attachment 63317 [details]
Proposed patch (newer version)

(In reply to comment #14)
> It's worth a shot.  Normally the queue outright rejects it in such a case, but I suspect this was hitting on some bug in the queue.

Attached

[Jeremy Orlow]

Comment on attachment 63317 [details]
Proposed patch (newer version)

Rubber stamping per earlier r+

[Eric Seidel (no email)]

No, the CQ is stuck because the leopard build is broken. :(  We need to update the "unable to build and test" message to be more specific.

[Eric Seidel (no email)]

Comment on attachment 60026 [details]
Proposed patch

Cleared Xan Lopez's review+ from obsolete attachment 60026 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 63317 [details]
Proposed patch (newer version)

Clearing flags on attachment: 63317

Committed r64659: <http://trac.webkit.org/changeset/64659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64659 might have broken GTK Linux 64-bit Debug