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.)
The implementation of adjust_horizontal_to_limit() is written with UI in
mind. As such, when there's a selection and we "adjust horizontal", the
selection will be cleared and the cursor will and up at the start/end of
the previous selection. This is what happens when you have a selection
and you press an arrow key on your keyboard, but it isn't the behaviour
we want when programmatically changing the value.
Instead, we need to first clear the selection, and then move the cursor
to the end. (We also need to reset the selection direction when clearing
the selection.)
Per the spec, selectionStart and selectionEnd should return the same
values regardless of the selectionDirection. (That is, selectionStart is
always less than or equal to selectionEnd; the direction then implies
which of selectionStart or selectionEnd is the cursor position.)
There was no explicit WPT test for this, so I added one.
This bug was initially quite hard to wrap my head around, and I think
part of the problem is the code in TextInput. Therefore, in the process
of fixing it I have refactored the implementation of TextInput:
* Rename selection_begin to selection_origin. This value doesn't
necessarily correspond directly to the selectionStart DOM value - in
the case of a backward selection, it corresponds to selectionEnd.
I feel that "origin" doesn't imply a specific ordering as strongly as
"begin" (or "start" for that matter) does.
* In various other cases where "begin" is used as a synonym for "start",
just use "start" for consistency.
* Implement selection_start() and selection_end() methods (and their
_offset() variants) which directly correspond to their DOM
equivalents.
* Rename other related methods to make them less wordy and more
consistent / intention-revealing.
* Add assertions to assert_ok_selection() to ensure that our assumptions
about the ordering of selection_origin and edit_point are met. This
then revealed a bug in adjust_selection_for_horizontal_change() where
the value of selection_direction was not maintained correctly (causing
a unit test failure when the new assertion failed).
Handle cases where selection API doesn't apply
The selection API only applies to certain <input> types:
https://html.spec.whatwg.org/multipage/#do-not-apply
This commit ensures that we handle that correctly.
Some notes:
1. TextControl::set_dom_selection_direction now calls
set_selection_range(), which means that setting selectionDirection will
now fire a selection event, as it should per the spec.
2. There is a test for the firing of the select event in
tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
however the test did not run due to this syntax error:
(pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"
This happens due to the us of the "for (const foo of ...)" construct.
Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
this should actually work, so it's somewhat unsatisfying to have to
change the test.
3. I removed tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-not-application-textarea.html
because it doesn't seem to add any extra value - the selection API
always applies to textarea elements, and the API is tested elsewhere.
4. If an `<input>`'s type is unset, it defaults to a text, and the
selection API applies. Also, if an `<input>`'s type is set to an
invalid value, it defaults to a text too. This second case doesn't
currently work, and I'll need to do more restructuring of the code in
a future commit. See discussion with nox in IRC:
https://mozilla.logbot.info/servo/20171201#c13946454-c13946594
<!-- 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/19461)
<!-- Reviewable:end -->
The selection API only applies to certain <input> types:
https://html.spec.whatwg.org/multipage/#do-not-apply
This commit ensures that we handle that correctly.
Some notes:
1. TextControl::set_dom_selection_direction now calls
set_selection_range(), which means that setting selectionDirection will
now fire a selection event, as it should per the spec.
2. There is a test for the firing of the select event in
tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/select-event.html,
however the test did not run due to this syntax error:
(pid:26017) "ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/html/semantics/forms/textfieldselection/select-event.html:50:11 missing = in const declaration"
This happens due to the us of the "for (const foo of ...)" construct.
Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
this should actually work, so it's somewhat unsatisfying to have to
change the test.
4. If an <input>'s type is unset, it defaults to a text, and the
selection API applies. Also, if an <input>'s type is set to an
invalid value, it defaults to a text too. I've expanded the tests
to account for this second case.
