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

Don't fire keyboard shortcuts while SQL query area is focused #16026

Merged
merged 1 commit into from Mar 19, 2020

Conversation

@JayEn84
Copy link
Contributor

JayEn84 commented Mar 12, 2020

Fixes a bug where the keyboard shortcuts functions fire while typing in the SQL query area. The problem was that the SQL query area is not (always) a <textarea> but a <div contenteditable="true">

Fixes #15224

Copy link
Member

williamdes left a comment

Thank you for the fix @JayEn84 !
I will fix the git base branch for you :)

@williamdes williamdes added this to In progress in pull-requests via automation Mar 12, 2020
@williamdes williamdes added this to the 5.0.2 milestone Mar 12, 2020
Fixes a bug where the keyboard shortcuts functions fire while typing in the SQL query area. Problem is that the SQL query area is not (always) a `<textarea>` but a `<div contenteditable="true">`

Signed-off-by: Jan Nitsche <noreply@jan-nitsche.de>
Signed-off-by: JayEn84 <4518738+JayEn84@users.noreply.github.com>
@williamdes williamdes self-assigned this Mar 12, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 12, 2020

Codecov Report

❗️ No coverage uploaded for pull request base (master@f12364b). Click here to learn what that means.
The diff coverage is 35.22%.

@@            Coverage Diff            @@
##             master   #16026   +/-   ##
=========================================
  Coverage          ?   51.55%           
  Complexity        ?    14963           
=========================================
  Files             ?      471           
  Lines             ?    62161           
  Branches          ?        0           
=========================================
  Hits              ?    32048           
  Misses            ?    30113           
  Partials          ?        0
@williamdes williamdes changed the base branch from master to QA_5_0 Mar 12, 2020
@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Mar 12, 2020

while testing I could not validate the fix because something seems wrong

console.log($(':focus').prop('contenteditable'), e.target, $(e.target).prop('contenteditable'));

when I type s into the query box

inherit <a href=​"#Navi_panel">​
            Panneau de navigation
            ​</a>​ inherit

when I hit tab

true <div class=​"CodeMirror-code" role=​"presentation" autocorrect=​"off" autocapitalize=​"off" spellcheck=​"false" contenteditable=​"true" tabindex=​"100">​…​</div>​ true
@JayEn84

This comment has been minimized.

Copy link
Contributor Author

JayEn84 commented Mar 16, 2020

So it seems that this only fixes the problem in desktop browser that are sending mobile client header information.

I honestly don't know how to solve this at the moment, because it seems the wrong HTML element is passed to the function that checks if the shortcut functionalities should fire or not.

pull-requests automation moved this from In progress to Reviewer approved Mar 16, 2020
@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Mar 19, 2020

In fact:

  • I had added the console.log on keyup and not on keydown
  • $(e.target).prop('contenteditable') === 'true' works and not $(e.target).prop('contenteditable') === true
@williamdes williamdes merged commit 646f83c into phpmyadmin:QA_5_0 Mar 19, 2020
0 of 3 checks passed
0 of 3 checks passed
DCO DCO
Details
codeclimate 2 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
pull-requests automation moved this from Reviewer approved to Done Mar 19, 2020
@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Mar 19, 2020

Merged as 646f83c

@williamdes

This comment has been minimized.

Copy link
Member

williamdes commented Mar 19, 2020

Thank you for your contribution @JayEn84 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
pull-requests
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.