Closed Bug 1098257 Opened 5 years ago Closed 5 years ago

Inlinize block-level boxes inside ruby

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: xidorn, Assigned: xidorn)

References

(Depends on 1 open bug, )

Details

Attachments

(2 files, 5 obsolete files)

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.
Flags: needinfo?(dbaron)
Summary: Inlinize block-level boxes and suppress line breaks inside ruby → Inlinize block-level boxes inside ruby
Assignee: nobody → quanxunzhen
Sorry for the needinfo request. I forgot to clear that field before I submitted the changes.
Flags: needinfo?(dbaron)
Attached patch patch (obsolete) — Splinter Review
Attachment #8522700 - Flags: review?(cam)
Attached patch patch (obsolete) — Splinter Review
The previous patch doesn't consider position and float, and rule tree cache.
Attachment #8522700 - Attachment is obsolete: true
Attachment #8522700 - Flags: review?(cam)
Attachment #8522897 - Flags: review?(cam)
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.
Attachment #8522897 - Flags: review?(cam)
(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.
(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.
Flags: needinfo?(fantasai.bugs)
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.
Flags: needinfo?(fantasai.bugs)
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8522897 - Attachment is obsolete: true
Attachment #8525630 - Flags: review?(cam)
(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?
(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 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"
Attachment #8525630 - Flags: review?(cam)
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
Summary: Inlinize block-level boxes inside ruby → Add test for inlinize block-level boxes inside ruby
Attached patch patch for tests (obsolete) — Splinter Review
Sorry for not finding that fact earlier...

Test for list-item should be added after bug 1105868 fixed.
Attachment #8525630 - Attachment is obsolete: true
Attachment #8529965 - Flags: review?(cam)
Blocks: css-ruby
No longer blocks: enable-css-ruby
The previous implementation is not complete...
Blocks: enable-css-ruby
No longer blocks: css-ruby
Summary: Add test for inlinize block-level boxes inside ruby → Inlinize block-level boxes inside ruby
Attached patch patchSplinter Review
Attachment #8530244 - Flags: review?(cam)
Attached patch patch for testsSplinter Review
Add one test for testing in-ruby block directly inside inline box.
Attachment #8529965 - Attachment is obsolete: true
Attachment #8529965 - Flags: review?(cam)
Attachment #8530246 - Flags: review?(cam)
Attachment #8530249 - Flags: review?(cam)
Attachment #8530249 - Attachment is obsolete: true
Attachment #8530249 - Flags: review?(cam)
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.
Attachment #8530244 - Flags: review?(cam) → review+
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?
Attachment #8530246 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/1472cc0c85b2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.