added keyboard shortcuts for navigation inside text box #15666

clementmiao · 2017-02-21T00:29:09Z

PR to implement keyboard shortcuts per issue #12278, r? jdm

Thanks for letting me help!

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #12278 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-02-21T00:29:15Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2017-02-21T00:29:15Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/Cargo.toml, components/script/textinput.rs
@KiChjang: components/script/Cargo.toml, components/script/textinput.rs

asajeffrey

Mostly looks good, though there are some UI issues that I'm not sure about, not being a UI person.

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

@@ -4,11 +4,14 @@

 //! Common handling of keyboard input and state management for text input controls

+extern crate unicode_segmentation;


Add this extern crate to lib.rs?

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

 use clipboard_provider::ClipboardProvider;
 use dom::bindings::str::DOMString;
 use dom::keyboardevent::KeyboardEvent;
 use msg::constellation_msg::{ALT, CONTROL, SHIFT, SUPER};
 use msg::constellation_msg::{Key, KeyModifiers};
+use self::unicode_segmentation::UnicodeSegmentation;


Can we make this use unicode... rather than use self::unicode...?

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

+                            None => 0,
+                            Some(i) => {
+                                - (input.len() as isize + newline_adjustment as isize
+                                - input.rfind(i).expect("no words to the left on this line") as isize)


Is there a way to avoid using rfind here? We're finding the last word, then searching back for it, which is less efficient than it could be.

I just updated with a new commit that does not use rfind

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

+            (_, Key::E) if mods.contains(CONTROL | ALT) => {
+                self.adjust_horizontal_to_end(Direction::Forward, maybe_select);
+                KeyReaction::RedrawSelection
+            }


Do we want these key bindings on all OSs? On, e.g. Linux, CTRL-F is "find", not "forward".

this will be for if CONTROL + ALT + F are all pressed together, so in theory it should not trigger the CTRL-F shortcut. I can add that as a test though

Silly me, forgot about the semantics of | on bitflags!

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

            (Some('a'), _) if is_control_key(mods) => {
                self.select_all();
                KeyReaction::RedrawSelection
-            },
+            }


Why are commas vanishing?

I noticed before my commit that some are them are without commas between match branches, not sure which one is more idiomatic. Do you prefer with commas or without?

I prefer with, but it's not exactly urgent.

asajeffrey · 2017-02-21T21:33:57Z

components/script/textinput.rs

+            (None, Key::Right) if mods.contains(ALT) => {
+                self.adjust_horizontal_by_word(Direction::Forward, maybe_select);
+                KeyReaction::RedrawSelection
+            }


Should ALT-Left and ALT-Right be forward and backward traversal of the session history?

I was going by the instructions in issue #12278 , but I can change it if you would prefer ALT-LEFT and ALT-Right to be used for something else. This would be within a text box only though.

Not sure about this, I'm not exactly a UI person! @paulrouget, any opinions?

asajeffrey · 2017-02-21T21:33:57Z

tests/unit/script/textinput.rs

+    assert_eq!(textinput.edit_point.index, 0);
+}
+
+#[test]


asajeffrey · 2017-02-27T15:19:52Z

components/script/textinput.rs

+                            None => break,
+                            Some(x) => {
+                                shift_temp += - (x.len() as isize);
+                                if x.chars().any(|x| x.is_alphabetic() || x.is_numeric()) {


Why the test for alphanumerics?

The unicode-segmentation crate (using .split_word_bounds()) determines word boundaries differently than how it should be implemented for this keyboard shortcut.

For example, "hello .. world" would be split up as ["hello", " ", ".", ".", " ","world"], whereas if the cursor was for example at the end of "hello", you would want this function to jump to the end of the "world" word. Hence you want it to ignore whitespaces and non alphabetic or numeric characters when evaluating where the next jump to is.

OK, seems sensible.

asajeffrey · 2017-02-27T21:01:34Z

At this point, the code looks good from a programming point of view, it could do with a review from a UI perspective though. @paulrouget?

paulrouget · 2017-02-28T19:39:23Z

It's pretty good!

I've found 2 issues:

fn+option+right/left go at the end/beginning of the line. I don't think it's supposed to
ctrl+a and ctrl+e, cmd+up, cmd+down are missing

Also, a more general issue: we should use physical lines (ends where text wraps) not text lines (ends at \n) when moving the cursor around. For example, cmd+right should jump right before where the line wraps, not before the \n.

But I think this can be addressed in another issue.

asajeffrey · 2017-02-28T21:48:20Z

@paulrouget thanks!

@clementmiao if you address Paul's comments, this is good to go.

nox · 2017-03-03T10:27:21Z

Also, a more general issue: we should use physical lines (ends where text wraps) not text lines (ends at \n) when moving the cursor around. For example, cmd+right should jump right before where the line wraps, not before the \n.

Depends which shortcut you are talking about. C^e should go at the end of the text line.

clementmiao · 2017-03-05T09:27:06Z

@paulrouget So I did not know this, but actually Fn+Left is doing Home, and Fn+Right does End at the OS level on OSX/macos. In chrome though, it seems it is overriden and instead does "top of page" and "bottom of page" instead.

Should I override the OS behavior to do Home/End for Fn+Left and Fn+Right respectively?

paulrouget · 2017-03-06T02:29:46Z

@paulrouget So I did not know this, but actually Fn+Left is doing Home, and Fn+Right does End at the OS level on OSX/macos. In chrome though, it seems it is overriden and instead does "top of page" and "bottom of page" instead.
Should I override the OS behavior to do Home/End for Fn+Left and Fn+Right respectively?

It's surprising that Fn+Left/Right doesn't do anything in the textbox itself. So yep, I guess we should not do anything special for Fn+Arrows then.

clementmiao · 2017-03-14T07:20:54Z

Not committed yet, but I have added Fn+Left/Right to do nothing inside the text box.

Cmd+Up and Cmd+Down were relatively straightforward.

Now regarding control+A and Control+E for macos, the distinction between text line end and text wrap end is not very clear. TextPoint.line is based on text line end (\n) and not on wrap-based lines, so it's not clear to me how I determine if something is on the next physical line with wrap (vs logical line with \n). line 0 can wrap unto the next physical line but is still line 0.

Being unfamiliar with how text boxes work, is there a way to determine if something is wrapped?

asajeffrey · 2017-03-16T19:37:43Z

@clementmiao I think it's going to be quite difficult to use physical lines rather than logical lines, since the linebreaking is (I believe) done by layout, and not very easily accessible in script. I'm okay with this going through as-is, and dealing with logical/physical lines separately.

clementmiao · 2017-03-23T05:22:56Z

sorry for the delay (been busy at work, ironically with rust stuff). I added the last remaining shortcuts as discussed. Ideally it would be as we talked to physical lines vs logical lines, but that's done in the layout logic.

asajeffrey · 2017-03-23T14:04:20Z

The unit tests aren't compiling, can you run ./mach test-unit -p script? Thanks!

clementmiao · 2017-03-24T07:05:56Z

replaced test function calls with new names. This is why you run your tests. Thanks!

asajeffrey · 2017-03-27T01:27:29Z

The tests appear to still be failing:

error[E0412]: cannot find type `DummyClipboardContext` in this scope
  --> C:\projects\servo\tests\unit\script\textinput.rs:17:51
   |
17 | fn text_input(lines: Lines, s: &str) -> TextInput<DummyClipboardContext> {
   |                                                   ^^^^^^^^^^^^^^^^^^^^^ not found in this scope
   |
   = help: possible candidate is found in another module, you can import it into scope:
             `use script::clipboard_provider::DummyClipboardContext;`

nox · 2017-04-04T11:56:51Z

@clementmiao Are you still working on this?

clementmiao · 2017-04-05T06:30:22Z

@nox I am, not sure why I did not get an e-mail notification. Let me see what's going on.

clementmiao · 2017-04-06T15:16:17Z

all the checks have passed now @nox

asajeffrey · 2017-04-06T15:54:08Z

LGTM, can you squash the commits?


        new keyboard shortcuts inside text input

clementmiao · 2017-04-07T15:01:12Z

@asajeffrey Squashed

jdm · 2017-04-12T03:44:41Z

@bors-servo: r=asajeffrey

bors-servo · 2017-04-12T03:44:42Z

📌 Commit ea3c840 has been approved by asajeffrey

bors-servo · 2017-04-12T08:01:18Z

⌛ Testing commit ea3c840 with merge 45f20f0...


        Auto merge of #15666 - clementmiao:keyboard_shortcuts, r=asajeffrey

added keyboard shortcuts for navigation inside text box PR to implement keyboard shortcuts per issue #12278, r? jdm Thanks for letting me help! ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12278 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15666)

bors-servo · 2017-04-12T08:36:43Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: asajeffrey
Pushing 45f20f0 to master...

asajeffrey · 2017-04-17T14:33:09Z

Yay, it landed, thanks @clementmiao!

clementmiao · 2017-04-17T18:30:30Z

Yay, This was a lot of fun! Thank you @asajeffrey, @paulrouget and @nox for your help! Let me know if there's anything I can help on.

highfive assigned asajeffrey Feb 21, 2017

highfive added the S-awaiting-review label Feb 21, 2017

asajeffrey reviewed Feb 21, 2017

View changes

asajeffrey reviewed Feb 27, 2017

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Apr 4, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 5, 2017

asajeffrey added S-needs-squash and removed S-awaiting-review labels Apr 6, 2017

highfive added the S-awaiting-review label Apr 7, 2017

clementmiao force-pushed the clementmiao:keyboard_shortcuts branch from 7c2ae65 to f42d4d0 Apr 7, 2017

new keyboard shortcuts inside text input

Loading status checks…

ea3c840

clementmiao force-pushed the clementmiao:keyboard_shortcuts branch from f42d4d0 to ea3c840 Apr 7, 2017

jdm removed the S-needs-squash label Apr 7, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 12, 2017

bors-servo merged commit ea3c840 into servo:master Apr 12, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 12, 2017

		@@ -4,11 +4,14 @@

		//! Common handling of keyboard input and state management for text input controls

		extern crate unicode_segmentation;

servo / servo

Join GitHub today

added keyboard shortcuts for navigation inside text box #15666

added keyboard shortcuts for navigation inside text box #15666

Conversation

clementmiao commented Feb 21, 2017 • edited by larsbergstrom

Thanks for letting me help!

highfive commented Feb 21, 2017

highfive commented Feb 21, 2017

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

clementmiao Feb 27, 2017 • edited

This comment has been minimized.

asajeffrey commented Feb 27, 2017

paulrouget commented Feb 28, 2017

asajeffrey commented Feb 28, 2017

nox commented Mar 3, 2017

clementmiao commented Mar 5, 2017

paulrouget commented Mar 6, 2017

clementmiao commented Mar 14, 2017

asajeffrey commented Mar 16, 2017

clementmiao commented Mar 23, 2017

asajeffrey commented Mar 23, 2017 • edited by jdm

clementmiao commented Mar 24, 2017

asajeffrey commented Mar 27, 2017

nox commented Apr 4, 2017

clementmiao commented Apr 5, 2017

clementmiao commented Apr 6, 2017

asajeffrey commented Apr 6, 2017

clementmiao commented Apr 7, 2017

jdm commented Apr 12, 2017

bors-servo commented Apr 12, 2017

bors-servo commented Apr 12, 2017

bors-servo commented Apr 12, 2017

asajeffrey commented Apr 17, 2017

clementmiao commented Apr 17, 2017

clementmiao commented Feb 21, 2017 •

edited by larsbergstrom

clementmiao Feb 27, 2017 •

edited

asajeffrey commented Mar 23, 2017 •

edited by jdm