GNOME Bugzilla – Bug 503706
Cycle through form fields with tab button
Last modified: 2014-06-25 10:24:34 UTC
It's great that we can now fill out PDF forms with evince, but it would be much easier if I would move to the next form field by pressing the tab key on the keyboard instead of having to click the mouse in the next form field.
Yes, that would be great. Also, it will be very useful for checking whether the fields highlight in the correct order.
*** Bug 635538 has been marked as a duplicate of this bug. ***
*** Bug 693103 has been marked as a duplicate of this bug. ***
+1
Yes, please.
Created attachment 260185 [details] [review] Add form field tab navigation and highlighting
I'd like to propose the attached patch to fix this. It adds form field tab navigation as well as field highlighting. Forward and backward (shift+tab) navigation should work. Space should activate buttons (check, radio, push). The only issue is you will encounter warnings such as: (lt-evince:28003): Gtk-WARNING **: EvView 0x1ae21a0 is mapped but visible child GtkCheckButton 0x1fd1ab0 is not mapped This is because button widgets are unmapped (so they do not draw). If anyone knows how to quiet this warning, let me know!
Comment on attachment 260185 [details] [review] Add form field tab navigation and highlighting >diff --git a/libview/ev-view-private.h b/libview/ev-view-private.h >index 7633c3a..fc3333f 100644 >--- a/libview/ev-view-private.h >+++ b/libview/ev-view-private.h >@@ -221,6 +221,11 @@ struct _EvView { > gboolean cursor_visible; > guint cursor_blink_timeout_id; > guint cursor_blink_time; >+ >+ /* Field tab navigation */ >+ GList *form_field_order; >+ gint form_field_current; >+ gboolean form_field_focused; > }; > > struct _EvViewClass { >diff --git a/libview/ev-view.c b/libview/ev-view.c >index da69022..905ab05 100644 >--- a/libview/ev-view.c >+++ b/libview/ev-view.c >@@ -134,6 +134,11 @@ static char* tip_from_link (EvView > static EvFormField *ev_view_get_form_field_at_location (EvView *view, > gdouble x, > gdouble y); >+static gboolean >+ev_view_form_key_press (GtkWidget *widget, >+ GdkEventKey *event, >+ EvView *view); >+ > > /*** Annotations ***/ > static EvAnnotation *ev_view_get_annotation_at_location (EvView *view, >@@ -183,6 +188,9 @@ static void highlight_find_results (EvView > static void highlight_forward_search_results (EvView *view, > cairo_t *cr, > int page); >+static void highlight_form_field (EvView *view, >+ cairo_t *cr); >+ > static void draw_one_page (EvView *view, > gint page, > cairo_t *cr, >@@ -2262,19 +2270,61 @@ ev_view_form_field_button_toggle (EvView *view, > cairo_region_destroy (region); > } > >+static void >+ev_view_form_button_toggled (GtkButton *button, EvView *view) >+{ >+ EvFormField *field; >+ >+ if (!view->document) >+ return; >+ >+ field = g_object_get_data (G_OBJECT (button), "form-field"); >+ ev_view_form_field_button_toggle (view, field); >+ if (field->activation_link) >+ ev_view_handle_link (view, field->activation_link); >+} >+ > static GtkWidget * > ev_view_form_field_button_create_widget (EvView *view, > EvFormField *field) > { > EvMappingList *form_mapping; > EvMapping *mapping; >+ GtkWidget *button = NULL; >+ EvFormFieldButton *field_button = EV_FORM_FIELD_BUTTON (field); > > form_mapping = ev_page_cache_get_form_field_mapping (view->page_cache, > field->page->index); > mapping = ev_mapping_list_find (form_mapping, field); > ev_view_set_focused_element (view, mapping, field->page->index); > >- return NULL; >+ switch (field_button->type) { >+ case EV_FORM_FIELD_BUTTON_PUSH: >+ button = gtk_button_new (); >+ break; >+ case EV_FORM_FIELD_BUTTON_CHECK: >+ button = gtk_check_button_new (); >+ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (button), field_button->state); >+ g_signal_connect (button, "toggled", >+ G_CALLBACK (ev_view_form_button_toggled), >+ view); >+ break; >+ case EV_FORM_FIELD_BUTTON_RADIO: >+ button = gtk_radio_button_new (NULL); >+ /* >+ GTK doesn't actually allow a radio group with no selected button... >+ >+ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (button), field_button->state); >+ */ >+ g_signal_connect (button, "toggled", >+ G_CALLBACK (ev_view_form_button_toggled), >+ view); >+ break; >+ } >+ >+ g_signal_connect (button, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); >+ >+ return button; > } > > static void >@@ -2371,6 +2421,9 @@ ev_view_form_field_text_create_widget (EvView *view, > g_signal_connect_after (text, "activate", > G_CALLBACK (ev_view_form_field_destroy), > view); >+ g_signal_connect (text, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); > break; > case EV_FORM_FIELD_TEXT_MULTILINE: { > GtkTextBuffer *buffer; >@@ -2389,6 +2442,10 @@ ev_view_form_field_text_create_widget (EvView *view, > g_signal_connect (buffer, "changed", > G_CALLBACK (ev_view_form_field_text_changed), > field); >+ g_signal_connect (text, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); >+ > } > break; > } >@@ -2563,6 +2620,9 @@ ev_view_form_field_choice_create_widget (EvView *view, > g_signal_connect_after (selection, "changed", > G_CALLBACK (ev_view_form_field_destroy), > view); >+ g_signal_connect (selection, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); > } else if (field_choice->is_editable) { /* ComboBoxEntry */ > gchar *text; > >@@ -2582,6 +2642,9 @@ ev_view_form_field_choice_create_widget (EvView *view, > "activate", > G_CALLBACK (ev_view_form_field_destroy), > view); >+ g_signal_connect (choice, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); > } else { /* ComboBoxText */ > GtkCellRenderer *renderer; > >@@ -2602,6 +2665,9 @@ ev_view_form_field_choice_create_widget (EvView *view, > g_signal_connect_after (choice, "changed", > G_CALLBACK (ev_view_form_field_destroy), > view); >+ g_signal_connect (choice, "key_press_event", >+ G_CALLBACK (ev_view_form_key_press), >+ view); > } > > g_object_unref (model); >@@ -2614,12 +2680,19 @@ ev_view_form_field_choice_create_widget (EvView *view, > } > > static void >+field_widget_destroy (GtkWidget *object, EvView *view) { >+ view->form_field_focused = FALSE; >+} >+ >+static void > ev_view_focus_form_field (EvView *view, > EvFormField *field) > { > GtkWidget *field_widget = NULL; > EvMappingList *form_field_mapping; > EvMapping *mapping; >+ GtkStyleContext *context; >+ GdkRGBA color; > > ev_view_set_focused_element (view, NULL, -1); > >@@ -2640,6 +2713,16 @@ ev_view_focus_form_field (EvView *view, > if (!field_widget) > return; > >+ /* Highlight focused fields */ >+ context = gtk_widget_get_style_context (GTK_WIDGET (view)); >+ gtk_style_context_get_background_color (context, GTK_STATE_FLAG_SELECTED, &color); >+ gtk_widget_override_background_color (field_widget, GTK_STATE_FLAG_NORMAL, &color); >+ view->form_field_focused = TRUE; >+ >+ g_signal_connect (field_widget, "destroy", >+ G_CALLBACK (field_widget_destroy), >+ view); >+ > g_object_set_data_full (G_OBJECT (field_widget), "form-field", > g_object_ref (field), > (GDestroyNotify)g_object_unref); >@@ -2650,6 +2733,8 @@ ev_view_focus_form_field (EvView *view, > ev_view_put_to_doc_rect (view, field_widget, field->page->index, &mapping->area); > gtk_widget_show (field_widget); > gtk_widget_grab_focus (field_widget); >+ if (EV_IS_FORM_FIELD_BUTTON (field)) >+ gtk_widget_unmap (field_widget); > } > > static void >@@ -2659,14 +2744,93 @@ ev_view_handle_form_field (EvView *view, > if (field->is_read_only) > return; > >+ if (EV_IS_FORM_FIELD_BUTTON (field)) >+ ev_view_form_field_button_toggle (view, field); >+ > ev_view_focus_form_field (view, field); > > if (field->activation_link) > ev_view_handle_link (view, field->activation_link); > >- if (EV_IS_FORM_FIELD_BUTTON (field)) >- ev_view_form_field_button_toggle (view, field); >+} > >+static gboolean >+ev_view_form_key_press (GtkWidget *widget, GdkEventKey *event, EvView *view) >+{ >+ EvFormField *field; >+ switch (event->keyval) { >+ case GDK_KEY_ISO_Left_Tab: >+ case GDK_KEY_Tab: >+ if (event->state & GDK_SHIFT_MASK) { >+ view->form_field_current++; >+ if (view->form_field_current == g_list_length (view->form_field_order)) >+ view->form_field_current = 0; >+ } else { >+ view->form_field_current--; >+ if (view->form_field_current < 0) >+ view->form_field_current = g_list_length (view->form_field_order) - 1; >+ } >+ field = EV_FORM_FIELD(g_list_nth_data (view->form_field_order, view->form_field_current)); >+ ev_view_remove_all (view); >+ ev_view_focus_form_field (view, field); >+ return TRUE; >+ case GDK_KEY_space: >+ field = EV_FORM_FIELD(g_list_nth_data (view->form_field_order, view->form_field_current)); >+ if (EV_IS_FORM_FIELD_BUTTON (field)) { >+ EvFormFieldButton *button = EV_FORM_FIELD_BUTTON (field); >+ if (button->type == EV_FORM_FIELD_BUTTON_RADIO) { >+ gtk_toggle_button_toggled (GTK_TOGGLE_BUTTON(widget)); >+ return TRUE; >+ } >+ } >+ break; >+ } >+ return FALSE; >+} >+ >+static GList* >+page_form_field_order (EvView *view, guint page) { >+ EvMappingList *mappings = ev_page_cache_get_form_field_mapping (view->page_cache, page); >+ GList *list; >+ GList *order = NULL; >+ >+ if (!mappings) >+ return NULL; >+ >+ /* use structure order */ >+ for (list = ev_mapping_list_get_list(mappings); list; list = list->next) { >+ EvMapping *mapping = list->data; >+ EvFormField *field = mapping->data; >+ /* >+ ignore read-only fields >+ ignore signature fields (until they are implemented) >+ */ >+ if (!field->is_read_only && !EV_IS_FORM_FIELD_SIGNATURE(field)) >+ order = g_list_append (order, field); >+ } >+ return order; >+} >+ >+static void >+create_form_field_order (EvView *view) { >+ GList *order = NULL; >+ guint page; >+ if (!EV_IS_DOCUMENT_FORMS (view->document)) >+ return; >+ >+ guint n_pages = ev_document_get_n_pages (view->document); >+ for (page = 0; page < n_pages; page++) { >+ GList *list = page_form_field_order (view, page); >+ order = g_list_concat (order, list); >+ } >+ view->form_field_order = order; >+ view->form_field_current = -1; >+ view->form_field_focused = FALSE; >+} >+ >+static void >+set_form_field_current (EvView *view, EvFormField *field) { >+ view->form_field_current = g_list_index (view->form_field_order, field); > } > > /* Annotations */ >@@ -4255,6 +4419,8 @@ ev_view_draw (GtkWidget *widget, > draw_focus (view, cr, i, &clip_rect); > if (page_ready && view->synctex_result) > highlight_forward_search_results (view, cr, i); >+ if (page_ready && view->form_field_focused) >+ highlight_form_field (view, cr); > #ifdef EV_ENABLE_DEBUG > if (page_ready) > draw_debug_borders (view, cr, i, &clip_rect); >@@ -4671,6 +4837,10 @@ ev_view_button_press_event (GtkWidget *widget, > } else if ((field = ev_view_get_form_field_at_location (view, event->x, event->y))) { > ev_view_remove_all (view); > ev_view_handle_form_field (view, field); >+ if (!view->form_field_order) >+ create_form_field_order (view); >+ >+ set_form_field_current (view, field); > } else if ((link = get_link_mapping_at_location (view, event->x, event->y, &page))){ > ev_view_set_focused_element (view, link, page); > } else if (!location_in_text (view, event->x + view->scroll_x, event->y + view->scroll_y) && >@@ -5710,11 +5880,6 @@ ev_view_activate_form_field (EvView *view, > handled = TRUE; > } > >- if (EV_IS_FORM_FIELD_BUTTON (field)) { >- ev_view_form_field_button_toggle (view, field); >- handled = TRUE; >- } >- > return handled; > } > >@@ -5911,6 +6076,26 @@ highlight_forward_search_results (EvView *view, > } > > static void >+highlight_form_field (EvView *view, >+ cairo_t *cr) >+{ >+ GdkRectangle rect; >+ EvMappingList *form_field_mapping; >+ EvMapping *mapping; >+ EvFormField *field = EV_FORM_FIELD (g_list_nth_data (view->form_field_order, view->form_field_current)); >+ >+ /* We let text field widgets highlight on their own (via background color) */ >+ if (EV_IS_FORM_FIELD_TEXT (field)) >+ return; >+ >+ form_field_mapping = ev_page_cache_get_form_field_mapping (view->page_cache, >+ field->page->index); >+ mapping = ev_mapping_list_find (form_field_mapping, field); >+ _ev_view_transform_doc_rect_to_view_rect (view, field->page->index, &mapping->area, &rect); >+ draw_rubberband (view, cr, &rect, 0.6); >+} >+ >+static void > draw_surface (cairo_t *cr, > cairo_surface_t *surface, > gint x, >@@ -6676,6 +6861,9 @@ ev_view_init (EvView *view) > view->pixbuf_cache_size = DEFAULT_PIXBUF_CACHE_SIZE; > view->caret_enabled = FALSE; > view->cursor_page = 0; >+ view->form_field_order = NULL; >+ view->form_field_current = -1; >+ view->form_field_focused = FALSE; > } > > /*** Callbacks ***/
Comment on attachment 260185 [details] [review] Add form field tab navigation and highlighting Push button activation bug
Created attachment 260191 [details] [review] Add field navigation and field highlighting
Created attachment 260193 [details] [review] Add field navigation and field highlighting Fix warning
Sorry about all the noise. I believe the most recent patch is good. There should not be any more warning. I hope someone will review and apply this soon.
Created attachment 260277 [details] [review] Add form field tab navigation and highlighting Fix memory leak Reverse form_field_order to be more logical
I built Evince from master with Daniel's latest patch. Seems to work just fine - thanks! Could one of the Evince developers review the code? Would be great to see this land.
Ping? It would be really nice to have this feature for Evince 3.12.
Review of attachment 260277 [details] [review]: Sorry for the delay reviewing this. Ideally we should be able to move the focus to any other focusable element, not only form fields, but alos links, or annotations, but just allowing to focus form fields is a huge improvement. I've tried the patch and I've noticed some issues: - It doesn't seem to be possible to get the focus out of the view. Instead of cycling the fields in a loop, we should probably move the focus out when reaching the last form field. - Sometimes it doesn't scroll correctly to show the new focused form field - You always have to focus the first form field with the mouse, maybe we could focus the form field automatically when the view gets the focus. - It's not possible to add tabs in multiline text fields, I think we should use the move-focus-out signal in that case. ::: libview/ev-view.c @@ +2298,3 @@ ev_view_set_focused_element (view, mapping, field->page->index); + button = gtk_button_new (); I don't think we should use a GtkWidget for radio, check, and button fields, we should just draw the focus ring and handle space/intro keys when the current focus element is a button form field. @@ +2700,3 @@ + color.alpha = 0.0; + + gtk_widget_override_background_color (field_widget, GTK_STATE_FLAG_NORMAL, &color); Why changing the bg color? I think it's enough with showing the widget/drawing the focus ring. It's not easy to see the typed text in text areas with the blue background. @@ +2740,3 @@ +{ + EvFormField *field; + switch (event->keyval) { Leave an empty line between the declaration block and the function body. @@ +2742,3 @@ + switch (event->keyval) { + case GDK_KEY_ISO_Left_Tab: + case GDK_KEY_Tab: Instead of doing this, I think we should use the focus/move-focus signals of a GtkWidget, keeping our own focus chain, and moving the focus out when we reach the end of the chain. @@ +2760,3 @@ +} + +static GList* GList* -> GList * @@ +2790,3 @@ + return; + + guint n_pages = ev_document_get_n_pages (view->document); Move the variable declaration at the beginning of the function. @@ +2794,3 @@ + GList *list = page_form_field_order (view, page); + order = g_list_concat (order, list); + } I think we need to do this when loading the document or using a job after it's been loaded. This doesn't work for pages that hasn't been pre-rendered yet. @@ +4392,3 @@ highlight_forward_search_results (view, cr, i); + if (page_ready && view->form_field_focused) + highlight_form_field (view, cr); I think form fields should be focused like any other focusable element in the page. For form fields using widgets, showing the widget should be enough.
(In reply to comment #16) > Review of attachment 260277 [details] [review]: > > @@ +2794,3 @@ > + GList *list = page_form_field_order (view, page); > + order = g_list_concat (order, list); > + } > > I think we need to do this when loading the document or using a job after it's > been loaded. This doesn't work for pages that hasn't been pre-rendered yet. > This is the only part I'm not sure how to implement. Any hints on how to do this?
Fortunately and unfortunately I will be working full-time while a full-time student starting tomorrow so I will not have time for this. :( I hope someone will build upon my patch and improve it with Carlos' input.
Is there anything with Daniel's patch that would be considered a regression? If not, maybe we should commit it and then work on fine tuning it to address Carlos' input.
(In reply to comment #19) > Is there anything with Daniel's patch that would be considered a regression? > If not, maybe we should commit it and then work on fine tuning it to address > Carlos' input. In Evince (as in many projects), it works the other way around: first address the issues in the proposed patch, then apply the patch.
Created attachment 271701 [details] [review] Patch that adds support of cycling through form fields with tab button
Review of attachment 271701 [details] [review]: The patch looks clean, navigation through the form fields works, can also exit/enter the pdf form using only the tab/shift-tab combination. The only (minor) problem I have found is that, unlike text fields, checkboxes do not seem to be focused, when they are. Pressing space ticks/unticks them, but the visual clue that they are in focus is almost invisible. The form I have tested with was http://www.irs.gov/pub/irs-pdf/fw4.pdf.
Created attachment 272116 [details] screenshot #1
Created attachment 272117 [details] screenshot #2
Created attachment 272118 [details] screenshot #3
Thank you for so fast review. Focus rectangle is rendered by gtk_render_focus function for view->focused_element (not in my code, I just use ev_view_focus_form_field function, in which view->focused_element are set) and it's appearance depends on gtk theme and zoom level. In this document in some zoom levels focus rectangle overlaps document's checkbox symbol and probably could be invisible. I tested it with different gtk themes (Ambiance, Radiance and High Contrast) and I can see this rectangle(see attached screenshots). //Sorry for my english.
Review of attachment 271701 [details] [review]: Great job, this works mostly fine in general :-) There are just a few problems (see below) and a lot of coding style issues, so I've cleaned up the patch, fixed the problems and pushed it to git master. ::: libview/ev-view.c @@ +6456,3 @@ +static +gint ev_mapping_compare (gconstpointer _a, gconstpointer _b) static and return type go in the same line. We could use GCompareDataFunc and also receive the text direction @@ +6458,3 @@ +gint ev_mapping_compare (gconstpointer _a, gconstpointer _b) +{ + const EvMapping* a = _a; EvMapping* -> EvMapping * @@ +6459,3 @@ +{ + const EvMapping* a = _a; + const EvMapping* b = _b; Ditto. @@ +6471,3 @@ + /*if (text_direction == GTK_TEXT_DIR_RTL) + return (x1 < x2) ? 1 : ((x1 == x2) ? 0 : -1); + else*/ Do not leave commented code, we can simply use g_list_sort_with_data to receive the text direction and implement this. @@ +6478,3 @@ +} + +static GList* GList* -> GList * @@ +6479,3 @@ + +static GList* +ev_view_get_sorted_mappings_list(EvView* view, GtkDirectionType direction, gint page) EvView* view -> EvView *view And everywhere. @@ +6483,3 @@ + GList *list; + EvMappingList *forms_mapping; + forms_mapping = ev_page_cache_get_form_field_mapping (view->page_cache, view->current_page); You should use the passed in page instead of current page. @@ +6485,3 @@ + forms_mapping = ev_page_cache_get_form_field_mapping (view->page_cache, view->current_page); + + GList *tlist = g_list_copy(ev_mapping_list_get_list(forms_mapping)); This doesn't work if the page contains readonly fields, we need to filter them out. @@ +6504,3 @@ + GList* l = g_list_next (list); + return l; + } This is g_list_find() + g_list_next() @@ +6547,3 @@ + + if (finish) { + if (!ev_view_next_page(view)) You should move to the previous page when direction is GTK_DIR_TAB_BACKWARD. The problem is that when moving backwards it jumps to the beginning of the page breaking the focus chain. We could schedule a child_focus after changing the page with the hope that the form fields info is already available and the next/prev page has form fields. @@ +6550,3 @@ + { + ev_view_remove_all (view); + ev_view_set_focused_element (view, NULL, -1); I think we should always reset the focused element when there are no more fields in the page, no matter whether we change page or not. @@ +6564,3 @@ + ev_view_focus_form_field (view, field); + if (!EV_IS_FORM_FIELD_TEXT (field)) + gtk_widget_grab_focus (widget); I think this should be done by ev_view_focus_form_field() when focusing a form fields that doesn't use widgets. @@ +6571,3 @@ + { + gtk_widget_grab_focus (widget); + return TRUE; This is done by GtkContainer, so I think we can just chain up and let the parent do the job.
So, this is fixed now, please file new bug reports for any remaining issues.
Fantastic - it's great to see this over-6-year-old feature request implemented! I'll find this very useful. Thanks to everyone who helped out with this.