stylo: Need to implement inlinization of ruby kids

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
11 days ago

People

(Reporter: bz, Assigned: xidorn)

Tracking

(Blocks: 3 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
(Reporter)

Description

3 months ago
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.
(Reporter)

Comment 1

3 months ago
Fixing this will likely fix bug 1323716 too.
Blocks: 1323716
(Reporter)

Comment 2

3 months ago
Servo issue: https://github.com/servo/servo/issues/16825
Blocks: 1324348
Priority: -- → P2
(Assignee)

Updated

a month ago
Blocks: 1323697
Priority: P2 → P1
Blocks: 1342331
(Assignee)

Updated

22 days ago
Assignee: nobody → xidorn+moz
(Assignee)

Updated

20 days ago
Depends on: 1378287
(Assignee)

Updated

20 days ago
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

20 days ago
Patches here are on top of the patch in bug 1378287. Without that patch, crashtest 1233135-{1,2}.html would still fail.
(Assignee)

Comment 12

20 days ago
Hmmm, some unexpected PASS and one unexpected FAIL... I would need to investigate that failure tomorrow.
(Assignee)

Updated

20 days ago
Attachment #8883491 - Flags: review?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Attachment #8883491 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

20 days ago
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 hidden (mozreview-request)

Comment 24

20 days ago
mozreview-review
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 25

20 days ago
mozreview-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 26

20 days ago
mozreview-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 27

20 days ago
mozreview-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 28

20 days ago
mozreview-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 29

20 days ago
mozreview-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.
(Assignee)

Comment 31

19 days ago
mozreview-review-reply
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 32

19 days ago
mozreview-review
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 33

19 days ago
mozreview-review
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+
(Assignee)

Comment 34

19 days ago
mozreview-review-reply
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 35

19 days ago
mozreview-review-reply
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 36

19 days ago
mozreview-review
Comment on attachment 8883496 [details]
Add style adjustments for ruby.

https://reviewboard.mozilla.org/r/154408/#review159814
Attachment #8883496 - Flags: review?(cam) → review+

Comment 37

19 days ago
mozreview-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`.
(Assignee)

Comment 38

19 days ago
mozreview-review-reply
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.
(Assignee)

Comment 39

19 days ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

19 days ago
Attachment #8883492 - Attachment is obsolete: true
(Assignee)

Comment 47

19 days ago
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+
(Assignee)

Updated

17 days ago
Blocks: 1379441
(Assignee)

Comment 48

12 days ago
Servo PR: servo/servo#17722
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

12 days ago
Attachment #8883490 - Attachment is obsolete: true
(Assignee)

Updated

12 days ago
Attachment #8883756 - Attachment is obsolete: true
(Assignee)

Updated

12 days ago
Attachment #8883493 - Attachment is obsolete: true
(Assignee)

Updated

12 days ago
Attachment #8883496 - Attachment is obsolete: true

Comment 52

12 days ago
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

Comment 53

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afd686d76aa6
https://hg.mozilla.org/mozilla-central/rev/35dbf37b06f0
https://hg.mozilla.org/mozilla-central/rev/9b84d45b2c26
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.