Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
830053004:
Suppress accessibility events when user is navigating away. (Closed)
|
Created:
5 years, 11 months ago by dmazzoni Modified:
5 years, 10 months ago Reviewers:
nasko CC:
chromium-reviews, creis+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, nasko+codewatch_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuppress accessibility events when user is navigating away.
When the user navigates to a new page, stop sending accessibility
events on the old page.
BUG=421116, 450409
Committed: https://crrev.com/6ce40a1e561892849c1f6ac070dda140f6cc0115
Cr-Commit-Position: refs/heads/master@{#314812}
Committed: https://crrev.com/bf8cec44f5f9e9640028b292d201d888c4e2c690
Cr-Commit-Position: refs/heads/master@{#315231}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Switch to DidStartLoading, send notification on reload #Patch Set 3 : Fix indentation #
Total comments: 7
Patch Set 4 : Address feedback #Patch Set 5 : Renamed test so it only runs on win #Patch Set 6 : Rebase #Patch Set 7 : Rebase, disable test for now #
Messages
Total messages: 30 (8 generated)
dmazzoni@chromium.org changed reviewers: + nasko@chromium.org
This is the smallest working change that demonstrates what I'd like to do, with a test.
https://codereview.chromium.org/830053004/diff/1/chrome/browser/ui/views/acce... File chrome/browser/ui/views/accessibility/navigation_accessibility_win_uitest.cc (right): https://codereview.chromium.org/830053004/diff/1/chrome/browser/ui/views/acce... chrome/browser/ui/views/accessibility/navigation_accessibility_win_uitest.cc:247: EXPECT_NE("First Page", name); The test will fail on this line if we don't get an accessibility notification that the user has started navigating away in time. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2528: void WebContentsImpl::AboutToNavigateRenderFrame( This function is called at the right time, but (1) render_frame_host is the new RFH, not the old one still visible, and (2) it doesn't have reload_type, which I'd really like to pass to accessibility. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2549: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); This is the only function that gets called early enough, and has |reload_type| - but |render_frame_host| is the new RFH, not the "current" one. It'd be nice to have a notification on the current RFH that it's going to be replaced with another one.
https://codereview.chromium.org/830053004/diff/1/content/browser/accessibilit... File content/browser/accessibility/browser_accessibility_manager.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/accessibilit... content/browser/accessibility/browser_accessibility_manager.cc:157: void BrowserAccessibilityManager::UserIsNavigatingAway(bool is_reload) { This is unused. Why add this parameter now? https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2506: BrowserAccessibilityManager* manager = nit: wrong indent. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2528: void WebContentsImpl::AboutToNavigateRenderFrame( On 2015/01/21 18:59:03, dmazzoni wrote: > This function is called at the right time, but (1) render_frame_host is the new > RFH, not the old one still visible, and (2) it doesn't have reload_type, which > I'd really like to pass to accessibility. It doesn't need to be the new RFH in all cases. If navigation is to the same site, it will be the same one I think. OnDidStartLoading is much better signal to use, as it will catch both renderer-initiated navigations as well as browser-initiated navigations. It doesn't have the reload flag, but that one is also browser-initiated navigations specific. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2549: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); On 2015/01/21 18:59:04, dmazzoni wrote: > This is the only function that gets called early enough, and has |reload_type| - > but |render_frame_host| is the new RFH, not the "current" one. It'd be nice to > have a notification on the current RFH that it's going to be replaced with > another one. Architecturally, we can't notify before commit time what the new RFH is going to be. Commit time is too late for you, based on my understanding, otherwise there is such thing - RenderFrameHostChanged.
https://codereview.chromium.org/830053004/diff/1/content/browser/accessibilit... File content/browser/accessibility/browser_accessibility_manager.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/accessibilit... content/browser/accessibility/browser_accessibility_manager.cc:157: void BrowserAccessibilityManager::UserIsNavigatingAway(bool is_reload) { On 2015/02/03 17:28:05, nasko wrote: > This is unused. Why add this parameter now? I added it here to let you know it's something I'd like to make use of in a follow-up. I didn't add the code to make use of it because it'd just increase the size of this CL, but I could add it in if you want. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2528: void WebContentsImpl::AboutToNavigateRenderFrame( On 2015/02/03 17:28:05, nasko wrote: > On 2015/01/21 18:59:03, dmazzoni wrote: > > This function is called at the right time, but (1) render_frame_host is the > new > > RFH, not the old one still visible, and (2) it doesn't have reload_type, which > > I'd really like to pass to accessibility. > > It doesn't need to be the new RFH in all cases. If navigation is to the same > site, it will be the same one I think. > > OnDidStartLoading is much better signal to use, as it will catch both > renderer-initiated navigations as well as browser-initiated navigations. It > doesn't have the reload flag, but that one is also browser-initiated navigations > specific. So what should I use if I want to catch both browser-initiated and renderer-initiated, and the reload flag? Can we extend one of the notifications to provide all that? https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2549: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); On 2015/02/03 17:28:05, nasko wrote: > On 2015/01/21 18:59:04, dmazzoni wrote: > > This is the only function that gets called early enough, and has |reload_type| > - > > but |render_frame_host| is the new RFH, not the "current" one. It'd be nice to > > have a notification on the current RFH that it's going to be replaced with > > another one. > > Architecturally, we can't notify before commit time what the new RFH is going to > be. Commit time is too late for you, based on my understanding, otherwise there > is such thing - RenderFrameHostChanged. What I'm observing is that for a browser-initiated navigation, |render_frame_host| is the *new* RFH. I had to add this code to get the current frame host from the frame tree node in order to get the *old* RFH, because that's the one that's currently in the accessibility tree and we want to notify it that it should enter the "busy" state.
https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2528: void WebContentsImpl::AboutToNavigateRenderFrame( On 2015/02/03 17:40:54, dmazzoni wrote: > On 2015/02/03 17:28:05, nasko wrote: > > On 2015/01/21 18:59:03, dmazzoni wrote: > > > This function is called at the right time, but (1) render_frame_host is the > > new > > > RFH, not the old one still visible, and (2) it doesn't have reload_type, > which > > > I'd really like to pass to accessibility. > > > > It doesn't need to be the new RFH in all cases. If navigation is to the same > > site, it will be the same one I think. > > > > OnDidStartLoading is much better signal to use, as it will catch both > > renderer-initiated navigations as well as browser-initiated navigations. It > > doesn't have the reload flag, but that one is also browser-initiated > navigations > > specific. > > So what should I use if I want to catch both browser-initiated and > renderer-initiated, and the reload flag? Can we extend one of the notifications > to provide all that? Let's step back a second and clarify why is "reload" flag needed for fixing this issue? What will the flag help us know? https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2549: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); On 2015/02/03 17:40:54, dmazzoni wrote: > On 2015/02/03 17:28:05, nasko wrote: > > On 2015/01/21 18:59:04, dmazzoni wrote: > > > This is the only function that gets called early enough, and has > |reload_type| > > - > > > but |render_frame_host| is the new RFH, not the "current" one. It'd be nice > to > > > have a notification on the current RFH that it's going to be replaced with > > > another one. > > > > Architecturally, we can't notify before commit time what the new RFH is going > to > > be. Commit time is too late for you, based on my understanding, otherwise > there > > is such thing - RenderFrameHostChanged. > > What I'm observing is that for a browser-initiated navigation, > |render_frame_host| is the *new* RFH. I had to add this code to get the current > frame host from the frame tree node in order to get the *old* RFH, because > that's the one that's currently in the accessibility tree and we want to notify > it that it should enter the "busy" state. What you are observing is correct for this very specific bug you are trying to fix. If you go to google.com and then type another google.com, you shouldn't be getting a new RFH different from the current one.
https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2528: void WebContentsImpl::AboutToNavigateRenderFrame( On 2015/02/03 18:05:19, nasko wrote: > On 2015/02/03 17:40:54, dmazzoni wrote: > > On 2015/02/03 17:28:05, nasko wrote: > > > On 2015/01/21 18:59:03, dmazzoni wrote: > > > > This function is called at the right time, but (1) render_frame_host is > the > > > new > > > > RFH, not the old one still visible, and (2) it doesn't have reload_type, > > which > > > > I'd really like to pass to accessibility. > > > > > > It doesn't need to be the new RFH in all cases. If navigation is to the same > > > site, it will be the same one I think. > > > > > > OnDidStartLoading is much better signal to use, as it will catch both > > > renderer-initiated navigations as well as browser-initiated navigations. It > > > doesn't have the reload flag, but that one is also browser-initiated > > navigations > > > specific. > > > > So what should I use if I want to catch both browser-initiated and > > renderer-initiated, and the reload flag? Can we extend one of the > notifications > > to provide all that? > > Let's step back a second and clarify why is "reload" flag needed for fixing this > issue? What will the flag help us know? As soon as a navigation starts we can fire a notification telling screen readers exactly what's happening: either "page busy" (meaning navigation) or "page reloading". A prerequisite to firing those events is that we need to *not* fire any events on the "old" page, which is what this changelist fixes. Once we stop firing events on the old page, I can add the code to fire the busy/reload events along with tests for that. https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2549: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); On 2015/02/03 18:05:18, nasko wrote: > On 2015/02/03 17:40:54, dmazzoni wrote: > > On 2015/02/03 17:28:05, nasko wrote: > > > On 2015/01/21 18:59:04, dmazzoni wrote: > > > > This is the only function that gets called early enough, and has > > |reload_type| > > > - > > > > but |render_frame_host| is the new RFH, not the "current" one. It'd be > nice > > to > > > > have a notification on the current RFH that it's going to be replaced with > > > > another one. > > > > > > Architecturally, we can't notify before commit time what the new RFH is > going > > to > > > be. Commit time is too late for you, based on my understanding, otherwise > > there > > > is such thing - RenderFrameHostChanged. > > > > What I'm observing is that for a browser-initiated navigation, > > |render_frame_host| is the *new* RFH. I had to add this code to get the > current > > frame host from the frame tree node in order to get the *old* RFH, because > > that's the one that's currently in the accessibility tree and we want to > notify > > it that it should enter the "busy" state. > > What you are observing is correct for this very specific bug you are trying to > fix. If you go to http://google.com and then type another http://google.com, you shouldn't be > getting a new RFH different from the current one. That's true. The point is that I need a notification telling me that a navigation *may* be happening, and I need to know the current/previous RFH at that time. If the navigation leads us to the same RFH or a different RFH, or if it fails and we end up with the original RFH, either way we'll catch that in either DidCommitProvisionalLoad or DidFailProvisionalLoadWithError.
As discussed, this now uses DidStartLoading for its initial synchronous trigger, and we figure out whether it's a reload in DidStartProvisionalLoad.
https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2506: BrowserAccessibilityManager* manager = On 2015/02/03 17:28:05, nasko wrote: > nit: wrong indent. Done.
Awesome! Just a couple of comments to ensure we are always picking the right accessibility manager. https://codereview.chromium.org/830053004/diff/40001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/830053004/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility_manager_win.cc:195: // Don't fire events when this document is stale and the user has nit: s/document is stale and/document might be stale as/ https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2578: render_frame_host->browser_accessibility_manager(); Don't you need the same code as above to get the manager from the current host? If we have a cross-process navigation that fails, the provisional load is for the pending RFH and the fail will be for the pending RFH. Above you set UserIsReloading on the current, but here you reset the pending. https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2658: render_frame_host->browser_accessibility_manager(); This however should be fine, as we have committed and the pending RFH is now the current one and passed as the parameter. https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3834: // current RenderFrameHost. TODO(dmazzoni): do this using a It needn't navigate away from the current RFH. It is starting a navigation away from the current document, but the RFH might still be the same. nit: TODO on a new line.
https://codereview.chromium.org/830053004/diff/40001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/830053004/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility_manager_win.cc:195: // Don't fire events when this document is stale and the user has On 2015/02/04 21:12:08, nasko wrote: > nit: s/document is stale and/document might be stale as/ Done. https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2578: render_frame_host->browser_accessibility_manager(); On 2015/02/04 21:12:08, nasko wrote: > Don't you need the same code as above to get the manager from the current host? > If we have a cross-process navigation that fails, the provisional load is for > the pending RFH and the fail will be for the pending RFH. Above you set > UserIsReloading on the current, but here you reset the pending. Good catch, thanks. https://codereview.chromium.org/830053004/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3834: // current RenderFrameHost. TODO(dmazzoni): do this using a On 2015/02/04 21:12:08, nasko wrote: > It needn't navigate away from the current RFH. It is starting a navigation away > from the current document, but the RFH might still be the same. > > nit: TODO on a new line. Done.
LGTM
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830053004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
New patchsets have been uploaded after l-g-t-m from nasko@chromium.org
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830053004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830053004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6ce40a1e561892849c1f6ac070dda140f6cc0115 Cr-Commit-Position: refs/heads/master@{#314812}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/892513004/ by kelvinp@chromium.org. The reason for reverting is: NavigationAccessibilityTest.TestNavigateToNewUrl is failing on Win7 Tests (dbg)(1). Sample failure: http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28....
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830053004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bf8cec44f5f9e9640028b292d201d888c4e2c690 Cr-Commit-Position: refs/heads/master@{#315231} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
