Reland Input type=text Shadow DOM With Performance Improvement#37483
Merged
xiaochengh merged 3 commits intoservo:mainfrom Jul 23, 2025
Merged
Reland Input type=text Shadow DOM With Performance Improvement#37483xiaochengh merged 3 commits intoservo:mainfrom
type=text Shadow DOM With Performance Improvement#37483xiaochengh merged 3 commits intoservo:mainfrom
Conversation
Contributor
Author
Performance ImpactNote that these changes will definitely introduce some performance regression due to the bulkier Specifically, the main cause of the regression should be the construction of the UA shadow DOM. Particularly, it will construct 6 additional nodes that include a shadow root, three div elements, and two text nodes. The previous version of Future Plan
cc: @xiaochengh |
stevennovaryo
force-pushed
the
input-type-text-reland
branch
from
a86aff8 to
52697ae
Compare
stevennovaryo
changed the title
type=text With Performance Improvementtype=text Shadow DOM With Performance Improvement
This was referenced Jun 18, 2025
stevennovaryo
force-pushed
the
input-type-text-reland
branch
from
52697ae to
5fd2d6e
Compare
stevennovaryo
requested review from
Loirooriol,
gterzian,
mrobinson and
nicoburns
as code owners
|
🔨 Triggering try run (#16185265029) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16185265029): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (23)
Stable unexpected results (6)
|
|
|
stevennovaryo
force-pushed
the
input-type-text-reland
branch
from
5fd2d6e to
8fb0489
Compare
|
🔨 Triggering try run (#16187653685) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16187653685): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (17)
|
|
✨ Try run (#16187653685) succeeded. |
mrobinson
previously requested changes
Jul 10, 2025
Comment on lines
+1
to
+2
| [input_placeholder.html] | ||
| expected: FAIL |
Member
There was a problem hiding this comment.
Why is this test now failing?
Contributor
Author
There was a problem hiding this comment.
We are now using grey color for the placeholder text, while that test assumes placeholder and value share the appearence. #37065 (comment)
Contributor
There was a problem hiding this comment.
We should modify the test case instead.
Contributor
Author
There was a problem hiding this comment.
Per offline discussion with @xiaochengh. The test looks quite flawed, in terms that it was assuming placeholder text should have a same style as the editable text. So we are removing the test. The new input could relies on the new appearance test.
Comment on lines
+1229
to
+1236
|
|
||
| // The addition of zero-width space here forces the text input to have an inline formatting | ||
| // context that might otherwise be trimmed if there's no text. This is important to ensure | ||
| // that the input element is at least as tall as the line gap of the caret: | ||
| // <https://drafts.csswg.org/css-ui/#element-with-default-preferred-size>. | ||
| // | ||
| // This is also used to ensure that the caret will still be rendered when the input is empty. | ||
| // TODO: Is there a less hacky way to do this? |
Member
There was a problem hiding this comment.
Why is this copied from layout? Can we remove the corresponding code there as well? I It's possible that with shadow DOM and styling support this hack can be removed
Contributor
There was a problem hiding this comment.
Instead of hacking the text value, can we append a <br> to the inner editor element?
I just tested the latest Servo and confirmed that <div><br></div> doesn't collapse.
Contributor
Author
There was a problem hiding this comment.
Why is this copied from layout?
Practically, we are moving this from layout to script. Though the documentation in layout should be changed.
Can we remove the corresponding code there as well?
We are not able to, because the other types of input are still using that hack. Added a documentation for this.
Contributor
Author
There was a problem hiding this comment.
I It's possible that with shadow DOM and styling support this hack can be removed
It might be possible with using something like <br> or modifying the styles. We could file a issue for this, but I am not sure it should be filed before the PR getting merged or after.
Instead of hacking the text value, can we append a
<br>to the inner editor element?
I am not sure that It would not disturb with the DOM modification presented by the PRs. Since we are using the CharacterData right now and modifying the inner data of it to improve the text editing performance, while <br> is another type of element.
Contributor
Author
There was a problem hiding this comment.
Per offline discussion, using appending extra <br> to input, alongside the CharacterData for editable text seems very reasonable, but this would prevent the caret from being rendered. This behavior seems unintended, so we should fix the caret rendering first before removing this hack.
components/layout/dom_traversal.rs
Outdated
| } | ||
|
|
||
| /// Whether this is a container for the editable text within a single-line text input. | ||
| /// This is used to solve the special case of line height for a text editor. |
Member
There was a problem hiding this comment.
Nit: "editor" isn't quite the right word here as a "text editor" is a program for editing text. What is it exactly that you are trying to describe? Is it the text inside the text entry surrounded by the border, etc? I would call it the "text entry text" or something along those lines maybe so "is_single_line_text_entry_text_element" or something like that.
Contributor
There was a problem hiding this comment.
We probably copied this name from Blink, using "inner editor" to refer to the container element of the value text inside the shadow DOM. The element is implemented as a contenteditable in Blink.
Contributor
Author
There was a problem hiding this comment.
Added more context for the TODOs. Ideally this would refer only to the text container within input element. But because we are supporting the older version of input element (one without UA widget) as well, then we are still considering the HTMLInputElement here.
Comment on lines
+1
to
+14
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <title>Appearance of an Input type=text With a Definite Width</title> | ||
| <link rel="stylesheet" href="./supports/input-text-ref.css"> | ||
| </head> | ||
| <body> | ||
| Display of an input type=text should match the display generated by the CSS reference. | ||
| <div> | ||
| <div id="input" class="definite-width"> | ||
| Foo | ||
| </div> | ||
| </div> | ||
| </body> |
Member
There was a problem hiding this comment.
I have to think about these tests a bit, as I think they are just verifying that the style of the fields doesn't change from what you have originally chosen for them. It may be enough just to have a manual text with all of the form controls. More important are things such as ensuring the placeholder works properly. That said, I think they probably don't hurt.
Contributor
Author
There was a problem hiding this comment.
The purpose of these tests is to make sure that the appearance is correct without comparing it to another input element. Non Servo-specific WPTs are unable to do these test since each browser have their own input styles and appearance.
Ideally we could do comparison to an image like how Chromium handles this, but keeping input elements up to the standard would be more important.
xiaochengh
reviewed
Jul 11, 2025
xiaochengh
reviewed
Jul 11, 2025
stevennovaryo
force-pushed
the
input-type-text-reland
branch
from
58731a8 to
1cd0cfc
Compare
xiaochengh
approved these changes
Jul 16, 2025
stevennovaryo
force-pushed
the
input-type-text-reland
branch
from
1cd0cfc to
b0d7a75
Compare
xiaochengh
dismissed
mrobinson’s stale review
Review comments are already addressed without further feedback
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-reland
branch
from
b0d7a75 to
b2c5310
Compare
Member
|
Out of curiosity, what kind of results do you see before and after when running Speedometer 2.1? |
Contributor
Author
From one of the test:
It was from a test more than one month ago. cc: @webbeef My tests before getting this PR merged also produces similar results (i.e., reduce the performance regression by ~66%) |
18 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 25, 2025
Depend on: - #37427 - #37483 Utilize input `type=text` for the display of all textual input. In which, consist of https://html.spec.whatwg.org/#the-input-element-as-a-text-entry-widget and https://html.spec.whatwg.org/#the-input-element-as-domain-specific-widgets inputs. For `password`, `url`, `tel`, and, `email` input, the appearance of input container is exactly the same as the `text` input. Other types of textual input simply extends `text` input by adding extra components inside the container. Testing: Servo textual input appearance WPT. --------- Signed-off-by: stevennovaryo <[email protected]> Signed-off-by: Jo Steven Novaryo <[email protected]>
minghuaw
pushed a commit
to minghuaw/servo
that referenced
this pull request
Aug 1, 2025
Depend on: - servo#37427 - servo#37483 Utilize input `type=text` for the display of all textual input. In which, consist of https://html.spec.whatwg.org/#the-input-element-as-a-text-entry-widget and https://html.spec.whatwg.org/#the-input-element-as-domain-specific-widgets inputs. For `password`, `url`, `tel`, and, `email` input, the appearance of input container is exactly the same as the `text` input. Other types of textual input simply extends `text` input by adding extra components inside the container. Testing: Servo textual input appearance WPT. --------- Signed-off-by: stevennovaryo <[email protected]> Signed-off-by: Jo Steven Novaryo <[email protected]>
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
3 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.
Depends on #37427.
In addition to the changes introduced by #37065, there are several performance improvements and nits as follows:
Textnode inside a text container. This is followed by the modification of the innerTextnode instead of usingSetTextContentwhich is more expensive.implemented_pseudo_elementinstead oftext_control_inner_editorNodeFlagto handle the special cases that these elements should follow, specifically the:Servo's side of: servo/stylo#217
Testing: No new unexpected WPT failure, except for the one introduced by #37065.
Fixes: #36307 #37205