Skip to content

Conversation

@RichardTjokroutomo
Copy link
Contributor

@RichardTjokroutomo RichardTjokroutomo commented Nov 18, 2025

Enable text-overflow for Servo. Companion PR: #40526

delan and others added 13 commits November 9, 2025 05:52
Any ancestors of this commit are from upstream mozilla-central, with
some filtering and renaming. Our patches and sync tooling start here.

The sync tooling has all been squashed into this commit, based on:
https://github.com/servo/stylo/commits/64731e10dc8ef87ef52aa2fb9f988c3b2530f3a7
This is a rebase of aac89c5

Signed-off-by: Oriol Brufau <[email protected]>
- Bump stylo_* to 0.9.0
- Bump servo_arc to 0.4.3
- Bump selectors to 0.33.0
- Bump to_shmem to 0.3.0

Signed-off-by: Oriol Brufau <[email protected]>
This will allow de-duplicating phf in Servo.

Signed-off-by: Oriol Brufau <[email protected]>
Copy link
Collaborator

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said in the Servo review, since it's not handling strings or 2 values, this should alter the parser to reject these cases for Servo.

Alternatively, you can put this property behind a flag, and then in Servo make it an experimental web platform feature flag. Then it won't be enabled by default, but it will be used in tests.

* Support font-optical-sizing in servo

Signed-off-by: Simon Wülker <[email protected]>

* Add font-variation-settings to list of font shorthands for servo

Signed-off-by: Simon Wülker <[email protected]>

---------

Signed-off-by: Simon Wülker <[email protected]>
@RichardTjokroutomo RichardTjokroutomo force-pushed the txt-overflow branch 4 times, most recently from ebdcfa5 to 1f40f82 Compare November 26, 2025 09:00
input: &mut Parser<'i, 't>,
) -> Result<TextOverflow, ParseError<'i>> {
let first = TextOverflowSide::parse(context, input)?;
let first = <TextOverflowSide as Parse>::parse(context, input)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this?

Copy link
Contributor Author

@RichardTjokroutomo RichardTjokroutomo Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for some reason, stylo now can't correctly decide which Parse to use (or rather, for some reason in the past, it can choose the right one), and so if you don't specify, it will result in a compilation error:

error[E0061]: this function takes 1 argument but 2 arguments were supplied
   --> style\values\specified\text.rs:231:21
    |
231 |         let first = TextOverflowSide::parse(context, input)?;
    |                     ^^^^^^^^^^^^^^^^^^^^^^^ ------- unexpected argument #1 of type `&ParserContext<'_>`
    |
note: associated function defined here
   --> style\values\specified\text.rs:178:5
    |
178 |     Parse,
    |     ^^^^^
    = note: this error originates in the derive macro `Parse` (in Nightly builds, run with -Z macro-backtrace for more info)
help: remove the extra argument
    |
231 -         let first = TextOverflowSide::parse(context, input)?;
231 +         let first = TextOverflowSide::parse(input)?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo it, no longer needed

first,
second,
sides_are_logical: false,
if let Ok(second) = input.try_parse(|input| <TextOverflowSide as Parse>::parse(context, input)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the other case, the compiler now can't resolve to the correct Parse.

error[E0061]: this function takes 1 argument but 2 arguments were supplied
   --> style\values\specified\text.rs:231:21
    |
231 |         let first = TextOverflowSide::parse(context, input)?;
    |                     ^^^^^^^^^^^^^^^^^^^^^^^ ------- unexpected argument #1 of type `&ParserContext<'_>`
    |
note: associated function defined here
   --> style\values\specified\text.rs:178:5
    |
178 |     Parse,
    |     ^^^^^
    = note: this error originates in the derive macro `Parse` (in Nightly builds, run with -Z macro-backtrace for more info)
help: remove the extra argument
    |
231 -         let first = TextOverflowSide::parse(context, input)?;
231 +         let first = TextOverflowSide::parse(input)?;

@RichardTjokroutomo
Copy link
Contributor Author

@Loirooriol I noticed earlier today that a PR on Stylo has been merged (#271), so I tried to rebase. However, I got some compilation errors.

Copy link
Collaborator

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since changing the struct affects things more than what I expected, maybe just do something like

        #[cfg(feature = "servo")]
        if matches!(first, TextOverflowSide::String(_)) {
            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
        }
        #[cfg(feature = "gecko")]
        if let Ok(second) = input.try_parse(|input| TextOverflowSide::parse(context, input)) {
            return Ok(Self {
                first,
                second,
                sides_are_logical: false,
            });
        }
        Ok(Self {
            first: TextOverflowSide::Clip,
            second: first,
            sides_are_logical: true,
        })

Or add support for strings as Nico says in servo/servo#40526 (comment)

Signed-off-by: Richard Tjokroutomo <[email protected]>
Co-authored-by: Oriol Brufau <[email protected]>
@RichardTjokroutomo RichardTjokroutomo force-pushed the txt-overflow branch 2 times, most recently from 6bdf687 to cb461f6 Compare November 27, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants