Implement string-valued text-overflow in Servo

[Manishearth]

Part of #13702

In #13701, we added support for parsing the full range of values that text-overflow supports. However, this is disabled in Servo mode

While supporting the two-value form of text-overflow (e.g. text-overflow: "..." clip) is cumbersome, it should be pretty straightforward to support the one-value form.

Steps:

• Remove the conditional in https://github.com/Manishearth/servo/blob/ea765688cbc7a7f5e223cf060f2bf1341dfa0ae5/components/style/properties/longhand/text.mako.rs#L15 , and use the <%helpers:longhand ... one unconditionally.
• Try compiling it. Layout will probably fail in push_fragment_to_line() in components/layout/inline.rs, because text_overflow::clip/text_overflow::ellipsis are now text_overflow::Side::Clip/text_overflow::Side::Ellipsis, and what used to be an enum is now a struct. Match on fragment.style().get_text().text_overflow.first instead, and use the new names for the variants.
• Make it possible to pass arbitrary strings to transform_into_ellipsis() as an argument. In push_fragment_to_line() add an extra match arm that is similar to the ellipsis one, but pushes a string. In the ellipsis one, pass "…".
• Test that the lorem ipsum example on MDN works

Relevant layout code: components/layout/inline.rs , components/layout/fragment.rs

Tests: None, but we will add some when this is done.

[Manishearth]

@notriddle @jdm does this look like E-easy material? I tried to break it into steps to make it easier to work on.

[jdm]

I would call it E-less easy.

[cynicaldevil]

I'll take a look at this one.

[Manishearth]

Cool! Let us know if you need help!

[Manishearth]

@cynicaldevil Any updates? I got a ping from you yesterday but you weren't around when I came back online. Feel free to ask any questions here.

[cynicaldevil]

@Manishearth I get a Could not find Side in style::computed_values::text_overflow error for both match arms. I don't know what's wrong, because the text-overflow module does contain a Side enum, according to mako.rs.

[Manishearth]

style::properties::text_overflow contains it. The computed value module does not.

[cynicaldevil]

Negative on that as well.

[Manishearth]

Could you put your code somewhere and pastebin the error?

[Manishearth]

Also, did you remove the conditional as was mentioned in the first point above?

[Manishearth]

https://github.com/Manishearth/servo/blob/ea765688cbc7a7f5e223cf060f2bf1341dfa0ae5/components/style/properties/longhand/text.mako.rs#L15 should not be there anymore, you need to change it to unconditionally use the helpers:longhand version

[cynicaldevil]

Yes, I'd removed the conditional statement. I'll push my changes onto my fork now.

[cynicaldevil]

https://github.com/cynicaldevil/servo/commit/74c89e61eece0df4d19ba76996de4ce0238e08bc
Error:

error[E0433]: failed to resolve. Could not find `text_overflow` in `style::properties`
   --> /home/nikhil/workspace/mozilla/servo/components/layout/inline.rs:692:13
    |
692 |             properties::text_overflow::Side::Clip => {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Could not find `text_overflow` in `style::properties`

error[E0433]: failed to resolve. Could not find `text_overflow` in `style::properties`
   --> /home/nikhil/workspace/mozilla/servo/components/layout/inline.rs:693:13
    |
693 |             properties::text_overflow::Side::Ellipsis => {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Could not find `text_overflow` in `style::properties`

[Manishearth]

It's properties::longhands::text_overflow, sorry.

[cynicaldevil]

Copied from IRC:
I tried to do something like this; but the ref string is forcing a borrow of fragment which is not being returned. I don't get why this is happening.

        let passed_str: Option<&str> = match fragment.style().get_text().text_overflow.first {
            longhands::text_overflow::Side::Clip => None,
            longhands::text_overflow::Side::Ellipsis => {
                need_ellipsis = fragment.margin_box_inline_size() > available_inline_size;
                Some("...")
            }
            longhands::text_overflow::Side::String(ref string) => {
                need_ellipsis = fragment.margin_box_inline_size() > available_inline_size;
                Some(string)
            }
        };

[jdm]

What's the error message that you get?

[cynicaldevil]

error[E0505]: cannot move out of `fragment` because it is borrowed
    |
691 |         let passed_str: Option<&str> = match fragment.style().get_text().text_overflow.first {
    |                                              -------- borrow of `fragment` occurs here
...
704 |             self.push_fragment_to_line_ignoring_text_overflow(fragment, layout_context);
    |                                                               ^^^^^^^^ move out of `fragment` occurs here

[jdm]

Can you scope passed_str so that it is dropped before line 704?

[cynicaldevil]

I can't do that, because I have to pass it to a function call which occurs after line 704.

[jdm]

In that case it's not possible to hold on to a pointer derived from fragment after it gets moved.

[Manishearth]

There's no need to try and borrow it in this case, just store an Option<String>. The function it gets passed to calls .to_owned() on it anyway.