Event dispatch rewritten to align to spec, activate on clicks better
I went over the changes to the event dispatch spec that had accumulated over the past few years, rewriting dispatch/invoke/inner-invoke almost completely and modifying other code where it was relevant. Most of the remaining obvious deviations from spec are things that will only come up when we start handling events in shadow DOM.
I am pushing now because I want to see CI test results, but please do not approve this PR just if automated test improvements look good. I may have broken some actual UI interactions in the course of fixing synthetic events, and some manual testing is needed, including checking that manual interactions with interactive content continue to fire the events they're supposed to.
---
<!-- 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
- [X] These changes fix#25384 and fix#22783 and fix#25199
<!-- Either: -->
- [ ] There are automated tests for the synthetic-click parts of these changes, BUT the effects on real UI events need some manual testing before merging
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Simplify and fix iteration over radio button group
- Code for iterating over radio button group elements has been extracted to `radio_group_iter`
- `in_same_group` now checks if elements are in the same tree
- `radio_group_name` returns `None` if name is empty
- Radio buttons now group together in orphan trees (#25486)
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#25486
- [ ] There are tests for these changes
Implement HTMLInputElement cloning steps
This change adds missing [cloning steps for input elements](https://html.spec.whatwg.org/multipage/#the-input-element%3Aconcept-node-clone-ext).
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix#25385
- [X] There are tests for these changes
Issue 21810/improve validation methods
<!-- Please describe your changes on the following line: -->
This is a start at addressing #21810. I'm putting these changes out early to get some feedback on the following items:
1. I added unit tests for the validation methods mentioned in #21810, because I couldn't tell whether any of the existing WPT tests covered them. Are these tests worthwhile? Are any of them unnecessary?
2. I changed the implementation for `is_valid_floating_point_number_string` so that it passed the tests. The previous version of the function wasn't restrictive enough (it allowed certain whitespace characters before the number string).
3. I changed the catch-all condition in `htmlinputelement.rs` to account for the remaining input types that don't have a value sanitization algorithm. This last change seems good to me since we won't be able to add a new input type without adding it to the case and checking the spec for an algorithm.
---
<!-- 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
- [x] These changes fix#21810
<!-- Either: -->
- [x] There are tests for these changes
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Replaced catch-all with explicit case for inputs that do not have
a value sanitization algorithm. This should prevent us from
forgetting to implement a sanitization for an input, since they
must all be accounted for in the match expression.
Handle "Multiple" attribute for email value sanitization
Fix code formatting
Tests for email value sanitization should pass
Make failing wpt tests pass
Change new_value => sanitized
The following IDLs have the src/href attributes typed as a DOMString
while in the spec the attribute has been updated to be a USVString:
- HTMLIFrameElement
- HTMLImageElement
- HTMLInputElement
- HTMLLinkElement
- HTMLMediaElement
- HTMLScriptElement
Forward WebDriver CompositionEvent
Dispatch composition events in JS.
Insert characters from composition events to text input.
CompositionEvents currently can only be
created by WebDriver and not by embedders.
<!-- Please describe your changes on the following line: -->
---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).
<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->
<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
<!-- 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/22224)
<!-- Reviewable:end -->
Dispatch composition events in JS.
Insert characters from composition events to text input.
CompositionEvents currently can only be
created by WebDriver and not by embedders.
I moved away from the `Window` struct all the logic to handle task
sources, into a new struct called `TaskManager`. In a happy world, I'd
be able to just have there two functions, of the types:
```rust
fn task_source<T: TaskSource>(&self, name: TaskSourceName) -> Box<T>
fn task_source_with_canceller<T: TaskSource>(&self, name: TaskSourceName)
-> (Box<T>, TaskSourceCanceller)
```
And not so much duplicated code. However, because TaskSource can't be a
trait object (because it has generic type parameters), that's not
possible. Instead, I decided to reduce duplicated logic through macros.
For reasons[1], I have to pass both the name of the function with
canceller and the name of the function without, as I'm not able to
concatenate them in the macro itself. I could probably use
`concat_idents` to create both types already defined and reduce the
amount of arguments by one, but that macro is nightly only. At the same
time, not being able to declare macros inside `impl` forces me to pass
`self` as an argument.
All this makes this solution more verbose than it would be ideally. It
does reduce duplication, but it doesn't reduce the size of the file.
[1](https://github.com/rust-lang/rust/issues/29599)
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.
However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.
I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)
The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:
https://github.com/whatwg/html/issues/2424
The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
The intention here is to make the flow more explicit. I.e. rather than
calling `self.dom_select()` and relying on the programmer to
know/realise that this method is provided by a trait, we call
`self.selection().dom_select()` and the programmer can inspect the
definition of `self.selection()` to follow the code.
This came out of a discussion with KiChjang here:
https://github.com/servo/servo/pull/19544#discussion_r156167044
Note that I tried to make "selection" be a member field of
HTML{Input,TextArea}Element but it opened up a whole can of worms with
lifetimes, so it seemed simpler to not do that since it is not
essential for this code to work.
Spec: https://html.spec.whatwg.org/multipage/input.html#input-type-change
In short, this resets the selection to the start of the field when the
type has changed from one which doesn't support the selection API to one
that does.
I couldn't see an existing WPT test covering this.
Spec: https://html.spec.whatwg.org/multipage/#dom-textarea/input-setrangetext
In order to do this, we need to define the SelectionMode enum in WebIDL:
https://html.spec.whatwg.org/multipage/#selectionmode
Since the enum is used by HTMLTextAreaElement and HTMLInputElement, it
doesn't seem to make sense to define it in the WebIDL file for one or
other of those.
However, we also can't create a stand-alone SelectionMode.webidl file,
because the current binding-generation code won't generate a "pub mod
SelectionMode;" line in mod.rs unless SelectionMode.webidl contains
either an interface or a namespace. (This logic happens in
components/script/dom/bindings/codegen/Configuration.py:35, in the
Configuration.__init__ method.)
I thought about changing the binding-generation code, but that seems
difficult. So I settled for placing the enum inside
HTMLFormElement.webidl, as that seems like a "neutral" location. We
could equally settle for putting it under HTMLTextAreaElement or
HTMLInputElement, it probably doesn't really matter.
The setRangeText algorithm set the "dirty value flag" on the
input/textarea. I made some clean-ups related to this:
1. HTMLTextAreaElement called its dirty value flag "value_changed"; I
changed this to "value_dirty" to be consistent with the spec.
2. HTMLInputElement had a "value_changed" field and also a "value_dirty"
field, which were each used in slightly different places (and
sometimes in both places). I consolidated these into a single
"value_dirty" field, which was necessary in order to make some of the
tests pass.
TextControl::set_dom_range_text replaces part of the existing textinput
content with the replacement string (steps 9-10 of the algorithm). My
implementation changes the textinput's selection and then replaces the
selection. A downside of this approach is that we lose the original
selection state from before the call to setRangeText. Therefore, we have
to save the state into the original_selection_state variable so that we
can later pass it into TextControl::set_selection_range. This allows
TextControl::set_selection_range to correctly decide whether or not to
fire the select event.
An alternative approach would be to implement a method on TextInput
which allows a subtring of the content to be mutated, without touching
the current selection state. However, any such method would potentially
put the TextInput into an inconsistent state where the edit_point and/or
selection_origin is a TextPoint which doesn't exist in the content. It
would be up to the caller to subsequently make sure that the TextInput
gets put back into a valid state (which would actually happen, when
TextControl::set_selection_range is called).
I think TextInput's public API should not make it possible to put it
into an invalid state, as that would be a potential source of bugs.
That's why I didn't take this approach. (TextInput's public API does
currently make it possible to create an invalid state, but I'd like to
submit a follow-up patch to lock this down.)
