Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919973002:
desktop_aura: do not restore focused view if it has modal transient child (Closed)
|
Created:
3 years, 6 months ago by Qiang(Joe) Xu Modified:
3 years, 6 months ago Reviewers:
sky CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiondesktop_aura: do not restore focused view if it has modal transient child
Changes:
RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated.
BUG=727641
TEST=emulator test saw bug fixed and added test coverage
Review-Url: https://codereview.chromium.org/2919973002
Cr-Commit-Position: refs/heads/master@{#476835}
Committed: https://chromium.googlesource.com/chromium/src/+/81ead2ab2380d5f624b9ce63df050a76de134f06
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #Patch Set 3 : added test coverage #
Messages
Total messages: 22 (15 generated)
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== desktop_aura: do not restore focused view if it has modal transient child BUG=727641 ========== to ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 ==========
warx@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 ========== to ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 TEST=emulator test saw bug fixed ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi sky, PTAL thanks
Please add test coverage. https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:80: ui::ModalType type = Widget::GetWidgetForNativeView(window) Are you sure you don't want GetModalTransient(widget->GetNativeView()) ? https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:381: // The desktop native widget may have modal transient child, whose focus This just documents the code, the interesting part is the why. Why don't we restore focus in this case?
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Description was changed from ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 TEST=emulator test saw bug fixed ========== to ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 TEST=emulator test saw bug fixed and added test coverage ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi sky, comments addressed and test coverage added, ptal thanks https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:80: ui::ModalType type = Widget::GetWidgetForNativeView(window) On 2017/06/02 17:34:10, sky wrote: > Are you sure you don't want GetModalTransient(widget->GetNativeView()) ? I should use this. Thanks! https://codereview.chromium.org/2919973002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:381: // The desktop native widget may have modal transient child, whose focus On 2017/06/02 17:34:10, sky wrote: > This just documents the code, the interesting part is the why. Why don't we > restore focus in this case? Done.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496447265234130,
"parent_rev": "323d8da5b25996009a501ac3094ef33af52944b6", "commit_rev":
"81ead2ab2380d5f624b9ce63df050a76de134f06"}
Message was sent while issue was closed.
Description was changed from ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 TEST=emulator test saw bug fixed and added test coverage ========== to ========== desktop_aura: do not restore focused view if it has modal transient child Changes: RestoreFocusedView on DesktopNativeWidgetAura::HandleActivationChanged is added to process Linux desktop native widget asynchronous activation. This works when all the child widgets are managed by top level's FocusManager. However, this is not true for modal transient child, whose FocusManager is not the same as top level's FocusManager. In this case on activation, we shall not restore focused view on top level's FocusManager. Instead, the modal transient child window will get activated and restore focused view through its own NativeWidgetAura::OnWindowActivated. BUG=727641 TEST=emulator test saw bug fixed and added test coverage Review-Url: https://codereview.chromium.org/2919973002 Cr-Commit-Position: refs/heads/master@{#476835} Committed: https://chromium.googlesource.com/chromium/src/+/81ead2ab2380d5f624b9ce63df05... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/81ead2ab2380d5f624b9ce63df05...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2921253002/ by blundell@chromium.org. The reason for reverting is: The newly-added test is flaky: https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9... Reverting the CL rather than disabling the test as it's not clear where the flake is coming from: is it a problem in the test or in the production code added in this CL?. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
