GNOME Bugzilla – Bug 725864
Add GtkPopover a11y
Last modified: 2014-03-10 22:13:43 UTC
I'm attaching a few patches that add minimal a11y support for popovers. From what I learned today in #a11y, Orca would do fine with popovers by just figuring out the relationship, even if the atkobject hierarchy doesn't strictly match, so the patches let the popovers' AtkObject be a child of the toplevel's, but set the POPUP_FOR relationship with the relative-to accessible. This turned out to be a lot less intricate than faking a hierarchy between popovers and their relative-to widget.
Created attachment 271154 [details] [review] window: Add private _gtk_window_list_popovers() This function will be useful in situations in GTK+ internals. https://bugzilla.gnome.org/show_bug.cgi?id=725727
Created attachment 271155 [details] [review] window: Add popover-added/removed signals This will be necessary for a11y to catch up with popovers being added/removed.
Created attachment 271156 [details] [review] a11y: Make GtkWindowAccessible know about popovers
Created attachment 271157 [details] [review] a11y: Add GtkPopopverAccessible And let GtkPopover use it as its GtkAccessible implementation, this accessible sets the POPUP_FOR relationship to the relative-to widget, and keeps track of changes there.
Created attachment 271158 [details] [review] testsuite: Update a11y/menubutton3.ui test expectations Popover is now minimally accessible.
Review of attachment 271157 [details] [review]: Looks ok, but you need to include gtkpopoveraccessible.h in gtk-a11y.h. With that fix, fine to commit.
Review of attachment 271155 [details] [review]: Do we still need this if we make popovers (internal) children of the window, as proposed in the dnd bug ?
(In reply to comment #6) > Review of attachment 271157 [details] [review]: > > Looks ok, but you need to include gtkpopoveraccessible.h in gtk-a11y.h. > With that fix, fine to commit. Oh sure, will fix that. (In reply to comment #7) > Review of attachment 271155 [details] [review]: > > Do we still need this if we make popovers (internal) children of the window, as > proposed in the dnd bug ? I could make GtkWindowAccessible to rely on gtk_container_forall(), as opposed to GtkContainerAccessible's usage of gtk_container_get_children(so _foreach). Although I fear we still need the signals, as AFAIK there nothing we can catch when internal children are added. Maybe for the time being, we can do with making a couple a11y functions available to GtkWindow, or just handling directly there the signal emission to the AtkObjects.
(In reply to comment #9) > > Maybe for the time being, we can do with making a couple a11y functions > available to GtkWindow, or just handling directly there the signal emission to > the AtkObjects. Yes, thats what I would suggest. It is what we've done elsewhere.
Created attachment 271422 [details] [review] a11y: Make GtkWindowAccessible know about popovers
Created attachment 271423 [details] [review] window: Emit a11y signals directly on popover added/removed As those are internal children, there's no signal that GtkWindowAccessible could catch when those are added or removed, so make GtkWindow poke ATK signals directly when that happens.
Created attachment 271424 [details] [review] a11y: Add GtkPopopverAccessible And let GtkPopover use it as its GtkAccessible implementation, this accessible sets the POPUP_FOR relationship to the relative-to widget, and keeps track of changes there.
Created attachment 271425 [details] [review] testsuite: Update a11y/menubutton3.ui test expectations Popover is now minimally accessible.
*** Bug 725920 has been marked as a duplicate of this bug. ***
Review of attachment 271422 [details] [review]: ::: gtk/a11y/gtkwindowaccessible.c @@ +299,3 @@ + + count = g_list_length (children); + g_list_free (children); Ugh. Could perhaps get the counting done without assembling a list as a side effect... pass &count and do (*count)++ in the callback. I guess the same is true for the containeraccessible implementation. @@ +315,3 @@ + (GtkCallback) prepend_widget, &children); + ref_child = g_list_nth_data (children, i); + g_list_free (children); Same here, although it may not matter that much in the end.
Review of attachment 271423 [details] [review]: ::: gtk/gtkwindow.c @@ +12262,3 @@ + g_object_notify (G_OBJECT (child_accessible), "accessible-parent"); + g_signal_emit_by_name (accessible, "children-changed::add", + -1, child_accessible, NULL); I think it would be a bit nicer to expose this as a a GtkContainerAccessible method, in gtkcontaineraccessibleprivate.h, and just use it here. @@ +12292,3 @@ + g_object_notify (G_OBJECT (child_accessible), "accessible-parent"); + g_signal_emit_by_name (accessible, "children-changed::remove", + -1, child_accessible, NULL); Same here
Review of attachment 271424 [details] [review]: sure
Review of attachment 271425 [details] [review]: sure, ok
Created attachment 271439 [details] [review] Thanks Matthias for the suggestions, I've applied those, and also did some changes to GtkPopoverAccessible based on suggestions on #a11y: * Stay on ATK_ROLE_PANEL, to be set to POPUP_MENU when we handle menu models, and one is present. * Set the modal state accordingly, so modal popovers can be discerned from more transient, non-modal ones. a11y: Make GtkWindowAccessible know about popovers
Created attachment 271440 [details] [review] a11y: Add private GtkContainerAccessible functions to add/remove a child This may be useful in container implementations, or for internal children that trigger no signal emission.
Created attachment 271441 [details] [review] a11y: Make GtkWindowAccessible know about popovers
Created attachment 271442 [details] [review] a11y: Add private GtkContainerAccessible functions to add/remove a child This may be useful in container implementations, or for internal children that trigger no signal emission.
Created attachment 271443 [details] [review] window: Emit a11y signals directly on popover added/removed As those are internal children, there's no signal that GtkWindowAccessible could catch when those are added or removed, so make GtkWindow use the private GtkContainerAccessible methods to add/remove the child accessible when that happens.
Created attachment 271444 [details] [review] a11y: Add GtkPopopverAccessible And let GtkPopover use it as its GtkAccessible implementation, this accessible sets the POPUP_FOR relationship to the relative-to widget, and keeps track of changes there.
Created attachment 271445 [details] [review] testsuite: Update a11y/menubutton3.ui test expectations Popover is now minimally accessible.
Small mishap with git-bz here, sorry for the noise...
Review of attachment 271441 [details] [review]: ok
Review of attachment 271442 [details] [review]: ok
Review of attachment 271443 [details] [review]: ok
Review of attachment 271444 [details] [review]: ok
Review of attachment 271445 [details] [review]: ok
Thanks Matthias for the review, this is all now in master. Attachment 271441 [details] pushed as 9d54fee - a11y: Make GtkWindowAccessible know about popovers Attachment 271442 [details] pushed as d4d6968 - a11y: Add private GtkContainerAccessible functions to add/remove a child Attachment 271443 [details] pushed as 168e4fa - window: Emit a11y signals directly on popover added/removed Attachment 271444 [details] pushed as 31cd153 - a11y: Add GtkPopopverAccessible Attachment 271445 [details] pushed as faa6db8 - testsuite: Update a11y/menubutton3.ui test expectations