Bug 31018 – Accessibility: Implement isSelected in DRT for GTK

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

This will allow the aria-tab-controls.html layout test to pass

[Mario Sanchez Prada]

(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

[chris fleizach]

yes that is what i meant

[Mario Sanchez Prada]

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)

[WebKit Review Bot]

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.

[Mario Sanchez Prada]

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

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Mario Sanchez Prada]

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 :-)

[Xan Lopez]

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.

[Mario Sanchez Prada]

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.

[Xan Lopez]

Comment on attachment 55581 [details]
Patch to fix this bug

Looks good to me.

[WebKit Commit Bot]

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).

[Xan Lopez]

Comment on attachment 55581 [details]
Patch to fix this bug

I believe this should work now.

[WebKit Commit Bot]

Comment on attachment 55581 [details]
Patch to fix this bug

Clearing flags on attachment: 55581

Committed r59531: <http://trac.webkit.org/changeset/59531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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