Reland: Remove EnsureRenderFrameHostVisibilityConsistent [chromium/src : main]
Michael Thiessen (Gerrit)
Michael Thiessen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Thiessen
| Commit-Queue | +1 |
1 comment
alexmos, please review content (hopefully for the last time xD)
chromium-ui-views-reviews, please review ui/views (and hopefully ui/aura?)
Related details
- Alex Moshchuk
- Chromium UI Views Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
kere...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ui/views/config.gwsq)
Related details
- Alex Moshchuk
- Keren Zhu
- Michael Thiessen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Thiessen (Gerrit)
Michael Thiessen added 1 comment![]( "Open in Gerrit")
- Alex Moshchuk
- Colin Blundell
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Thiessen (Gerrit)
Michael Thiessen added 1 comment![]( "Open in Gerrit")
blundell please review ui/
err I guess also chrome/browser and components/exo :)
Michael Thiessen (Gerrit)
Michael Thiessen added 1 comment![]( "Open in Gerrit")
WindowOpenDisposition::NEW_BACKGROUND_TAB,I don't know if this was intended to be supported or not, but this change breaks because we were trying to inject input into a background tab (which now fails). Do I need to carve out an exception for devtools protocol for visibility requirements?
Related details
- Alex Moshchuk
- Colin Blundell
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! I'll review for OWNERS after the others have given LGTM's on the technical changes.
Related details
- Alex Moshchuk
- Keren Zhu
- Michael Thiessen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 6 comments![]( "Open in Gerrit")
alexmos, please review content (hopefully for the last time xD)
chromium-ui-views-reviews, please review ui/views (and hopefully ui/aura?)
Thanks - admittedly the latest changes are making it more difficult for me to evaluate whether this change is ok, as I'm getting out of my depth. Hopefully other reviewers more familiar with the UI and rendering side of things can help review this as well.
On a high level, is my understanding correct that previously EnsureRenderFrameHostVisibilityConsistent forced the RWHV to pretend it was visible before the navigation actually committed (presumably in newly opened tabs?). That allowed aura::Window to route input events to it, allowing Ctrl+Tab to work. Now, the RWHV stays hidden until it actually commits, but that implies we need to punch holes to allow input to go to hidden windows in cases that rely on it today? And we're moving that enforcement from the UI layer to WebContents?
WindowOpenDisposition::NEW_BACKGROUND_TAB,I don't know if this was intended to be supported or not, but this change breaks because we were trying to inject input into a background tab (which now fails). Do I need to carve out an exception for devtools protocol for visibility requirements?
I'm not sure - this is a question for a DevTools reviewer. @ca...@chromium.org, can you help answer this or redirect to someone who might now?
I'd imagine DevTools probably does need to inject input into hidden tabs (e.g., for automation like ChromeDriver to drive background tabs, etc). Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
// TODO(https://crbug.com/521200679): Removing this breaks keyboard tab
// switching while a new tab is loading, despite the tab appearing to become
// visible at the correct time.Does this need to be updated?
}Why did you need to move this and also why skip the IsIgnoringWebInputEvents() check?
// WebContents that are never composited, like Extension background pages on
// ChromeOS still need to handle input while not visible.Does this comment only cover the `!is_never_composited_` exception? Can the comment explain the core check as well, which is to block input going to hidden pages? Presumably, this is the replacement for the aura::Window check?
if (!window_->IsVisible()) {It seems pretty dangerous to me to remove the IsVisible() checks in this layer. Are we certain that the new check in WebContentsImpl is guaranteed to cover the same cases as before? I don't know much about this code, but did this cover any widgets that didn't correspond to WebContents, like hidden buttons, dialogs, etc? It seems like we should avoid regressing this check for cases like that, if any.
Given this, I guess I wonder if your longer-term plan of decoupling whether the page is rendering vs whether the window is visible should be a prerequisite rather than a followup. I'm not really familiar with expectations at this layer, though, so happy to defer to an owner of this code.
Related details
- Andrey Kosyakov
- Keren Zhu
- Michael Thiessen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 3 comments![]( "Open in Gerrit")
Last step of making WebContents the sole manager of whether content
would like the RenderWidgetHostView to be visible.
Does the RWHV in this context include OOPIF's RWHVs?
Conceptually, I think the WebContent should only manage the visibility of the root frame and child frames / RWHVs should be managed by its parent frame.
Consider an invisible OOP <iframe> due to `display: none`, the WebContents will have no idea about the `display: none`, but the parent blink::FrameView does. The visibility state is propagate from blink::FrameView -> blink::RemoteFrameHost -> RenderFrameProxyHost -> CrossProcessFrameConnector -> RWHVChildFrame. The WebContents is no involved.
Per the comment in the code, EnsureRenderFrameHostVisibilityConsistent
should no longer be necessary as we unconditionally hide and show when
committing a navigation.To help me understand, can you point me to the comment?
if (!window_->IsVisible()) {It seems pretty dangerous to me to remove the IsVisible() checks in this layer. Are we certain that the new check in WebContentsImpl is guaranteed to cover the same cases as before? I don't know much about this code, but did this cover any widgets that didn't correspond to WebContents, like hidden buttons, dialogs, etc? It seems like we should avoid regressing this check for cases like that, if any.
Given this, I guess I wonder if your longer-term plan of decoupling whether the page is rendering vs whether the window is visible should be a prerequisite rather than a followup. I'm not really familiar with expectations at this layer, though, so happy to defer to an owner of this code.
+1. I would expect an invisible aura::Window or views::Widget to drop mouse or keyboard inputs. Note that these layers are used also in Browser UI and ChromeOS system UIs. These early returns are important to prevent users from unknowingly entering sensitive information into hidden UIs, e.g., by tricking user to press a quick succession of key combination which triggers and confirms an autofill popup. On ChromeOS I think there're apps from third parties. On Browser UI I think some Buy Now Pay Later (BNPL) UI is written in views and could be subject to this attack vector.
Related details
- Andrey Kosyakov
- Michael Thiessen
Michael Thiessen (Gerrit)
Michael Thiessen added 8 comments![]( "Open in Gerrit")
Alex Moshchukalexmos, please review content (hopefully for the last time xD)
chromium-ui-views-reviews, please review ui/views (and hopefully ui/aura?)
Thanks - admittedly the latest changes are making it more difficult for me to evaluate whether this change is ok, as I'm getting out of my depth. Hopefully other reviewers more familiar with the UI and rendering side of things can help review this as well.
On a high level, is my understanding correct that previously EnsureRenderFrameHostVisibilityConsistent forced the RWHV to pretend it was visible before the navigation actually committed (presumably in newly opened tabs?). That allowed aura::Window to route input events to it, allowing Ctrl+Tab to work. Now, the RWHV stays hidden until it actually commits, but that implies we need to punch holes to allow input to go to hidden windows in cases that rely on it today? And we're moving that enforcement from the UI layer to WebContents?
Yep, that's exactly right. Hopefully this is the right long term move - surely checking for visibility for input should be a cross-platform thing and not tied to aura.
I was discussing these changes with flackr@ maybe he can help with reviews?
Last step of making WebContents the sole manager of whether content
would like the RenderWidgetHostView to be visible.
Does the RWHV in this context include OOPIF's RWHVs?
Conceptually, I think the WebContent should only manage the visibility of the root frame and child frames / RWHVs should be managed by its parent frame.
Consider an invisible OOP <iframe> due to `display: none`, the WebContents will have no idea about the `display: none`, but the parent blink::FrameView does. The visibility state is propagate from blink::FrameView -> blink::RemoteFrameHost -> RenderFrameProxyHost -> CrossProcessFrameConnector -> RWHVChildFrame. The WebContents is no involved.
OOPIFs continue to be managed CrossProcessFrameConnector (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/cross_process_frame_connector.cc;drc=d503f89588de845f4903558934b7513747b1be95;l=432)
Perhaps I should clarify this comment to be about non-child RWHVs.
Per the comment in the code, EnsureRenderFrameHostVisibilityConsistent
should no longer be necessary as we unconditionally hide and show when
committing a navigation.To help me understand, can you point me to the comment?
WindowOpenDisposition::NEW_BACKGROUND_TAB,Alex MoshchukI don't know if this was intended to be supported or not, but this change breaks because we were trying to inject input into a background tab (which now fails). Do I need to carve out an exception for devtools protocol for visibility requirements?
I'm not sure - this is a question for a DevTools reviewer. @ca...@chromium.org, can you help answer this or redirect to someone who might now?
I'd imagine DevTools probably does need to inject input into hidden tabs (e.g., for automation like ChromeDriver to drive background tabs, etc). Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Yes
// TODO(https://crbug.com/521200679): Removing this breaks keyboard tab
// switching while a new tab is loading, despite the tab appearing to become
// visible at the correct time.Does this need to be updated?
Done
Why did you need to move this and also why skip the IsIgnoringWebInputEvents() check?
MayRenderWidgetForwardKeyboardEvent itself calls IsIgnoringWebInputEvents, so if I don't move it down here, the browser doesn't get a chance to handle key events (which happens in WebContentsImpl::PreHandleKeyboardEvent). I think the browser should be able to intercept key events regardless of the state of the renderer.
IsIgnoringWebInputEvents is also called a second time by input_router()->SendKeyboardEvent, so now we're calling it twice instead of three times :)
// WebContents that are never composited, like Extension background pages on
// ChromeOS still need to handle input while not visible.Does this comment only cover the `!is_never_composited_` exception? Can the comment explain the core check as well, which is to block input going to hidden pages? Presumably, this is the replacement for the aura::Window check?
Yep, this is the replacement for the aura::Window check. Expanded the comment.
Keren ZhuIt seems pretty dangerous to me to remove the IsVisible() checks in this layer. Are we certain that the new check in WebContentsImpl is guaranteed to cover the same cases as before? I don't know much about this code, but did this cover any widgets that didn't correspond to WebContents, like hidden buttons, dialogs, etc? It seems like we should avoid regressing this check for cases like that, if any.
Given this, I guess I wonder if your longer-term plan of decoupling whether the page is rendering vs whether the window is visible should be a prerequisite rather than a followup. I'm not really familiar with expectations at this layer, though, so happy to defer to an owner of this code.
+1. I would expect an invisible aura::Window or views::Widget to drop mouse or keyboard inputs. Note that these layers are used also in Browser UI and ChromeOS system UIs. These early returns are important to prevent users from unknowingly entering sensitive information into hidden UIs, e.g., by tricking user to press a quick succession of key combination which triggers and confirms an autofill popup. On ChromeOS I think there're apps from third parties. On Browser UI I think some Buy Now Pay Later (BNPL) UI is written in views and could be subject to this attack vector.
Okay, reverted this change. I'm a little concerned this will lead to breakage, but the browser shortcuts like ctrl+tab continue to work so maybe it's fine.
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Keren Zhu
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
WindowOpenDisposition::NEW_BACKGROUND_TAB,Alex MoshchukI don't know if this was intended to be supported or not, but this change breaks because we were trying to inject input into a background tab (which now fails). Do I need to carve out an exception for devtools protocol for visibility requirements?
Michael ThiessenI'm not sure - this is a question for a DevTools reviewer. @ca...@chromium.org, can you help answer this or redirect to someone who might now?
I'd imagine DevTools probably does need to inject input into hidden tabs (e.g., for automation like ChromeDriver to drive background tabs, etc). Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Yes
Yes, we totally need input dispatched to background tabs to work, otherwise the browser automation clients would all break. FWIW, if you need a way to differentiate devtools-injected input events, this should help:
Related details
- Alex Moshchuk
- Keren Zhu
- Michael Thiessen
- Robert Flack
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
return false;Same here, can we keep this early return?
// DCHECK(content_window_->IsVisible());Can we keep the DCHECK here and in NativeWidgetAura?
Related details
- Alex Moshchuk
- Michael Thiessen
- Robert Flack
Michael Thiessen (Gerrit)
Michael Thiessen added 2 comments![]( "Open in Gerrit")
return false;Same here, can we keep this early return?
We cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
// DCHECK(content_window_->IsVisible());Can we keep the DCHECK here and in NativeWidgetAura?
No, tests fail on these DCHECKS because it's possible to get events before the page commits. We could turn them into early returns - I don't know if that would break anything.
Related details
- Alex Moshchuk
- Keren Zhu
- Robert Flack
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
return false;Michael ThiessenSame here, can we keep this early return?
We cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
In that case I think the tab's aura::Window shouldn't have the focus. Some other aura::Window, e.g., the browser UI's aura::Window should have focus and handle Ctrl+Tab directly.
We have some code to prevent an invisible window from receiving focus, and hiding a window will cause a focus change, see [FocusController::OnWindowVisibilityChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=174;drc=d81edbd51ba06e32b3ecb78a6016ba7ff42f41e1) -> WindowLostFocusFromDispositionChange -> [SetFocusWindow](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=489;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1).
// DCHECK(content_window_->IsVisible());Michael ThiessenCan we keep the DCHECK here and in NativeWidgetAura?
No, tests fail on these DCHECKS because it's possible to get events before the page commits. We could turn them into early returns - I don't know if that would break anything.
Can you point me to the test failure? DesktopNativeWidgetAura is used by the top-level window (e.g. the browser UI window), i.e., it is not the direct container of a WebContents. Very likely we can fix the test and keep this DCHECK.
Related details
- Alex Moshchuk
- Michael Thiessen
- Robert Flack
Michael Thiessen (Gerrit)
Michael Thiessen added 2 comments![]( "Open in Gerrit")
return false;Michael ThiessenSame here, can we keep this early return?
Keren ZhuWe cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
In that case I think the tab's aura::Window shouldn't have the focus. Some other aura::Window, e.g., the browser UI's aura::Window should have focus and handle Ctrl+Tab directly.
We have some code to prevent an invisible window from receiving focus, and hiding a window will cause a focus change, see [FocusController::OnWindowVisibilityChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=174;drc=d81edbd51ba06e32b3ecb78a6016ba7ff42f41e1) -> WindowLostFocusFromDispositionChange -> [SetFocusWindow](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=489;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1).
No, the Window is supposed to have focus, as it's visible to the user. The bug is that Window::IsVisible does *not* track visibility, it tracks whether content is rendering.
I filed https://issues.chromium.org/issues/526983047, but that felt too big to tackle in this change and I'm probably not the person to do it.
// DCHECK(content_window_->IsVisible());Michael ThiessenCan we keep the DCHECK here and in NativeWidgetAura?
Keren ZhuNo, tests fail on these DCHECKS because it's possible to get events before the page commits. We could turn them into early returns - I don't know if that would break anything.
Can you point me to the test failure? DesktopNativeWidgetAura is used by the top-level window (e.g. the browser UI window), i.e., it is not the direct container of a WebContents. Very likely we can fix the test and keep this DCHECK.
Check patchset 1 of this CL - there are a ton of failing tests.
Related details
- Alex Moshchuk
- Keren Zhu
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Thiessen (Gerrit)
Michael Thiessen added 1 comment![]( "Open in Gerrit")
WindowOpenDisposition::NEW_BACKGROUND_TAB,Alex MoshchukI don't know if this was intended to be supported or not, but this change breaks because we were trying to inject input into a background tab (which now fails). Do I need to carve out an exception for devtools protocol for visibility requirements?
Michael ThiessenI'm not sure - this is a question for a DevTools reviewer. @ca...@chromium.org, can you help answer this or redirect to someone who might now?
I'd imagine DevTools probably does need to inject input into hidden tabs (e.g., for automation like ChromeDriver to drive background tabs, etc). Did this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Andrey KosyakovDid this also change because the input blocking was moved from aura to WebContentsImpl::ShouldIgnoreWebInputEvents?
Yes
Yes, we totally need input dispatched to background tabs to work, otherwise the browser automation clients would all break. FWIW, if you need a way to differentiate devtools-injected input events, this should help:
- Alex Moshchuk
- Andrey Kosyakov
- Keren Zhu
- Robert Flack
Keren Zhu (Gerrit)
Keren Zhu added 3 comments![]( "Open in Gerrit")
return false;Michael ThiessenSame here, can we keep this early return?
Keren ZhuWe cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
Michael ThiessenIn that case I think the tab's aura::Window shouldn't have the focus. Some other aura::Window, e.g., the browser UI's aura::Window should have focus and handle Ctrl+Tab directly.
We have some code to prevent an invisible window from receiving focus, and hiding a window will cause a focus change, see [FocusController::OnWindowVisibilityChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=174;drc=d81edbd51ba06e32b3ecb78a6016ba7ff42f41e1) -> WindowLostFocusFromDispositionChange -> [SetFocusWindow](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=489;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1).
No, the Window is supposed to have focus, as it's visible to the user. The bug is that Window::IsVisible does *not* track visibility, it tracks whether content is rendering.
I filed https://issues.chromium.org/issues/526983047, but that felt too big to tackle in this change and I'm probably not the person to do it.
It sounds wrong to me that an `IsVisble() == false` aura::Window is visible to the user. Such window should not have the focus - `aura::Window::Hide()` will ultimately call SetFocusWindow() to set focus on a different window.
On Linux / Win, the aura::Window hierarchy is like the following:
```
Each layer owns an aura::Window.
- DesktopNativeWidgetAura (Browser UI)
- WebContentsViewAura (Tab)
- RenderWidgetHostViewAura (primary main frame)
- RenderWidgetHostViewAura (optional, BFCache)
```
Before navigation commit I think `WebContentsViewAura`'s aura::Window should be visible to user, but RWHV's aura::Window shouldn't.
// DCHECK(content_window_->IsVisible());Michael ThiessenCan we keep the DCHECK here and in NativeWidgetAura?
Keren ZhuNo, tests fail on these DCHECKS because it's possible to get events before the page commits. We could turn them into early returns - I don't know if that would break anything.
Michael ThiessenCan you point me to the test failure? DesktopNativeWidgetAura is used by the top-level window (e.g. the browser UI window), i.e., it is not the direct container of a WebContents. Very likely we can fix the test and keep this DCHECK.
Check patchset 1 of this CL - there are a ton of failing tests.
It seems like a [kMouseCaptureChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/aura/window_event_dispatcher.cc;l=434;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1) event is sent to an invisible event during browser shutdown. This is probably OK.
```
#13 0x5d6193622503 logging::CheckError::~CheckError()
#14 0x5d619341317b views::DesktopNativeWidgetAura::OnMouseEvent()
...
#19 0x5d6195e501fa aura::WindowEventDispatcher::UpdateCapture()
...
#25 0x5d6195e3a20f aura::Window::SetVisibleInternal()
...
#28 0x5d6193410622 views::DesktopNativeWidgetAura::Hide()
#29 0x5d61933a841a views::Widget::Hide()
```
There're also tests that sens `kMouseExited` to a hidden window.
Instead of removing the CHECK entirely, can we CHECK that
```
if (!IsVisible()) {
CHECK(event->type() == ui::EventType::kMouseCaptureChanged
|| ui::EventType::kMouseExited);
}
```
here and in NativeWidgetAura::OnMouseEvent?
Again the aura::Window / NativeWidget layers are used beyond web content so we want to be very careful here.
// DCHECK(window_->IsVisible() || event->IsEndingEvent());I couldn't find a test failure related to this. Can you point me to it?
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Michael Thiessen
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
Should we also be checking IsIgnoringWebInputEvents like the skipped code above used to?
// ChromeOS still need to handle input while not visible.extension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Michael Thiessen
Michael Thiessen (Gerrit)
Michael Thiessen voted and added 5 comments![]( "Open in Gerrit")
Votes added by Michael Thiessen
| Commit-Queue | +1 |
5 comments
Should we also be checking IsIgnoringWebInputEvents like the skipped code above used to?
We do twice still, once in MayRenderWidgetForwardKeyboardEvent and once more in input_router()->SendKeyboardEvent. I could add it back probably, but it's redundant.
// ChromeOS still need to handle input while not visible.extension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
See the failing tests in Patchset 5, all of the SpokenFeedback tests are broken because they attempt to deliver input to an extension's background WebContents (this took a very long time to figure out xD).
It's just normal key events that are supposed to navigate around OS UI as far as I can tell.
return false;Michael ThiessenSame here, can we keep this early return?
Keren ZhuWe cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
Michael ThiessenIn that case I think the tab's aura::Window shouldn't have the focus. Some other aura::Window, e.g., the browser UI's aura::Window should have focus and handle Ctrl+Tab directly.
We have some code to prevent an invisible window from receiving focus, and hiding a window will cause a focus change, see [FocusController::OnWindowVisibilityChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=174;drc=d81edbd51ba06e32b3ecb78a6016ba7ff42f41e1) -> WindowLostFocusFromDispositionChange -> [SetFocusWindow](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=489;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1).
Keren ZhuNo, the Window is supposed to have focus, as it's visible to the user. The bug is that Window::IsVisible does *not* track visibility, it tracks whether content is rendering.
I filed https://issues.chromium.org/issues/526983047, but that felt too big to tackle in this change and I'm probably not the person to do it.
It sounds wrong to me that an `IsVisble() == false` aura::Window is visible to the user. Such window should not have the focus - `aura::Window::Hide()` will ultimately call SetFocusWindow() to set focus on a different window.
On Linux / Win, the aura::Window hierarchy is like the following:
```
Each layer owns an aura::Window.
- DesktopNativeWidgetAura (Browser UI)
- WebContentsViewAura (Tab)
- RenderWidgetHostViewAura (primary main frame)
- RenderWidgetHostViewAura (optional, BFCache)
```Before navigation commit I think `WebContentsViewAura`'s aura::Window should be visible to user, but RWHV's aura::Window shouldn't.
It's possible there's an input routing bug, but empirically the input is getting delivered to the aura::Window that's not visible when you input keyboard events after following a link that opens in a new window (the window that becomes visible when the page in the new window commits).
Tests are still failing with this revised check - I'm seeing kMouseEntered and kMouseMoved on NativeWidgetAura::OnMouseEvent
// DCHECK(window_->IsVisible() || event->IsEndingEvent());I couldn't find a test failure related to this. Can you point me to it?
- Alex Moshchuk
- Andrey Kosyakov
- Keren Zhu
- Michael Thiessen
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
return false;Michael ThiessenSame here, can we keep this early return?
Keren ZhuWe cannot, otherwise we don't deliver events to the browser (like ctrl+tab) before the page commits.
Michael ThiessenIn that case I think the tab's aura::Window shouldn't have the focus. Some other aura::Window, e.g., the browser UI's aura::Window should have focus and handle Ctrl+Tab directly.
We have some code to prevent an invisible window from receiving focus, and hiding a window will cause a focus change, see [FocusController::OnWindowVisibilityChanged](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=174;drc=d81edbd51ba06e32b3ecb78a6016ba7ff42f41e1) -> WindowLostFocusFromDispositionChange -> [SetFocusWindow](https://source.chromium.org/chromium/chromium/src/+/main:ui/wm/core/focus_controller.cc;l=489;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1).
Keren ZhuNo, the Window is supposed to have focus, as it's visible to the user. The bug is that Window::IsVisible does *not* track visibility, it tracks whether content is rendering.
I filed https://issues.chromium.org/issues/526983047, but that felt too big to tackle in this change and I'm probably not the person to do it.
Michael ThiessenIt sounds wrong to me that an `IsVisble() == false` aura::Window is visible to the user. Such window should not have the focus - `aura::Window::Hide()` will ultimately call SetFocusWindow() to set focus on a different window.
On Linux / Win, the aura::Window hierarchy is like the following:
```
Each layer owns an aura::Window.
- DesktopNativeWidgetAura (Browser UI)
- WebContentsViewAura (Tab)
- RenderWidgetHostViewAura (primary main frame)
- RenderWidgetHostViewAura (optional, BFCache)
```Before navigation commit I think `WebContentsViewAura`'s aura::Window should be visible to user, but RWHV's aura::Window shouldn't.
It's possible there's an input routing bug, but empirically the input is getting delivered to the aura::Window that's not visible when you input keyboard events after following a link that opens in a new window (the window that becomes visible when the page in the new window commits).
Do you have a reproducer or if there is a test case?
I see kMouseMoved on linux-chromeos-rel in PS16. I don't see kMouseEntered. A hidden window should never receive kMouseEntered. If it actually happens we should fix it.
There are multiple callers of `PostSynthesizeMouseMove()`. Could you debug the origin of kMouseMoved event in those failing tests? I think it can be reproduced using UTR or locally with [ChromeOS on Linux](https://g3doc.corp.google.com/company/teams/unicorn/family-link/chromeos/development/workflows/chromeos/linux-chromeos.md?cl=head).
Let me know if you have difficulties debugging this and I can help debug.
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Michael Thiessen
- Robert Flack
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
Since this change affects input stack on ChromeOS, +oshima san for review.
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Michael Thiessen
- Mitsuru Oshima
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Thiessen (Gerrit)
Michael Thiessen added 2 comments![]( "Open in Gerrit")
https://issues.chromium.org/issues/521200679
I can reproduce locally by hitting ctrl+tab shortly after following a link that opens in a new tab and is slow to commit. Links from gmail tend to work well for this.
(In order to repro, you need to patch this CL (remove EnsureRenderFrameHostVisibilityConsistent), but revert the changes to aura::Window)
I'm heading out on vacation next week - if you have time to help debug I would love the help.
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Keren Zhu
- Mitsuru Oshima
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
Michael ThiessenShould we also be checking IsIgnoringWebInputEvents like the skipped code above used to?
We do twice still, once in MayRenderWidgetForwardKeyboardEvent and once more in input_router()->SendKeyboardEvent. I could add it back probably, but it's redundant.
Acknowledged, thanks!
// ChromeOS still need to handle input while not visible.Michael Thiessenextension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
See the failing tests in Patchset 5, all of the SpokenFeedback tests are broken because they attempt to deliver input to an extension's background WebContents (this took a very long time to figure out xD).
It's just normal key events that are supposed to navigate around OS UI as far as I can tell.
Do you know where in the code we direclty dispatch events to the background page? This seems like it contradicts the idea that events are only dispatched to the visible contents, plus I don't know why a user would ever expect key input events to go to a background page or why extension background pages would ever expect to receive them - they can't be focused right? I see there's a shortcuts api https://developer.chrome.com/docs/extensions/reference/api/commands but this doesn't send the key event but just a special command event.
Or is the background page allowed to attach event listeners to the foreground page's dom? I'm not sure if this is how content scripts work, but if so, we should probably be doing a visibility check of the contents the event was targeted at, but could treat this as a todo.
Related details
- Alex Moshchuk
- Andrey Kosyakov
- Keren Zhu
- Michael Thiessen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Thiessen (Gerrit)
Michael Thiessen added 1 comment![]( "Open in Gerrit")
// ChromeOS still need to handle input while not visible.Michael Thiessenextension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
Robert FlackSee the failing tests in Patchset 5, all of the SpokenFeedback tests are broken because they attempt to deliver input to an extension's background WebContents (this took a very long time to figure out xD).
It's just normal key events that are supposed to navigate around OS UI as far as I can tell.
Do you know where in the code we direclty dispatch events to the background page? This seems like it contradicts the idea that events are only dispatched to the visible contents, plus I don't know why a user would ever expect key input events to go to a background page or why extension background pages would ever expect to receive them - they can't be focused right? I see there's a shortcuts api https://developer.chrome.com/docs/extensions/reference/api/commands but this doesn't send the key event but just a special command event.
Or is the background page allowed to attach event listeners to the foreground page's dom? I'm not sure if this is how content scripts work, but if so, we should probably be doing a visibility check of the contents the event was targeted at, but could treat this as a todo.
The page is created here: https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/extension_host.cc;drc=54dd37fb9545f90b93fed90434ebf30cadacf65e;l=148
I don't know how input is routed to it - the input routing code is very hard for me to follow as somebody not familiar with it.
Maybe dtseng@ knows how this works and what the correct solution would be?
Related details
- Alex Moshchuk
- Andrey Kosyakov
- David Tseng
- Keren Zhu
- Mitsuru Oshima
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
// ChromeOS still need to handle input while not visible.Michael Thiessenextension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
Robert FlackSee the failing tests in Patchset 5, all of the SpokenFeedback tests are broken because they attempt to deliver input to an extension's background WebContents (this took a very long time to figure out xD).
It's just normal key events that are supposed to navigate around OS UI as far as I can tell.
Michael ThiessenDo you know where in the code we direclty dispatch events to the background page? This seems like it contradicts the idea that events are only dispatched to the visible contents, plus I don't know why a user would ever expect key input events to go to a background page or why extension background pages would ever expect to receive them - they can't be focused right? I see there's a shortcuts api https://developer.chrome.com/docs/extensions/reference/api/commands but this doesn't send the key event but just a special command event.
Or is the background page allowed to attach event listeners to the foreground page's dom? I'm not sure if this is how content scripts work, but if so, we should probably be doing a visibility check of the contents the event was targeted at, but could treat this as a todo.
The page is created here: https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/extension_host.cc;drc=54dd37fb9545f90b93fed90434ebf30cadacf65e;l=148
I don't know how input is routed to it - the input routing code is very hard for me to follow as somebody not familiar with it.
Maybe dtseng@ knows how this works and what the correct solution would be?
If I understand correctly, this seems to just be a one-off for chromevox. How about we create a method on WebContentsDelegate::CanDeliverInputWhileHidden, overridden by ExtensionHost with something like this:
```c++
bool ExtensionHost::CanDeliverInput() const {
// Chromevox directly receives key events even though it is not visible
return extension_id_ == extension_misc::kChromeVoxExtensionId;
}
```
Then we can call it here through the `delegate_` interface. This should hopefully not break anything and also ensure that we don't allow keyboard events to accidentally be sent to regular extension pages.
Related details
- Alex Moshchuk
- David Tseng
- Keren Zhu
- Michael Thiessen
- Mitsuru Oshima
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
+katie. IIUC, after ChromeVox MV3, we would no longer rely upon sending keys to the background page. I don't reacall if we need to send keys to other pages e.g. offscreen documents.
Related details
- Alex Moshchuk
- Katie D
- Keren Zhu
- Michael Thiessen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Katie D (Gerrit)
Katie D added 1 comment![]( "Open in Gerrit")
+katie. IIUC, after ChromeVox MV3, we would no longer rely upon sending keys to the background page. I don't reacall if we need to send keys to other pages e.g. offscreen documents.
We will send keys to the background page through the new flow that's outside of extensions, the flow that was buggy during MV3 rollout.
But I see a ['keydown registered'](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/chromeos/accessibility/chromevox/mv3/offscreen/offscreen.ts;l=73?q=%27keydown%27%20-definitions%2F%20-braille%2F&ss=chromium%2Fchromium%2Fsrc:chrome%2Fbrowser%2Fresources%2Fchromeos%2Faccessibility%2F&start=11) in MV3 offscreen for learn mode. This might break?
Otherwise I believe we only send events to foreground extension pages after that like the tutorial and the chromevox panel.
I don't think the other extensions get keys directly at this point, except I'm not sure how braille works.
Related details
- Alex Moshchuk
Michael Thiessen (Gerrit)
Michael Thiessen added 3 comments![]( "Open in Gerrit")
Katie D+katie. IIUC, after ChromeVox MV3, we would no longer rely upon sending keys to the background page. I don't reacall if we need to send keys to other pages e.g. offscreen documents.
We will send keys to the background page through the new flow that's outside of extensions, the flow that was buggy during MV3 rollout.
But I see a ['keydown registered'](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/chromeos/accessibility/chromevox/mv3/offscreen/offscreen.ts;l=73?q=%27keydown%27%20-definitions%2F%20-braille%2F&ss=chromium%2Fchromium%2Fsrc:chrome%2Fbrowser%2Fresources%2Fchromeos%2Faccessibility%2F&start=11) in MV3 offscreen for learn mode. This might break?
Otherwise I believe we only send events to foreground extension pages after that like the tutorial and the chromevox panel.
I don't think the other extensions get keys directly at this point, except I'm not sure how braille works.
Ack, I think flackr's suggestion will cover this?
// ChromeOS still need to handle input while not visible.Michael Thiessenextension background pages handle input events? what kinds? are there some event types that should be allowed generally on hidden pages?
Robert FlackSee the failing tests in Patchset 5, all of the SpokenFeedback tests are broken because they attempt to deliver input to an extension's background WebContents (this took a very long time to figure out xD).
It's just normal key events that are supposed to navigate around OS UI as far as I can tell.
Michael ThiessenDo you know where in the code we direclty dispatch events to the background page? This seems like it contradicts the idea that events are only dispatched to the visible contents, plus I don't know why a user would ever expect key input events to go to a background page or why extension background pages would ever expect to receive them - they can't be focused right? I see there's a shortcuts api https://developer.chrome.com/docs/extensions/reference/api/commands but this doesn't send the key event but just a special command event.
Or is the background page allowed to attach event listeners to the foreground page's dom? I'm not sure if this is how content scripts work, but if so, we should probably be doing a visibility check of the contents the event was targeted at, but could treat this as a todo.
Robert FlackThe page is created here: https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/extension_host.cc;drc=54dd37fb9545f90b93fed90434ebf30cadacf65e;l=148
I don't know how input is routed to it - the input routing code is very hard for me to follow as somebody not familiar with it.
Maybe dtseng@ knows how this works and what the correct solution would be?
If I understand correctly, this seems to just be a one-off for chromevox. How about we create a method on WebContentsDelegate::CanDeliverInputWhileHidden, overridden by ExtensionHost with something like this:
```c++
bool ExtensionHost::CanDeliverInput() const {
// Chromevox directly receives key events even though it is not visible
return extension_id_ == extension_misc::kChromeVoxExtensionId;
}
```Then we can call it here through the `delegate_` interface. This should hopefully not break anything and also ensure that we don't allow keyboard events to accidentally be sent to regular extension pages.
Done
Have you had a chance to look into this Keren? I'll re-iterate that I believe the problem is that aura::Window visibility is tied to WebContents rendering, and not whether the window is actually visible to the user (and has input focus). Because of this we end up with a period of time where a new tab has been created, has input focus, and can be seen by the user before the WebContents starts rendering and marks the Window as Visible.
We cannot reject input during this time, or we drop inputs like ctrl+tab. If we were to keep focus on the previous tab until the new tab is rendering that would also probably not work as the previous tab would then be handling input while no longer visible.
Related details
- Alex Moshchuk
- David Tseng
- Katie D
- Keren Zhu
- Mitsuru Oshima
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted Code-Review+1![]( "Open in Gerrit")
- Alex Moshchuk
- David Tseng
- Katie D
- Keren Zhu
- Michael Thiessen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 1 comment![]( "Open in Gerrit")
I haven't yet. Sorry I am a bit overloaded at the moment. I'll spend some time today and hopefully get back to you tomorrow.
Related details
- Alex Moshchuk
- David Tseng
- Katie D
- Michael Thiessen
- Mitsuru Oshima
Keren Zhu (Gerrit)
I looked into how a new tab's `aura::Window` gets focused. The stack trace:
```
#0 content::RenderWidgetHostViewAura::Focus()
#1 content::WebContentsViewAura::Focus()
#2 content::WebContentsViewAura::SetInitialFocus()
#3 content::WebContentsViewAura::RestoreFocus()
#4 content::WebContentsImpl::RestoreFocus()
#5 BrowserView::OnActiveTabChanged()
#6 BrowserView::OnTabStripModelChanged()
#7 TabStripModel::NotifyIfActiveOrSelectionChanged()
#8 TabStripModel::ActivateTabAt()
#9 TabStripModel::AddTab()
```
`RenderWidgetHostViewAura::Focus()` successfully focuses its owning `aura::Window` on tab switch. During this process, `Window::CanFocus()` is called and returns true. `CanFocus()` does not check if the window is visible and therefore the focus change goes through. Looks like "hidden window is not focusable" is not a strict invariant in Aura.
I tried the idea to focus the WebContentsViewAura's `aura::Window` first, then when the page is ready, forward the focus to RWHV. The idea works and test passes, but I am not happy with the complexity it adds. https://paste.googleplex.com/5100868130177024.
So I am inclined to +1 to your solution - that is, to allow a hidden window to accept input events. To avoid breaking other `aura::Window` usage, I think we should limit this behavior to only RWHV's window. Can we add an virtual `aura::WindowDelegate::AllowsInputsForHiddenAuraWindow()`? Then, we can return true from `RenderWidgetHostViewAura` which is a subclass `WindowDelegate`.
Note that I am not the owner of `ui/aura`. Please ask osh...@chromium.org or zorai...@chromium.org for review. Thanks.
Related details
- Alex Moshchuk
- David Tseng
- Katie D
- Michael Thiessen
- Mitsuru Oshima
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |