Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17431003:
Dragging panels near screen edge (Closed)
|
Created:
7 years, 6 months ago by varkha Modified:
7 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, sadrul, extensions-reviews_chromium.org, ben+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDragging panels near screen edge
BUG=251413
BUG=251421
TBR=jeremya@chromium.org
TEST=Drag Hangout panels near screen edge with dual screens. Verify
that panels stay where they are left when mouse is released.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207972
Patch Set 1 : Dragging panels near screen edge (fixed broken test) #
Total comments: 13
Patch Set 2 : Dragging panels near screen edge (comments) #
Total comments: 1
Patch Set 3 : Dragging panels near screen edge (comments) #
Total comments: 3
Patch Set 4 : Dragging panels near screen edge (comments) #
Total comments: 4
Patch Set 5 : Dragging panels near screen edge (comments) #
Total comments: 4
Patch Set 6 : Dragging panels near screen edge (comments) #
Total comments: 2
Patch Set 7 : Dragging panels near screen edge (comments) #
Messages
Total messages: 33 (0 generated)
Please take a look. This could be taken into two separate CLs, but the bugs describe simmilar behavior so maybe this can go together.
https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... ash/wm/panels/panel_layout_manager.cc:668: visible_panels[i].window->SetProperty(internal::kPanelAttachedKey, true); Any panel that's being managed by panel layout manager should already have kPanelAttachedKey set in order for the panel to have been reparented to the correct layer. Is this not the case? https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... File ash/wm/panels/panel_window_resizer.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... ash/wm/panels/panel_window_resizer.cc:199: gfx::Point start_location_in_screen(GetInitialLocation()); Add comment that we use the start location to make sure that it is reparented to the same display as it was initially on. There may be a clearer way of ensuring this (i.e. just passing the root window's bounds) https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... ash/wm/panels/panel_window_resizer.cc:203: near_start_location); nit: Construct gfx::Rect in place for the parameter since it's only used once.
https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/tabs/tabs_api.cc:806: controller->window()->SetBounds(bounds); Avoiding this SetBounds call if the values don't change may work for hangouts, but I suspect if an application actually changes its bounds (uses a value other than its current bounds) during startup the error will still occur. I think the right thing to do is either revert the drag if we see a setbounds call during a drag or prevent the bounds update if the window is being dragged. I think the former makes more sense.
PTAL. Do you think dealing with real bounds change from inside the app while dragging needs to happen in this CL? https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... ash/wm/panels/panel_layout_manager.cc:668: visible_panels[i].window->SetProperty(internal::kPanelAttachedKey, true); This was not the case when the panel was getting attached by setting size / position from inside the app / API. While I have added some protection from calling SetBounds redundantly, it is still possible to use API to force panels to a particular position including position that it will attach to shelf. This is why I have added this here - it avoids assertions (debug builds actually terminate unless running under debugger) when trying to drag a panel after its position has been "stolen". https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... File ash/wm/panels/panel_window_resizer.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... ash/wm/panels/panel_window_resizer.cc:199: gfx::Point start_location_in_screen(GetInitialLocation()); On 2013/06/19 15:16:49, flackr wrote: > Add comment that we use the start location to make sure that it is reparented to > the same display as it was initially on. There may be a clearer way of ensuring > this (i.e. just passing the root window's bounds) Done. https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_window... ash/wm/panels/panel_window_resizer.cc:203: near_start_location); No need with using the root coordinates. https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/tabs/tabs_api.cc:806: controller->window()->SetBounds(bounds); True, but this has wider UX impact and probably no easy answer. For example when only the size changes (especially if it increases or shrinks but only vertically) reverting the drag may not be what a user would expect. Preventing bounds update may also be unexpected. In reality most drags would be fine so closing most glaring troubles could work. Because this is not bulletproof I had to set the attached property in the Relayout.
https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... ash/wm/panels/panel_layout_manager.cc:668: visible_panels[i].window->SetProperty(internal::kPanelAttachedKey, true); On 2013/06/19 15:49:22, varkha wrote: > This was not the case when the panel was getting attached by setting size / > position from inside the app / API. While I have added some protection from > calling SetBounds redundantly, it is still possible to use API to force panels > to a particular position including position that it will attach to shelf. This > is why I have added this here - it avoids assertions (debug builds actually > terminate unless running under debugger) when trying to drag a panel after its > position has been "stolen". It's my understanding that even if an application sets the bounds to be adjacent to the shelf this would not actually attach the panel since that wouldn't change the attached property and only the PanelWindowResizer can attach/detach panels.
https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/tabs/tabs_api.cc:806: controller->window()->SetBounds(bounds); On 2013/06/19 15:49:22, varkha wrote: > True, but this has wider UX impact and probably no easy answer. For example when > only the size changes (especially if it increases or shrinks but only > vertically) reverting the drag may not be what a user would expect. Preventing > bounds update may also be unexpected. In reality most drags would be fine so > closing most glaring troubles could work. > Because this is not bulletproof I had to set the attached property in the > Relayout. Since this should be fine for the simple case where none of the values are changed we can do this, but we should add a TODO with a bug and start a discussion about what the right thing to do in the general case is, when the values actually are changed.
https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... ash/wm/panels/panel_layout_manager.cc:668: visible_panels[i].window->SetProperty(internal::kPanelAttachedKey, true); The problem with dual screens was that SetBounds would reparent the window to a root window that has more than 50% of the screen. This would call PanelWindowResizer::FinishDragging an set the attached property to false. Then the panel would get added to the panel layout manager of the other display and slide_in logic would attach it to the shelf. Any drag after that would cause reparenting logic to try and reparent it to the same parent that the panel already has causing an assertion. Setting this property for panels that are slid_in and are added to the launcher avoids it closing the loophole. With http://crbug.com/251813 now open for discussion do you think this may stay as a safeguard? It could alternatively be placed in line 658 limiting the case a bit. https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/tabs/tabs_api.cc:806: controller->window()->SetBounds(bounds); Done. See http://crbug.com/251813.
https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/3001/ash/wm/panels/panel_layout... ash/wm/panels/panel_layout_manager.cc:668: visible_panels[i].window->SetProperty(internal::kPanelAttachedKey, true); When SetBounds is about to reparent the window, it should fire ScopedWindowResizer::OnWindowHierarchyChanging to finish the drag before the window is reparented. I suspect the problem might be that we compute the new default parent before actually trying to reparent (which will then change the attached property before the window is actually reparented). If this is the case I suppose we could remove the window before asking for the default parent when reparenting. On 2013/06/19 17:54:09, varkha wrote: > The problem with dual screens was that SetBounds would reparent the window to a > root window that has more than 50% of the screen. This would call > PanelWindowResizer::FinishDragging an set the attached property to false. Then > the panel would get added to the panel layout manager of the other display and > slide_in logic would attach it to the shelf. Any drag after that would cause > reparenting logic to try and reparent it to the same parent that the panel > already has causing an assertion. Setting this property for panels that are > slid_in and are added to the launcher avoids it closing the loophole. With > http://crbug.com/251813 now open for discussion do you think this may stay as a > safeguard? It could alternatively be placed in line 658 limiting the case a bit.
https://codereview.chromium.org/17431003/diff/11002/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/11002/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:331: return; If there's not an easy way to avoid the panel being added to the panel layout manager we *could* check the attached key here and reparent the panel if it's not actually attached. This would do quite the reparenting dance and I suspect we'd want a better long term solution but it should do the right thing.
Tried another approach - effectively calling CompleteDrag later rather than sooner. PTAL. https://codereview.chromium.org/17431003/diff/26002/ash/wm/toplevel_window_ev... File ash/wm/toplevel_window_event_handler.cc (right): https://codereview.chromium.org/17431003/diff/26002/ash/wm/toplevel_window_ev... ash/wm/toplevel_window_event_handler.cc:66: virtual void OnWindowHierarchyChanged( PTAL. I think this is a cleaner solution. This avoids reentrant calls to AddChild and mean that CompleteDrag only gets called after the window has been reparented. In our case it will get reparented by SetBounds to a container with the same id ("Panel") in second root window and then in FinishDragging will get reparented to workspace (because it is not attached). So it will end up in workspace as we want it to be. Interestingly this does not break any tests - at least in ash_unittests. I will try some more but I doubt we consciously designed with that AddChild reentrancy in mind.
https://codereview.chromium.org/17431003/diff/26002/ash/wm/toplevel_window_ev... File ash/wm/toplevel_window_event_handler.cc (right): https://codereview.chromium.org/17431003/diff/26002/ash/wm/toplevel_window_ev... ash/wm/toplevel_window_event_handler.cc:66: virtual void OnWindowHierarchyChanged( On 2013/06/20 04:09:22, varkha wrote: > PTAL. I think this is a cleaner solution. This avoids reentrant calls to > AddChild and mean that CompleteDrag only gets called after the window has been > reparented. In our case it will get reparented by SetBounds to a container with > the same id ("Panel") in second root window and then in FinishDragging will get > reparented to workspace (because it is not attached). So it will end up in > workspace as we want it to be. Interestingly this does not break any tests - at > least in ash_unittests. I will try some more but I doubt we consciously designed > with that AddChild reentrancy in mind. I had assumed this wouldn't work. When the window is added to the panel container on the second display, that PanelLayoutManager should be notified that it was added, and not knowing that the panel is being dragged would proceed to move it over the panel icon. This doesn't happen? I don't think this is the right thing to do, given that the WindowResizer code doesn't know that the dragged panel has been reparented out from under it and the state at the time it was reparented may have been some temporary state just for the purposes of dragging.
No, this trouble doesn't happen. I think that the drag only gets stopped _after_ that reparenting is complete and so Relayout does not yet know about the drag finishing and does not move the window. Then the drag completes, clears "attached" property and reparents to workspace. Also I don't think this changes the order of notifications. DragComplete is still called after all the layout managers were notified. The difference is that all containers have been actually updated by then and there is no trouble with assuming that ownership changes in the middle of AddChild or RemoveChild.
Correction. The window does get moved to above the icon when panel Relayout is called with slid_in set but that happens while still inside ScreenPositionController::SetBounds so before SetBounds exits it moves the window restoring the position. I think it is a lesser trouble than possibility of reentrant calling AddChild from inside AddChild.
On 2013/06/20 16:29:31, varkha wrote: > Correction. The window does get moved to above the icon when panel Relayout is > called with slid_in set but that happens while still inside > ScreenPositionController::SetBounds so before SetBounds exits it moves the > window restoring the position. I think it is a lesser trouble than possibility > of reentrant calling AddChild from inside AddChild. I'm not sure I understand why calling AddChild during the notification phase of AddChild is troubling. It isn't in the middle of changing any state and it waits until after issuing the NotifyWindowHierarchyChange to determine the target window's root window and parents. Seems like it should be okay to reparent the child (in response to the hierarchy changing notification). https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... There are a number of things that aren't quite right when we change the parent without first cancelling the drag. PanelLayoutManager::Relayout sets the bounds for the window and the opacity of new windows to zero (it probably works in this case because DragWindowResizer::CompleteDrag sets the opacity back to the initial opacity but if it were to set a property that wasn't reset that would remain). Additionally, if there are other panels in the target display it will begin an animation to move them to their new positions to make room for the new panel. WorkspaceWindowResizer::CompleteDrag will snap the window if it was going to from the drag, however we want the window to have the bounds from the UpdateBounds call apply last. I think completing the drag after the SetBoundsInScreen reparenting is going to be fragile and make it easy to regress behavior when changing seemingly unrelated things.
> I'm not sure I understand why calling AddChild during the notification phase of > AddChild is troubling. It isn't in the middle of changing any state and it waits > until after issuing the NotifyWindowHierarchyChange to determine the target > window's root window and parents. Seems like it should be okay to reparent the > child (in response to the hierarchy changing notification). > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... The trouble with it is that in our case we actually expect the end result of the whole transaction to be that the panel ends in workspace. By processing notification before reparenting to another root is complete we cause workspace parent to be only transient - the panel ends in panel container with a confused "attached" state. Also technically there is assertion in AddChild about the child not already being in children_. This condition might be true when we enter AddChild but may not be true if the hierarchy change notification reparents it. Mirror condition is checked in RemoveChild - again the quiet assumption is that notification phase does not remove the child from children_. I understand about Relayout doing some work that may end up being temporary or wrong. I'll have to think about it. I think ideally the drag should be cancelled in changing and reparenting should happen in changed. Do you think this is practical?
Since there is a bug tracking the general issue of a bounds update during a window drag, how about for the sake of this bug, checking that all windows added to PanelLayoutManager (i.e. in PanelLayoutManager::OnWindowAddedToLayout) are indeed attached and reparenting them if not (I made a comment in patchset 2 about this) and add a TODO with a reference to http://crbug.com/230600 that we should be cancelling the drag before. Doing the right thing in general (for crbug.com/230600) will probably require an earlier notification that the bounds are being updated (not by the resizer) for something being actively dragged and that the dragged window should be dropped first: probably adding something like OnWindowBoundsChanging to WindowObserver. https://codereview.chromium.org/17431003/diff/26002/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/26002/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:801: if (set_bounds) { FYI, this comment may be relevant as well: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window.cc&... There may be important side effects that setting the bounds again will cause.
PTAL. I will move dealing with the bounds change from within the application into a separate CL. We can maybe detect in NativeWidgetAura::SetBounds that the window is resized rather than moved and not move it into a different root. This would I think avoid 90% of the visibly jarring drag stopping in the middle when app decides to update its size (I believe it is rare and incorrect for apps to change the origin). I agree that this is a different issue (http://crbug.com/251813) in any case.
https://codereview.chromium.org/17431003/diff/38001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/38001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:337: // back to appropriate container and ignore it. Add TODO to remove this, with a reference to the general bounds update during drag bug. https://codereview.chromium.org/17431003/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:756: // solution is needed. See http://crbug.com/251813 . nit: I'd put this on the call to SetBounds.
https://codereview.chromium.org/17431003/diff/38001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/38001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:337: // back to appropriate container and ignore it. On 2013/06/20 23:00:57, flackr wrote: > Add TODO to remove this, with a reference to the general bounds update during > drag bug. Done. https://codereview.chromium.org/17431003/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:756: // solution is needed. See http://crbug.com/251813 . On 2013/06/20 23:00:57, flackr wrote: > nit: I'd put this on the call to SetBounds. Done.
LGTM with nits. https://codereview.chromium.org/17431003/diff/45001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/45001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:338: // TODO: Updating bounds during a drag can cause problems and a more general TODO(varkha): https://codereview.chromium.org/17431003/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/45001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:798: // TODO: Updating bounds during a drag can cause problems and a more general TODO(varkha):
https://codereview.chromium.org/17431003/diff/45001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/45001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:338: // TODO: Updating bounds during a drag can cause problems and a more general On 2013/06/20 23:28:40, flackr wrote: > TODO(varkha): Done. https://codereview.chromium.org/17431003/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/17431003/diff/45001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:798: // TODO: Updating bounds during a drag can cause problems and a more general On 2013/06/20 23:28:40, flackr wrote: > TODO(varkha): Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/17431003/50001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+jeremya@ or aa@ for OWNERS review (a comment in https://chromiumcodereview.appspot.com/17431003/diff/50001/chrome/browser/ext...).
https://codereview.chromium.org/17431003/diff/50001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/50001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:340: child->SetDefaultParentByRootWindow( It makes me uncomfortable that this could cause an infinite recursion if the default parent was for some reason the panel container. You should probably guard against that similar to how Relayout prevents infinite recursion.
https://codereview.chromium.org/17431003/diff/50001/ash/wm/panels/panel_layou... File ash/wm/panels/panel_layout_manager.cc (right): https://codereview.chromium.org/17431003/diff/50001/ash/wm/panels/panel_layou... ash/wm/panels/panel_layout_manager.cc:340: child->SetDefaultParentByRootWindow( Done. Cannot really happen with our current logic that depends on the same variable but I am all for additional safeguards.
I think stevenjb would be a better person to review this.
lgtm I'm not an owner for extensions/ but you can TBR it for just an added comment.
Thanks, TBR for comment in https://chromiumcodereview.appspot.com/17431003/diff/50001/chrome/browser/ext...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/17431003/53002
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/17431003/53002
Message was sent while issue was closed.
Change committed as 207972 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
