Properly dispatch keypress event #14738

Merged
merged 3 commits into from Jan 4, 2017

Projects

None yet

5 participants

@Wafflespeanut
Member
Wafflespeanut commented Dec 26, 2016 edited

This was an attempt to fix #14659. It turned out that the problem wasn't what I thought it was. So, I didn't fix that. On the brighter side, this fixes two related issues.

  • Previously, we were unable to launch keypress events from input and textarea elements, because we'd been cancelling the key events, so that they don't trigger window navigation - #8400). I've introduced an enum to represent an additional state to an event's cancellation.
  • According to the spec, keypress (if available) should be dispatched immediately after keydown, and it should be followed by input. Canceling keypress should also cancel input. But, we'd been dispatching input before keypress. We now dispatch input once the keypress event is on the respective elements.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it's a refactor?

r? @jdm or anyone interested


This change is Reviewable

@jdm jdm was assigned by highfive Dec 26, 2016
@highfive

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/document.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/event.rs, components/script/dom/htmlinputelement.rs
  • @kichjang: components/script/dom/document.rs, components/script/dom/htmltextareaelement.rs, components/script/dom/event.rs, components/script/dom/htmlinputelement.rs
@highfive

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
components/script/dom/event.rs
@@ -77,14 +77,21 @@ impl From<bool> for EventCancelable {
}
}
+#[derive(JSTraceable, HeapSizeOf, Copy, Clone, PartialEq)]
+pub enum EventDefault {
@Ms2ger
Ms2ger Dec 26, 2016 Contributor

This is going to need some clear documentation

@Wafflespeanut
Wafflespeanut Dec 27, 2016 Member

Certainly :)

@Wafflespeanut
Member

Do we need tests for this?

Wafflespeanut added some commits Dec 27, 2016
@Wafflespeanut Wafflespeanut Allow 'keypress' event to emerge from input and textarea elements 08662cc
@Wafflespeanut Wafflespeanut Fire 'input' event after 'keypress' in input and textarea elements
5f0b3bd
@jdm
Member
jdm commented Dec 27, 2016

I don't think we can write automated tests for this yet, because we can't synthesize key input.

@jdm
Member
jdm commented Jan 4, 2017

@bors-servo: r+
Nice work. Sorry about the review delay!

@bors-servo
Contributor

📌 Commit 5f0b3bd has been approved by jdm

@Wafflespeanut
Member

Oh please... I haven't actually fixed the issue yet. I'll get back into it once again this weekend 😄

@bors-servo
Contributor

⌛️ Testing commit 5f0b3bd with merge 6f9ff7b...

@bors-servo bors-servo added a commit that referenced this pull request Jan 4, 2017
@bors-servo bors-servo Auto merge of #14738 - Wafflespeanut:keypress, r=jdm
Properly dispatch keypress event

<!-- Please describe your changes on the following line: -->

This was an attempt to fix #14659. It turned out that the problem wasn't what I thought it was. So, I didn't fix that. On the brighter side, this fixes two related issues.

- Previously, we were unable to launch `keypress` events from `input` and `textarea` elements, because [we'd been cancelling](https://github.com/servo/servo/blob/1327ebd52f53f5f6637a12fab6cf0cad0aa0be6f/components/script/dom/htmlinputelement.rs#L1120-L1124) the key events, so that they don't trigger window navigation - #8400). I've introduced an enum to represent an additional state to an event's cancellation.
- [According to the spec](https://w3c.github.io/uievents/#keypress-event-order), `keypress` (if available) should be dispatched immediately after `keydown`, and it should be followed by `input`. Canceling `keypress` should also cancel `input`. But, we'd been dispatching `input` before `keypress`. We now dispatch `input` once the `keypress` event is on the respective elements.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] These changes do not require tests because it's a refactor?

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

r? @jdm or anyone interested

<!-- 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/14738)
<!-- Reviewable:end -->
6f9ff7b
@bors-servo bors-servo merged commit 5f0b3bd into servo:master Jan 4, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@Wafflespeanut Wafflespeanut deleted the Wafflespeanut:keypress branch Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment