Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541653002:
Add a dead zone when dragging tabs horizontally on Mac (Closed)
|
Created:
4 years ago by njagsarran Modified:
4 years ago CC:
chromium-reviews, dcheng, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a dead zone when dragging tabs horizontally on Mac
When trying to drag tabs vertically out of their windows, they don't drag until the cursor has been moved a certain distance away from the mouse down point. Adding the same behaviour when moving tabs horizontally.
BUG=90526
TEST=Create multiple tabs, slowly drag one of them directly to the left or right. It won't actually start moving until it's torn out of its dead zone.
Committed: https://crrev.com/b74d1db1e9f18844ac6929d6ca7116ef00f0379f
Cr-Commit-Position: refs/heads/master@{#437058}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing review comments. #
Total comments: 3
Messages
Total messages: 28 (10 generated)
njagsarran@google.com changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.h chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm
avi@chromium.org changed reviewers: + erikchen@chromium.org
It's also been a while since I've dug into here; asking Erik to double-check my work. https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { Do we want kTearDistance? That's 36 pixels, and when I play on Views to see what it does, the horizontal tear distance is a lot less. What value does Views use here? https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:179: if (outOfTabHorizontalDeadZone_) You'd need to adjust the indentation here with a new if, and use {}.
https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { On 2016/11/30 00:01:22, Avi wrote: > Do we want kTearDistance? That's 36 pixels, and when I play on Views to see what > it does, the horizontal tear distance is a lot less. > > What value does Views use here? Agreed that this should be significantly smaller. Given that the original bug was about misclicks turning into horizontal drags, I'm thinking maybe 5, or even 3? https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:438: outOfTabHorizontalDeadZone_ = NO; presumably we always want to reset this when the drag ends.
https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { On 2016/11/30 15:05:16, erikchen wrote: > On 2016/11/30 00:01:22, Avi wrote: > > Do we want kTearDistance? That's 36 pixels, and when I play on Views to see > what > > it does, the horizontal tear distance is a lot less. > > > > What value does Views use here? > > Agreed that this should be significantly smaller. Given that the original bug > was about misclicks turning into horizontal drags, I'm thinking maybe 5, or even > 3? Looks like Views uses 10, what do you guys think of that? Still too much? https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co...
On 2016/11/30 17:44:49, njagsarran wrote: > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): > > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if > (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { > On 2016/11/30 15:05:16, erikchen wrote: > > On 2016/11/30 00:01:22, Avi wrote: > > > Do we want kTearDistance? That's 36 pixels, and when I play on Views to see > > what > > > it does, the horizontal tear distance is a lot less. > > > > > > What value does Views use here? > > > > Agreed that this should be significantly smaller. Given that the original bug > > was about misclicks turning into horizontal drags, I'm thinking maybe 5, or > even > > 3? > > Looks like Views uses 10, what do you guys think of that? Still too much? > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co... I would be happy to match Views.
https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { On 2016/11/30 17:44:49, njagsarran wrote: > On 2016/11/30 15:05:16, erikchen wrote: > > On 2016/11/30 00:01:22, Avi wrote: > > > Do we want kTearDistance? That's 36 pixels, and when I play on Views to see > > what > > > it does, the horizontal tear distance is a lot less. > > > > > > What value does Views use here? > > > > Agreed that this should be significantly smaller. Given that the original bug > > was about misclicks turning into horizontal drags, I'm thinking maybe 5, or > even > > 3? > > Looks like Views uses 10, what do you guys think of that? Still too much? > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co... Done. https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:179: if (outOfTabHorizontalDeadZone_) On 2016/11/30 00:01:22, Avi wrote: > You'd need to adjust the indentation here with a new if, and use {}. Done. https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:438: outOfTabHorizontalDeadZone_ = NO; On 2016/11/30 15:05:16, erikchen wrote: > presumably we always want to reset this when the drag ends. Done.
On 2016/11/30 20:26:32, njagsarran wrote: > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): > > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:176: if > (!outOfTabHorizontalDeadZone_ && fabs(offset) > kTearDistance) { > On 2016/11/30 17:44:49, njagsarran wrote: > > On 2016/11/30 15:05:16, erikchen wrote: > > > On 2016/11/30 00:01:22, Avi wrote: > > > > Do we want kTearDistance? That's 36 pixels, and when I play on Views to > see > > > what > > > > it does, the horizontal tear distance is a lot less. > > > > > > > > What value does Views use here? > > > > > > Agreed that this should be significantly smaller. Given that the original > bug > > > was about misclicks turning into horizontal drags, I'm thinking maybe 5, or > > even > > > 3? > > > > Looks like Views uses 10, what do you guys think of that? Still too much? > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co... > > Done. > > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:179: if > (outOfTabHorizontalDeadZone_) > On 2016/11/30 00:01:22, Avi wrote: > > You'd need to adjust the indentation here with a new if, and use {}. > > Done. > > https://codereview.chromium.org/2541653002/diff/1/chrome/browser/ui/cocoa/tab... > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:438: > outOfTabHorizontalDeadZone_ = NO; > On 2016/11/30 15:05:16, erikchen wrote: > > presumably we always want to reset this when the drag ends. > > Done. LGTM
https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:181: if (outOfTabHorizontalDeadZone_) { I patched this in and tried tab dragging. It seemed to work fine, but I also saw a large number of: """ tab_window_controller.mm(352)] Not implemented reached in -[TabWindowController menubarOffset] """ It's not clear to me how you're actually preventing tab dragging by inserting a placeholder on line 182. I imagine that you want an early return? Can you explain how you're trying to prevent tab dragging?
https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:181: if (outOfTabHorizontalDeadZone_) { On 2016/11/30 21:07:34, erikchen wrote: > I patched this in and tried tab dragging. It seemed to work fine, but I also saw > a large number of: > > """ > tab_window_controller.mm(352)] Not implemented reached in -[TabWindowController > menubarOffset] > """ > > It's not clear to me how you're actually preventing tab dragging by inserting a > placeholder on line 182. I imagine that you want an early return? Can you > explain how you're trying to prevent tab dragging? I'm seeing that error, too. I undid my changed and rebuilt, but I'm still seeing that error. Do you also still see it without my patch, or am I missing something? I think the placeholder is the tab under the mouse when moving it within the strip, so now it can't be created unless we drag beyond the dead zone. I don't think we want an early return because we still want to check for vertical dragging below.
On 2016/12/02 16:43:12, njagsarran wrote: > https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): > > https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:181: if > (outOfTabHorizontalDeadZone_) { > On 2016/11/30 21:07:34, erikchen wrote: > > I patched this in and tried tab dragging. It seemed to work fine, but I also > saw > > a large number of: > > > > """ > > tab_window_controller.mm(352)] Not implemented reached in > -[TabWindowController > > menubarOffset] > > """ > > > > It's not clear to me how you're actually preventing tab dragging by inserting > a > > placeholder on line 182. I imagine that you want an early return? Can you > > explain how you're trying to prevent tab dragging? > > I'm seeing that error, too. I undid my changed and rebuilt, but I'm still seeing > that error. Do you also still see it without my patch, or am I missing > something? I only saw this error when I patched in your change. > > I think the placeholder is the tab under the mouse when moving it within the > strip, so now it can't be created unless we drag beyond the dead zone. I don't > think we want an early return because we still want to check for vertical > dragging below.
On 2016/12/06 17:53:28, erikchen wrote: > On 2016/12/02 16:43:12, njagsarran wrote: > > > https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): > > > > > https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:181: if > > (outOfTabHorizontalDeadZone_) { > > On 2016/11/30 21:07:34, erikchen wrote: > > > I patched this in and tried tab dragging. It seemed to work fine, but I also > > saw > > > a large number of: > > > > > > """ > > > tab_window_controller.mm(352)] Not implemented reached in > > -[TabWindowController > > > menubarOffset] > > > """ > > > > > > It's not clear to me how you're actually preventing tab dragging by > inserting > > a > > > placeholder on line 182. I imagine that you want an early return? Can you > > > explain how you're trying to prevent tab dragging? > > > > I'm seeing that error, too. I undid my changed and rebuilt, but I'm still > seeing > > that error. Do you also still see it without my patch, or am I missing > > something? > I only saw this error when I patched in your change. > > > > > I think the placeholder is the tab under the mouse when moving it within the > > strip, so now it can't be created unless we drag beyond the dead zone. I don't > > think we want an early return because we still want to check for vertical > > dragging below. I synced with the current build in the tree, confirmed my changes are no longer in my code, but the error still shows up. Can you revert my patch and check please? You may not have gone through the steps to trigger that error before you patched in my change which is why you didn't see it. I'm only seeing it when trying to attach a tab to a new tab strip, or dragging it off its current one and back.
lgtm https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right): https://codereview.chromium.org/2541653002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:181: if (outOfTabHorizontalDeadZone_) { On 2016/12/02 16:43:12, njagsarran wrote: > On 2016/11/30 21:07:34, erikchen wrote: > > I patched this in and tried tab dragging. It seemed to work fine, but I also > saw > > a large number of: > > > > """ > > tab_window_controller.mm(352)] Not implemented reached in > -[TabWindowController > > menubarOffset] > > """ > > > > It's not clear to me how you're actually preventing tab dragging by inserting > a > > placeholder on line 182. I imagine that you want an early return? Can you > > explain how you're trying to prevent tab dragging? > > I'm seeing that error, too. I undid my changed and rebuilt, but I'm still seeing > that error. Do you also still see it without my patch, or am I missing > something? > > I think the placeholder is the tab under the mouse when moving it within the > strip, so now it can't be created unless we drag beyond the dead zone. I don't > think we want an early return because we still want to check for vertical > dragging below. You're right. Tried again and I can reproduce the error withyour your CL. It looks like placeholderTab_ is used to determine "isDragSessionActive", which is why setting it is required to start dragging.
Thanks for the review, guys. This is my first patch, can one of you run the try jobs for me?
The CQ bit was checked by njagsarran@google.com 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...
On 2016/12/07 20:01:30, njagsarran wrote: > Thanks for the review, guys. > > This is my first patch, can one of you run the try jobs for me? Never mind, I think I can run it.
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 njagsarran@google.com
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": 20001, "attempt_start_ts": 1481144777529820,
"parent_rev": "86cb3f8c3604b88ad8d8c0108289124db69598e2", "commit_rev":
"0087882ca8ded66a76af4c6f1dc0f9816f4de83d"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add a dead zone when dragging tabs horizontally on Mac When trying to drag tabs vertically out of their windows, they don't drag until the cursor has been moved a certain distance away from the mouse down point. Adding the same behaviour when moving tabs horizontally. BUG=90526 TEST=Create multiple tabs, slowly drag one of them directly to the left or right. It won't actually start moving until it's torn out of its dead zone. ========== to ========== Add a dead zone when dragging tabs horizontally on Mac When trying to drag tabs vertically out of their windows, they don't drag until the cursor has been moved a certain distance away from the mouse down point. Adding the same behaviour when moving tabs horizontally. BUG=90526 TEST=Create multiple tabs, slowly drag one of them directly to the left or right. It won't actually start moving until it's torn out of its dead zone. Committed: https://crrev.com/b74d1db1e9f18844ac6929d6ca7116ef00f0379f Cr-Commit-Position: refs/heads/master@{#437058} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b74d1db1e9f18844ac6929d6ca7116ef00f0379f Cr-Commit-Position: refs/heads/master@{#437058}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2557343002/ by ellyjones@chromium.org. The reason for reverting is: Likely introduced https://crbug.com/672427. Quoting from that bug: """ I believe this is caused by https://codereview.chromium.org/2541653002/: In [TabStripDragController continueDrag:], tabWasDragged_ is still set to YES even if the tab drag is not done because of the newly-introduced horizontal dead zone. Then, in [TabStripDragController endDrag:], the expected target placeholder tab is missing, but tabWasDragged_ is still YES, so [dropController moveTabViews:fromController:] causes a fatality. """.
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
