GNOME Bugzilla – Bug 639932
Expose text attributes in atkText [a11y]
Last modified: 2013-07-12 07:06:04 UTC
It's needed to implement some method to get text attributes like font size, font family, color, etc, from backends to expose it to atktext interface. This can be done adding two methods to EvDocumentText interface returning a PangoAttrList with text attributes. One method to get default attributes and another one to get run attributes, similar to AtkText interface.
Created attachment 179372 [details] [review] four commits that exposes text attributes These patches implement atk_text_get_run_attributes, giving a way to expose text attributes to backends and working with PangoAttrList. The pdf backend patch depends on poppler bug https://bugs.freedesktop.org/show_bug.cgi?id=33269 that has a patch waiting for revision.
I wonder whether we could get the attributes while getting the text and apply the attributes as tags to the text buffer so that we can use gail_misc_buffer_get_run_attributes()
(In reply to comment #2) > I wonder whether we could get the attributes while getting the text and apply > the attributes as tags to the text buffer so that we can use > gail_misc_buffer_get_run_attributes() Yes, that's a great idea, I'll do that.
Created attachment 181446 [details] [review] four commits that exposes text attributes Exposing text attributes using GtkTextTag and gail_misc. Doing that I've found several bugs at gail_misc_buffer_get_run_attributes and I've send patches, and now I think it's working ok.
Ping. :-) Yeah, I know, I'm becoming a noodge. Sorry! But I'm on a mission to have fully accessible PDFs in time for GNOME 3.2. If someone could review this, it would be much appreciated. Thanks!
I'm not sure it will be possible to have this in time for 3.2, first we need to add the poppler changes in 0.18.
Created attachment 194307 [details] [review] Three patch to add text attributes to a11y This patch use the last poppler patch to get text attributes: https://bugs.freedesktop.org/show_bug.cgi?id=33269#c9
Created attachment 194368 [details] [review] a11y text attributes Patch updated with latest poppler changes https://bugs.freedesktop.org/show_bug.cgi?id=33269
Review of attachment 194368 [details] [review]: ::: libdocument/ev-document.h @@ +210,3 @@ + gint end; + PangoAttrList *attrs; +}; PangoAttribute already has start_index and end_index, do we really need this EvTextInfo struct? Would it be possible to just return a list of PangoAttrList where every attribute has start/end_index filled already? or are those indices a different thing?
Daniel, any information regarding to Carlos' question about your patch?
(In reply to comment #10) > Daniel, any information regarding to Carlos' question about your patch? Currently this patch depends on gtk/atk headers so I need to look at the gtk/atk status, make a11y work again in evince and then take a look to this patch to make it work with new code. https://bugzilla.gnome.org/show_bug.cgi?id=677348 https://bugzilla.gnome.org/show_bug.cgi?id=685828 So... yes I'll re-review this patch after we have a simple a11y implementation running in evince. The current one didn't work.
Created attachment 248769 [details] [review] Create a PangoAttrList using the text attributes returned by poppler
Created attachment 248771 [details] [review] Rewrite ev_view_accessible_get_run_attributes to return the new attribute list
Comment on attachment 248769 [details] [review] Create a PangoAttrList using the text attributes returned by poppler Split in 3 different patches, and pushed to git master. Thanks!
Review of attachment 248771 [details] [review]: Please, fix the coding style issues. ::: libview/ev-view-accessible.c @@ +365,3 @@ + +static AtkAttributeSet * +pango_get_run_attributes (PangoAttrList *attr, Calling this pango_get_run_attributes is confusing because it looks like a pango function, use something like get_run_attributes_from_pango_attr_list() or simply get_run_attributes(). @@ +383,3 @@ + gchar *value; + + len = g_utf8_strlen (text, -1); Text could be NULL here, I think. @@ +386,3 @@ + + if (attr == NULL) + { Move the { to the if line. @@ +388,3 @@ + { + *start_offset = 0; + *end_offset = len; What's the point of setting these if we are returning NULL? Does atk need the offset to be always filled? @@ +398,3 @@ + offset = len; + else if (offset < 0) + offset = 0; You can probably do offset = CLAMP (offset, 0, len); @@ +415,3 @@ + } + is_next = pango_attr_iterator_next (iter); + pango_attr_iterator_range (iter, &start_index, &end_index); do we want to do this even when is_next is false? I think thi loop would be easier to read as a do while loop do { pango_attr_iterator_range (iter, &start_index, &end_index); if (index >= start_index && index < end_index) { ..... } while (pango_attr_iterator_next (iter)); @@ +419,3 @@ + + /* Get attributes */ + pango_string = (PangoAttrString*) pango_attr_iterator_get (iter, PANGO_ATTR_FAMILY); You haven't checked if the loop finished without finding the iterator, is it safe to assume it should always find one? In that case we could add an assert to make sure. @@ +509,3 @@ + attributes = add_attribute (attributes, ATK_TEXT_ATTR_BG_COLOR, value); + g_free (value); + } It seems we are parsing a lot more attributes that the ones currently provided by the backends (only pdf for now), no? @@ +533,3 @@ return NULL; + return pango_get_run_attributes (ev_page_cache_get_text_attrs (view->page_cache, view->current_page), Maybe we should check we have text and attrs before calling this?
Created attachment 248949 [details] [review] Rewrite ev_accessible_get_run_attributes to return the new attributes list Updated patch.
Review of attachment 248949 [details] [review]: Thanks, I've fixed my comments below and some other minor style issues and pushed it to git master. ::: libview/ev-view-accessible.c @@ +358,3 @@ + + attr->name = g_strdup (atk_text_attribute_get_name (attr_type)); + attr->value = g_strdup (attr_value); We can save some allocations, by adopting the attr value instead of duplicating it. @@ +381,3 @@ + + g_return_val_if_fail (attrs != NULL, NULL); + g_return_val_if_fail (text != NULL, NULL); Don't use g_return macros in private functions. In this particular case, the only caller of this already returns early when attrs or text are NULL, so we don't need these. @@ +404,3 @@ + + if (!has_attrs) + return NULL; You are leaking the iterator here. @@ +409,3 @@ + pango_string = (PangoAttrString*) pango_attr_iterator_get (iter, PANGO_ATTR_FAMILY); + if (pango_string) { + attr_value = g_strdup_printf ("%s", pango_string->value); You don't need g_strdup_printf to duplicate a string, you can simply use g_strdup()