Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable TAB key if CTRL was down #85

Closed
wants to merge 1 commit into from

Conversation

jaywink
Copy link

@jaywink jaywink commented Sep 3, 2017

This stops the editor receiving a TAB event if using CTRL-TAB to cycle browser tabs. Otherwise the tab works as it should.

Related to #66.

One problem though, not entirely sure how to deal with it. While the tab does get ignored when doing CTRL+TAB, for some reason the cursor jumps to the end of the text area. I'm not sure what to do about that.

This stops the editor receiving a TAB event if using CTRL-TAB to cycle browser tabs.
@xenatisch
Copy link
Collaborator

This is all good in terms of implementation (thank you).

However, as far as I can remember, I have never seen the ctrl key being used in such capacity. Could you provide a basis for this usage?

The reason for this request is that changing the default behaviour of a key can become very confusing to people, unless it is done in context.

My approach to this would have been to allow API users to disable the hotkeys all together in the settings, or alternatively provide them with the means to enable/disable it as a part of their models and / or forms through a boolean argument.

Now you've done some works here, so we should at least explore your approach and maybe find out other people's opinion on matter as well.

Thanks again!

@jaywink
Copy link
Author

jaywink commented Sep 3, 2017

Thanks for the quick reply!

Initially I did the "disable it all" as per this comment. But I realized then it wasn't necessary, as we can just filter out the "CTRL-TAB" combo. I think the keyboard shortcuts are useful, and personally I don't want to disable them, except to switch the browser tab switching.

Re the CTRL-TAB feature, in Firefox it is explained here. Basically it's just like a window switcher (ALT-TAB) but for tabs. I think it's very common in many apps, at least in Atom, Sublime, etc, at least on Linux. Chrome also supports it, though their behaviour is to always go to next tab. I have only tested the editor behaviour (breaking) with Firefox.

I somehow doubt anyone would be using the combination of CTRL-TAB to create a TAB character in the editor. I understand your concern of course regarding functionality change. My gut feeling is no one would willingly want to indent content by using CTRL-TAB which is a common way to cycle tabs in applications.

@xenatisch
Copy link
Collaborator

Thank you for you reply.

I understand the default function of Ctrl+Tab in the browser. What we are doing here, however, it that we are disabling the default functionality of the editor (i.e. Tab indentation) with Ctrl. What your comment clarifies to now, however, is that by pressing Ctrl, we are essentially reverting back to the default settings of the browser by temporarily disabling the custom features enabled by the editor. The temporary part is the key here.

I agree with this approach. If @adi- has nothing to add, I think we can have it committed for version 2.1.

Once again, thank you for your contribution.

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

Before thinking of merging, any ideas if there is something that can be done about this?

> One problem though, not entirely sure how to deal with it. While the tab does get ignored when doing CTRL+TAB, for some reason the cursor jumps to the end of the text area. I'm not sure what to do about that.

Although now there is no tab inserted, now there is the problem of a jumping cursor to the end of the text area when doing a CTRL-TAB. It's kind of annoying, though less annoying than a tab character :)

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

Highlight of the problem:

peek 2017-09-04 12-46

There is something causing the jump of the cursor because we're returning false but I can't make out what..

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

False alert, the jumping cursor is not a problem when running the bundled development manage.py runserver so I must assume it's possibly a problem with the bootstrap markdown wrapper I have on the field - so this seems to be working as intended! 👍 Should have tested this first, but I only noticed it once integrating to my app.

@xenatisch
Copy link
Collaborator

I see what you mean.

This is part of the code had originally been implement using JQuery. I then reimplemented it in pure JS - and didn't make it particularly easy (to modify) as the primary objective was responsiveness. It's JavaScript after all, and it's not known for tidiness. Nonetheless, I have tried to - where possible, give sufficient explanations through documentations and comments.

You have done a great job in finding out the precise location that needed to be modified for this particular functionality.

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

On the contrary, I think the JS (or TypeScript) is very clear to read and figure out what it does, good job! 👍 That is why I was wondering about the cursor jump because nothing indicates it should happen. And it seems it doesn't happen, it's just something weird in my project where I also have bootstrap-markdown on top.

Sorry for the noise and confusion, just to clarify, the is no cursor jump issue AFAICT in this PR after all :)

@adi-
Copy link
Member

adi- commented Sep 4, 2017

I don't see the code while I write this, but my suggestion would be to add a Tab handler only to textarea. Having that, there should be also a mechanism which disables setting focus to that textarea on page load - it should help switching tabs. What do you think about that?

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

@adi- I think the tab handler is already only on the text area? At least pressing tab while focus is elsewhere doesn't cause a tab character in the text area. The issue is not that switching browser tabs with CTRL-TAB outside the text area causes a tab. The issue that this PR fixes is switching browser tabs when the focus is in the text area.

@adi-
Copy link
Member

adi- commented Sep 4, 2017

And this is my point. If user set focus on textarea by himself (mouse click i.e.), it feels natural (for me) that TAB is "eaten" by this textarea. However, if focus is automatically set on textarea when page loads (and I think that most of web browsers do so) it can be problematic. My "workaround" would be to disable autofocus on textarea field when page loads – I guess it can be set by html param in django-markdownx.

@xenatisch
Copy link
Collaborator

That aside, @adi- ; I think there is a point here.

We are essentially disabling a default feature with no way to temporarily reinstate it. This approach provides a tool for doing just that by holding the Ctrl key, which as I far as I can think, as no other functions that may be influenced as a result of this. In other words and as I understand it, you hold Ctrl, and you can use the tab function for switching between stuff as per default.

I don't know if you remember, but we had a similar discussion on drag and drops, where we disable the default behaviour of the browser temporarily to prevent the browser from turning into an image viewer in case someone drops the image outside of the textarea.

@adi-
Copy link
Member

adi- commented Sep 4, 2017

Ok, so what is final version? ;)

@jaywink
Copy link
Author

jaywink commented Sep 4, 2017

I don't see (personally) that the focus is of any issue. To me that is a great usability feature, but of course it depends on how you're using the editor.

How I'm using the editor: Myself, I quite often have several other tabs I refer to for information to whatever I'm writing. I don't like using the mouse. So while writing, I do a lot of switching to other tabs to look up information while writing.

Without this patch, every time I cross-check information, I get an extra TAB which I need to then delete. If I want to avoid that, I have to first un-focus the editor and then re-focus, which is one more step (and requires the mouse).

I cannot honestly believe anyone would try to insert a TAB in the editor but hold the CTRL key down at the same time. At least firefox will still cycle to the next tab even if the editor eats the tab. The current code does not stop the browser from cycling tabs. This patch just refines the current functionality a bit by suppressing the TAB if and only if CTRL is held down.

@jaywink
Copy link
Author

jaywink commented Mar 10, 2018

Any decision if this can be merged in at some point? I'm fine maintaining it in own fork if not, but would be nice to get this PR resolved. If suppressing TAB when CTRL is down is ok to be added, I'll solve the conflict here.

@adi-
Copy link
Member

adi- commented Nov 3, 2018

So, I think pressing CTRL is not too obvious for everybody. I guess this patch needs to be used in private/custom app only. Otherwise, it will be confusing.

But I am still open for any better "mechanism".

@adi- adi- closed this Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants