GNOME Bugzilla – Bug 706244
Make links and form buttons focusable
Last modified: 2014-06-10 17:39:45 UTC
Evince users should be able to activate the link address of a focused link by pressing Enter or to toggle the value of a focused check button by pressing the spacebar, for example.
Created attachment 252120 [details] [review] Added ev_mapping_list_get function
Created attachment 252121 [details] [review] Rewritten focus annotations in a generic way
Created attachment 252122 [details] [review] Grab keyboard focus for link on left click 'Enter' and 'Return' key press events activate the link action
Created attachment 252123 [details] [review] Grab keyboard focus for form field buttons on left click 'Enter' and 'Return' key press events activate the form field activation link. 'Spacebar' key press events toggle check and radio form field buttons.
Created attachment 252124 [details] [review] Grab keyboard focus for form field buttons, links and annotations on right click
The patches above grab the keyboard focus for links and form field buttons when the user presses the right or left mouse buttons. A reasonalbe next step for Evince keyboard users would probably be to make form fields and links Tab focusable.
Review of attachment 252121 [details] [review]: Looks good in general, sorry for the delay reviewing this. ::: libview/ev-view.c @@ +2072,3 @@ +/*** Focus ***/ +static void +ungrab_focus (EvView *view) This name is a bit confusing, because the view might already have the focus. I would use something like ev_view_set_focused_element, where passing NULL would be your ungrab_focus and passing a EvMapping would be your grab_focus @@ +2078,3 @@ + + view->focused_element = NULL; + gtk_widget_queue_draw (GTK_WIDGET (view)); It would be nice if we only redraw the focused rectangle at least. @@ +2099,3 @@ + &view_rect); + ensure_rectangle_is_visible (view, &view_rect); + gtk_widget_queue_draw (GTK_WIDGET (view)); Ditto. We would need a region with previous and new focus rectangles. @@ +3918,3 @@ +static void +render_focus_indicator_on_focused_element (EvView *view, render_focus or draw_focus would be enough, I think. @@ +3921,3 @@ + cairo_t *cr, + gint page, + GdkRectangle *clip) I know this was already unused in previous focus_annotation method, but it would be nice if we use it now.
Review of attachment 252122 [details] [review]: ::: libview/ev-view.c @@ +1641,3 @@ + return mapping->data; + + return NULL; This could be simplified as return mapping ? mapping->data : NULL; @@ +4537,3 @@ EvAnnotation *annot; EvFormField *field; + EvMapping *link; link -> link_mapping. @@ +4565,3 @@ ev_view_handle_form_field (view, field, event->x, event->y); + } else if ((link = get_link_mapping_at_location (view, event->x, event->y, &page))){ + grab_focus (view, link, page); Why only on left click? @@ +5604,3 @@ + if (propagate_key_press_event (view, event)) + return TRUE; I think we should use key bindings for this too.
Created attachment 255372 [details] [review] Added ev_mapping_list_get Patch updated
Created attachment 255373 [details] [review] Grab focus for form fields and links on right, left and middle mouse clicks Patch updated according to comments in the review.
Created attachment 255374 [details] [review] Toggle form radio and check buttons on space bar press Updated to use gtkbindings
Created attachment 255375 [details] [review] Activate form fields and links on Enter and Return keypresses Patch updated to use gtkbindings
Review of attachment 255373 [details] [review]: Looks good in general. Thanks. ::: libview/ev-view.c @@ +2084,3 @@ +/*** Focus ***/ +static cairo_region_t * +ev_view_get_region_from_mapping (EvView *view, If you return the rectangle instead of the region you could reuse this function in more places @@ +2092,3 @@ + _ev_view_transform_doc_rect_to_view_rect (view, page, &mapping->area, &area); + area.x -= view->scroll_x; + area.y -= view->scroll_y; You should consider here that the focus is not drawn exactly in the mapping rect, but it's one pixel bigger. @@ +2114,3 @@ + view->focused_element, + view->focused_element_page); + ev_view_reload_page (view, view->focused_element_page, region); This is not correct, the focus is not rendered by the backend, so we don't need to reload the page, just to redraw the damage area. @@ +2131,3 @@ + region = cairo_region_create_rectangle (&view_rect); + + ev_view_reload_page (view, view->focused_element_page, region); Ditto. @@ +2133,3 @@ + ev_view_reload_page (view, view->focused_element_page, region); + cairo_region_destroy (region); + } This function can be simplified if you use a method that returns the rectangle as I suggested. @@ +2664,3 @@ + (EV_FORM_FIELD_BUTTON (field)->type == EV_FORM_FIELD_BUTTON_CHECK || + EV_FORM_FIELD_BUTTON (field)->type == EV_FORM_FIELD_BUTTON_RADIO)) + toggle_form_field_button (view, field); I would call this ev_view_form_field_button_toggle, and move the type checks to the method to simply the if a bit. @@ +4022,3 @@ + rect.y -= view->scroll_y; + rect.width += 1; + rect.height += 1; Here you could use the method I mentioned before that returns the focused rectangle. @@ +4024,3 @@ + rect.height += 1; + + gdk_rectangle_intersect (&rect, clip, &intersect); You should check if it actually intersects. @@ +4649,3 @@ + gint page; + + ev_view_set_focused_element (view, NULL, -1); This should not be needed.
Comment on attachment 255372 [details] [review] Added ev_mapping_list_get Pushed to git master, thanks!
Comment on attachment 255373 [details] [review] Grab focus for form fields and links on right, left and middle mouse clicks I've just fixed all my comments and pushed to git master.
Most of the gtk widgets emit activate on both space and return key press because GtkWindow defines those keybindings. I think we could handle both cases with a single activate signal, but ignoring space key press in case of links, for consistency with web browsers. I've squashed both patches, reworked to use a single activate signal and pushed to git master.
*** Bug 701724 has been marked as a duplicate of this bug. ***