GNOME Bugzilla – Bug 733919
Form field push buttons lack accessible names
Last modified: 2014-07-30 15:16:50 UTC
Created attachment 281954 [details] [review] proposed patch Form field push buttons lack accessible names. As a result, Orca users know that a button is present, but not what will happen when they activate it.
Review of attachment 281954 [details] [review]: ::: libview/ev-form-field-accessible.c @@ +169,3 @@ + if (c_x < priv->area.x1 || c_x > priv->area.x2 || + c_y < priv->area.y1 || c_y > priv->area.y2) { + Remove this empty line, please. @@ +185,3 @@ + + start = get_start_index (atk_object); + end = get_end_index (atk_object); So, we call get_start_index, but get_end_index() also need the start_index, and ends up calling it again. Could we mrege those method somehow? or pass the start_index to get_end_index to avoid doing get_start_index() twice? If we always want both it would be better to have a single method with two output parameters.
Created attachment 281998 [details] [review] proposed patch Thanks for the review! This version has a single method to get both indices.
Review of attachment 281998 [details] [review]: Looks much simpler, thanks. I still have a few questions more. ::: libview/ev-form-field-accessible.c @@ +126,3 @@ + } + + for (i = priv->start_index + 1; i < n_areas; i++) { Shouldn't we check here that start_index is no longer -1? In the previous patch we returned -1 in that case, even though that return value was not checked. Should we return early in that case? @@ +151,3 @@ + + *start = priv->start_index; + *end = priv->end_index; Same here for the end_index, and this could be in an inconsistent state, for example if we found the start index, but failed to get the end_index, we might end up with start_index >= 0 and end_index == -1. We should probably return early and reset both indices to -1 when something does wrong. @@ +158,3 @@ +{ + gint start = 0; + gint end = 0; So, these are initialized to 0, but in case of failing to get any of the indices, they are set to -1. And we don't check them before calling atk_text_get_text @@ +161,3 @@ + + get_indices_in_parent (atk_object, &start, &end); + return start != end ? atk_text_get_text (ATK_TEXT (atk_object_get_parent (atk_object)), start, end) : NULL; Ah, we check they are not the same, so it doesn't really matter if they are 0 or -1, but as I said before they could be in an inconsistent state. I think it would be easier if we make get_indices_in_parent boolean, and we return false when something fails, so that we can just ignore the output parameters. if (get_indices_in_parent (atk_object, &start, &end)) return start != end ? atk_text_get_text (ATK_TEXT (atk_object_get_parent (atk_object)), start, end) : NULL; return NULL; Something like this.
Created attachment 282005 [details] [review] proposed patch
Review of attachment 282005 [details] [review]: Perfect!, thanks
Comment on attachment 282005 [details] [review] proposed patch Thanks! https://git.gnome.org/browse/evince/commit/?id=11225b2