Switch tabs onmousedown #124
Conversation
|
@KeitIG You've got a broken spec which needs to be fixed: |
|
I got the error, (conflict between two mousedown events). Is it possible to test Travis builds without committing to avoit commits pollution ? |
|
now done. 630883e was more a code cleanup than a spec fix. |
|
@KeitIG thank you for taking the time to contribute! 🙇 I quickly tested it and, apparently, dragging tabs is not working anymore. My first concern is that specs are not failing, probably because we fake the drag event when testing this behavior. Moreover, UX-wise, I do have one more concern: what would happen if I simply want to drag a tab which is not currently open? This way we would always open the tab, even if the user didn't want to. Personally, I would find this a bit confusing but I am not sure it's wrong either. @kevinsawicki: what do you think? |
|
@as-cii Yep, got the bug too. I may have an idea about why. On the UX part, looking at Sublime, imagine you'are at I'd be curious what other people are thinking about this. |
|
@as-cii This is how Chrome does it, but it does feel a bit weird when you didn't actually want the tab to open. |
|
I like it. It feels a bit more "effortless".
Currently if you drag a not opened tab, it still opens that tab on And I'm not sure which is the better behaviour. Sometimes you want to drag + edit a closed tab and therefore having it open without an extra click, is nice too. |
* ✅ Document right-click (and ctrlKey) behavior
Avoid opening a tab on ctrl-left click
On `mousedown` our production code prevented the default browser behavior. This, however, was not documented in our tests: for instance, specs were green even if dragging was not working anymore. Since writing an integration test or simulating a drag event in the current suite was quite painful, I opted to spy on `event.preventDefault` and ensure that it was called appropriately to preserve our dragging behavior. * 🎨 Move event helpers out of tabs-spec.coffee
@simurai: nice observation! For some reason I assumed that the @KeitIG: I opened another pull-request on your fork to shield us from the dragging regression in the future. As I described there, writing an integration test (e.g. through a web driver) was not an option, and simulating the dragging events through a series of /cc: @kevinsawicki |
Add missing specs for event.preventDefault()
|
So this is ready to go? |
|
I guess so, as-cii have done a nice job. (Thanks btw, I would not have figured it out myself) |
|
@kevinsawicki: yep! I have just merged it 👍 @KeitIG: thank you for this great contribution! ✨ |
|
@as-cii @KeitIG I think this PR broke some focus-related things when switching tabs -- see atom/atom#6662 for details. Specifically, I think 6cb57b8 might be responsible. Could you take a look and double-check? 🍩 |
|
Indeed, It may be due to that, looking for a way to rewrite this. edit: Changing to |
|
@KeitIG You might try calling |
|
Nevermind, I see activate is already being called and somehow focus is still getting stolen. |
Fixes atom/atom#6662 Refs #124 This was introduced in 310c058, with a description that it prevented focus from being stolen when the close icon is clicked. I don’t really understand the reasoning there, and removing it fixes the above issue and doesn’t seem to introduce any problems with focus on clicking the close icon. @kevinsawicki perhaps you can comment on whether this makes sense?
|
I've opened a PR which you can see above which fixes the issue for me. @izuzak want to give it a spin and see if it breaks anything else. I couldn't find anything but I'm a sucky QA tester. |
following #123
I tested it, It works quite well and feels more reactive. Merge if you feel the need.