Inlinize block-level boxes inside ruby

RESOLVED FIXED in mozilla37

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Depends on: 1 bug)

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(dbaron)
Summary: Inlinize block-level boxes and suppress line breaks inside ruby → Inlinize block-level boxes inside ruby
(Assignee)

Updated

3 years ago
Assignee: nobody → quanxunzhen
(Assignee)

Comment 2

3 years ago
Sorry for the needinfo request. I forgot to clear that field before I submitted the changes.
Flags: needinfo?(dbaron)
(Assignee)

Comment 3

3 years ago
Created attachment 8522700 [details] [diff] [review]
patch
Attachment #8522700 - Flags: review?(cam)
(Assignee)

Comment 4

3 years ago
Created attachment 8522897 [details] [diff] [review]
patch

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)
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
(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)

Comment 8

3 years ago
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)
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
Created attachment 8525630 [details] [diff] [review]
patch
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?
(Assignee)

Comment 12

3 years ago
(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)
(Assignee)

Comment 14

3 years ago
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
(Assignee)

Updated

3 years ago
Summary: Inlinize block-level boxes inside ruby → Add test for inlinize block-level boxes inside ruby
(Assignee)

Comment 15

3 years ago
Created attachment 8529965 [details] [diff] [review]
patch for tests

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)
(Assignee)

Updated

3 years ago
Blocks: 256274
No longer blocks: 1039006
(Assignee)

Comment 16

3 years ago
The previous implementation is not complete...
Blocks: 1039006
No longer blocks: 256274
Summary: Add test for inlinize block-level boxes inside ruby → Inlinize block-level boxes inside ruby
(Assignee)

Comment 17

3 years ago
Created attachment 8530244 [details] [diff] [review]
patch
Attachment #8530244 - Flags: review?(cam)
(Assignee)

Comment 18

3 years ago
Created attachment 8530246 [details] [diff] [review]
patch for tests

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)
(Assignee)

Comment 19

3 years ago
Created attachment 8530249 [details] [diff] [review]
patch 2 - solve dependency in fixup code
Attachment #8530249 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1369103
You need to log in before you can comment on or make changes to this bug.