GNOME Bugzilla – Bug 711862
Regression: Accessible text not updated when caret moves to a new page
Last modified: 2013-11-16 11:53:17 UTC
Created attachment 259589 [details] accessible-event listener Steps to reproduce: 1. Launch the attached accessible-event listener in a terminal 2. Open to (to be) attached test case in Evince 3. Enable caret navigation 4. Position the caret in the first line in the first page 5. Down Arrow from the top of the first page to the top of the second page Expected results: The text of the line containing the caret would be printed each time the caret moves, i.e.: ('Here is some text on page one. It is not especially exciting text, but I am not feeling particularly\n', 0, 101) ('inspired at the moment, because it is Monday. And Mondays tend to be wretched days.', 101, 184) ('In a universe controlled by me, there would be no Mondays and an extra Saturday.', 0, 80) Actual results: The text of the line containing the caret is printed for the lines on the first page. When the caret moves to top of the second page, the first line from the first page is printed out, i.e.: ('Here is some text on page one. It is not especially exciting text, but I am not feeling particularly\n', 0, 101) ('inspired at the moment, because it is Monday. And Mondays tend to be wretched days.', 101, 184) ('Here is some text on page one. It is not especially exciting text, but I am not feeling particularly\n', 0, 101) Impact: When an Orca user arrows to a new document page, Orca presents the text from the previous page and fails to present the actual current content. 8709b399d4911702dfbea4670b14deb92cc2f647 is the first bad commit commit 8709b399d4911702dfbea4670b14deb92cc2f647 Author: Carlos Garcia Campos <carlosgc@gnome.org> Date: Sun Jul 28 10:45:02 2013 +0200 libview: Update the current page also when pending scroll is to find a location Pending scroll SCROLL_TO_FIND_LOCATION, can jump to another page if the location is in a different page. We should change the current page in the model in this case too. :040000 040000 ae3824876a2facf59ccacacf6d45ec797c9090fc 26fa4a025947bcb264f213202c0296d2e4c57ab8 M libview
Created attachment 259590 [details] test case
FWIW, I have been testing this and confirmed the bug. Additionally ... > 8709b399d4911702dfbea4670b14deb92cc2f647 is the first bad commit > commit 8709b399d4911702dfbea4670b14deb92cc2f647 > Author: Carlos Garcia Campos <carlosgc@gnome.org> > Date: Sun Jul 28 10:45:02 2013 +0200 > > libview: Update the current page also when pending scroll is to find a > location > > Pending scroll SCROLL_TO_FIND_LOCATION, can jump to another page if the > location is in a different page. We should change the current page in > the model in this case too. > > :040000 040000 ae3824876a2facf59ccacacf6d45ec797c9090fc > 26fa4a025947bcb264f213202c0296d2e4c57ab8 M libview ... I tried to just revert this commit. And the regression gets solved.
I have been looking a little to this. The problem with that commit is that it affects how the current_page stored at evince is saved. In order to show this, I will put here some output (I added some printfs on evince code). I added one each time view->current_page is updated, and also when we got the text from the accessible object. So before that commit we have something like this: [view_update_range_and_current_page] entering if, view->pending_scroll==0 best_current_page = 0 [ev_view_accessible_get_text_buffer] Creating new buffer, using page 0 [ev_view_accessible_get_text_buffer] reusing buffer [ev_view_change_page] setting current page to 1 [view_update_range_and_current_page] [view_update_range_and_current_page] [ev_view_accessible_get_text_buffer] Creating new buffer, using page 1 And with the commit we have something like this: [view_update_range_and_current_page] entering if, view->pending_scroll==0 best_current_page = 0 [ev_view_accessible_get_text_buffer] Creating new buffer, using page 0 [ev_view_accessible_get_text_buffer] reusing buffer [ev_view_change_page] setting current page to 1 [view_update_range_and_current_page] entering if, view->pending_scroll==3 best_current_page = 0 setting current page to 0 [view_update_range_and_current_page] entering if, view->pending_scroll==3 best_current_page = 0 [ev_view_accessible_get_text_buffer] Creating new buffer, using page 0 As you can see the difference is that after the commit, the current_page value set on ev_view_change_page is overriden on ev_view_update_range_and_current_page using a wrong value. Now it enters on that if because that value is also contemplated. So in summary, the problem is that the value of view->current_page is wrong, something that is problematic, as that value seems to be used a lot. And after a quick check, is not only affecting accessibility. Another bug I realized is that if you have the thumbnails view (at the left) is not properly focused on the current page. And probably this is affecting other places. I guess that the correct solution would be compute properly best_current_page, but looking at the code that is not trivial. FWIW, we are right now on 3.10.2 week. So I'm wondering if it is worth to revert this change for a new evince stable release.
I think this is the expected behaviour, in continuous mode the current page is the one more visible in the screen. This is the reason why we keep current page and cursor_page as different things, because the cursor might not be in the current page (from the UI POV). The problem is that using the current page in ev-view-accessible has always been a problem. I think we should use the cursor_page in ev-view-accessible when caret navigation is enabled.
Created attachment 259917 [details] [review] Use cursor_page, only in this case This is a small change, that solves the issue only in this case, in order to minimize the place to change.
Created attachment 259918 [details] [review] Uses cursor_page as current_page (if caret navigation is enabled) on accessibility code on every place This patch replaces all view->current_page with a method to return the "relevant" page. If caret navigation is enabled, that is the cursor_page, if not current_page. In theory, this is the right thing to do, using cursor_page always, but this means more places modified, so more to test. FWIW, the specific of this bug is solved with this patch.
(In reply to comment #6) > > FWIW, the specific of this bug is solved with this patch. After some extra testing (thanks Joanmarie) this patch doesn't solve going up to the previous page. So probably cursor_page was not being updated properly while doing that.
Created attachment 259922 [details] [review] Updated patch The model page was being resetted on view_update_range_and_current_page to best_current_page. But if we are on cursor navigation mode, model needs to be using cursor_page. That is being updated on cursor_move callbacks.
Review of attachment 259922 [details] [review]: ::: libview/ev-view-accessible.c @@ +82,3 @@ + return view->cursor_page; + else + return view->current_page; Do not use else after a return, you can either not use the else, or do something like: return ev_view_is_caret_navigation_enabled (view) ? view->cursor_page : view->current_page; ::: libview/ev-view.c @@ +739,3 @@ ev_view_set_loading (view, FALSE); + if (!ev_view_is_caret_navigation_enabled (view)) + ev_document_model_set_page (view->model, best_current_page); I still don't understand this, we want the best current page to be the same no matter whether caret navigation is enabled or not, and that's why we keep current_page and cursor_page. If only one line of the second page is shown we want the first one to be the current one, even if the cursor is in the second one.
(In reply to comment #9) > Review of attachment 259922 [details] [review]: > > ::: libview/ev-view-accessible.c > @@ +82,3 @@ > + return view->cursor_page; > + else > + return view->current_page; > > Do not use else after a return, you can either not use the else, or do > something like: > > return ev_view_is_caret_navigation_enabled (view) ? view->cursor_page : > view->current_page; Ok > ::: libview/ev-view.c > @@ +739,3 @@ > ev_view_set_loading (view, FALSE); > + if (!ev_view_is_caret_navigation_enabled (view)) > + ev_document_model_set_page (view->model, > best_current_page); > > I still don't understand this, we want the best current page to be the same no > matter whether caret navigation is enabled or not, With that code best_current_page doesnt change, what change is what is being used on the model. > and that's why we keep > current_page and cursor_page. If only one line of the second page is shown we > want the first one to be the current one, even if the cursor is in the second > one. And as I said, that line doesn't change view->current_page or view->cursor_page value, only avoid to override which one is assigned to the model. Taking into account that ev_view_move_cursor is setting cursor_page to the model, I assumed that with caret_navigation_enabled, the model should been keep updated with cursor page.
hmm, good point, we are usually changing the page in the model before calling ensure_rectangle_is_visible, but I think we shouldn't, because it receives a view rect, and the scrolling used to make the rectangle visible will change the current page accordingly.
Created attachment 259929 [details] [review] Updated patch > ::: libview/ev-view-accessible.c > @@ +82,3 @@ > + return view->cursor_page; > + else > + return view->current_page; > > Do not use else after a return, you can either not use the else, or do > something like: > > return ev_view_is_caret_navigation_enabled (view) ? view->cursor_page : > view->current_page; Solved. > ::: libview/ev-view.c > @@ +739,3 @@ > ev_view_set_loading (view, FALSE); > + if (!ev_view_is_caret_navigation_enabled (view)) > + ev_document_model_set_page (view->model, > best_current_page); > > I still don't understand this, we want the best current page to be the same no > matter whether caret navigation is enabled or not, and that's why we keep > current_page and cursor_page. If only one line of the second page is shown we > want the first one to be the current one, even if the cursor is in the second > one. For me it is not clear which page should be assigned to the model (see my previous comment). But it is clear which page is relevant to accessibility with cursor. So I removed that change, and just clear the cache on the accessible if cursor_page changes. In order to do that, I added a check on cursor moved cb, and saved the previous cursor page. Doing that, this patch is only modifying ev-view-accessible. I didn't remove the clear_cache for page-changed, for the case of not navigation mode. Additionally I also made a change on ev_view_accessible_get_caret_offset. Before it automatically returned (so in the practice returned -1) if cursor_page and current_page are different. With this change that is allowed, so that extra check is not needed anymore. This change is needed in order to return a proper offset (Joanmarie detected it).
Comment on attachment 259929 [details] [review] Updated patch Pushed to both branches, thanks!