1098257 - Inlinize block-level boxes inside ruby

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Per CSS Ruby spec, any in-flow block-level boxes directly contained by ruby boxes are forced to be inline-level boxes, and their display value are computed accordingly.

Specifically, we should do the following display value conversion:
block -> inline-block
list-item -> inline-list-item
table -> inline-table
flex -> inline-flex
grid -> inline-grid

AFAIK, we don't support inline-list-item, but it won't be a big problem since we are using nsBlockFrame for processing list-item. Simply mapping inline-list-item to inline-block may work.

In addition, the spec proposed that forced line breaks inside ruby annotations shall be suppressed.

I feel that the line breaks should also be suppressed in ruby bases, at least currently, since my current implementation in bug 1052924 cannot properly handle line breaks inside ruby base. We may fix it later.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Inlinze block-level boxes: http://dev.w3.org/csswg/css-ruby/#anon-gen-inlinize
Suppress line breaks: http://dev.w3.org/csswg/css-ruby/#anon-gen-unbreak

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Sorry for the needinfo request. I forgot to clear that field before I submitted the changes.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

The previous patch doesn't consider position and float, and rule tree cache.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8522897 [details] [diff] [review]
patch

Review of attachment 8522897 [details] [diff] [review]:
-----------------------------------------------------------------

The spec says "This computation occurs after any intermediary anonymous-box fixup (such as that required by internal table elements)."  Can you explain how this does/doesn't affect the changes in this patch?

::: layout/style/nsRuleNode.cpp
@@ +4882,5 @@
>  
> +static uint8_t
> +InlinizeBlockLevelDisplay(uint8_t aDisplay)
> +{
> +    switch (aDisplay) {

Too much indent.

@@ +5627,5 @@
>  
>    }
>  
> +  // Inlinize in-flow block-level boxes inside ruby.
> +  display->mInlinizeChild = false;

Can you explain why we need to store mInlinizeChild?  It looks like this handles cases like:

  <ruby><rb><span><div>x</div></span></rb></ruby>

by computing div's display to inline-block.  Is that the right behaviour?  I'm interpreting "directly contained by" in the spec to mean it's looking just at the parent, not an ancestor.

@@ +5631,5 @@
> +  display->mInlinizeChild = false;
> +  if (display->IsRubyDisplayType()) {
> +    display->mInlinizeChild = true;
> +  } else if (parentDisplay->mInlinizeChild &&
> +             display->mPosition == NS_STYLE_POSITION_STATIC &&

Are you sure this mPosition check is right?  The spec says to inlinize in-flow block-level boxes, which according to the definition of "in-flow" at http://dev.w3.org/csswg/css-position/#pos-sch would include any position except absolute.

You can use nsStyleDisplay::IsOutOfFlowStyle.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8522897 [details] [diff] [review]
> patch
> 
> Review of attachment 8522897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The spec says "This computation occurs after any intermediary anonymous-box
> fixup (such as that required by internal table elements)."  Can you explain
> how this does/doesn't affect the changes in this patch?

It doesn't affect the changes in this patch.

Anonymous ruby boxes are only generated directly inside or outside some existing ruby boxes, which means if some content is in an anonymous ruby box, it must have already been in some solid ruby box, though misparented. The only exception is whitespace, which may have anonymous ruby parent even when it is not in any solid ruby box. But whitespaces are never be block elements, so they are not affected.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8522897 [details] [diff] [review]
> patch
> 
> Review of attachment 8522897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +5627,5 @@
> >  
> >    }
> >  
> > +  // Inlinize in-flow block-level boxes inside ruby.
> > +  display->mInlinizeChild = false;
> 
> Can you explain why we need to store mInlinizeChild?  It looks like this
> handles cases like:
> 
>   <ruby><rb><span><div>x</div></span></rb></ruby>
> 
> by computing div's display to inline-block.  Is that the right behaviour? 
> I'm interpreting "directly contained by" in the spec to mean it's looking
> just at the parent, not an ancestor.

In my understanding, the "contain" here means block-level containing, so that in your example, the div is directly contained by the rb on block level, hence the inline-level elements inherit the inlinizing behavior. Anyway, there is at least a problem in my patch that block-inside box doesn't stop the inlinizing. I'll fix it.

fantasai, could you explain this item a bit more? I feel block-level containing makes more sense here.

Comment 8

[fantasai]

The goal is to avoid block-in-inline splits that split ruby. So pretend the ruby elements are all inline. If any box would have caused a block-in-inline split of one of the ruby elements, then that box must be turned into an atomic inline box.

<ruby><rb><span><div><p></p></div></span></rb><rt><table>...</table></rt></ruby>

The <div> and <table> here get turned into an inline-block/inline-table.

Does that make sense? I'm happy to consider other options for solving the problem.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to fantasai from comment #8)
> The goal is to avoid block-in-inline splits that split ruby. So pretend the
> ruby elements are all inline. If any box would have caused a block-in-inline
> split of one of the ruby elements, then that box must be turned into an
> atomic inline box.
> 
> <ruby><rb><span><div><p></p></div></span></rb><rt><table>...</table></rt></
> ruby>
> 
> The <div> and <table> here get turned into an inline-block/inline-table.
> 
> Does that make sense? I'm happy to consider other options for solving the
> problem.

Yes, it seems that my understanding is right. Given that some people may realize the spec in a different way, I guess it might worth to make the item more clear that inline elements inherit the behavior.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #6)
> (In reply to Cameron McCormack (:heycam) from comment #5)
> > The spec says "This computation occurs after any intermediary anonymous-box
> > fixup (such as that required by internal table elements)."  Can you explain
> > how this does/doesn't affect the changes in this patch?
> 
> It doesn't affect the changes in this patch.
> 
> Anonymous ruby boxes are only generated directly inside or outside some
> existing ruby boxes, which means if some content is in an anonymous ruby
> box, it must have already been in some solid ruby box, though misparented.
> The only exception is whitespace, which may have anonymous ruby parent even
> when it is not in any solid ruby box. But whitespaces are never be block
> elements, so they are not affected.

Ah, I think I understand the spec quote now.  It mentions tables explicitly because if you have say a display:table-cell element in a ruby context, then it needs to have its containing anonymous display:table-row and display:table boxes generated first, so that the anonymous display:table box can be converted to display:inline-table.

Can you add some tests to the patch, and verify that this works?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #11)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #6)
> > (In reply to Cameron McCormack (:heycam) from comment #5)
> > > The spec says "This computation occurs after any intermediary anonymous-box
> > > fixup (such as that required by internal table elements)."  Can you explain
> > > how this does/doesn't affect the changes in this patch?
> > 
> > It doesn't affect the changes in this patch.
> > 
> > Anonymous ruby boxes are only generated directly inside or outside some
> > existing ruby boxes, which means if some content is in an anonymous ruby
> > box, it must have already been in some solid ruby box, though misparented.
> > The only exception is whitespace, which may have anonymous ruby parent even
> > when it is not in any solid ruby box. But whitespaces are never be block
> > elements, so they are not affected.
> 
> Ah, I think I understand the spec quote now.  It mentions tables explicitly
> because if you have say a display:table-cell element in a ruby context, then
> it needs to have its containing anonymous display:table-row and
> display:table boxes generated first, so that the anonymous display:table box
> can be converted to display:inline-table.

That's what I was concerned in bug 1088489. But now I think it should work, as the frame constructor seems to correctly create a pseudo inline-table instead of table when it is in ruby: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9900

> Can you add some tests to the patch, and verify that this works?

Yes, sure. I'll write some tests for this bug.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8525630 [details] [diff] [review]
patch

Review of attachment 8525630 [details] [diff] [review]:
-----------------------------------------------------------------

You need to compare mInlinizeChild in nsStyleDisplay::CalcDifference and return nsChangeHint_NeutralChange if it's different.

If a field on a style struct does not need any change handling itself (like mInlinizeChild) you must return nsChangeHint_NeutralChange so that two style structs that differ only in that field aren't considered equal (the restyle process can avoid doing some work if it discovers equal style structs).  Probably the only exception to this is a field on a style struct that just caches something that would get recomputed again to the same value anyway.

Please add some tests too.

::: layout/style/nsStyleStruct.h
@@ +2023,5 @@
>    bool mBreakBefore;    // [reset]
>    bool mBreakAfter;     // [reset]
> +  bool mInlinizeChild;          // Whether block-level children of the children
> +                                // should be inlinized. It is reset to false in
> +                                // any block-level node, and inhereted from any

"inherited"

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

It seems that this has been implemented before here: http://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#563

The only left problems are:
1. Add tests for them
2. Handle list-item

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch for tests

Sorry for not finding that fact earlier...

Test for list-item should be added after bug 1105868 fixed.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

The previous implementation is not complete...

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch for tests

Add one test for testing in-ruby block directly inside inline box.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - solve dependency in fixup code

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8530244 [details] [diff] [review]
patch

Review of attachment 8530244 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this comment addressed.

::: layout/style/nsStyleContext.cpp
@@ +564,5 @@
> +    if (!disp->IsOutOfFlowStyle() &&
> +        ((containerDisp->mDisplay == NS_STYLE_DISPLAY_INLINE &&
> +          containerContext->IsDirectlyInsideRuby()) ||
> +         containerDisp->IsRubyDisplayType())) {
> +      mBits |= NS_STYLE_IS_DIRECTLY_INSIDE_RUBY;

For this new bit, you will need to add a case to ElementRestyler::ComputeRestyleResultFromNewContext in RestyleManager.cpp so that we return eRestyleResult_Continue just like we do with NS_STYLE_HAS_TEXT_DECORATION_LINES and NS_STYLE_HAS_PSEUDO_ELEMENT_DATA.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8530246 [details] [diff] [review]
patch for tests

Review of attachment 8530246 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/css-ruby/inlinize-blocks-1-ref.html
@@ +2,5 @@
> +<html lang="en">
> +<head>
> +  <meta charset="UTF-8">
> +  <title>Bug 1098257 - Inlinize block-level boxes inside ruby</title>
> +  <link rel="stylesheet" href="common.css">

There's no common.css in this directory.  Remove this <link> from all the files?

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9d773f8434ec

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f291ce1eacd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1472cc0c85b2

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1472cc0c85b2