GNOME Bugzilla – Bug 638905
Implement caret navigation
Last modified: 2013-06-12 10:42:43 UTC
Evince has atktext interface implementation but user experience is not as well as it could be. If we have a caret mode, similar to firefox F7, the accessibility user experience will be better. The caret mode: * Change between caret and normal mode presing F7 * Move inside text with normal keys: * LEFT/RIGHT - move one char * UP/DOWN - move between lines * CTRL + LEFT/RIGHT - move between words * SHIFT + move key - select text * Emit atk signal when moving to allow orca to read.
Created attachment 177745 [details] [review] Caret mode support for evince
Review of attachment 177745 [details] [review]: Hey Dani, thanks for the patch, it seems the patch might be split up into several smaller patches. It would make it easier to review. ::: data/evince-ui.xml @@ +52,3 @@ <menuitem name="ViewReload" action="ViewReload"/> + <separator/> + <menuitem name="ViewCaretMode" action="ViewCaretMode"/> I'm not sure we need any UI to enable caret mode, do browsers add a menu item for it? ::: libview/ev-view-accessible.c @@ +64,3 @@ GtkScrollType idle_scroll; GtkTextBuffer *buffer; + GtkTextIter *first_selection_iter; What's first_selection_iter? why first? @@ +708,3 @@ + gint *w, + gint *h) +{ Use GdkRectangle instead of x, y, width, height, it will make everything easier @@ +779,3 @@ + else { + gtk_text_iter_backward_char (&text_iter); + } Don't use braces for single line cluases, and, when needed, leave the else in the same line than the braces } else { @@ +786,3 @@ + go_next_page = TRUE; + } + else if (event->state & GDK_CONTROL_MASK) { In the Left case you didn't use an else if here, but a different if, is this correct? @@ +819,3 @@ + return TRUE; + } + else if (go_prev_page) { Don't use an else when the other if branch has a return. @@ +827,3 @@ + return TRUE; + } + instead of using go_next_page, go_prev_page and handled varaibles, you could factor it out using functions for go next/previus, something like: static gboolean ev_view_accesible_go_next_page (); and call it directly from the switch, instead of go_next_page = TRUE; return ev_view_accesible_go_next_page (); ::: libview/ev-view-accessible.h @@ +36,3 @@ + gint *y, + gint *w, + gint *h); These methods are public because they are used by ev-view, but ev-view-accessible is private so prefix them with an underscore to avoid confusions. ::: libview/ev-view-private.h @@ +199,3 @@ gboolean a11y_enabled; + gboolean caret_mode; + gint current_caret_offset; current_caret_offset is only used in ev-view-accessible.c, couldn't it be better moved to EvViewAccessiblePriv? ::: libview/ev-view.c @@ +4118,3 @@ + if (view->caret_mode) { + handled = ev_view_accessible_key_press_event (ev_view_get_accessible (widget), event); + if (handled) { This could be a single if: if (view->caret_mode && ev_view_accessible_key_press_event (ev_view_get_accessible (widget), event)) { ...... return TRUE; } we don't really need the handled variable. @@ +4134,3 @@ + view_rect.height *= view->scale; + view_rect.x = view_rect.x * view->scale + page_area.x; + view_rect.y = view_rect.y * view->scale + page_area.y; This is exactly what doc_rect_to_view_rect() does, if ev_view_accessible_get_caret_position() returned a EvRectangle you would only need: ev_view_accessible_get_caret_position (ev_view_get_accessible (widget), &doc_rect); doc_rect_to_view_rect (view, current_page, &doc_rect, &view_rect); @@ +4482,3 @@ + w = w * view->scale; + h = h * view->scale; + The same here, using EvRectangle and doc_rect_to_view_rect would make this easier. @@ +4486,3 @@ + cairo_translate (cr, (x * view->scale) + real_page_area.x, + (y * view->scale) + real_page_area.y); + caret_surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, w, h); Why do you need a new image surface?, why not just draw the rectangle directly into the widget cairo context. @@ +4489,3 @@ + cr2 = cairo_create (caret_surface); + + cairo_set_source_rgba (cr2, caret_r, caret_g, caret_b, caret_a); Why using variables? they have a const value. Also, if you are using 1.0 alpha don't use set_source_rgba(), use set_source_rgb() instead. @@ +4495,3 @@ + cairo_set_source_surface (cr, caret_surface, 0, 0); + cairo_paint (cr); + cairo_restore (cr); you should cairo_destroy cr2 here too. @@ +6384,3 @@ + gboolean mode) +{ + if (view->a11y_enabled) { Does caret mode need atk for something? I mean, could it be enabled even though a11y is not enabled? @@ +6392,3 @@ +void +ev_view_caret_selection (EvView *view) +{ This function might be more generic and not specific to caret mode, if we pass the rectangle we want to select. something like: ev_view_select_rectangle (EvView *view, EvRectangle *rect); and we create the rectangle from ev-view-accessible. @@ +6393,3 @@ +ev_view_caret_selection (EvView *view) +{ + if (EV_IS_SELECTION (view->document)) { Use and early return here. @@ +6413,3 @@ + x = (x * view->scale) + page_area.x; + y = (y * view->scale) + page_area.y; + Again, doc_rect_to_view_rect() does this. @@ +6416,3 @@ + if (view->selection_info.in_selection && view->selection_info.selections) { + view->motion.x = x; + view->motion.y = y; I don't understand this, view->motion is only changed when the pointer moves, but we are selecting with the keyboard, no? @@ +6439,3 @@ + + view->selection_info.in_selection = FALSE; + clear_selection (view); clear_selection already sets in_selection = FALSE, maybe we only need to change the name of clear_selection and make it public. The function is not specific to caret mode since it clear the current selection even if it was created with the mouse, so it should be simply ev_view_clear_selection() or something like that ::: shell/ev-window.c @@ +3969,3 @@ + "pages, allowing you to select text with the " + "keyboard. Do you want to turn Caret viewing on?"); + } Instead of asking the user, I would just let her know that caret mode is enabled/disabled, with information about how to enable/disable it. @@ +3974,3 @@ + GTK_MESSAGE_INFO, + GTK_BUTTONS_YES_NO, + message); Do not use a dialog, use the info bar, see EvMessageArea. We should probably add a checkbox "Don't show this again", this message could be very annoying.
Created attachment 177922 [details] [review] three patches to implement caret mode in evince I've fixed all things that Carlos commented about the previous patch. I splitted it in three patch too, to make patch revision easier.
Created attachment 178699 [details] [review] Positioning cursor at click This patch set the caret at clicked position when you're in caret mode.
Review of attachment 177922 [details] [review]: The order of the patches doesn't look correct, you are using ev_view_set_caret_mode() in the first patch, but the method is added in the last one. There are some other minor changes that look unrelated that might be committed now. ::: data/org.gnome.Evince.gschema.xml.in @@ +49,3 @@ + <key name="show-caret-msg" type="b"> + <default>true</default> + </key> This key shouldn't be added to the Default schema, but to the main one, since we want to always save this setting, not only when the user clikcks on save current settings as default. Also, I think we should remember whether caret mode is enabled or not, I'm not sure if it makes sense to remember it per document or if it should be global. ::: shell/ev-window.c @@ +3973,3 @@ + "keyboard."); + + ev_view_set_caret_mode (view, !ev_view_get_caret_mode (view)); why not adding ev_view_toggle_caret_mode()? ::: libview/ev-view.c @@ +273,1 @@ static void clear_link_selected (EvView *view); ev_view_get_accesible() might return NULL when a11y is not enabled @@ +6040,2 @@ { if (view->selection_info.selections) { Since Gtk 3.0 there's gtk_draw_insertion_cursor(), maybe we could use it here. ::: libview/ev-view-accessible.c @@ +131,3 @@ retval = ev_page_cache_get_text (page_cache, view->current_page); + if (retval) + gtk_text_buffer_set_text (priv->buffer, retval, -1); This looks unrelated, I guess this fixes a crash when page doesn't contain text. @@ +416,3 @@ return; + if (offset >= n_areas) This looks unrelated too. @@ +746,3 @@ + rect->x2 = evrect->x2; + rect->y1 = evrect->y1; + rect->y2 = evrect->y2; *rect = *evrect; @@ +1048,2 @@ atk_object_set_name (ATK_OBJECT (accessible), _("Document View")); + atk_object_set_role (ATK_OBJECT (accessible), ATK_ROLE_DOCUMENT_FRAME); Is this needed for caret mode? or is it unrelated too?
Comment on attachment 177922 [details] [review] three patches to implement caret mode in evince >+ >+void >+ev_view_set_caret_mode (EvView *view, >+ gboolean mode) >+{ >+ view->caret_mode = mode; >+ gtk_widget_queue_draw (GTK_WIDGET (view)); make sure the value actually changed and only queue a draw in such case. Also make sure a11y is enabled before setting care mode. >+} >+ >+gboolean >+ev_view_get_caret_mode (EvView *view) >+{ >+ return view->caret_mode; >+} >+
Created attachment 184662 [details] [review] a11y caret mode implemention Made changes proposed by Carlos in review.
Ping. :-) What's the status of this? It would be beyond awesome to finally have accessible PDFs in time for GNOME 3.2.
(In reply to comment #8) > Ping. :-) > > What's the status of this? It would be beyond awesome to finally have > accessible PDFs in time for GNOME 3.2. Now that 3.6 is out the door.... It would be beyond awesome to finally have accessible PDFs in time for GNOME 3.8. ;) Carlos I know you are crazy-busy, but could you please review Daniel's patch when you get a chance?
Carlos: Ping. :)
is there a way to get this in gnome 3.8 :)? this would pritty cool.
Hey, IMHO, the caret navigation should be independent of the a11y implementation as it is not really an a11y thing but something more general that can be used independently of having the accessibility support enabled or not. Based on Daniel's patches, I've been working on the caret navigation part independent of a11y (a11y support is left for later implementation). Apart from making it independent of a11y, I got rid of the GtkTextBuffer to avoid duplication of text for every page, for that reason, the functions to move the caret cursor are implemented by hand, and might need modifications for internationalization support. The keys currently supported are: - Left/right arrow: Move one character to the left/right. - Up/down arrow: Move up/down one line. - Ctrl + left/right arrow: Move to the beginning/end of the previous/next word. - Home/End: Move to the beginning/end of the current line. - Shift + arrow keys: Move from the current position to the destination position, selecting all text between the two positions. - Position the cursor after a click. Regards. Attaching the patches.
Created attachment 245530 [details] [review] Enable/disable caret navigation throught F7 shell, data, libview: Enable/disable the caret cursor navigation through F7. A EvMessageArea is shown to indicate that the caret navigation has been enabled/disabled.
Created attachment 245531 [details] [review] Caret navigation implementation libview: Navigation by character, word, next/previous line and beginning/end of the line using the caret cursor. The routines to move the cursor don't use GtkTextBuffer to avoid the duplication of the text for every page. - Left/right arrow: Move one character to the left/right. - Up/down arrow: Move up/down one line. - Ctrl + left/right arrow: Move to the beginning/end of the previous/next word. - Home/End: Move to the beginning/end of the current line.
Created attachment 245533 [details] [review] Position the caret after a mouse click ev-view: If caret cursor navigation is enabled, position the caret cursor in the right position after a mouse click.
Created attachment 245534 [details] [review] Text selection using the caret ev-view: Text selection using the caret cursor, if caret navigation is enabled. Shift + arrow keys: Move from the current position to the destination position, selecting all text between the two positions.
Hey, thanks for these patches. The main thing I see with these is the fact you don't use the TextBuffer. While I understand that you want to avoid duplication, the upside of using the TextBuffer is that it is already fairly optimized (when lines are not long) and it already supports Unicode word breaking (well, more or less, pango is still a little bit behind the latest recomendation)... so while your method might work well for english, it won't work for languages that do use unicode more agressively. Otherwise, you could think of using a pango_layout to know where the breaks are, but the good thing about text buffer is that it already does that.
I agree GtkTextBuffer makes the code easier, and that's the only reason why we used it in ev-view-accessible. We could use all the gail util functions that already work with GtkTextBuffers. But it's also very unefficient, since we are duplicating the pages text everytime we create the text buffer. Also, offsets in the text buffer don't match the ones in text layout. In ev-view-accessible we are currently using GtkTextBuffer for some cases and the poppler text layout in others. PDF layout is fixed so I don't think we really need PangoLayout, we already know the bbox of every character. So ideally we should get rid of the GtkTextBuffer in ev-view-accessible too and use always text layout. If there are bugs in poppler text layout we should fix them instead of workaround them. What do you think?
(In reply to comment #18) > I agree GtkTextBuffer makes the code easier, and that's the only reason why we > used it in ev-view-accessible. We could use all the gail util functions that > already work with GtkTextBuffers. FWIW: at this moment most project that in the past were using gail util functions based on GtkTextBuffers are starting to avoid its usage. Gtk made an a11y refactoring at the beginning of 2012 and it stopped to use GailTextUtil (fwiw, it has a private implementation based on Pango). Clutter uses a similar approach. WebKitGTK is also working on remove the dependency with gail-util [1]. In short, gail-util is a deprecated library, and should be stopped to be used, and obviously, not create new code based on it. [1] https://bugs.webkit.org/show_bug.cgi?id=114867
Carlos, I mainly agree with you. The only part I don't want to recreate here is the word breaking algorithm, which is already implemented by pango (and conforms, more or less to the latest Unicode recomendation for word breaking). There we should use that were possible to not redo this algorithm. Maybe we should investigate whether a new api in pango can be exposed so we can use the same algorithms in PopplerTextLayout.
I'm fine with using pango layout for word/line breaks.
Changing the summary as per comment 12. I will open a new bug for adding the accessible events associated with this support and make it depend upon this bug.
Review of attachment 245530 [details] [review]: For now, I think we can leave this patch a side until the caret navigation works well enough, We can move the EvView API to the patch that implements caret navigation in the view.
Review of attachment 245531 [details] [review]: Patch looks good in general, I agree that we should implement word/line breaking with pango and not assume that text is ASCII, but it works for many documents already and we can use it as an initial base implementation. I've merged the API part of the previous patch (slightly renamed) into this one, and fixed all the minor issues I mention below and pushed it to git master so that we can continue working on top of this. Thanks! ::: libview/ev-view.c @@ +3554,2 @@ static gboolean +get_caret_cursor_doc_rect_from_offset (EvView *view, Every time this is used returned rect is transformed to view coords, because the view rect is what we want. We could make the conversion here to make the code simpler for the callers. Renamed as get_caret_cursor_rect_from_offset @@ +3612,3 @@ + + if (style_color) + { Move brace to the if line. @@ +3624,3 @@ +} + +// This is a clone of the deprecated function gtk_draw_insertion_cursor. Don't use C++ comments. @@ +3637,3 @@ + + context = gtk_widget_get_style_context (widget); + get_cursor_color(context, &cursor_color); Leave a space between function name and parentheses. @@ +4498,3 @@ + if (view->cursor_offset >= n_areas) { + return ev_view_next_page (view); + } Don't use braces for single statements ifs. @@ +4524,3 @@ + return FALSE; + + // Skip blanks and new lines Don't use C++ comments, we should add a FIXME here to implement this properly using pango. @@ +4525,3 @@ + + // Skip blanks and new lines + for (i = view->cursor_offset - 1; i >= 0 && (page_text[i] == ' ' || page_text[i] == '\n'); i--); Since we are already assuming text is ascii we could use g_ascii_isspace() instead. @@ +4534,3 @@ + + // Move to the beginning of the word + for (j = i; j >= 0 && page_text[j] != ' ' && page_text[j] != '\n'; j--); Same comments here. @@ +4539,3 @@ + view->cursor_offset = 0; + else + view->cursor_offset = ++j; We don't really want to change the alue of j, I think this is easier to read as view->cursor_offset = j + 1; @@ +4564,3 @@ + + // Skip blanks and new lines + for (i = view->cursor_offset; i < n_areas && (page_text[i] == ' ' || page_text[i] == '\n'); i++); Same comments here about C++ comments, FIXME and g_ascii_isspace @@ +4573,3 @@ + + // Move to the end of the word + for (j = i; j < (gint) n_areas && page_text[j] != ' ' && page_text[j] != '\n' ; j++); Ditto. @@ +4607,3 @@ + view->cursor_offset = 0; + } else { + view->cursor_offset = ++i; Same here, I think view->cursor_offset = i + 1; is a bit easier to read. @@ +4616,3 @@ +cursor_backward_line (EvView *view) +{ + if (!cursor_go_to_line_start (view)) The behaviour of the cursor when moving between lins is very confusing. Going to the next line always moves the cursor at the beginning of the line, and moving to the previous line move the cursor to end. When moving down it's easy to see where the cursor is, but if you then move the previous line, it seems like the cursor has been lost, because it's at the end of the previous line. I think we should save the offset of the cursor inside the line and use the same for the prev/next line. If the offset is too long for the next/prev line we position it at the end of the line. I think this is what other apps do. We could just add a FIXME here and implement this in a follow up patch. @@ +4715,3 @@ + } + + gtk_widget_queue_draw (GTK_WIDGET (view)); At this point we might not draw the caret at the new position if get_caret_cursor_doc_rect_from_offset returns FALSE below, so I think we could move this after the ensure_rectangle_is_visible(). @@ +7161,3 @@ + ev_page_cache_get_text_layout (view->page_cache, view->current_page, &areas, &n_areas); + if (areas) + view->cursor_offset = n_areas; I think we could move this a function cursor_go_to_page_end() similar to cursor_go_to_line_end(). And also add cursor_got_to_pagestart for consistency even if it's simply sets the cursor offset to 0.
I've just pushed a couple of patches to use pango to move the caret cursor. There are at least two important things to fix in this base impementation: - Don't move the caret to the beginning/end of the line when moving between lines - I've noticed that the caret cursor is automatically updated every time the current page changes. I don't think this is a good idea and has some weird effects. For example, in continuous mode, place the cursor at the last line and start scrolling slowly. As soon as the next page is marked as the current one the caret jumps to the next page. I think we should save both the page where the caret is, and the offset inside the page and never change the page automatically. To move the care between pages faster we should handle also page up/down and move the caret one page backward/forward. This is what all other apps do (libreoffice, acroread, etc.) I also wonder if we should make the caret cursor blink, when moving between pages it's very easy to lose the cursor and quite hard to find.
To make it clear the remaining tasks, I've just opened new bug reports for all them. So, I'm closing this bug and marking current patches as obsolete. Please submit rebased patches to the new bugs. https://bugzilla.gnome.org/show_bug.cgi?id=702079 https://bugzilla.gnome.org/show_bug.cgi?id=702068 https://bugzilla.gnome.org/show_bug.cgi?id=702071 https://bugzilla.gnome.org/show_bug.cgi?id=702073 https://bugzilla.gnome.org/show_bug.cgi?id=702074 https://bugzilla.gnome.org/show_bug.cgi?id=702075 https://bugzilla.gnome.org/show_bug.cgi?id=702076 https://bugzilla.gnome.org/show_bug.cgi?id=702078