[Fix for #26016] In the docs js: support keyboard events for non-english keyboard layouts #26675

azerupi · 2015-06-30T07:45:12Z

Like explained in #26016, typing ? had no effect with non-english keyboard layouts in the docs.

This patch seems to resolve this issue, tested with AZERTY keyboard in Google Chrome and Firefox. I haven't tested it with more exotic keyboard layouts or with other browsers though.

This code is based on the information found on: http://javascript.info/tutorial/keyboard-events

More specifically:

The only event which reliably provides the character is keypress.

And

// event.type must be keypress
function getChar(event) {
  if (event.which == null) {
    return String.fromCharCode(event.keyCode) // IE
  } else if (event.which!=0 && event.charCode!=0) {
    return String.fromCharCode(event.which)   // the rest
  } else {
    return null // special key
  }
}

? and S work, escape however does not (on an Azerty keyboard).

It would be good if some people could test it with other browsers and keyboard layouts: http://www.mathieudavid.org/test/rustdoc/std/index.html

Edit:

swedish layout works on Firefox and Chromium
french (azerty) mac layout works on Safari

rust-highfive · 2015-06-30T07:45:24Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2015-06-30T16:32:42Z

Doesn't jQuery have some accessor which does all this for us?

steveklabnik · 2015-06-30T16:38:10Z

... and wasn't jquery removed recently?

steveklabnik · 2015-06-30T16:38:22Z

src/librustdoc/html/static/main.js

+    // Helper function for Keyboard events,
+    // Get's the char from the keypress event
+    //
+    // This method is used because e.wich === x is not


Gankra · 2015-06-30T16:38:28Z

I'm personally cool with not using jquery as much as possible.

(jquery was removed from the book, not rustdoc)

Gankra · 2015-06-30T16:39:06Z

src/librustdoc/html/static/main.js

            if (e.shiftKey && $('#help').hasClass('hidden')) {
                e.preventDefault();
                $('#help').removeClass('hidden');
            }
-        } else if (e.which === 27) { // esc
+        } else if (getChar(e) === 's' || getChar(e) === 'S') {


A deep part of my soul wants these to be cached in a variable, but then I remember this is javascript and it doesn't matter at all.

alexcrichton · 2015-06-30T16:46:23Z

... and wasn't jquery removed recently?

From rustbook, not rustdoc.

I'm personally cool with not using jquery as much as possible.

I'm worried about the JS becoming a pile of impenetrable lists of platform-specific details for all browsers and weird combinations. I'd much rather have something else take care of all this so we don't have to keep updating little bits like this.

steveklabnik · 2015-06-30T16:47:09Z

I'm worried about the JS becoming a pile of impenetrable lists of platform-specific details for all browsers and weird combinations

Same, very much so.

azerupi · 2015-06-30T16:53:37Z

If jQuery has a method I haven't found it, and I didn't add jQuery, it was already there :)

I am totally pro using jQuery, because they do a lot better job at implementing browser support and abstractions. If there is a jQuery way for this I'm all in, but I didn't find any unfortunately.

I could drop the helper function and replace it with just

String.fromCharCode( e.which )

But I have no idea how it behaves in IE and I can't test it.

azerupi · 2015-06-30T17:00:00Z

@gankro removing jQuery is in my opinion not really a win. Because it offers so much convenience methods to shorten and simplify the code. It's a lot easier to maintain than vanilla js. On top of that you get good browser compatibility removing / reducing the need to hack your way through it ;)

Also jQuery is used on a lot of websites loaded from the official CDN, if we used that CDN jQuery would most likely already be cached in the users browsers and thus practically bring it's load time to zero.

Gankra · 2015-06-30T17:32:43Z

jquery is well-known to generally have significant overhead over "plain" DOM manipulations. Rustdoc being snappy is important to me. Especially as we make its render more dynamic in the future.

Gankra · 2015-06-30T17:33:05Z

That said, there's a reason I don't push hard for removing it ;)

azerupi · 2015-06-30T18:07:46Z

I am not totally convinced about the pure js vs jQuery performance. Sure jQuery has a bit of overhead because it has support for most browsers etc. But it is designed by a bunch of people who are a lot more skilled than myself and probably most contributors here, they probably know how to get that last bit of performance out of it.

Well written vanilla js can be faster than jQuery but it has to be well written ;)
And, here I speak for myself, I am probably not skilled enough to always know when something is well or poorly written.

One thing that comes to mind though is that it's a lot easier to use other css selectors instead of id's with jQuery because of the convenience methods, and this can drastically slow down the dom manipulations.

I could be wrong, but I think we still have a pretty big marge before we begin to have performance issues in the rustdoc :)

alexcrichton · 2015-07-01T05:06:21Z

Hm ok, if there's nothing in jquery to already do this then it's probably fine. Can you squash the commits together as well?

…ith non-english keyboard layouts. Fixes #26016 Fixes #16572

azerupi · 2015-07-01T08:15:39Z

Done :)

alexcrichton · 2015-07-01T16:21:09Z

@bors: r+ 49b73e4

Like explained in #26016, typing `?` had no effect with non-english keyboard layouts in the docs. This patch seems to resolve this issue, **tested with AZERTY keyboard in Google Chrome and Firefox**. I haven't tested it with more exotic keyboard layouts or with other browsers though. This code is based on the information found on: http://javascript.info/tutorial/keyboard-events **More specifically:** > The only event which reliably provides the character is keypress. **And** >``` // event.type must be keypress function getChar(event) { if (event.which == null) { return String.fromCharCode(event.keyCode) // IE } else if (event.which!=0 && event.charCode!=0) { return String.fromCharCode(event.which) // the rest } else { return null // special key } } ``` `?` and `S` work, `escape` however does not (on an Azerty keyboard). It would be good if some people could test it with other browsers and keyboard layouts: http://www.mathieudavid.org/test/rustdoc/std/index.html **Edit:** - swedish layout works on Firefox and Chromium - french (azerty) mac layout works on Safari

bors · 2015-07-01T16:21:12Z

⌛ Testing commit 49b73e4 with merge 9d67b9f...

bors · 2015-07-01T17:55:19Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

Gankra · 2015-07-01T22:33:39Z

We should endeavour to back-port client-side rustdoc fixes as much as possible.

brson · 2015-07-29T16:57:15Z

Not accepted.

rust-highfive assigned alexcrichton Jun 30, 2015

steveklabnik reviewed Jun 30, 2015
View reviewed changes

Gankra reviewed Jun 30, 2015
View reviewed changes

azerupi mentioned this pull request Jun 30, 2015

Rust doc's question mark shortcut does not work well with all keyboard layouts. #16572

Closed

In js from the docs, change keyboard eventlistener to be compatible w…

49b73e4

…ith non-english keyboard layouts. Fixes #26016 Fixes #16572

bors merged commit 49b73e4 into rust-lang:master Jul 1, 2015

Gankra added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 1, 2015

aturon added the T-tools label Jul 29, 2015

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 29, 2015

[Fix for #26016] In the docs js: support keyboard events for non-english keyboard layouts #26675

[Fix for #26016] In the docs js: support keyboard events for non-english keyboard layouts #26675

Conversation

azerupi commented Jun 30, 2015

Uh oh!

rust-highfive commented Jun 30, 2015

Uh oh!

alexcrichton commented Jun 30, 2015

Uh oh!

steveklabnik commented Jun 30, 2015

Uh oh!

steveklabnik Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

Gankra commented Jun 30, 2015

Uh oh!

Gankra Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jun 30, 2015

Uh oh!

steveklabnik commented Jun 30, 2015

Uh oh!

azerupi commented Jun 30, 2015

Uh oh!

azerupi commented Jun 30, 2015

Uh oh!

Gankra commented Jun 30, 2015

Uh oh!

Gankra commented Jun 30, 2015

Uh oh!

azerupi commented Jun 30, 2015

Uh oh!

alexcrichton commented Jul 1, 2015

Uh oh!

azerupi commented Jul 1, 2015

Uh oh!

alexcrichton commented Jul 1, 2015

Uh oh!

bors commented Jul 1, 2015

Uh oh!

bors commented Jul 1, 2015

Uh oh!

Gankra commented Jul 1, 2015

Uh oh!

brson commented Jul 29, 2015

Uh oh!

Uh oh!