Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2070163002:
Fix "modal isn't modal in multi displays" issue (Closed)
|
Created:
4 years, 6 months ago by oshima Modified:
4 years, 6 months ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix "modal isn't modal in multi displays" issue
This is pretty bad bug because you can do almost everything while system modal dialog is open.
* Generalized the logic to check if the window is above blocking container.
I believe I kept the original behavior otherwise. Please let me know if you noticed something different.
BUG=620406
TEST=manual, covered by tests
Committed: https://crrev.com/9a61ecf55390bfbaa15ec3da2ad529bda9d63e9c
Cr-Commit-Position: refs/heads/master@{#400589}
Patch Set 1 : Modal fix #
Total comments: 2
Patch Set 2 : Modal fix #
Total comments: 23
Patch Set 3 : Modal fix #
Total comments: 3
Patch Set 4 : addressed #Patch Set 5 : rebase #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070163002/80001
Description was changed from ========== Modal fix Remove obsolete comment. BUG=None TEST=None Review-Url: https://codereview.chromium.org/2068223002 Cr-Commit-Position: refs/heads/master@{#399927} ========== to ========== Modal fix BUG=620406 TEST=manual, covered by tests ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070163002/120001
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Modal fix BUG=620406 TEST=manual, covered by tests ========== to ========== Fix "modal isn't modal in multi displays" issue This is pretty bad bug because you can do almost everything while system modal dialog is open. * Generalized the logic to check if the window is above blocking container. I believe I kept the original behavior otherwise. Please let me know if you noticed something different. BUG=620406 TEST=manual, covered by tests ==========
oshima@chromium.org changed reviewers: + jamescook@chromium.org
https://codereview.chromium.org/2070163002/diff/120001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.cc (left): https://codereview.chromium.org/2070163002/diff/120001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.cc:125: return true; This was the culprit. If you have another modal screen, this return always true. https://codereview.chromium.org/2070163002/diff/120001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager_unittest.cc (right): https://codereview.chromium.org/2070163002/diff/120001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:659: InputTestDelegate() {} According to the c++11 thread, this is still ok. (sorry for false alarm)
https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:265: bool IsWindowAboveContainer(aura::Window* window, nit: move up above the class definitions, near the other functions https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:299: DCHECK_EQ(common_parent, blocking->parent()); I don't understand how this algorithm works. Maybe you could explain it in a comment? In particular, imagine this hierarchy: Root --> LockScreenContainers --> LockScreenModal --> my_window blocking starts at LockScreenModal target starts at my_window I think we get to here and target->parent() and blocking->parent() are not the same. I must be misunderstanding something. https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:432: if (modal_container) { Shouldn't we always have a modal_container? https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:438: modal_layout_manager = nullptr; do you need this else clause? https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.cc:118: return modal_window() != nullptr && For my knowledge: Do we now prefer comparing to nullptr vs. if (!pointer) and if (pointer) ? https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.h (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:57: // True if the window is either contianed by the top most modal window, nit: "contained" https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:59: bool IsActiveModalWindows(aura::Window* window); nit: maybe IsActiveModalWindow() singular? https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager_unittest.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:683: ui::test::EventGenerator e1(Shell::GetPrimaryRootWindow(), window.get()); nit: e1 -> generator, or event_generator, or something longer https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:684: e1.ClickLeftButton(); optional: Do you need to do this click? It might be clearer just to go to the "events should be blocked" case and have the mouse_event_count() be 0.
https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:265: bool IsWindowAboveContainer(aura::Window* window, On 2016/06/16 20:31:35, James Cook wrote: > nit: move up above the class definitions, near the other functions Done. https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:299: DCHECK_EQ(common_parent, blocking->parent()); On 2016/06/16 20:31:35, James Cook wrote: > I don't understand how this algorithm works. Maybe you could explain it in a > comment? In particular, imagine this hierarchy: > > Root --> LockScreenContainers --> LockScreenModal --> my_window > > blocking starts at LockScreenModal > target starts at my_window > > I think we get to here and target->parent() and blocking->parent() are not the > same. I must be misunderstanding something. Comparison start from the root and that's probably what you missed. I added comment at the beginning of the while loop. * If children are same, continue because it's in the same path. * If different - and if target's container is above blocking container, then it's above blocking path. - and if target's container is below blocking container, then it's below blocking path. https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:432: if (modal_container) { On 2016/06/16 20:31:35, James Cook wrote: > Shouldn't we always have a modal_container? yes, you're right. Done. https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:438: modal_layout_manager = nullptr; On 2016/06/16 20:31:35, James Cook wrote: > do you need this else clause? Changed to modal_container for line 450. The condition was a bit different (was using modal_layout_manager), and I forgot to update this. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.cc:118: return modal_window() != nullptr && On 2016/06/16 20:31:35, James Cook wrote: > For my knowledge: Do we now prefer comparing to nullptr vs. if (!pointer) and if > (pointer) ? I saw the discussion but I haven't read to the conslusion. I'm happy to change, so let me know No, I don't think there is preference. Changed to !pointer to be consistent. (I think UI code tends to prefer this, probably because it's shorter) I've seen a recent discussion but I don't think there was any consensus on this. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.h (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:57: // True if the window is either contianed by the top most modal window, On 2016/06/16 20:31:35, James Cook wrote: > nit: "contained" Done. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:59: bool IsActiveModalWindows(aura::Window* window); On 2016/06/16 20:31:36, James Cook wrote: > nit: maybe IsActiveModalWindow() singular? There can be multiple windows that are part of active window. It could be transient window, or TYPE_CHILD window for example. I'm happy to change if you have better idea, so let me know. (I was using "IsPartOfActiveModalWindow".) https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager_unittest.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:683: ui::test::EventGenerator e1(Shell::GetPrimaryRootWindow(), window.get()); On 2016/06/16 20:31:36, James Cook wrote: > nit: e1 -> generator, or event_generator, or something longer Done. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:684: e1.ClickLeftButton(); On 2016/06/16 20:31:36, James Cook wrote: > optional: Do you need to do this click? It might be clearer just to go to the > "events should be blocked" case and have the mouse_event_count() be 0. I want to make sure I can receive the event first, but I can remove if you want. let me know.
LGTM https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/root_window_contro... ash/root_window_controller.cc:299: DCHECK_EQ(common_parent, blocking->parent()); On 2016/06/17 00:06:12, oshima wrote: > On 2016/06/16 20:31:35, James Cook wrote: > > I don't understand how this algorithm works. Maybe you could explain it in a > > comment? In particular, imagine this hierarchy: > > > > Root --> LockScreenContainers --> LockScreenModal --> my_window > > > > blocking starts at LockScreenModal > > target starts at my_window > > > > I think we get to here and target->parent() and blocking->parent() are not the > > same. I must be misunderstanding something. > > Comparison start from the root and that's probably what you missed. I added > comment at the beginning of the while loop. > > * If children are same, continue because it's in the same path. > * If different > - and if target's container is above blocking container, then it's above > blocking path. > - and if target's container is below blocking container, then it's below > blocking path. Ah, yes, I missed that the roots were at the end of the array. That makes more sense now. The comment helps. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.cc:118: return modal_window() != nullptr && On 2016/06/17 00:06:12, oshima wrote: > On 2016/06/16 20:31:35, James Cook wrote: > > For my knowledge: Do we now prefer comparing to nullptr vs. if (!pointer) and > if > > (pointer) ? > > I saw the discussion but I haven't read to the conslusion. I'm happy to change, > so let me know > > No, I don't think there is preference. Changed to !pointer to be consistent. (I > think UI code tends to prefer this, probably because it's shorter) > > I've seen a recent discussion but I don't think there was any consensus on this. I prefer !pointer myself, just for consistency with old code. (Out of curiosity, on which mailing list have you seen these discussions? Chromium-dev?) https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.h (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:59: bool IsActiveModalWindows(aura::Window* window); On 2016/06/17 00:06:12, oshima wrote: > On 2016/06/16 20:31:36, James Cook wrote: > > nit: maybe IsActiveModalWindow() singular? > > There can be multiple windows that are part of active window. It could be > transient window, or TYPE_CHILD window for example. > > I'm happy to change if you have better idea, so let me know. > (I was using "IsPartOfActiveModalWindow".) IsPartOfActiveModalWindow() sounds good. https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager_unittest.cc (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:684: e1.ClickLeftButton(); On 2016/06/17 00:06:12, oshima wrote: > On 2016/06/16 20:31:36, James Cook wrote: > > optional: Do you need to do this click? It might be clearer just to go to the > > "events should be blocked" case and have the mouse_event_count() be 0. > > I want to make sure I can receive the event first, but I can remove if you want. > let me know. No, this is fine. https://codereview.chromium.org/2070163002/diff/160001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2070163002/diff/160001/ash/root_window_contro... ash/root_window_controller.cc:215: // A root window is put at the last so that we compare windows at nit: Either "The root window is put at the end" or "The root window is the last item in the list." https://codereview.chromium.org/2070163002/diff/160001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager_unittest.cc (right): https://codereview.chromium.org/2070163002/diff/160001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager_unittest.cc:704: window.reset(); Nice test, BTW.
https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... File ash/wm/system_modal_container_layout_manager.h (right): https://codereview.chromium.org/2070163002/diff/140001/ash/wm/system_modal_co... ash/wm/system_modal_container_layout_manager.h:59: bool IsActiveModalWindows(aura::Window* window); On 2016/06/17 16:04:43, James Cook wrote: > On 2016/06/17 00:06:12, oshima wrote: > > On 2016/06/16 20:31:36, James Cook wrote: > > > nit: maybe IsActiveModalWindow() singular? > > > > There can be multiple windows that are part of active window. It could be > > transient window, or TYPE_CHILD window for example. > > > > I'm happy to change if you have better idea, so let me know. > > (I was using "IsPartOfActiveModalWindow".) > > IsPartOfActiveModalWindow() sounds good. Done. https://codereview.chromium.org/2070163002/diff/160001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2070163002/diff/160001/ash/root_window_contro... ash/root_window_controller.cc:215: // A root window is put at the last so that we compare windows at On 2016/06/17 16:04:43, James Cook wrote: > nit: Either "The root window is put at the end" or "The root window is the last > item in the list." Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2070163002/#ps180001 (title: "addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070163002/180001
The CQ bit was unchecked by oshima@chromium.org
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2070163002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070163002/240001
Message was sent while issue was closed.
Description was changed from ========== Fix "modal isn't modal in multi displays" issue This is pretty bad bug because you can do almost everything while system modal dialog is open. * Generalized the logic to check if the window is above blocking container. I believe I kept the original behavior otherwise. Please let me know if you noticed something different. BUG=620406 TEST=manual, covered by tests ========== to ========== Fix "modal isn't modal in multi displays" issue This is pretty bad bug because you can do almost everything while system modal dialog is open. * Generalized the logic to check if the window is above blocking container. I believe I kept the original behavior otherwise. Please let me know if you noticed something different. BUG=620406 TEST=manual, covered by tests ==========
Message was sent while issue was closed.
Committed patchset #5 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Fix "modal isn't modal in multi displays" issue This is pretty bad bug because you can do almost everything while system modal dialog is open. * Generalized the logic to check if the window is above blocking container. I believe I kept the original behavior otherwise. Please let me know if you noticed something different. BUG=620406 TEST=manual, covered by tests ========== to ========== Fix "modal isn't modal in multi displays" issue This is pretty bad bug because you can do almost everything while system modal dialog is open. * Generalized the logic to check if the window is above blocking container. I believe I kept the original behavior otherwise. Please let me know if you noticed something different. BUG=620406 TEST=manual, covered by tests Committed: https://crrev.com/9a61ecf55390bfbaa15ec3da2ad529bda9d63e9c Cr-Commit-Position: refs/heads/master@{#400589} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9a61ecf55390bfbaa15ec3da2ad529bda9d63e9c Cr-Commit-Position: refs/heads/master@{#400589} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
