Bug 31018

Summary: Accessibility: Implement isSelected in DRT for GTK
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: Joanmarie Diggs <jdiggs>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, jmalonzo, mario, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 38648    
Bug Blocks: 25531    
Attachments:
Description Flags
Patch to fix this bug
none
Patch to fix this bug
eric: review-, eric: commit-queue-
Patch to fix this bug
xan.lopez: review-
Patch to fix this bug none

Description chris fleizach 2009-11-02 10:31:00 PST
This will allow the aria-tab-controls.html layout test to pass
Comment 1 Mario Sanchez Prada 2010-05-06 09:45:07 PDT
(In reply to comment #0)
> This will allow the aria-tab-controls.html layout test to pass

Hmmm... do you mean accessibility/aria-controls-with-tabs.html?

Can't find aria-tab-controls.html anywhere
Comment 2 chris fleizach 2010-05-06 10:05:39 PDT
yes that is what i meant
Comment 3 Mario Sanchez Prada 2010-05-06 10:48:46 PDT
Created attachment 55266 [details]
Patch to fix this bug

(In reply to comment #2)
> yes that is what i meant

Ok, attaching then a patch that would fix the issue.

Please notice that fixing blocker bug 38648 first is mandatory for this to work (otherwise, the isSelected implementation would be always returning false, as m_element wouldn't be a valid AtkObject)
Comment 4 WebKit Review Bot 2010-05-06 10:49:40 PDT
Attachment 55266 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:367:  Declaration has space between type name and * in AtkStateSet *stateSet  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:373:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:374:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:375:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mario Sanchez Prada 2010-05-06 10:57:57 PDT
Created attachment 55268 [details]
Patch to fix this bug

(In reply to comment #4)
> Attachment 55266 [details] did not pass style-queue:
> 

Sorry, forgot to check style. Uploading new patch
Comment 6 Eric Seidel (no email) 2010-05-09 15:08:59 PDT
Comment on attachment 55268 [details]
Patch to fix this bug

LGTM.

Don't we have some sort of GRefPtr so we don't have to manually call g_object_unref?
Comment 7 Eric Seidel (no email) 2010-05-09 15:09:37 PDT
Comment on attachment 55268 [details]
Patch to fix this bug

Actually, we don't do c-style forward declarations of variables.  Please declare them inline.
Comment 8 Mario Sanchez Prada 2010-05-10 01:12:59 PDT
Created attachment 55526 [details]
Patch to fix this bug

(In reply to comment #7)
> (From update of attachment 55268 [details])
> Actually, we don't do c-style forward declarations of variables.  Please declare them inline.

Done.

(In reply to comment #6)
> (From update of attachment 55268 [details])
> LGTM.
> 
> Don't we have some sort of GRefPtr so we don't have to manually call g_object_unref?

I'm not sure that would be aplicable on here, since the extra ref comes from ATK lib, but not a strong opinion anyway. Glad to hear suggestions :-)
Comment 9 Xan Lopez 2010-05-10 10:08:41 PDT
Comment on attachment 55526 [details]
Patch to fix this bug

re GRefPtr, it does not really matter that you are not adding the ref yourself, what GRefPtr does is get rid of the reference when the object goes out of scope. You'd use it like:

GRefPtr<AtkStateSet> stateSet = atk_object_ref_state_set() ...

and then you can get rid of the g_object_unref.

I think it makes sense to use it, so r- because of that. Also, you can probably add the bug URL and title to the ChangeLog in the layout tests.
Comment 10 Mario Sanchez Prada 2010-05-10 11:38:25 PDT
Created attachment 55581 [details]
Patch to fix this bug

(In reply to comment #9)
> (From update of attachment 55526 [details])
> re GRefPtr, it does not really matter that you are not adding the ref yourself, what GRefPtr does is get rid of the reference when the object goes out of scope. You'd use it like:
> 
> GRefPtr<AtkStateSet> stateSet = atk_object_ref_state_set() ...
> 
> and then you can get rid of the g_object_unref.
> 
> I think it makes sense to use it, so r- because of that. Also, you can probably add the bug URL and title to the ChangeLog in the layout tests.

Done.
Comment 11 Xan Lopez 2010-05-10 11:48:27 PDT
Comment on attachment 55581 [details]
Patch to fix this bug

Looks good to me.
Comment 12 WebKit Commit Bot 2010-05-12 05:08:53 PDT
Comment on attachment 55581 [details]
Patch to fix this bug

Rejecting patch 55581 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55581, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
5581&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=31018&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55581 from bug 31018.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 13 Xan Lopez 2010-05-14 11:44:36 PDT
Comment on attachment 55581 [details]
Patch to fix this bug

I believe this should work now.
Comment 14 WebKit Commit Bot 2010-05-15 02:44:34 PDT
Comment on attachment 55581 [details]
Patch to fix this bug

Clearing flags on attachment: 55581

Committed r59531: <http://trac.webkit.org/changeset/59531>
Comment 15 WebKit Commit Bot 2010-05-15 02:44:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2010-05-15 04:09:26 PDT
http://trac.webkit.org/changeset/59531 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59530
http://trac.webkit.org/changeset/59531
http://trac.webkit.org/changeset/59532
http://trac.webkit.org/changeset/59533