Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
80323002:
Restores maximized browser before dragging a tab (Closed)
DescriptionRestores maximized browser before dragging a tab.
Allows a tab dragged out of a maximized browser to get properly side-snapped (similar to how docking is allowed).
BUG=319205
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238236
Patch Set 1 #
Total comments: 6
Patch Set 2 : Restores maximized browser before dragging a tab (no animations) #
Total comments: 2
Patch Set 3 : Restores maximized browser before dragging a tab (comments) #Patch Set 4 : Restores maximized browser before dragging a tab (ash only) #
Total comments: 3
Patch Set 5 : Restores maximized browser before dragging a tab (rebase) #
Messages
Total messages: 22 (0 generated)
sky@, I was not completely satisfied with https://codereview.chromium.org/77013003/. Can you please see if you like this behavior better? I tried to make dragging a single solo tab in a maximized browser behave truly same (at least from a user perspective) as dragging one of the non-solo tabs from a maximized browser into a new browser. I restore the browser first and use the same size calculation for both cases. It also makes it simpler restoring the maximized state at the end of the drag since there no longer a need to restore the browser - so there is only one code branch in CompleteDrag. I am performing restore / resize while the widget is hidden to avoid jarring animation (again same thing is done as when a new browser is created). Avoiding maximizing at the end of the drag when side-snapping seemed trivial enough to add as well. Thanks!
Have you tried this on windows non-metro? I have a feeling hiding is going to release capture and cause problems. https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2293: case DETACH_BEFORE: Why are you changing this?
https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:557: widget->Hide(); I need to see if this breaks non-metro Windows. https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2293: case DETACH_BEFORE: On 2013/11/21 16:43:07, sky wrote: > Why are you changing this? It does not seem to break the existing case and makes correct x-offset for the case when the widget is not born with empty origin as in my case when the existing browser gets restored.
https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2293: case DETACH_BEFORE: On 2013/11/21 16:59:35, varkha wrote: > On 2013/11/21 16:43:07, sky wrote: > > Why are you changing this? > > It does not seem to break the existing case and makes correct x-offset for the > case when the widget is not born with empty origin as in my case when the > existing browser gets restored. I wanted dragging up/down to appear to tear a new window from the existing window. Which means the position should remain the same.
Still need to try Windows non-metro. Do you think there is a simpler way to avoid animating Restore / SetBounds in this code? Hide / Show seemed hacky. Maybe introducing some methods similar to SetVisibilityChangedAnimationsEnabled but for bounds animations? https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2293: case DETACH_BEFORE: On 2013/11/21 17:15:35, sky wrote: > On 2013/11/21 16:59:35, varkha wrote: > > On 2013/11/21 16:43:07, sky wrote: > > > Why are you changing this? > > > > It does not seem to break the existing case and makes correct x-offset for the > > case when the widget is not born with empty origin as in my case when the > > existing browser gets restored. > > I wanted dragging up/down to appear to tear a new window from the existing > window. Which means the position should remain the same. That is what it seems to be doing (still in the old case as well as with the new case).
PTAL. I hope that not passing |window| to GetCrossFadeDuration was an oversight and not intentional. Doing it allows disabling crossfade animations for a window without setting scoped settings on both layers or something complicated like that.
https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/1/chrome/browser/ui/views/tabs/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2293: case DETACH_BEFORE: On 2013/11/21 17:23:10, varkha wrote: > On 2013/11/21 17:15:35, sky wrote: > > On 2013/11/21 16:59:35, varkha wrote: > > > On 2013/11/21 16:43:07, sky wrote: > > > > Why are you changing this? > > > > > > It does not seem to break the existing case and makes correct x-offset for > the > > > case when the widget is not born with empty origin as in my case when the > > > existing browser gets restored. > > > > I wanted dragging up/down to appear to tear a new window from the existing > > window. Which means the position should remain the same. > > That is what it seems to be doing (still in the old case as well as with the new > case). I understand now - the case I broke was dragging tabs out of a non-maximized browser (I guess I was too focused on maximized state to notice - my bad). I reverted this part and correct for that offset elsewhere.
https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:556: std::vector<gfx::Rect> drag_bounds = CalculateBoundsForDraggedTabs(); Does this do the right thing if you have lots of tab selected? Why not just layout as they normally would in this case? This code should also be ash and non-windows only as windows does the right thing already.
https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:556: std::vector<gfx::Rect> drag_bounds = CalculateBoundsForDraggedTabs(); On 2013/11/21 23:29:29, sky wrote: > Does this do the right thing if you have lots of tab selected? Why not just > layout as they normally would in this case? > > This code should also be ash and non-windows only as windows does the right > thing already. Thanks for noticing this. I think I was missing a part that scales tabs and shifts the browser window. I have extracted that part into a function and added the call here. Isn't the current behavior on Windows Ash / Metro that the maximized tab is still dragged maximized? I wanted to size it so that a user could see where to dock if necessary. Is this different from Chrome OS?
sky@, even though this works I have played with resized and non-resized maximized tab dragging and I must admit I am cooling off with this idea. I like the smoothness of dragging a browser that does not have that initial "jump". With the tearing tabs off resizing works OK but with a solo tab in a maximized browser this initial re-layout is a bit disturbing. What do you think? It was a useful experience getting this to work but I can easily revert most of the changes in this CL and leave just the bit about allowing side-snapped tabs to stay side-snapped at the end of the drag. Maybe I could leave the ability to disable cross-fading animation - it seems like a useful (and separate) change. Thanks for looking at this!
It is for ash, but for non-ash the desktop window manager restores the window when you attempt to drag a maximized window. -Scott On Thu, Nov 21, 2013 at 6:57 PM, <varkha@chromium.org> wrote: > > https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... > File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): > > https://codereview.chromium.org/80323002/diff/130001/chrome/browser/ui/views/... > chrome/browser/ui/views/tabs/tab_drag_controller.cc:556: > std::vector<gfx::Rect> drag_bounds = CalculateBoundsForDraggedTabs(); > On 2013/11/21 23:29:29, sky wrote: >> >> Does this do the right thing if you have lots of tab selected? Why not > > just >> >> layout as they normally would in this case? > > >> This code should also be ash and non-windows only as windows does the > > right >> >> thing already. > > > Thanks for noticing this. I think I was missing a part that scales tabs > and shifts the browser window. I have extracted that part into a > function and added the call here. > > Isn't the current behavior on Windows Ash / Metro that the maximized tab > is still dragged maximized? I wanted to size it so that a user could see > where to dock if necessary. Is this different from Chrome OS? > > https://codereview.chromium.org/80323002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Since the window ends up snapping right back I almost feel like we shouldn't allow dragging a maximized window. I don't mind the snapping to restored size, but I feel like it should stick, meaning on release it stays restored. -Scott On Thu, Nov 21, 2013 at 10:12 PM, <varkha@chromium.org> wrote: > sky@, even though this works I have played with resized and non-resized > maximized tab dragging and I must admit I am cooling off with this idea. I > like > the smoothness of dragging a browser that does not have that initial "jump". > With the tearing tabs off resizing works OK but with a solo tab in a > maximized > browser this initial re-layout is a bit disturbing. What do you think? It > was a > useful experience getting this to work but I can easily revert most of the > changes in this CL and leave just the bit about allowing side-snapped tabs > to > stay side-snapped at the end of the drag. > Maybe I could leave the ability to disable cross-fading animation - it seems > like a useful (and separate) change. > Thanks for looking at this! > > https://codereview.chromium.org/80323002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
>> ash and non-windows only as windows does the right thing already So what you are saying is that the new code should run under ash (including Chrome OS and Windows) but not in _non-ash_ Windows. Right (or am I still confused)?
That's right. I also don't feel particularly strongly. It's worth asking the UX team for their opinion. -Scott On Mon, Nov 25, 2013 at 9:55 AM, <varkha@chromium.org> wrote: >>> ash and non-windows only as windows does the right thing already > > > So what you are saying is that the new code should run under ash (including > Chrome OS and Windows) but not in _non-ash_ Windows. Right (or am I still > confused)? > > https://codereview.chromium.org/80323002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sky@, I have tried it again on Pixel and it looks quite good and consistent with a tab dragged out of a multi-tab browser. Can you please take another look. I seem to have also fixed a bug with the height being set too large during the drag. Thanks! https://codereview.chromium.org/80323002/diff/400001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:553: if (host_desktop_type_ == chrome::HOST_DESKTOP_TYPE_ASH && This behavior is now ash-only - including ash on Windows. https://codereview.chromium.org/80323002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2254: std::max(max_size.height() / 2, new_bounds.height())); This looked like copy and paste bug. Am I correct assuming that it was not intentional? This resulted in a dragged tab that has very wide restored bounds to also become very tall - regardless of its restore height. Hopefully fixed.
LGTM https://codereview.chromium.org/80323002/diff/400001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/80323002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:2254: std::max(max_size.height() / 2, new_bounds.height())); On 2013/11/27 19:54:45, varkha wrote: > This looked like copy and paste bug. Am I correct assuming that it was not > intentional? This resulted in a dragged tab that has very wide restored bounds > to also become very tall - regardless of its restore height. Hopefully fixed. Ya, looks like a copy/paste error.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/80323002/400001
Retried try job too often on linux_chromeos for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/80323002/440001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/80323002/440001
Message was sent while issue was closed.
Change committed as 238236 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
