Implement Input type=text UA Shadow DOM#37065
Merged
jdm merged 13 commits intoservo:mainfrom May 30, 2025
Merged
Conversation
stevennovaryo
force-pushed
the
input-type-text
branch
from
b22d913 to
28d1d97
Compare
Contributor
Author
|
🔨 Triggering try run (#15159140690) for Linux (WPT) |
components/layout/dom_traversal.rs
Outdated
Comment on lines
+224
to
+235
| } else if parent_element.is_text_editing_root() { | ||
| let info = NodeAndStyleInfo::new( | ||
| parent_element, | ||
| parent_element.style(context.shared_context()), | ||
| ); | ||
| for child in iter_child_nodes(parent_element) { | ||
| if child.is_text_node() { | ||
| handler.handle_text(&info, child.to_threadsafe().node_text_content()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Can you explain why it is necessary to skip non-text children of text editing roots? It looks like in the current version of the shadow tree there aren't even any other children there?
Contributor
Author
There was a problem hiding this comment.
In terms of text input, it would be incorrect for a text editing root to contain other type of nodes. I suppose I could put a debug log here.
Contributor
There was a problem hiding this comment.
Since we have full control over what is inside the shadow tree I don't think we should modify dom_traversal.rs at all.
Contributor
Author
There was a problem hiding this comment.
I guess we still need to handle this kind of style context passing. Since the current implementation of caret and selection depends on parent's style. Once all input type's shadow tree is supported, this part of the code will actually replace the "input" element specific if condition above. Which, also handle empty input text. I should add a documentation about this.
|
Test results for linux-wpt from try job (#15159140690): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (17)
Stable unexpected results (4)
|
|
|
Contributor
Author
|
For the WPT test:
|
stevennovaryo
requested review from
Loirooriol,
gterzian,
mrobinson and
nicoburns
as code owners
xiaochengh
reviewed
May 22, 2025
Contributor
xiaochengh
left a comment
xiaochengh
left a comment
There was a problem hiding this comment.
I haven't done a full review yet. Just some initial comments.
| let text_shadow_tree = self.text_shadow_tree(can_gc); | ||
| let value = self.Value(); | ||
|
|
||
| let placeholder_text = match value.is_empty() { |
Contributor
There was a problem hiding this comment.
I think we don't need to dynamically set the placeholder's text content.
We can use :host(:placeholder-shown) #input-placeholder { visibility: hidden !important } instead.
Contributor
Author
There was a problem hiding this comment.
Sure, Thanks!
We still need to change its content whenever placeholder is modified by JavaScript though. But I guess it is much better to cache this.
| // TODO: Is there a less hacky way to do this? | ||
| let value_text = match value.is_empty() { | ||
| false => value, | ||
| true => "\u{200B}".into(), |
Contributor
There was a problem hiding this comment.
A linebreak (\n) is enough.
Contributor
Author
There was a problem hiding this comment.
I found that line break \n wouldn't trigger caret to render right now.
Contributor
There was a problem hiding this comment.
So in Servo <div><br></div> has zero height? That seems like a bug.
Contributor
Author
There was a problem hiding this comment.
Height for <div><br></div> seems normal. For node with line break \n as a text content, I suppose it happens because that kind of whitespace is trimmed somewhere in layout.
tests/wpt/meta/content-security-policy/form-action/form-action-src-default-ignored.sub.html.ini
Show resolved
Hide resolved
stevennovaryo
force-pushed
the
input-type-text
branch
from
06a2282 to
3300188
Compare
xiaochengh
reviewed
May 23, 2025
components/script/dom/node.rs
Outdated
|
|
||
| /// Whether this node has a serve as the text container for editable content of | ||
| /// <input> or <textarea> element. | ||
| const IS_TEXT_CONTROL_INNER_EDITOR = 1 << 13; |
Contributor
There was a problem hiding this comment.
Suggested change
| const IS_TEXT_CONTROL_INNER_EDITOR = 1 << 13; | |
| const IS_TEXT_CONTROL_INNER_EDITOR = 1 << 12; |
|
|
||
| // FIXME: These styles should be inside UA stylesheet, but it is not possible without internal pseudo element support. | ||
| const TEXT_TREE_STYLE: &str = " | ||
| #input-editor::selection, #input-placeholder::selection { |
Contributor
There was a problem hiding this comment.
placeholder should not be selectable and therefore should not have ::selection style.
Contributor
Author
There was a problem hiding this comment.
Sure, thanks!
| } | ||
|
|
||
| #input-container { | ||
| position: relative !important; |
Contributor
There was a problem hiding this comment.
We can drop !important if no external CSS can match this element.
I think only the placeholder can be matched via ::placeholder and therefore needs !important
Contributor
Author
There was a problem hiding this comment.
Sure, thanks!
| true => "\u{200B}".into(), | ||
| }; | ||
|
|
||
| // TODO(stevennovaryo): Introduce caching to prevent costly update. |
Contributor
There was a problem hiding this comment.
What kind of caching are you suggesting here?
Contributor
Author
There was a problem hiding this comment.
The most basic is storing "placeholder" as an attribute in HTMLInputElement. After some thoughts, the current placeholder does have that kind of role for type=text input. I will move the placeholder update into attribute mutated method instead.
| // TODO: Is there a less hacky way to do this? | ||
| let value_text = match value.is_empty() { | ||
| false => value, | ||
| true => "\u{200B}".into(), |
Contributor
There was a problem hiding this comment.
So in Servo <div><br></div> has zero height? That seems like a bug.
tests/wpt/meta/content-security-policy/form-action/form-action-src-default-ignored.sub.html.ini
Show resolved
Hide resolved
components/layout/dom_traversal.rs
Outdated
| } else if parent_element.is_text_control_inner_editor() { | ||
| // We are using parent's style for caret and selection color. | ||
| // FIXME: Once textarea and input element's shadow tree is supported, this would replace the if condition above. | ||
| let info = NodeAndStyleInfo::new( |
Contributor
There was a problem hiding this comment.
I don't understand this. Shouldn't child style be the same as parent_element?
Contributor
There was a problem hiding this comment.
I also don't understand why we need this if branch at all
Contributor
Author
There was a problem hiding this comment.
This part is also quite strange for me. It does looks like some workarounds for handling selection. At first, I thought that this would affect some selection styling.
But I have been looking at it again, and it seems that only selection query is depending on this one.
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
from
3300188 to
76b91c1
Compare
Member
|
I think this should be approved by @simonwuelker before landing as he is the one who has the most knowledge about this part of Servo. |
Contributor
Author
Definitely! I am also looking forward for the review. |
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
from
76b91c1 to
dc0b467
Compare
simonwuelker
requested changes
May 25, 2025
Contributor
simonwuelker
left a comment
simonwuelker
left a comment
There was a problem hiding this comment.
Looks mostly good to me, though I'm not sure if we actually need the IS_TEXT_CONTROL_INNER_EDITOR flag (see my question about the if branch in dom_traversal.rs.
Also, right now all input types that don't have their own dedicated widget render as a text input. Could we fall back to your type=text shadow tree for those too? That way we don't end up with multiple text input implementations.
components/layout/dom_traversal.rs
Outdated
| } else if parent_element.is_text_control_inner_editor() { | ||
| // We are using parent's style for caret and selection color. | ||
| // FIXME: Once textarea and input element's shadow tree is supported, this would replace the if condition above. | ||
| let info = NodeAndStyleInfo::new( |
Contributor
There was a problem hiding this comment.
I also don't understand why we need this if branch at all
Contributor
Author
There are three usage of
I suppose it could be assumed as a temporary flag, as pseudo element marking could replace this.
There should be a incompatibility with input that was implemented halfway through, like |
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
2 times, most recently
from
a57f22f to
a7175c5
Compare
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
from
a7175c5 to
d867aa6
Compare
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
from
67fdc2d to
9f65efe
Compare
simonwuelker
approved these changes
May 30, 2025
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
input-type-text
branch
from
dafdb7f to
f4c86c7
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Contributor
Author
|
I'm not sure what happened with the merge queue. All WPT tests seem to be canceled. |
Member
I believe what happened was that the unit test job failed, and job failures have the ability to cancel future jobs. |
vlindhol
added a commit
to vlindhol/servo
that referenced
this pull request
Jun 1, 2025
* main: (510 commits) DevTools: Fix empty `debugger > source` panel (servo#37197) dom: implement signal abort on controller and signal (servo#37192) build(deps): bump parking_lot from 0.12.3 to 0.12.4 (servo#37199) layout: Split overflow calculation after fragment tree construction (servo#37203) build(deps): bump parking_lot_core from 0.9.10 to 0.9.11 (servo#37202) build(deps): bump lock_api from 0.4.12 to 0.4.13 (servo#37201) build(deps): bump cc from 1.2.24 to 1.2.25 (servo#37198) Constellation can now optionally report memory usage when the page is loaded. (servo#37151) Implement Input `type=text` UA Shadow DOM (servo#37065) constellation: Wait for canvas thread to shut down before shutting down system font service (servo#37182) Add slot default display style test (servo#37189) Send synthetic keydown/keyup at ime_insert_text (servo#37175) script: Let canvas serialization to image fail gracefully (servo#37184) Implement basics of link preloading (servo#37036) compositor: Add an initial RefreshDriver (servo#37169) pixels: Add limitation to max image total bytes length (servo#37172) Chore: Remove unused variable in `transition-zero-duration-with-delay.html` (servo#37179) build(deps): bump ohos-ime from 0.2.0 to 0.3.0 (servo#37180) Add a user agent style for the `<slot>` element (servo#37174) build(deps): bump hitrace from 0.1.4 to 0.1.5 (servo#37170) ...
webbeef
added a commit
to webbeef/servo
that referenced
this pull request
Jun 6, 2025
This reverts commit 5580704. Signed-off-by: webbeef <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 6, 2025
This reverts commit 5580704. Let's re-land that fix when a working solution is found. Keeping that regression makes it hard to evaluate other potential improvements. Signed-off-by: webbeef <[email protected]>
stevennovaryo
added a commit
to stevennovaryo/servo
that referenced
this pull request
Jun 9, 2025
Implement Shadow Tree construction for input `type=text`, adding a text control inner editor container and placeholder container. Subsequently, due to the changes of the DOM tree structure, the changes will add a new NodeFlag `IS_TEXT_CONTROL_INNER_EDITOR` to handle the following cases. - If a mouse click button event hits a text control inner editor, it will redirect the focus target to its shadow host. - In text run's construction, the text control inner editor container queries the selection from its shadow host. This is later used to resolve caret and selection painting in the display list. This will be the first step of fixing input `type=text` and other single-line text input element widgets. Such as, implementing `::placeholder` selector. Testing: Existing WPT test and new Servo specific appearance WPT. Fixes: servo#36307 --------- Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
added a commit
to stevennovaryo/servo
that referenced
this pull request
Jun 9, 2025
This reverts commit 5580704.
stevennovaryo
added a commit
to stevennovaryo/servo
that referenced
this pull request
Jun 10, 2025
Implement Shadow Tree construction for input `type=text`, adding a text control inner editor container and placeholder container. Subsequently, due to the changes of the DOM tree structure, the changes will add a new NodeFlag `IS_TEXT_CONTROL_INNER_EDITOR` to handle the following cases. - If a mouse click button event hits a text control inner editor, it will redirect the focus target to its shadow host. - In text run's construction, the text control inner editor container queries the selection from its shadow host. This is later used to resolve caret and selection painting in the display list. This will be the first step of fixing input `type=text` and other single-line text input element widgets. Such as, implementing `::placeholder` selector. Testing: Existing WPT test and new Servo specific appearance WPT. Fixes: servo#36307 --------- Signed-off-by: stevennovaryo <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 23, 2025
) Depends on #37427. In addition to the changes introduced by #37065, there are several performance improvements and nits as follows: - Use the internal pseudo element for style matching, this will reduce the performance regression by ~66%. - Manual construction of the `Text` node inside a text container. This is followed by the modification of the inner `Text` node instead of using `SetTextContent` which is more expensive. - Use `implemented_pseudo_element` instead of `text_control_inner_editor` `NodeFlag` to handle the special cases that these elements should follow, specifically the: - focus delegation workaround; - selections; and - line height resolving. - More documentation. Servo's side of: servo/stylo#217 Testing: No new unexpected WPT failure, except for the one introduced by #37065. Fixes: #36307 #37205 --------- Signed-off-by: stevennovaryo <[email protected]>
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement Shadow Tree construction for input
type=text, adding a text control inner editor container and placeholder container. Subsequently, due to the changes of the DOM tree structure, the changes will add a new NodeFlagIS_TEXT_CONTROL_INNER_EDITORto handle the following cases.This will be the first step of fixing input
type=textand other single-line text input element widgets. Such as, implementing::placeholderselector.Testing: Existing WPT test and new Servo specific appearance WPT.
Fixes: #36307