Bug 39997 - [Gtk] Make sure DRT return the right AXTitle for controls
Summary: [Gtk] Make sure DRT return the right AXTitle for controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 40009
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-01 07:47 PDT by Mario Sanchez Prada
Modified: 2010-08-04 12:00 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (6.44 KB, patch)
2010-06-01 08:18 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Proposed patch (5.00 KB, patch)
2010-06-01 11:11 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Proposed patch (4.93 KB, patch)
2010-06-29 09:15 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Proposed patch (newer version) (4.95 KB, patch)
2010-08-03 04:04 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2010-06-01 07:47:39 PDT
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.
Comment 1 Mario Sanchez Prada 2010-06-01 08:18:06 PDT
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,,,
Comment 2 Martin Robinson 2010-06-01 08:31:57 PDT
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.
Comment 3 Martin Robinson 2010-06-01 08:32:05 PDT
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.
Comment 4 Mario Sanchez Prada 2010-06-01 11:11:13 PDT
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
Comment 5 Xan Lopez 2010-06-29 05:06:39 PDT
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...
Comment 6 Xan Lopez 2010-06-29 05:11:20 PDT
Comment on attachment 57570 [details]
Proposed patch

Mario thinks this won't apply anymore to trunk, so removing cq+
Comment 7 Mario Sanchez Prada 2010-06-29 05:13:04 PDT
(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?
Comment 8 Mario Sanchez Prada 2010-06-29 09:15:28 PDT
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.
Comment 9 Eric Seidel (no email) 2010-06-30 03:15:12 PDT
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.
Comment 10 Xan Lopez 2010-08-02 14:16:09 PDT
Comment on attachment 60026 [details]
Proposed patch

OK.
Comment 11 Jeremy Orlow 2010-08-03 02:05:18 PDT
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.
Comment 12 Jeremy Orlow 2010-08-03 02:05:31 PDT
Sorry, meant to say commit queue.
Comment 13 Mario Sanchez Prada 2010-08-03 02:54:48 PDT
(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
Comment 14 Jeremy Orlow 2010-08-03 03:00:06 PDT
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.
Comment 15 Mario Sanchez Prada 2010-08-03 04:04:55 PDT
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
Comment 16 Jeremy Orlow 2010-08-03 04:09:46 PDT
Comment on attachment 63317 [details]
Proposed patch (newer version)

Rubber stamping per earlier r+
Comment 17 Eric Seidel (no email) 2010-08-03 08:37:48 PDT
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.
Comment 18 Eric Seidel (no email) 2010-08-03 18:49:22 PDT
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.
Comment 19 WebKit Commit Bot 2010-08-04 11:29:39 PDT
Comment on attachment 63317 [details]
Proposed patch (newer version)

Clearing flags on attachment: 63317

Committed r64659: <http://trac.webkit.org/changeset/64659>
Comment 20 WebKit Commit Bot 2010-08-04 11:29:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2010-08-04 12:00:52 PDT
http://trac.webkit.org/changeset/64659 might have broken GTK Linux 64-bit Debug