GNOME Bugzilla – Bug 728475
Links, images, and form fields are not exposed to assistive technologies
Last modified: 2014-06-24 15:17:49 UTC
Created attachment 274638 [details] [review] Expose accessible children of the page to assistive technologies Steps to reproduce: 1. Open any PDF (tagged or untagged) which has one or more of the following: a. Links b. Images c. Form fields 2. Examine the accessible hierarchy of the document in Accerciser Expected results: The items listed under 1a-1c would be found. Actual results: None of the items listed under 1a-1c are found. This renders those objects completely inaccessible to Orca users.
Created attachment 274718 [details] [review] Expose accessible children of the page to assistive technologies This fixes the FIXMEs I marked in my previous patch, and adds a bit more functionality. Carlos: I'll keep chipping away at this implementation until you have time to review it. When you think you'll have some time, please let me know and I'll split the latest version of the patch into smaller chunks -- if doing so is helpful. Thanks!
Created attachment 275107 [details] [review] Expose links as AtkObject children of the page
Created attachment 275110 [details] [review] Expose images as AtkObject children of the page
Created attachment 275115 [details] [review] Expose form fields as AtkObject children of the page
Review of attachment 275107 [details] [review]: Thanks! ::: libview/ev-link-accessible.c @@ +386,3 @@ + page = ev_page_accessible_get_page (self->priv->page); + link_mapping = ev_page_cache_get_link_mapping (view->page_cache, page); + mapping = ev_mapping_list_find (link_mapping, self->priv->link); Could this be called before the page has been cached? In that case mapping would be NULL ::: libview/ev-page-accessible.c @@ +33,3 @@ GHashTable *links; + GPtrArray *children; + gboolean initialized; This actually means whether the children have been initialized not the object itself. I think it should be called children_initialized instead @@ +109,3 @@ + + links = ev_page_cache_get_link_mapping (view->page_cache, self->priv->page); + children = g_list_concat (children, g_list_copy (ev_mapping_list_get_list (links))); children is always NULL here, this is the same as children = g_list_copy (ev_mapping_list_get_list (links)); but less confusing this way I would say. @@ +115,3 @@ + + for (l = children; l && l->data; l = l->next) { + EvMapping *m = l->data; Use mapping, or link_mapping, or link, but not m :-) @@ +118,3 @@ + AtkObject *child = NULL; + + if (links && ev_mapping_list_find (links, m->data)) { How could this happen? if links is NULL, children should be NULL as well, so the loop will never iterate. And the link should be present, since children is a copy of the mapping list. Or am I understanding this wrong? @@ +121,3 @@ + EvLinkAccessible *link = ev_link_accessible_new (self, EV_LINK (m->data), &m->area); + AtkHyperlink *atk_link = atk_hyperlink_impl_get_hyperlink (ATK_HYPERLINK_IMPL (link)); + child = atk_hyperlink_get_object (atk_link, 0); Leave an empty line between var declarations and body. @@ +1036,2 @@ page->priv = G_TYPE_INSTANCE_GET_PRIVATE (page, EV_TYPE_PAGE_ACCESSIBLE, EvPageAccessiblePrivate); + page->priv->initialized = FALSE; This is already FALSE at this point, instance structs are initialized to 0 when allocated. ::: libview/ev-page-accessible.h @@ +52,3 @@ EvViewAccessible *ev_page_accessible_get_view_accessible (EvPageAccessible *page_accessible); EvView *ev_page_accessible_get_view (EvPageAccessible *page_accessible); +void ev_page_accessible_initialize_children (EvPageAccessible *page_accessible); Why does this need to be public? ::: libview/ev-view-private.h @@ +287,3 @@ GdkPoint *end_point); +void ev_view_set_focused_element (EvView *view, Use _ prefix for internal public API.
Created attachment 275360 [details] [review] Expose links as AtkObject children of the page Thanks for the review. (In reply to comment #5) > ::: libview/ev-link-accessible.c > @@ +386,3 @@ > + page = ev_page_accessible_get_page (self->priv->page); > + link_mapping = ev_page_cache_get_link_mapping (view->page_cache, page); > + mapping = ev_mapping_list_find (link_mapping, self->priv->link); > > Could this be called before the page has been cached? In that case mapping > would be NULL Since you asked, I'm extremely hesitant to give you an emphatic "no!" So I'll go with a reasonably-confident "I sure hope not." ;) In order for ev_link_accessible_grab_focus to be called, an assistive technology would first need to know that the link is there in the tree. And what causes the link to appear in the tree is the initialization of the children of the parent page. And that initialization does not (cannot) occur if the page hasn't been cached yet. Having said that, if I'm missing something, please let me know. In the meantime, I've not changed anything in response. > ::: libview/ev-page-accessible.c > @@ +33,3 @@ > GHashTable *links; > + GPtrArray *children; > + gboolean initialized; > > This actually means whether the children have been initialized not the object > itself. I think it should be called children_initialized instead Done. > @@ +109,3 @@ > + > + links = ev_page_cache_get_link_mapping (view->page_cache, > self->priv->page); > + children = g_list_concat (children, g_list_copy (ev_mapping_list_get_list > (links))); > > children is always NULL here, this is the same as children = g_list_copy > (ev_mapping_list_get_list (links)); but less confusing this way I would say. Done. > @@ +115,3 @@ > + > + for (l = children; l && l->data; l = l->next) { > + EvMapping *m = l->data; > > Use mapping, or link_mapping, or link, but not m :-) Done. I went with "mapping" because it's only link in the first patch. Once the subsequent patches are also applied, the mapping could also be for a form field or an image. > @@ +118,3 @@ > + AtkObject *child = NULL; > + > + if (links && ev_mapping_list_find (links, m->data)) { > > How could this happen? if links is NULL, children should be NULL as well, so > the loop will never iterate. And the link should be present, since children is > a copy of the mapping list. Or am I understanding this wrong? Not understanding it wrong; but perhaps out of the context of the two patches which follow. I had one ginormous patch and had to split it up somehow. I split it up based on object type. That said, for any given page there might or might not be links, there might or might not be images, and there might or might not be form fields. So the combined three patches get whatever objects happen to be there, sorts the combined result spatially so the page children are in a reasonably sane order, and then creates accessibles for each child based on type. I'd be happy to do something different if you'd prefer. > @@ +121,3 @@ > + EvLinkAccessible *link = ev_link_accessible_new (self, EV_LINK > (m->data), &m->area); > + AtkHyperlink *atk_link = atk_hyperlink_impl_get_hyperlink > (ATK_HYPERLINK_IMPL (link)); > + child = atk_hyperlink_get_object (atk_link, 0); > > Leave an empty line between var declarations and body. Done. > @@ +1036,2 @@ > page->priv = G_TYPE_INSTANCE_GET_PRIVATE (page, > EV_TYPE_PAGE_ACCESSIBLE, EvPageAccessiblePrivate); > + page->priv->initialized = FALSE; > > This is already FALSE at this point, instance structs are initialized to 0 when > allocated. Done. > ::: libview/ev-page-accessible.h > @@ +52,3 @@ > EvViewAccessible *ev_page_accessible_get_view_accessible (EvPageAccessible > *page_accessible); > EvView *ev_page_accessible_get_view (EvPageAccessible > *page_accessible); > +void ev_page_accessible_initialize_children (EvPageAccessible > *page_accessible); > > Why does this need to be public? Uhhhh. Excellent question. :) Done. > ::: libview/ev-view-private.h > @@ +287,3 @@ > GdkPoint *end_point); > > +void ev_view_set_focused_element (EvView *view, > > Use _ prefix for internal public API. Done.
Created attachment 275365 [details] [review] Expose images as AtkObject children of the page (updated to make it apply on top of the updated links patch)
Created attachment 275369 [details] [review] Expose form fields as AtkObject children of the page (updated to make it apply on top of the updated links and images patches)
*** Bug 701721 has been marked as a duplicate of this bug. ***
Review of attachment 275365 [details] [review]: LGTM, please check my review comments before pushing. ::: libview/ev-image-accessible.c @@ +76,3 @@ +} + +static const gchar* gchar* -> gchar * @@ +83,3 @@ +} + +static const gchar* Ditto. @@ +99,3 @@ + ev_image_accessible_get_atk_rect (ATK_OBJECT (atk_image), ATK_XY_WINDOW, &atk_rect); + *width = atk_rect.x2 - atk_rect.x1; + *height = atk_rect.y2 - atk_rect.y1; Are width/height required parameters? Are they checked with g_return macros in the interface public method? Otherwise, please, check they are not null before setting them. @@ +112,3 @@ + ev_image_accessible_get_atk_rect (ATK_OBJECT (atk_image), ATK_XY_WINDOW, &atk_rect); + *x = atk_rect.x1; + *y = atk_rect.y1; Same here for x, y. Ah!, and same for out parameters of get_extents now that I notice it. @@ +156,3 @@ +ev_image_accessible_finalize (GObject *object) +{ + G_OBJECT_CLASS (ev_image_accessible_parent_class)->finalize (object); Do not add finalize implementation just to chain up. I think you should free the EvImage here in any case. @@ +166,3 @@ + + object_class->finalize = ev_image_accessible_finalize; + g_type_class_add_private (klass, sizeof (EvImageAccessiblePrivate)); This looks weird here, please move it after the vfuncs initializations. @@ +167,3 @@ + object_class->finalize = ev_image_accessible_finalize; + g_type_class_add_private (klass, sizeof (EvImageAccessiblePrivate)); + atk_class->get_parent = ev_image_accessible_get_parent; extra space before = @@ +168,3 @@ + g_type_class_add_private (klass, sizeof (EvImageAccessiblePrivate)); + atk_class->get_parent = ev_image_accessible_get_parent; + atk_class->ref_state_set = ev_image_accessible_ref_state_set; Ditto. @@ +180,3 @@ + +void +ev_image_accessible_image_iface_init (AtkImageIface *iface) This should be static method @@ +182,3 @@ +ev_image_accessible_image_iface_init (AtkImageIface *iface) +{ + g_return_if_fail (iface != NULL); I don't think this can happen, don't use g_return macros in private methods @@ +190,3 @@ +} + +EvImageAccessible* EvImageAccessible* -> EvImageAccessible * @@ +199,3 @@ + atk_image = g_object_new (EV_TYPE_IMAGE_ACCESSIBLE, NULL); + atk_image->priv->page = page; + atk_image->priv->image = g_object_ref (image); This is never unrefed
Review of attachment 275360 [details] [review]: Ok, I would split the patch in 4 though, one to make ev_view_set_focused_element() and make it public internally, another one to add ev_page_cache_is_page_cached(), another one to add is_doc_rect_showing() and then the one to expose links. I have a few other comments too. ::: libview/ev-link-accessible.c @@ +206,3 @@ + end_index = ev_hyperlink_get_end_index (ATK_HYPERLINK (priv->hyperlink)); + + return atk_text_get_text (ATK_TEXT (atk_object_get_parent (atk_object)), start_index, end_index); This is not correct, the function returns a const char *, but atk_text_get_text returns a new allocated string. We could probably save the name in the private struct, this function would check if we have a name already and return that. ::: libview/ev-page-accessible.c @@ +93,3 @@ + return dy; + + return dx; return ABS (dy) > 10 ? dy : dx; @@ +112,3 @@ + + children = g_list_sort (children, (GCompareFunc) compare_mappings); + self->priv->children = g_ptr_array_new_full (g_list_length (children), (GDestroyNotify) g_object_unref); I think we can avoid creating the array if the page doesn't have links, we could return early here. @@ +125,3 @@ + } + + g_ptr_array_add (self->priv->children, child); Do we want to add a NULL children here when child is NULL? Can it really happen? @@ +324,3 @@ + self = EV_PAGE_ACCESSIBLE (accessible); + if (!self->priv->children_initialized) + ev_page_accessible_initialize_children (self); Instead of adding the if everywhere, check it in ev_page_accessible_initialize_children() and return early if it already initialized.
Review of attachment 275369 [details] [review]: Ok, as in images patch, fix my comments before pushing. ::: libview/ev-form-field-accessible.c @@ +74,3 @@ + mapping = ev_mapping_list_find (field_mapping, self->priv->form_field); + _ev_view_set_focused_element (view, mapping, page); + _ev_view_focus_form_field (view, self->priv->form_field); focus_form_field already calls set_focused_element, and the first thing it does is resetting it, so you shouldn't call _ev_view_set_focused_element in this case. @@ +206,3 @@ +ev_form_field_accessible_finalize (GObject *object) +{ + G_OBJECT_CLASS (ev_form_field_accessible_parent_class)->finalize (object); Same comments that in images patch. @@ +216,3 @@ + + object_class->finalize = ev_form_field_accessible_finalize; + g_type_class_add_private (klass, sizeof (EvFormFieldAccessiblePrivate)); Ditto. @@ +237,3 @@ + atk_form_field = g_object_new (EV_TYPE_FORM_FIELD_ACCESSIBLE, NULL); + atk_form_field->priv->page = page; + atk_form_field->priv->form_field = g_object_ref (form_field); Ditto.
Created attachment 279024 [details] [review] Rename ev_view_set_focused_element and make it public internally
Created attachment 279025 [details] [review] ev-page-cache: Add method to check if a given page has been cached
Created attachment 279026 [details] [review] ev-view-accessible: Add a method to determine if a given doc rect is showing
Created attachment 279029 [details] [review] Expose links as AtkObject children of the page (In reply to comment #11) > Review of attachment 275360 [details] [review]: > > Ok, I would split the patch in 4 though, one to make > ev_view_set_focused_element() and make it public internally, another one to add > ev_page_cache_is_page_cached(), another one to add is_doc_rect_showing() Done. See last three patches. > and > then the one to expose links. Done here. > ::: libview/ev-link-accessible.c > @@ +206,3 @@ > + end_index = ev_hyperlink_get_end_index (ATK_HYPERLINK (priv->hyperlink)); > + > + return atk_text_get_text (ATK_TEXT (atk_object_get_parent (atk_object)), > start_index, end_index); > > This is not correct, the function returns a const char *, but atk_text_get_text > returns a new allocated string. We could probably save the name in the private > struct, this function would check if we have a name already and return that. Fixed and done. > ::: libview/ev-page-accessible.c > @@ +93,3 @@ > + return dy; > + > + return dx; > > return ABS (dy) > 10 ? dy : dx; Done. > @@ +112,3 @@ > + > + children = g_list_sort (children, (GCompareFunc) compare_mappings); > + self->priv->children = g_ptr_array_new_full (g_list_length (children), > (GDestroyNotify) g_object_unref); > > I think we can avoid creating the array if the page doesn't have links, we > could return early here. Done. > @@ +125,3 @@ > + } > + > + g_ptr_array_add (self->priv->children, child); > > Do we want to add a NULL children here when child is NULL? Can it really > happen? Fixed. > @@ +324,3 @@ > + self = EV_PAGE_ACCESSIBLE (accessible); > + if (!self->priv->children_initialized) > + ev_page_accessible_initialize_children (self); > > Instead of adding the if everywhere, check it in > ev_page_accessible_initialize_children() and return early if it already > initialized. Done.
Created attachment 279045 [details] [review] Expose images as AtkObject children of the page (In reply to comment #10) > Review of attachment 275365 [details] [review]: > > LGTM, please check my review comments before pushing. Thanks! But this patch totally depends on the stuff in the previous patches, so here's a new version. All the spacing nits: Fixed. Regarding these: > Are width/height required parameters? Are they checked with g_return macros in > the interface public method? Otherwise, please, check they are not null before > setting them. [...] > Same here for x, y. Ah!, and same for out parameters of get_extents now that I > notice it. I was chatting with Piñeiro and we took a look at ATK and verified that both for the image interface methods and the component interface methods, ATK checks and also passes correct values if the caller does not. Thus I made no change in the patch. > Do not add finalize implementation just to chain up. I think you should free > the EvImage here in any case. Understood. EvImage freed. > This looks weird here, please move it after the vfuncs initializations. Done. > +void > +ev_image_accessible_image_iface_init (AtkImageIface *iface) > > This should be static method Done. > @@ +182,3 @@ > +ev_image_accessible_image_iface_init (AtkImageIface *iface) > +{ > + g_return_if_fail (iface != NULL); > > I don't think this can happen, don't use g_return macros in private methods Done. > @@ +199,3 @@ > + atk_image = g_object_new (EV_TYPE_IMAGE_ACCESSIBLE, NULL); > + atk_image->priv->page = page; > + atk_image->priv->image = g_object_ref (image); > > This is never unrefed Fixed as indicated above.
Review of attachment 279024 [details] [review]: Thanks
Review of attachment 279025 [details] [review]: Ok.
Review of attachment 279026 [details] [review]: Ok
Review of attachment 279029 [details] [review]: Thanks, fix my comments before pushing, please ::: libview/ev-link-accessible.c @@ +206,3 @@ + priv = EV_LINK_ACCESSIBLE (atk_object)->priv; + if (priv->name) + return (const gchar*)priv->name; I don't think you need the cast, but if you really need it leave space between gchar and * @@ +212,3 @@ + priv->name = atk_text_get_text (ATK_TEXT (atk_object_get_parent (atk_object)), start_index, end_index); + + return (const gchar*)priv->name; Ditto. @@ +263,2 @@ g_clear_object (&link->priv->hyperlink); + g_free (&link->priv->name); This is wrong, it should be either g_free (link->priv->name); or g_clear_pointer (&link->priv->name, g_free); ::: libview/ev-page-accessible.c @@ +107,3 @@ + if (!ev_page_cache_is_page_cached (view->page_cache, self->priv->page)) + return; + If you set self->priv->children_initialized = TRUE here you don't need to worry about any other early returns.
Review of attachment 279045 [details] [review]: Ok, please fix my comment before pushing. ::: libview/ev-page-accessible.c @@ -112,3 @@ - self->priv->children_initialized = TRUE; - return; - } Instead of removing this, you should probably return early if both links and images are NULL.
Created attachment 279054 [details] [review] Expose form fields as AtkObject children of the page New version of the form field patch: Master compatible.
Comment on attachment 279029 [details] [review] Expose links as AtkObject children of the page (In reply to comment #22) > I don't think you need the cast, but if you really need it leave space between > gchar and * Seems I don't. Cast removed in both places. > @@ +263,2 @@ > g_clear_object (&link->priv->hyperlink); > + g_free (&link->priv->name); > > This is wrong, it should be either g_free (link->priv->name); or Fixed. > ::: libview/ev-page-accessible.c > @@ +107,3 @@ > + if (!ev_page_cache_is_page_cached (view->page_cache, self->priv->page)) > + return; > + > > If you set self->priv->children_initialized = TRUE here you don't need to worry > about any other early returns. Good point. Done.
Comment on attachment 279045 [details] [review] Expose images as AtkObject children of the page (In reply to comment #23) > Review of attachment 279045 [details] [review]: > > Ok, please fix my comment before pushing. > > ::: libview/ev-page-accessible.c > @@ -112,3 @@ > - self->priv->children_initialized = TRUE; > - return; > - } > > Instead of removing this, you should probably return early if both links and > images are NULL. Done.
Created attachment 279063 [details] [review] Expose form fields as AtkObject children of the page Master-compatible patch
Review of attachment 279063 [details] [review]: Thanks
Comment on attachment 279063 [details] [review] Expose form fields as AtkObject children of the page Thanks Carlos! I know that was a lot to review and really appreciate it!!