servo / servo

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

Allows mac shortcuts to be used when inside a input element #12549

Closed

jimberlage wants to merge 1 commit into servo:master from jimberlage:12278/keybindings

+245 −4

Contributor

jimberlage commented Jul 22, 2016 •

edited by larsbergstrom

This adds the following navigation helpers on macOS:

Alt+LeftArrow / Alt+RightArrow - Jump to previous / next word in the
input.
Control+Alt+B / Control+Alt+F - Jump to previous / next word in the
input.
CMD+LeftArrow / CMD+RightArrow - Jump to beginning / end of the input.
Control+A / Control+E - Jump to the beginning / end of the
input.

It also respects shift when using these shortcuts to make a selection.

./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 assigned KiChjang

highfive commented Jul 22, 2016

Heads up! This PR modifies the following files:

@KiChjang: components/script/textinput.rs

highfive added the S-awaiting-review label


        Allows mac shortcuts to be used when inside a input element

f35888b

This adds the following navigation helpers on macOS:

Alt+LeftArrow / Alt+RightArrow - Jump to previous / next word in the
input.
Control+Alt+B / Control+Alt+F - Jump to previous / next word in the
input.
CMD+LeftArrow / CMD+RightArrow - Jump to beginning / end of the input.
Control+A / Control+E - Jump to the beginning / end of the
input.

It also respects shift when using these shortcuts to make a selection.

jimberlage force-pushed the jimberlage:12278/keybindings branch from 46e5777 to f35888b

Contributor

paulrouget commented Jul 22, 2016

Thank you for the PR.

Maybe in the future we'll want to use widget code for keybindings. For example: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventOverview/TextDefaultsBindings/TextDefaultsBindings.html

Member

KiChjang commented Jul 24, 2016

highfive assigned paulrouget and unassigned KiChjang

Member

KiChjang commented Jul 24, 2016

@bors-servo delegate=paulrouget

Contributor

bors-servo commented Jul 24, 2016

✌️ @paulrouget can now approve this pull request

Contributor

paulrouget commented Jul 26, 2016

I can review the behavior, but I'd like someone else to review the rust code (It looks good to me though).

Cmd+up/down, Ctrl+up/down, Cmd+home/end, Ctrl+a/e should move the cursor at the beginning/end of the whole text.

Option+up/down, should move the cursor at the beginning/end of the current line or of the next line if at the beginning/end of the current line.

There's probably more moves I missed. I think we should cover the bindings found in Gecko:

generic: https://dxr.mozilla.org/mozilla-central/source/dom/xbl/builtin/textareas-base.inc
mac specific: https://dxr.mozilla.org/mozilla-central/source/dom/xbl/builtin/mac/platformHTMLBindings.xml#33
I'm wondering if we also use this: https://dxr.mozilla.org/mozilla-central/source/dom/xbl/builtin/emacs/platformHTMLBindings.xml#14

We don't have to support the editing related bindings in this PR I think. Let's focus on getting the moves right.

jdm assigned KiChjang and unassigned paulrouget

pcwalton reviewed

View changes

components/script/textinput.rs

  
                            // UTF-8 character has variable size.

                            for ch in current_line[self.edit_point.index..].chars() {

                                i = i + (ch.len_utf8() as isize);

                            }

pcwalton Aug 8, 2016 •

edited

Contributor

How about current_line[self.edit_point.index..].chars().map(char::len_utf8()).sum() as isize?

pcwalton reviewed

View changes

components/script/textinput.rs

  
                                    Some(ch) => i = i - (ch.len_utf8() as isize),

                                    None => break,

                                }

                            }

pcwalton Aug 8, 2016

Contributor

How about i -= current_line[..self.edit_point.index].chars().map(char::len_utf8()).sum() as isize?

pcwalton reviewed

View changes

components/script/textinput.rs

  
                    (Some('c'), _) if is_control_key(mods) => {

                        if let Some(text) = self.get_selection_text() {

                            self.clipboard_provider.set_clipboard_contents(text);

                        }

                        KeyReaction::DispatchInput

                    },

                    #[cfg(target_os = "macos")]

                    (Some('e'), _) if (mods == CONTROL) || (mods == CONTROL | SHIFT) => {

pcwalton Aug 8, 2016

Contributor

Some of these parentheses are unnecessary.

pcwalton reviewed

View changes

components/script/textinput.rs

  
                    (None, Key::Right) => {

                        self.adjust_horizontal_by_one(Direction::Forward, maybe_select);

                        KeyReaction::RedrawSelection

                    }

                    },

pcwalton Aug 8, 2016

Contributor

nit: Remove this comma (and likewise for the above pattern arms)

pcwalton reviewed

View changes

components/script/textinput.rs

  
                let mut found_non_whitespace = false;

                let mut edit_point_index = Some(self.edit_point.index);

                'line: for line in self.lines[self.edit_point.line..].iter() {

pcwalton Aug 8, 2016

Contributor

nit: for line in &self.lines[self.edit_point.line..] {

pcwalton reviewed

View changes

components/script/textinput.rs

  
                        None => line.chars(),

                    };

                    'chars: for ch in char_iter {

pcwalton Aug 8, 2016

Contributor

Could just be for ch in line[edit_point_index.unwrap_or(0)..].chars() {. Also no need for the 'chars:

pcwalton reviewed

View changes

components/script/textinput.rs

  
                    // of the next line.

                    if found_non_whitespace { break 'line; }

                    adjust = adjust + 1; // Account for newline character

pcwalton Aug 8, 2016

Contributor

adjust += 1;

Member

KiChjang commented Aug 8, 2016

highfive assigned pcwalton and unassigned KiChjang

KiChjang added S-needs-code-changes and removed S-awaiting-review labels

Contributor

bors-servo commented Sep 9, 2016

☔ The latest upstream changes (presumably #13205) made this pull request unmergeable. Please resolve the merge conflicts.

highfive added the S-needs-rebase label

jdm added the S-needs-new-owner label

jdm mentioned this pull request

Support for native keybindings on input fields #12278

Closed

Member

jdm commented Jan 27, 2017

Closing due to lack of activity.

jdm closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.