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

Return `KeyReaction::Nothing` for a Tab event #14184

Merged
merged 1 commit into from Nov 12, 2016
Merged

Conversation

@jmcomets
Copy link
Contributor

jmcomets commented Nov 12, 2016

Do nothing instead of triggering the default action for a tab event.

Hitting the tab key in an html text input shouldn't submit the form, and for any text input, the tab key should have a particular action associated, not the default action.
This cleans up #12701.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

This change is Reviewable

Hitting the tab key in an html text input shouldn't submit the form, and
for any text input, the tab key should have a particular action
associated, not the default action. This cleans up #12701.
@highfive
Copy link

highfive commented Nov 12, 2016

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

@highfive
Copy link

highfive commented Nov 12, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlinputelement.rs, components/script/textinput.rs
  • @KiChjang: components/script/dom/htmlinputelement.rs, components/script/textinput.rs
@highfive
Copy link

highfive commented Nov 12, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
Copy link
Contributor

metajack left a comment

Looks good, but appears you accidentally deleted a TODO.

keyevent.AltKey(),
keyevent.MetaKey()),
// Issue #12071: Tab should not submit forms
// TODO(3982): Implement form keyboard navigation

This comment has been minimized.

Copy link
@metajack

metajack Nov 12, 2016

Contributor

Was it intentional that you removed this TODO?

@jmcomets
Copy link
Contributor Author

jmcomets commented Nov 12, 2016

It was, but I hesitated. I commented on #3982, explaining that I believe
form keyboard navigation shouldn't be implemented on an HTMLInputElement,
but globally. This TODO implies that #3982 should be implemented there,
which is why I deleted it.

On Sat, Nov 12, 2016 at 4:33 PM Jack Moffitt notifications@github.com
wrote:

@metajack requested changes on this pull request.

Looks good, but appears you accidentally deleted a TODO.

In components/script/dom/htmlinputelement.rs
#14184 (review):

@@ -1097,18 +1096,10 @@ impl VirtualMethods for HTMLInputElement {
let action = self.textinput.borrow_mut().handle_keydown(keyevent);
match action {
TriggerDefaultAction => {

  •                        if let Some(key) = keyevent.get_key() {
    
  •                            match key {
    
  •                                Key::Enter | Key::KpEnter =>
    
  •                                    self.implicit_submission(keyevent.CtrlKey(),
    
  •                                                             keyevent.ShiftKey(),
    
  •                                                             keyevent.AltKey(),
    
  •                                                             keyevent.MetaKey()),
    
  •                                // Issue #12071: Tab should not submit forms
    
  •                                // TODO(3982): Implement form keyboard navigation
    

Was it intentional that you removed this TODO?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#14184 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABzmgyy5p_QDE_7SyH27gbfFcoy25-Ciks5q9dxRgaJpZM4Kwb5Y
.

@metajack
Copy link
Contributor

metajack commented Nov 12, 2016

@bors-servo r+

That's fair.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2016

📌 Commit 3191536 has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2016

Testing commit 3191536 with merge f04033a...

bors-servo added a commit that referenced this pull request Nov 12, 2016
Return `KeyReaction::Nothing` for a Tab event

Do nothing instead of triggering the default action for a tab event.

Hitting the tab key in an html text input shouldn't submit the form, and for any text input, the tab key should have a particular action associated, not the default action.
This cleans up #12701.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14184)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 12, 2016

@bors-servo bors-servo merged commit 3191536 into servo:master Nov 12, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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