Closed Bug 1364274 Opened 7 years ago Closed 7 years ago

stylo: Need to implement inlinization of ruby kids

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

Bug 1364274 part 1 - Propagate style bits from Servo ComputedValues to ServoStyleContext.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364274 part 2 - Move HAS_TEXT_DECORATION_LINES bit computation back to ApplyStyleFixups.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364274 part 3 - Adjust test expectation.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Spec at https://drafts.csswg.org/css-ruby/#anon-gen-inlinize and Gecko impl in the ShouldSuppressLineBreak function and its callsite in nsStyleContext::ApplyStyleFixups

This is causing a bunch of the ruby failures on reftests.
Fixing this will likely fix bug 1323716 too.
Blocks: 1323716
Priority: -- → P2
Blocks: 1323697
Priority: P2 → P1
Assignee: nobody → xidorn+moz
Depends on: 1378287
Patches here are on top of the patch in bug 1378287. Without that patch, crashtest 1233135-{1,2}.html would still fail.
Hmmm, some unexpected PASS and one unexpected FAIL... I would need to investigate that failure tomorrow.
Attachment #8883491 - Flags: review?(cam)
Attachment #8883491 - Attachment is obsolete: true
Ruby is a feature which heavily relies on style adjustments, and we have a good test coverage to ensure they work as expected.

I expect patches in this bug to fix majority of ruby-related failures.
Comment on attachment 8883490 [details]
Rename HAS_TEXT_DECORATION_LINE to match gecko side name.

https://reviewboard.mozilla.org/r/154396/#review159776

::: servo/components/style/properties/computed_value_flags.rs:12
(Diff revision 1)
>          /// Whether the style or any of the ancestors has a text-decoration
>          /// property that should get propagated to descendants.
>          ///
>          /// text-decoration is a reset property, but gets propagated in the
>          /// frame/box tree.

While here, please do s/text-decoration/text-decoration-line/ on the comment.
Attachment #8883490 - Flags: review?(cam) → review+
Comment on attachment 8883756 [details]
Inherit computed value flags when inheriting computed values.

https://reviewboard.mozilla.org/r/154690/#review159778

Can you add a comment somewhere saying that all flags are currently inherited, and that if we ever want to add some flags that aren't inherited by default, we might want to add a propagate() function?
Attachment #8883756 - Flags: review?(cam) → review+
Comment on attachment 8883492 [details]
Make layout_parent_style be a member of StyleAdjuster.

https://reviewboard.mozilla.org/r/154400/#review159780
Attachment #8883492 - Flags: review?(cam) → review+
Comment on attachment 8883493 [details]
Move HAS_TEXT_DECORATION_LINES setting into StyleAdjuster.

https://reviewboard.mozilla.org/r/154402/#review159782

::: servo/components/style/style_adjuster.rs:5
(Diff revision 2)
>  //! A struct to encapsulate all the style fixups a computed style needs in order
>  //! for it to adhere to the CSS spec.

Since StyleAdjuster is now being used to set the flags also, can you update the comment to mention that?

::: servo/components/style/style_adjuster.rs:319
(Diff revision 2)
>      }
>  
> +    /// Set the HAS_TEXT_DECORATION_LINES flag based on parent style.
> +    fn adjust_for_text_decoration_lines(&mut self) {
> +        use properties::computed_value_flags::HAS_TEXT_DECORATION_LINES;
> +        if self.layout_parent_style.flags.contains(HAS_TEXT_DECORATION_LINES) ||

It's a bit funny that we do the inheritance of the HAS_TEXT_DECORATION_LINES flag here and on the StyleBuilder.  Is there any way we can avoid one of them?

::: servo/components/style/style_adjuster.rs:353
(Diff revision 2)
>              self.adjust_for_alignment();
>          }
>          self.adjust_for_border_width();
>          self.adjust_for_outline();
>          self.adjust_for_writing_mode();
> +        self.adjust_for_text_decoration_lines();

Since this isn't adjusting any styles, I wonder if we should name this function slightly differently.  What about "set_flags_for_text_decoration_lines"?

Also, maybe add a blank line before it and a comment like "// Flag updating." here and "// Style adjustments" at the top of the function?  And mention in the comment above this function that it also does the flag setting, and maybe point to GeckoStyleContext::SetStyleBits.
Attachment #8883493 - Flags: review?(cam) → review+
Comment on attachment 8883494 [details]
Bug 1364274 part 1 - Propagate style bits from Servo ComputedValues to ServoStyleContext.

https://reviewboard.mozilla.org/r/154404/#review159784

I guess this FFI call can go away once we start constructing the ServoStyleContexts from Rust.
Attachment #8883494 - Flags: review?(cam) → review+
Comment on attachment 8883495 [details]
Bug 1364274 part 2 - Move HAS_TEXT_DECORATION_LINES bit computation back to ApplyStyleFixups.

https://reviewboard.mozilla.org/r/154406/#review159786
Attachment #8883495 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #27)
> Since this isn't adjusting any styles, I wonder if we should name this
> function slightly differently.  What about
> "set_flags_for_text_decoration_lines"?
> 
> Also, maybe add a blank line before it and a comment like "// Flag
> updating." here and "// Style adjustments" at the top of the function?  And
> mention in the comment above this function that it also does the flag
> setting, and maybe point to GeckoStyleContext::SetStyleBits.

Feel free to ignore these comments; I see in the ruby adjustment patch we have a mix of flags setting and style adjustment happening in the one place, so it's probably not important to distinguish these two actions.
Comment on attachment 8883493 [details]
Move HAS_TEXT_DECORATION_LINES setting into StyleAdjuster.

https://reviewboard.mozilla.org/r/154402/#review159782

> It's a bit funny that we do the inheritance of the HAS_TEXT_DECORATION_LINES flag here and on the StyleBuilder.  Is there any way we can avoid one of them?

The two are different inheritance, no? Here is for element where we generate and inherit when necessary, and the one in StyleBuilder is for anonymous boxes which inherits directly.

For element, we should not inherit it directly, because we may get unnecessary flag from display:contents.

I'm thinking about whether we should also avoid inheriting this flag when we hit any out-of-flow. IIRC text decoration lines are not propagated into them.
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159790

::: servo/components/style/style_adjuster.rs:348
(Diff revision 3)
> +            display::ruby_base | display::ruby_text => true,
> +            // Ruby base container and text container are breakable.
> +            // Note that, when certain HTML tags, e.g. form controls, have ruby
> +            // level container display type, they could also escape from the
> +            // line break suppression flag while they shouldn't. However, it is
> +            // generally fine since themselves are non-breakable.

Nit: s/themselves/they themselves/

::: servo/components/style/style_adjuster.rs:352
(Diff revision 3)
> +            _ if parent_display.is_ruby_type() => true,
> +            _ => false,

Maybe just:

  _ => parent_display.is_ruby_type(),

unless you think that's less easy to understand.

::: servo/components/style/style_adjuster.rs:360
(Diff revision 3)
> +    /// * suppress border and padding for ruby level containers.
> +    /// * correct unicode-bidi,

Swap "." and ",".

::: servo/components/style/style_adjuster.rs:373
(Diff revision 3)
> +        let self_display = self.style.get_box().clone_display();
> +        // Check whether line break should be suppressed for this element.
> +        if self.should_suppress_linebreak() {
> +            self.style.flags.insert(SHOULD_SUPPRESS_LINEBREAK);
> +            // Inlinify the display type if allowed.
> +            if !flags.contains(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {

Gecko doesn't do this.  Why do we want to do it here?

::: servo/components/style/style_adjuster.rs:432
(Diff revision 3)
> +        #[cfg(feature = "gecko")]
> +        {
> +            self.adjust_for_ruby(default_computed_values, flags);
> +        }

To keep this in basically the same order as ApplyStyleFixups, should this go just below blockify_if_necessary?
Comment on attachment 8883497 [details]
Bug 1364274 part 3 - Adjust test expectation.

https://reviewboard.mozilla.org/r/154410/#review159794

Great!
Attachment #8883497 - Flags: review?(cam) → review+
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159790

> Gecko doesn't do this.  Why do we want to do it here?

Gecko does similar thing via checking pseudo tag explicitly: https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/layout/style/GeckoStyleContext.cpp#462-472

If we don't do this, there will be assertions complaining that we are touching display value that we should't be touching. See bug 1233135. This is also the reason why I need bug 1378287.
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159790

> Gecko does similar thing via checking pseudo tag explicitly: https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/layout/style/GeckoStyleContext.cpp#462-472
> 
> If we don't do this, there will be assertions complaining that we are touching display value that we should't be touching. See bug 1233135. This is also the reason why I need bug 1378287.

OK.  Can you please update the comment in properties.mako.rs for SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP to say that it also does this.
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159814
Attachment #8883496 - Flags: review?(cam) → review+
Comment on attachment 8883492 [details]
Make layout_parent_style be a member of StyleAdjuster.

https://reviewboard.mozilla.org/r/154400/#review159816

::: servo/ports/geckolib/glue.rs:1667
(Diff revision 2)
>      let style = if let Some(reference) = maybe_arc.as_ref() {
>          let mut style =
>              StyleBuilder::for_inheritance(reference,
>                                            &data.default_computed_values());
>          if for_text {
> -            StyleAdjuster::new(&mut style)
> +            StyleAdjuster::new(&mut style, reference)

This bit is not right, and it's why this wasn't done before.

`layout_parent_style` is the style of the closest ancestor without `display: contents`. This is passing down directly the inherited style, which can definitely have `display: contents`.
Comment on attachment 8883492 [details]
Make layout_parent_style be a member of StyleAdjuster.

https://reviewboard.mozilla.org/r/154400/#review159816

> This bit is not right, and it's why this wasn't done before.
> 
> `layout_parent_style` is the style of the closest ancestor without `display: contents`. This is passing down directly the inherited style, which can definitely have `display: contents`.

Oh, actually this commit is not necessary. I did this because I needed to inherit flag of text decoration lines in `adjust_for_text`. But later the code is changed to do so inside `StyleBuilder` instead, so this commit is no longer relevant. It can be safely remove from the patch set.
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159790

> To keep this in basically the same order as ApplyStyleFixups, should this go just below blockify_if_necessary?

It doesn't seem to me that it currently have the same order as ApplyStyleFixups at all... In ApplyStyleFixups, the only thing after ruby fixups is display fixup for different writing mode. Also it seems StyleAdjuster includes far more fixups than ApplyStyleFixups because many fixups which were in nsRuleNode are done in StyleAdjuster as well.

I don't think the order matters here, so I'd like to keep it as-is.
Attachment #8883492 - Attachment is obsolete: true
Comment on attachment 8883756 [details]
Inherit computed value flags when inheriting computed values.

You may want to have another look at the added comment.
Attachment #8883756 - Flags: review+ → review?(cam)
Attachment #8883756 - Flags: review?(cam) → review+
Blocks: 1379441
Attachment #8883490 - Attachment is obsolete: true
Attachment #8883756 - Attachment is obsolete: true
Attachment #8883493 - Attachment is obsolete: true
Attachment #8883496 - Attachment is obsolete: true
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afd686d76aa6
part 1 - Propagate style bits from Servo ComputedValues to ServoStyleContext. r=heycam
https://hg.mozilla.org/integration/autoland/rev/35dbf37b06f0
part 2 - Move HAS_TEXT_DECORATION_LINES bit computation back to ApplyStyleFixups. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9b84d45b2c26
part 3 - Adjust test expectation. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: