Open Bug 256274 (css-ruby) Opened 20 years ago Updated 2 years ago

[META] Implement CSS ruby module

Categories

(Core :: Layout: Ruby, enhancement)

enhancement

Tracking

()

People

(Reporter: annevk, Unassigned)

References

(Depends on 17 open bugs, Blocks 2 open bugs, )

Details

(4 keywords)

Attachments

(10 files, 32 obsolete files)

37.84 KB, image/gif
Details
3.71 KB, image/png
Details
10.10 KB, text/html
Details
9.19 KB, image/png
Details
17.12 KB, image/png
Details
11.23 KB, image/jpeg
Details
422 bytes, text/html
Details
32.38 KB, patch
Details | Diff | Splinter Review
70.03 KB, patch
Details | Diff | Splinter Review
65.05 KB, patch
Details | Diff | Splinter Review
When a CSS specification is in CR, UAs are encouraged to implement it. A simple
test case can be found here:

 <http://hixie.ch/tests/adhoc/css/box/ruby/001.html>
Keywords: testcase
How is this different from Bug 33339 (XHTML <ruby> support)?
<http://bugzilla.mozilla.org/show_bug.cgi?id=33339>
please ignore comment 1
Target Milestone: --- → Future
The test case works for me on Minefield 3.0a7pre
It does? What does it look like? There should be five lines: first the line "There should be four lines below", and then four more, numbered 1-4 in order.
Attached image Results of testcase
This is what I see
As far as I can tell, this is roughly what the testcase should look like.
I hope this will be fixed on Gecko2.0 (or Gecko1.9.x if possible). For that I'm thinking the implementation. However, the current spec (http://www.w3.org/TR/2003/CR-css3-ruby-20030514/) is too irresponsible. We need to think what is good for us.

The spec doesn't think about:

1. the ruby-base and/or ruby-text have non-single line content case.

I think they should be create new block context in them. And the ruby-base and ruby-text(the position is 'after') box's baseline should be the first line's baseline. The ruby-text(the position is 'before') box's should be the last line's.

2. the ruby-(base|text)-container has one or more non-ruby-(base|text) children case.

I think we should create anonymous ruby-base box to the child of the ruby-(base|text)-container.

3. the ruby has one or more non-ruby-(base|text)-container children case.

I think we should create anonymous ruby-*base*-container.

4. the ruby-base, ruby-text, ruby-base-container, ruby-text-container doesn't have ruby box for the parent case.

I think we should create anonymous ruby box and anonymous-*-container if needed.

5. Ruby box and line stacking

I think the spec should be inverted. I.e., the ruby-text box should impact to the line-height. Because IE of Windows (That is only implementation of XHTML ruby elements) does so. If we prefer the current spec, we have the incompatibility with IE. It's too bad for web designers.

6. the ruby for bopomofo (section 4.1)

I think we *cannot* implement this correctly until we implement the vertical text rendering. And that should not be auto-detected layout. That should be specified a new property, e.g., 'ruby-style'.

7. section 4.1

> 4.1 Ruby positioning: the 'ruby-position' property

> Applies to:  	the parent of elements with display: ruby-text.

I think that this spec is wrong. This property should apply to both ruby-text and ruby-text-container. Because we should not assume that the element for ruby-text-container is in all XML documents. (I think the spec depends on XHTML ruby spec, that is too bad.)

> If two rtc elements are set with the same ruby-position value, (for example both 'before'), the relative position of the two elements is undefined. This setting should not be used.

This paragraph may be wrong. A ruby box should be able to have two or more 'before' (and also 'after') containers.

8. We should add 'ruby-text-transform' property.

In Japanese typography, a small Kana in ruby text may be displayed as a normal Kana. (e.g., "ぎゅうにゅう" -> "ぎゆうにゆう") This helps the readability of small Japanese text. However, the ruby text will be important the accessibility of Japanese text. Then, the normal Kana hardcoded documents have the accessibility problem. Therefore, the display should be able to be changed by CSS like text-transform.
I start to work on this.
Assignee: nobody → masayuki
Target Milestone: Future → mozilla2.0
I began work on this, applying Nakano's diffs from the 2008-05-30 work, and continuing with the eight points listed in comment #7 above.

CSS keywords (and data structures) related to ruby display types applied to code in layout/style/, layout/base/, and layout/dom/. CSS properties for ruby-position and ruby-span disabled for the moment.

In the frame classes the current plan is to add a "nsRubyFrame" and "nsRubyRowFrame" class (for ruby-base-container and ruby-text-container display type elements), and use generic block frames or inline block frames for the contained ruby-base and ruby-text display type elements.
-- Generic CSS properties that shouldn't be valid for ruby-base and ruby-text display type elements --

The specs aren't clear so we'll have to decide for ourselves which generic CSS properties are valid and invalid for ruby-base and ruby-text display type elements. I'll make a first shot. Comments requested.

Valid:
  All properties that could be applied to a <span style='display: inline'>text</span> element are valid, except those noted next in the invalid list.

Invalid:
  CSS2: bottom, clear, clip, float!=none, left, letter-spacing (clashes with ruby-align), position=(absolute|fixed), right, text-align (clashes with ruby-align), text-indent, width, word-spacing (clashes with ruby-align)
  CSS3: any box-layout, aural media, or paged-media properties. 

(Note: the new CSS properties for rubies are ruby-align, ruby-overhang, and ruby-position.)
letter-spacing and word-spacing should apply; they don't conflict with ruby-align any more than they conflict with text-align in normal paragraph text. (Also width, height, text-align, and text-indent don't apply to non-replaced inline elements anyway.)

As a general comment, if you see any problems with the spec, post about them to www-style. I'll make sure they get addressed before the next publication.
p.s. Please put [css3-ruby] in the subject line of any such posts so they're easy to find.
Okay, width, height, text-align, and text-indent are all N/A.

Thanks for showing me letter-spacing and word-spacing are not invalid because of ruby-align in general. I realize now it is important to maintain letter-spacing to be equal, or maybe larger than, an inherited or specified value. All cases except ruby-align=(distribute-letter|distribute-space) should have the exact value; those two will expand spacing as necessary.

Re-doing the list:

Valid:
  All properties that could be applied to a non-replaced inline element are valid, except those noted below in the invalid list.

Invalid:
  bottom, clear, clip, float!=none, left, position=(absolute|fixed), right, top.

Varying case: 
  letter-spacing may be increased beyond specified or inherited lengths in the case of ruby-align=(distribute-letter|distribute-space).
Kurogane-san:

Sorry for the delay.

The spacing of letter-spacing is "additional" space for each characters. I.e., we should care a character width is: (a character width) + (letter-spacing). The letter-spacing implementation has some problems now. See bug 299943. You don't need to worry the letter-spacing issue. That will be fixed by bug 299943.

And see
http://www.w3.org/TR/CSS21/visuren.html#dis-pos-flo
floated/positioned ruby related display values should be computed to block, I think.
I stalled on this more than a month ago, I should have been more honest and 'fessed up then.

Creating new frame (and related child frame) classes was not too hard but I found that despite trying several different structures frames would not be rendered as expected. Debugging showed the frame structures being created as desired but the reflow steps would not render them. I presume I fail to understand all the parts to make a frame class have the right state for the reflow code.

If another person was to work on this, and they want to make the 'nsRubyFrame' class from the ground up as suggested, it should only be someone who already has a handle on the reflow code.

If it was me making a second pass I would duplicate the table frame classes, and then trim all that down piece by piece, learning what troubles the reflow/rendering code at each step I break it.
A new, interesting W3C document relating only to Japanese text has come out: http://www.w3.org/TR/jlreq/. (See Section 3.3 for ruby.) It doesn't form part of the CSS Ruby recommendation but it is informative.
(In reply to comment #79)
> We cannot use any adhoc ways for this bug. We should fix bug 256274 for this.
> We should support the ruby layout *by* CSS.
> 
> Please don't comment of the adhoc ways. And also please don't comment of the
> demand.

OK, it seems that the proposed solution using CSS is stalled. So I have to ask: why don't you want to implement Ruby using the techniques from the MathML layout engine, as it was initially planned? Also, for MathML, it appears that CSS is too limited to provide a good rendering so I wonder whether it is also the case for Ruby? Several issues from the CSS Ruby spec are mentioned in bug 256274 comment 7, hence I don't know whether it is relevant to implement ruby in this way.
I think that the way isn't stalled, just nobody is working on this bug now. Kurogane-san, are you still working on this?
No Nakano-san, I'm not working on this. I confessed to giving up on this over a year ago (see comment #15). I really got into other things for my extension, like cross-platform builds.

If you didn't read comment #15 please do now. The only thing I would add is it would be best to beg a regular reflow programmer to implement a simple ruby frame, then we could modify it to meet the complete specs. The reflow expert should be helped to see it as a couple of text frames, and not be alarmed by all the details such as text-spacing etc., or the fact that the specs always show japanese or chinese characters. The languages that ruby are usually used for are irrelevant for the CSS.

(Take a peek at http://hg.mozilla.org/mozilla-central/log/tip/layout/generic/nsHTMLReflowState.cpp
to see who's actively developing the reflow stuff.) 

In other news, Webkit is about to be patched for Ruby soon. Gecko will be the last of the major players :(
Ugh, O.K. But unfortunately, I don't have enough time now.
Assignee: masayuki → nobody
How is ruby supposed to interact with line breaking?  Should breaking inside a 'display:ruby' element be forbidden, or do we need to handle line breaking in the middle of ruby?  (How hard this is depends significantly on the answer.)  Does the most correct behavior differ from what other browsers do?
Breaking within a simple ruby should be forbidden.

For complex ruby, it would be nice if they could break on the borders between 'columns', however I note IE does not behave that way.
(In reply to comment #22)
> Breaking within a simple ruby should be forbidden.
> 
> For complex ruby, it would be nice if they could break on the borders between
> 'columns', however I note IE does not behave that way.

So IE doesn't allow breaking at all inside of ruby?
No, it does not. If I put a long piece of text in, it causes the box to overflow the right margin.

This is reasonable- rubies are simply not meant to be used for long pieces of text like that.
(In reply to comment #22)
> Breaking within a simple ruby should be forbidden.

I agree because inside a simple ruby characters can be one word. So, wrapping the line might cause the strange rendering.

But I worry about <br> element's line breaking inside the ruby related boxes. Therefore, I suggested that the ruby related boxes should create a new block context like inline block.

> For complex ruby, it would be nice if they could break on the borders between
> 'columns', however I note IE does not behave that way.

I agree this too. But I have a doubt. Do we really need the complex ruby? In Japan, IE still has big share. However, most websites don't use ruby, it seems that the current ruby marking up is very complex and troublesome.

IE and WebKit don't support the complex ruby, it can be dropped from CSS3 ruby module.
(In reply to comment #25)
> IE and WebKit don't support the complex ruby, it can be dropped from CSS3 ruby
> module.

http://www.w3.org/TR/html5/text-level-semantics.html#the-ruby-element

Ah, HTML5 dropped the complex ruby. But it still supports the multi-column.
I think that we should support:

display: ruby (P1), display: ruby-base (P1), display: ruby-text(P1), ruby-position (P2), ruby-align (P1), ruby-overhang (P3).

And I think we don't need to support two or more ruby-text per ruby-base. It's not supported on IE and that has very many issues like I suggested. If web designers want to use that, they should be able to nest the ruby element:

<ruby>
  <ruby>WWW<rp> (</rp><rt style="ruby-position: before;">ワールドワイドウェブ</rt><rp>) </rp>
  <rp> (</rp><rt style="ruby-position: after;">World Wide Web</rt><rp>)</rp>
</ruby>

But I think such needs are rare.
I agree we should support the ruby, ruby-base, ruby-text CSS position types, and the ruby-position property (= "before" | "after"). 

If a reflow expert could go that far I think we lesser mortals could do the other properties (ruby-align and ruby-overhang).
(In reply to comment #28)
> If a reflow expert could go that far I think we lesser mortals could do the
> other properties (ruby-align and ruby-overhang).

I think they are not difficult.
Currently I use the complex version of Ruby (with CSS hack) with BBCode.  :\

For example :

<ruby>
  <rbc>
    <rb>涼宮</rb>
    <rb>ハルヒ</rb>
  </rbc>
  <rtc>
    <rp>(</rp>
    <rt>ずずみや</rt> 
    <rt class="v">-</rt>
    <rp>)</rp>
  </rtc>
  <rtc>
    <rp>(</rp>
    <rt>SUZUMIYA</rt>
    <rt>Haruhi</rt>
    <rp>)</rp>
  </rtc>
</ruby>
Attached patch css-ruby module patch v1 (obsolete) — Splinter Review
I'm trying to implement css ruby-module, and attach my progress.

*** CSS (properties and their values, based on http://www.w3.org/TR/css3-ruby/ (CR, 14 May 2003)):
Working:
display: ruby, ruby-base, ruby-text, ruby-base-container, ruby-text-container
ruby-position: before, after (after: but needs some more work for line positioning)
ruby-align: start, left, center, end, right
ruby-overhang: auto, start, end, none (however, does currently not include checks of adjacent text)
(not a property) line height adjustment: line automatically gets higher if there isn't enough space for the ruby

Not yet:
ruby-position: right (needs very small vertical layout for overall horizontal layout)
ruby-align: distribute_letter, distribute_space, auto (depends on the above)
ruby-span

*** Markup:
If we apply these css display-property for ruby tags, we can use XHTML/HTML5 ruby markup.
e.g: ruby {display: ruby}, rb {display: ruby-base}, rt {display: ruby-text; font-size: 50%}, ...
Working:
- XHTML
Simple Ruby: <ruby><rb>AAA</rb><rt>aaa</rt></ruby>
Complex Ruby, without @rbspan: <ruby><rbc><rb>AAA</rb><rb>BBB</rb></rbc><rtc><rt>aaa</rt><rt>bbb</rt></rtc></ruby>
- HTML5
Simple Ruby: <ruby>AAA<rt>aaa</rt></ruby>
Complex Ruby:  <ruby>AAA<rt>aaa</rt>BBB<rt>bbb</rt></ruby>
also of course <rp>

Not yet:
- Complex Ruby, with @rbspan

* Classes
RubyFrame, RubyContainerFrame, RubyRowFrame, and RubyCellFrame (taking ideas from table layout and some comments in this bug.)

*Problems
With complex ruby, ruby-overhang doesn't work well.


Regards, 
Hajime.
Attachment #458539 - Flags: review?
Attachment #458539 - Flags: review? → review?(roc)
Looks great! A few quick comments:

Method names isNeedOverhang, getStartOverhangWidth, etc, should start with an uppercase letter.

The frame construction logic in ReconstructRubyFrame should probably go in nsCSSFrameConstructor. Instead of constructing a list of child frames normally and then modifying that list, it's better to get the list of FrameConstructionItems and then process that list to construct the right frame tree directly. This is how we handle other situations where certain kinds of children need special treatment.

You probably need to override GetMinWidth and GetPrefWidth in some of these classes so that floats and table cells containing ruby markup get the correct default and minimum widths.

You should split the patch into at least two pieces: one that just adds all the style properties and values, and another one that implements the frame types, frame construction and layout.
A few quick comments from a skim:

1)  FCDATA_DESIRED_PARENT_TYPE_TO_BITS(eTypeBlock) == 0 by design.  No need to
    list it explicitly.
2)  FCDATA_IS_RUBY_PART seems to be unused.  are there future plans to use it?
3)  I _think_ adding another struct is safe, but it's the last one we can add. 
    We should have a PR_STATIC_ASSERT that NS_STYLE_INHERIT_MASK is big enough
    to have one bit per struct.
4)  nsIDOMCSS2Properties is kinda-frozen (until zwol unfreezes it).  Those
    properties should go in nsIDOMNSCSS2Properties.
5)  The ConstructFramesFromItemList calls in the three new Construct* functions
    make no sense.  mChildItems is always empty there, so they're just no-ops. 
    So how does the container/cell stuff work at all?
6)  I don't see any handling of dynamic DOM mutations here.  It's not written
    yet, right?

I didn't look at the reflow methods.
Great! Thank you for your work!

FYI: our coding style is defined here:
https://developer.mozilla.org/En/Developer_Guide/Coding_Style
Assignee: nobody → hajime.shiozawa
Many thanks for quickly comments.

* for Robert O'Callahan
>Method names isNeedOverhang, getStartOverhangWidth, etc, should start with an
>uppercase letter.
I have fixed it.

>The frame construction logic in ReconstructRubyFrame should probably go in
>nsCSSFrameConstructor. Instead of constructing a list of child frames normally
>and then modifying that list, it's better to get the list of
>FrameConstructionItems and then process that list to construct the right frame
>tree directly. This is how we handle other situations where certain kinds of
>children need special treatment.
I'm changing the frame construction logic to your proposal.
Can I get the list from aItem that is a argument of construct function?

>You probably need to override GetMinWidth and GetPrefWidth in some of these
>classes so that floats and table cells containing ruby markup get the correct
>default and minimum widths.
I understand.

>You should split the patch into at least two pieces: one that just adds all the
>style properties and values, and another one that implements the frame types,
>frame construction and layout.
I have split it into two in the new patch.


* for Boris Zbarsky
>1)  FCDATA_DESIRED_PARENT_TYPE_TO_BITS(eTypeBlock) == 0 by design.  No need to
>    list it explicitly.
>2)  FCDATA_IS_RUBY_PART seems to be unused.  are there future plans to use it?
There aren't future plan to use FCDATA_IS_RUBY_PART, so I will remove
them and will use FCDATA_IS_INLINE flag instead.

>3)  I _think_ adding another struct is safe, but it's the last one we can add.
>    We should have a PR_STATIC_ASSERT that NS_STYLE_INHERIT_MASK is big enough
>    to have one bit per struct.
Do you mean by 'another struct' as nsRuleDataRuby struct?
Do you think that nsRuleDataRuby struct isn't necessary?

>4)  nsIDOMCSS2Properties is kinda-frozen (until zwol unfreezes it).  Those
>    properties should go in nsIDOMNSCSS2Properties.
I understand. Should I use -moz- prefix?

>5)  The ConstructFramesFromItemList calls in the three new Construct* functions
>    make no sense.  mChildItems is always empty there, so they're just no-ops.
>    So how does the container/cell stuff work at all?
I think that mChildItems isn't empty and constructed child frames is
in childItems.
Is it wrong?

>6)  I don't see any handling of dynamic DOM mutations here.  It's not written
>    yet, right?
Yes, it isn't written yet. Which part of the code should I look at
first, if I want to do the implementation?


Regards,
Hajime.
> Can I get the list from aItem that is a argument of construct function?

No, but you should be able to do whatever processing you need in ConstructFramesFromItemList if your constructors call ProcessChildren.  See what it does with the CreateNeededTablePseudos call, for example.

> will use FCDATA_IS_INLINE flag instead

Is that correct?  Do blocks inside ruby need to cause splits like they do inside inlines?  

> Do you mean by 'another struct' as nsRuleDataRuby struct?

Yes.

> Do you think that nsRuleDataRuby struct isn't necessary?

I think adding it is fine; we just need to add that PR_STATIC_ASSERT as well.

> I understand. Should I use -moz- prefix?

No, I don't think so (assuming the implementation matches the spec and has no major outstanding bugs and whatnot).

> Is it wrong?

I think in this case it is, yes.  The only times when mChildItems are nonempty on entry into the constructor are if FCDATA_IS_INLINE is set (but that also has other side-effects, like I said) and when something explicitly filled it (e.g. the table pseudo code).

> Which part of the code should I look at first, if I want to do the
> implementation?

It's all in the frame constructor.  To handle inserts/appends you need to either create the right frames and add them to the right places or modify WipeContainingBlock to reconstruct enough of the frame tree that things are updated correctly.  For ContentRemoved, you need to make sure that all the relevant frames are removed.  I haven't read the ruby spec yet, so I can't comment with more specifics...
(In reply to comment #37)
> > I understand. Should I use -moz- prefix?
> 
> No, I don't think so (assuming the implementation matches the spec and has no
> major outstanding bugs and whatnot).

I think that -moz- is needed for the new properties and the new values because the spec is unstable.
The spec is in Candidate Recommendation, so prefixes are not needed according to the rules.

Have other browsers implemented http://www.w3.org/TR/css3-ruby/ without prefixes?
(In reply to comment #39)
> The spec is in Candidate Recommendation, so prefixes are not needed according
> to the rules.

See http://www.w3.org/Style/CSS/current-work
The module will be back to working draft.
(In reply to comment #37)

> > Can I get the list from aItem that is a argument of construct function?
> No, but you should be able to do whatever processing you need in
> ConstructFramesFromItemList if your constructors call ProcessChildren.  See
> what it does with the CreateNeededTablePseudos call, for example.

Do you mean that when ProcessChildren() called from ConstructRuby() is running, all descendant frames is being constructed?
I thought in this way about new ruby frame construction logic.

1. creating FrameConstructionItemList that is the same structure as the one created in ReconstructRubyFrame(),
   by using aItem.mContent. (e.g. CreateRubyFrameFCItemList())
2. constructing frames directly calling ConstructFrameFromItemList() from this list

Please point me out if there are any problems on the process above.


> > will use FCDATA_IS_INLINE flag instead
> Is that correct?  Do blocks inside ruby need to cause splits like they do
> inside inlines? 
I think that it is correct, seeing http://dev.w3.org/csswg/css3-ruby/#ruby-line-breaking.
RubyFrame, RubyRowFrame: inline (FCDATA_IS_INLINE)
RubyCellFrame: block (FCDATA_ALLOW_BLOCK_STYLES is ok?)


> > Do you think that nsRuleDataRuby struct isn't necessary?
> I think adding it is fine; we just need to add that PR_STATIC_ASSERT as well.
I can't understand which codes I should add PR_STATIC_ASSERT().
I found the code below in nsStyleStruct.cpp
71: // Make sure we have enough bits in NS_STYLE_INHERIT_MASK.
72: PR_STATIC_ASSERT((((1 << nsStyleStructID_Length) - 1) &
73:                   ~(NS_STYLE_INHERIT_MASK)) == 0);
Do you think it isn't enough?


Regards,
Hajime.
> Do you mean that when ProcessChildren() called from ConstructRuby() is running,
> all descendant frames is being constructed?

That's generally the job of ProcessChildren: constructing the frames for the kids.

> Please point me out if there are any problems on the process above.

Generally speaking that should be fine, though there are a lot of fiddly details to get right.  For example, you would need to duplicate a bunch of ProcessChildren, no?  I'd like us to share as much code as possible if we can.

I just skimmed through the display section of the ruby draft linked in comment 41 and it seems to be .... severely underdefined, to the point where I'm not sure which implementation strategy makes more sense because I'm not sure what the proper desired behavior is.  What should happen when something with display:ruby has block-display kids?  Generated content?  Out-of-flow kids?  (What do other UAs do?)

> I think that it is correct, seeing
> http://dev.w3.org/csswg/css3-ruby/#ruby-line-breaking.

The text there doesn't actually answer my question...  The question can be summed up as this.  If I have:

  <span>
    <span style="display: ruby">
      <span style="display: block">
      </span>
    </span>
  </span>

then does that outermost span need to have block-inside-inline splitting performed on it?  Does it need to have that happen if the innermost block isn't there?

> RubyCellFrame: block (FCDATA_ALLOW_BLOCK_STYLES is ok?)

I don't know.  Should first-line and first-letter apply to ruby cell frames?

> 71: // Make sure we have enough bits in NS_STYLE_INHERIT_MASK.

Aha.  That's what I was looking for, yes.  No need to do anything else.
(In reply to comment #39)
> The spec is in Candidate Recommendation, so prefixes are not needed according
> to the rules.
> 
> Have other browsers implemented http://www.w3.org/TR/css3-ruby/ without
> prefixes?
Internet Explorer supports ruby-align, ruby-overhang, and ruby-position AFAIK. Maybe more.
(In reply to comment #40)
> (In reply to comment #39)
> > The spec is in Candidate Recommendation, so prefixes are not needed according
> > to the rules.
> 
> See http://www.w3.org/Style/CSS/current-work
> The module will be back to working draft.

That doesn't mean it's not stable.  It could mean minor clarifications need to be made.  If what we do is compatible with other implementations that have the properties unprefixed, it's far better to support them unprefixed.
Attached patch css-ruby property patch v1 (obsolete) — Splinter Review
I attached a splitted patch for adding css-ruby property. (ruby-frame layout patch isn't done yet...)

In this patch there are no frame constructor for 'display:ruby', so program assert "Should have frame construction data now." when browsing a page include 'display:ruby'.
Also I'm not using -moz- prefix now, but if necessary I can add -moz- prefix easily.

Regards,
Hajime.
Comment on attachment 459744 [details] [diff] [review]
css-ruby property patch v1

A few brief comments, though I didn't read through the whole patch carefully:

 1) We have a bunch of tests that exercise values of all CSS properties we implement, but they require that you add the necessary information to layout/style/test/property_database.js .  You should add appropriate values there, and then make sure that the mochitests in layout/style/test pass.

 2) The nsGkAtomList changes should not be needed.

 3) Inherited and non-inherited properties need to be in different style structs.  (By "inherited", I mean that the definition of the property in the spec says "Inherited: yes", which controls what happens when there is no value specified for a property: http://www.w3.org/TR/CSS21/cascade.html#specified-value .)  According to the spec, ruby-span is not inherited, but ruby-overhang and ruby-align are.  (I'm not sure this actually makes sense, though.  I was expecting that none of them would be inherited.  Do you know what other browsers implement?  This may just be a mistake in the spec.)

 4) I don't think there's any need to add a new style struct.  I think the new properties should be added to nsCSSDisplay and nsStyleDisplay (for non-inherited properties) or nsStyleVisibility (for inherited properties).  We've actually been thinking about reducing the number of style structs we have.  This change would also make the patch a good bit simpler.
(In reply to comment #46)
>  3) Inherited and non-inherited properties need to be in different style
> structs.  (By "inherited", I mean that the definition of the property in the
> spec says "Inherited: yes", which controls what happens when there is no value
> specified for a property:
> http://www.w3.org/TR/CSS21/cascade.html#specified-value .)  According to the
> spec, ruby-span is not inherited, but ruby-overhang and ruby-align are.  (I'm
> not sure this actually makes sense, though.  I was expecting that none of them
> would be inherited.  Do you know what other browsers implement?  This may just
> be a mistake in the spec.)

Er, sorry, you don't implement ruby-span, so it seems like they should all go in nsCSSDisplay and nsStyleVisibility.  So there isn't a problem here, assuming that the spec is correct.
Actually, I think the spec seems ok; I suppose the idea is that it's possible to specify the properties that are inherited anywhere in the document tree and have them apply to all ruby within that subtree of the document.  That seems like it makes sense.
Attached patch css-ruby property patch v2 (obsolete) — Splinter Review
I attatched a new css-ruby property patch for deleting codes around nsStyleRuby.
Also, I added ruby-position, ruby-align and ruby-overhang to nsCSSDisplay and nsStyleVisibility.

But it did't pass the Mochitest with test_compute_data_with_start_struct.html.
* failed | initial and other values of ruby-position are different - didn't expect "", but got it.
* failed | initial and other values of ruby-position are different - didn't expect "", but got it
* failed | initial and other values of ruby-align are different - didn't expect "", but got it
* failed | initial and other values of ruby-overhang are different - didn't expect "", but got it
* failed | initial and other values of ruby-position are different - didn't expect "", but got it
* failed | initial and other values of ruby-align are different - didn't expect "", but got it
* failed | initial and other values of ruby-overhang are different - didn't expect "", but got it

I think that a problem is about nsRuleNode::ComputeVisibilityData().
Do you have any advice?

Regards,
Hajime.
Attachment #459744 - Attachment is obsolete: true
Roland Steiner, the guy who patched in Ruby to Webkit, let me know their current implementation fulfills the HTML5 Ruby spec, but none of the CSS attributes or 'display' types.

He sums up his position (and that of most WebKit folks apparently) in his mail at http://lists.w3.org/Archives/Public/www-international/2010JanMar/0143.html

Regards,

Akira
Attached patch ruby-frame layout patch v1 (obsolete) — Splinter Review
I attached the splitted patch for ruby-frame layout.

I changed the logic to construct frames aroud ruby.
1: creating RubyCellFrame item list (FrameConstructionItemList) for
RubyRowFrames.
2: constructing frames calling ConstructFrameFromItemList().
3: setting these frames to RubyRowFrames as children.

In HTML5 ruby base is a text node, so I have to create a wrapper ruby
cell frame for it. but not yet.

Also, I had a problem.
<ruby>
 <rtc><rt>a</rt><rt>b</rt></rtc>
 <rbc><rb>A</rb><rb>B</rb></rbc>
</ruby>
in this case,
ChildIterator's loop is called not only twice but four times inside ConstructRuby() like this, <rtc>, <rt>, <rt> and <rbc>.
The problem doesn't occur if using <xx> instead of <rt>.

I didn't make any changes to the Reflow code.

Regards,
Hajime.
Attachment #458539 - Attachment is obsolete: true
Attachment #458539 - Flags: review?(roc)
So I'm trying to understand this last patch.  It looks like given a nsRubyFrame we take all its kids which are BASE_CONTAINER or TEXT_CONTAINER and put all _their_ kids in baseList, beforeList, or afterList depending on their display?

And that we put BASE kids of the nsRubyFrame into baseList and TEXT kids of the ruby frame into beforeList or afterList?

So the effect is that all the descendants of the frame are flattened into three lists: before, base, and after?

Is that the expected behavior?  Is there a quick (or at least thorough) ruby primer I can read to understand what we're trying to do?
(In reply to comment #52)
> So I'm trying to understand this last patch.  It looks like given a nsRubyFrame
> we take all its kids which are BASE_CONTAINER or TEXT_CONTAINER and put all
> _their_ kids in baseList, beforeList, or afterList depending on their display?
> And that we put BASE kids of the nsRubyFrame into baseList and TEXT kids of the
> ruby frame into beforeList or afterList?
> So the effect is that all the descendants of the frame are flattened into three
> lists: before, base, and after?
Yes, it is correct.
And also for HTML5, we should put TextNode kids of the nsRubyFrame into baseList.

> Is that the expected behavior?  Is there a quick (or at least thorough) ruby
> primer I can read to understand what we're trying to do?
How about this?
"W3C i18n article: Ruby Markup and Styling"
http://www.w3.org/International/articles/ruby/


Regards,
Hajime.
> http://www.w3.org/International/articles/ruby/

Perfect, than you.

So if I read this and the CSS3 Ruby module correctly, I believe the following claims are true of markup that "makes sense":

1)  A given display:ruby box has at most one ruby-base-container child.  If it
    has none, then it has at most one ruby-base child.
2)  A given display:ruby box has at most two ruby-text-container children.  If
    it has none, then it has at most one ruby-text child.

Is that at all correct?  If not, then what's the actual situation.

Now nothing keeps someone from sticking all sorts of gunk other than the above in a display:ruby; the question is what we do with it...  In the past dbaron has been opposed to just suppressing content that's messed up like this, but maybe it's the right thing to do...  what does IE do with HTML simple ruby if more than one <rt> or more than one <rb> is given?  What does the webkit implementation of this stuff do?

In any case, I think that we do have to construct frames for the ruby-base-container and ruby-text-container.  Not doing that will lead to incorrect styles for their kids on dynamic restyle, I see nothing in the spec that prevents relative positioning of the containers, etc.  It seems conceptually simpler to just create frames for them.

I'd like us to decide what we want to do with the various "misplaced" content before digging into the details of the frame construction, since one will affect the other, obviously.
So there are three relevant specifications here:
http://www.w3.org/TR/ruby/
http://www.w3.org/TR/css3-ruby/
http://www.w3.org/TR/html5/text-level-semantics.html#the-ruby-element

My understanding is that the parts relating to the "container" elements are "complex ruby", and other browsers aren't supporting those parts.

If other browsers aren't supporting *-container display types, I'd think we should also not support them, especially if they seem too complex for authors to be likely to use.  In other words, I think I agree with Masayuki's comment 25.


Which parts of the ruby model do WebKit and IE implement?  And do they do it without prefixes?
> Which parts of the ruby model do WebKit and IE implement? And do they do it
> without prefixes?

Webkit doesn't implement any of CSS3 Ruby.  It just implements the HTML5 ruby bits from http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-ruby-element (e.g. their html.css has display:block for "ruby > rt").  Vendor prefixes are a non-issue, therefore, since there's no CSS involved.  They seem to directly create the ruby-related RenderObjects when they need to do ruby stuff.

The HTML5 spec doesn't seem to define behavior in the "not valid content" cases, by the way; I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=10244 on that.

I can't speak to what IE does.
IE8 behaves odd. See bug 33339 comment 100. I tested the testcases and some additional cases on IE8 and Chrome.

Then, IE8 separates ruby box for each rb-rt pairs.

E.g.,
<ruby>before rb <rb>here is rb</rb> after rb <rt>here is rt</rt></ruby>

This makes one ruby box.

<ruby>before rb <rb>here is rb</rb> after rb <rt>here is rt</rt> here is invalid base</ruby>

This makes two ruby box, one is for the "before rb <rb>here is rb</rb> after rb <rt>here is rt</rt>" and the other is for " here is invalid base".

This doesn't make sense. And IE8 ignores all style rules for rb element.

Google Chrome also behaves interesting.

Ruby box doesn't contain ruby text box and ruby base box may overflow from ruby box, e.g., when rb's font-size is larger than ruby's one. And ruby text box expands to all ruby base boxes before the rt node. And it must not overlap with ruby base boxes even if it overflows from ruby box.

I think that ruby box must contains ruby base boxes and ruby text boxes except positioned boxes. And ruby text boxes and ruby base boxes must not overlap.

And there is an interesting case:

<ruby>
  <span style="font-size: 2em;">base1</span>
  <rt>text1</rt>
  base2
  <rt>text2</rt>
</ruby>

Where is best position for second rt?

+-----------------------------------------------+
|+---------------------+                        |
||      text1          |                        |
|+---------------------++----------------------+|
|+---------------------+|         text2        ||
||                     |+----------------------+|   
||      base1          |+----------------------+|
||                     ||         base2        ||
|+---------------------++----------------------+|
+-----------------------------------------------+

or:

+-----------------------------------------------+
|+---------------------++----------------------+|
||      text1          ||         text2        ||
|+---------------------++----------------------+|
|+---------------------+                        |
||                     |                        |   
||      base1          |+----------------------+|
||                     ||         base2        ||
|+---------------------++----------------------+|
+-----------------------------------------------+

I think the second is simpler for implementers. However, the first may be preferred more than the second by authors.
FYI: both IE8 and Google Chrome renders as the first.
(In reply to comment #54)
> So if I read this and the CSS3 Ruby module correctly, I believe the following
> claims are true of markup that "makes sense":
> 1)  A given display:ruby box has at most one ruby-base-container child.  If it
>     has none, then it has at most one ruby-base child.
> 2)  A given display:ruby box has at most two ruby-text-container children.  If
>     it has none,then it has at most one ruby-text child .
> Is that at all correct?  If not, then what's the actual situation.
It is correct about css3-ruby module and XHTML ruby markup.

> Now nothing keeps someone from sticking all sorts of gunk other than the above
> in a display:ruby; the question is what we do with it...  In the past dbaron
> has been opposed to just suppressing content that's messed up like this, but
> maybe it's the right thing to do...  what does IE do with HTML simple ruby if
> more than one <rt> or more than one <rb> is given?  What does the webkit
> implementation of this stuff do?
I attached the captured image displaying invalid ruby markup on IE and Chrome.

ruby {border: solid 2px red;}
rb {border: solid 1px black;}
rt {border: solid 1px green;}
<ruby><rb>AAAA</rb><rt>a</rt><rb>BBBB</rb><rt>b</rt></ruby>

This markup is invalid on both XHTML and HTML5, but I think we should accept this case like complex ruby.
Because I think that there are invalid markup like this on the Web. (<ruby> has multiple <rb>) 

> I'd like us to decide what we want to do with the various "misplaced" content
> before digging into the details of the frame construction, since one will
> affect the other, obviously.
Shimoda-san shows "broken" markup at https://bugzilla.mozilla.org/show_bug.cgi?id=33339#c100


Nakasano-san, thank you for your information!


Regards,
Hajime.
My idea about the ruby frame construction is like this. (I changed the name of RubyRowFrame to RubyContainerFrame.)

** Simple Ruby 
<ruby><rb>base</rb><rt>text</rt></ruby>
or
<ruby>base<rt>text</rt></ruby>

Constructed Frame Tree
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[text]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]


** Complex Ruby
<ruby>
  <rtc><rt>text1</rt><rt>text2</rt></rtc>
  <rbc><rb>base1</rb><rt>base2</rt></rbc>
</ruby>
or
<ruby>
  base1 <rt>text1</rt>
  base2 <rt>text2</rt>
</ruby>

Constructed Frame Tree
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[text1]
 |   +--[RubyCell]---[text2]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base1]
     +--[RubyCell]---[base2]


** Complex Ruby on both sides.
<ruby>
  <rtc><rt>before1</rt><rt>before2</rt></rtc>
  <rbc><rb>base</rb><rb>base2</rb></rbc>
  <rtc><rt>after1</rt><rt>after2</rt></rtc>
</ruby>

Constructed Frame Tree
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[before1]
 |   +--[RubyCell]---[before2]
 +--[RubyContainer(base)]
 |   +--[RubyCell]---[base1]
 |   +--[RubyCell]---[base2]
 +--[RubyContainer(after)]
     +--[RubyCell]---[after1]
     +--[RubyCell]---[after2]



I think that even if there aren't ruby-*-container explicitly I could just wrap RubyCell using RubyContainer. (like PseudoNeededTable())
Also text node or inline box (e.g. <span>) that is the child of <ruby> can be wrapped with RubyCell, and these becomes the child of RubyContainer(base).

Also, I will show the construction from my ideas with the broken markup at https://bugzilla.mozilla.org/show_bug.cgi?id=33339#c100
1. <ruby>base<rb></rb><rt>rubytext</rt></ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[]

2. <ruby><rb></rb>base<rt></rt>base2</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |    +--[RubyCell]---[]
 +--[RubyContainer(base)]
     +--[RubyCell]---[]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]

3. <ruby>base<rb>base2</rb><rt></rt>base3</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]
     +--[RubyCell]---[base3]

4. <ruby>base<rb>base2</rb><rt>rubytext</rt>base3</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]
     +--[RubyCell]---[base3]

5. <ruby><rb>base</rb>base2<rt></rt>base3</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]
     +--[RubyCell]---[base3]

6. <ruby><rb>base</rb>base2<rt>rubytext</rt>base3</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]
     +--[RubyCell]---[base3]

8. <ruby><rb>base</rb><rt>rubytext</rt><rb>base2</rb><rt>rubytext2rubytext2rubytext2</rt></ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 |   +--[RubyCell]---[rubytext2rubytext2rubytext2]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base2]

10-1. <ruby>base<rb></rb><rt>rubytext</rt></ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[]

9.<ruby>base</ruby><ruby><rb>base2</rb></ruby>base3
+[Ruby]
| +--[RubyContainer(base)]
|     +--[RubyCell]---[base]
+[Ruby]
| +--[RubyContainer(base)]
|     +--[RubyCell]---[base2]
+[base3]

10-2. <ruby><rb></rb>base<rt></rt>rubytext</ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[]
 +--[RubyContainer(base)]
     +--[RubyCell]---[]
     +--[RubyCell]---[base]
     +--[RubyCell]---[rubytext]

10-3. <ruby><rb>base</rb><rt>rubytext</rt><rb>base</rb><rt>rubytext</rt></ruby>
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[rubytext]
 |   +--[RubyCell]---[rubytext]
 +--[RubyContainer(base)]
     +--[RubyCell]---[base]
     +--[RubyCell]---[base]

7.<ruby>base<rb><rt>rubytextrubytextrubytext</rt>base2</ruby>
I don't know how the parser makes the content tree in this case...


Regards,
Hajime.
> I think that even if there aren't ruby-*-container explicitly I could just wrap
> RubyCell using RubyContainer. (like PseudoNeededTable())
> Also text node or inline box (e.g. <span>) that is the child of <ruby> can be
> wrapped with RubyCell, and these becomes the child of RubyContainer(base).

That sounds great.  What if there are too many ruby-*-containers?
Shiozawa-san, how do you think the compatibility issue which I suggested at comment 57? I guess that the invalid mark up is possible scenario. If we prefer the current CSS3 ruby spec, we need to give up the compatibility with Chrome in such case.

> 7.<ruby>base<rb><rt>rubytextrubytextrubytext</rt>base2</ruby>
> I don't know how the parser makes the content tree in this case...

I think that <rb> element should be closed forcedly before <rt>.
(In reply to comment #61)
> > I think that even if there aren't ruby-*-container explicitly I could just wrap
> > RubyCell using RubyContainer. (like PseudoNeededTable())
> > Also text node or inline box (e.g. <span>) that is the child of <ruby> can be
> > wrapped with RubyCell, and these becomes the child of RubyContainer(base).
> 
> That sounds great.  What if there are too many ruby-*-containers?

I have two ideas when there are too many ruby-*-containeres.

<ruby>
  <rtc><rt>text1-1</rt><rt>text1-2</rt></rtc>
  <rbc><rb>base1-1</rb><rb>base1-2</rb></rbc>
  <rtc><rt>text2-1</rt><rt>text2-2</rt></rtc>
  <rbc><rb>base2-1</rb><rb>base2-2</rb></rbc>
  <rtc><rt>text3-1</rt><rt>text3-2</rt></rtc>
</ruby>

In this case,
1: Ignoring the second ruby-base-container and the third ruby-text-container.
2: Constructing frame like this (if <rtc> has ruby-position: after, put RubyCell into RubyContainer(after))
[Ruby]
 +--[RubyContainer(before)]
 |   +--[RubyCell]---[text1-1]
 |   +--[RubyCell]---[text1-2]
 |   +--[RubyCell]---[text3-1]
 |   +--[RubyCell]---[text3-2]
 +--[RubyContainer(base)]
 |   +--[RubyCell]---[base1-1]
 |   +--[RubyCell]---[base1-2]
 |   +--[RubyCell]---[base2-1]
 |   +--[RubyCell]---[base2-2]
 +--[RubyContainer(after)]
     +--[RubyCell]---[text2-1]
     +--[RubyCell]---[text2-2]

The latter would be a better implementation, but it would get more complicated...
(In reply to comment #62)
> Shiozawa-san, how do you think the compatibility issue which I suggested at
> comment 57? I guess that the invalid mark up is possible scenario. If we prefer
> the current CSS3 ruby spec, we need to give up the compatibility with Chrome in
> such case.

I think the second one would be better because the implementation and its behavior is simpler.

<p>xxxxxxx<ruby>
   <span style='font-size: 0.5em;'>base1</span>
   <rt>text1</rt>
   <span style='font-size: 1.0em;'>base2</span>
   <rt>text2</rt>
   <span style='font-size: 1.5em;'>base3</span>
   <rt>text3</rt>
</ruby>xxxxx</p>

I attached the captured image of the markup above.
I think that the visualization of IE and Chrome would be strange.
When I change the font size larger, the ruby-text moves up.
But when I change the font size smaller, the ruby-text doesn't move.

I think that it would be better to apply the second and give up the compatibility.
(In reply to comment #64)
> Created attachment 461156 [details]

> I attached the captured image of the markup above.
> I think that the visualization of IE and Chrome would be strange.
> When I change the font size larger, the ruby-text moves up.
> But when I change the font size smaller, the ruby-text doesn't move.
> 
> I think that it would be better to apply the second and give up the
> compatibility.

I think we might want to ask the Japanese Layout Taskforce (http://www.w3.org/2007/02/japanese-layout/) about what they think.
> ** Complex Ruby
> <ruby>
>   <rtc><rt>text1</rt><rt>text2</rt></rtc>
>   <rbc><rb>base1</rb><rt>base2</rt></rbc>
> </ruby>

isn't it ?

<ruby>
  <rbc><rb>base1</rb><rt>base2</rt></rbc>   
  <rtc><rt>text1</rt><rt>text2</rt></rtc>
</ruby>

for that ↓

> Constructed Frame Tree
> [Ruby]
>  +--[RubyContainer(before)]
>  |   +--[RubyCell]---[text1]
>  |   +--[RubyCell]---[text2]
>  +--[RubyContainer(base)]
>      +--[RubyCell]---[base1]
>      +--[RubyCell]---[base2]
> 
> 
> ** Complex Ruby on both sides.
> <ruby>
>   <rtc><rt>before1</rt><rt>before2</rt></rtc>
>   <rbc><rb>base</rb><rb>base2</rb></rbc>
>   <rtc><rt>after1</rt><rt>after2</rt></rtc>
> </ruby>

And :

<ruby>
   <rbc><rb>base</rb><rb>base2</rb></rbc>
   <rtc><rt>before1</rt><rt>before2</rt></rtc>
   <rtc><rt>after1</rt><rt>after2</rt></rtc>
</ruby>

for that ? ↓

> Constructed Frame Tree
> [Ruby]
>  +--[RubyContainer(before)]
>  |   +--[RubyCell]---[before1]
>  |   +--[RubyCell]---[before2]
>  +--[RubyContainer(base)]
>  |   +--[RubyCell]---[base1]
>  |   +--[RubyCell]---[base2]
>  +--[RubyContainer(after)]
>      +--[RubyCell]---[after1]
>      +--[RubyCell]---[after2]
> The latter would be a better implementation,

Sort of.  How would it handle styling of the containers that aren't present in the frame tree?  It seems to me that just dropping the extra containers altogether is the right thing to do...
Attached patch css-ruby property patch v3 (obsolete) — Splinter Review
I attached the patch of which I solved the problem on Comment 49.
I added new properties to nsComputedDOMStyle.cpp/.h.

But it didn't passed some tests. (/layout/style/test/)
First, 'ruby-test' and 'ruby-base' which is the display property value didn't passed test_value_cloning.html.
Test program had shutdown with the error below.

++DOMWINDOW == 160 (16F8B570) [serial = 2285] [outer = 14179358]
nsStringStats
 => mAllocCount:        1051011
 => mReallocCount:       100851
 => mFreeCount:         1032374  --  LEAKED 18637 !!!
 => mShareCount:        1155895
 => mAdoptCount:         119940
 => mAdoptFreeCount:     119938  --  LEAKED 2 !!!
la\content\events\src\nseventdispatcher.cpp, line 348)
gklayout!nsEventTargetChainItem::HandleEventTargetChain+0x000000000000032D (c:\hajimedata\ruby-mozilla\content\events\src\nseventdispatcher.cpp, line 398)
gklayout!nsEventDispatcher::Dispatch+0x000000000000083F (c:\hajimedata\ruby-mozilla\content\events\src\nseventdispatcher.cpp, line 628)
.......

Another problems is about property 'font'.
Also when I build firefox without any changes, the test has the same error.
Do you have any advice you can give me?
Attachment #460113 - Attachment is obsolete: true
Attached patch ruby-frame construction patch v1 (obsolete) — Splinter Review
I attached a patch for new frame construction. (the patch has changed only at nsCSSFrameConstructor.cpp)

What the program is doing.
1: creating FCItemList from aItem.mContent's children.
2: wrapping text node of <ruby>'s child using RubyCellFrame.
3: wrapping <rt> or <rb> node of <ruby> child with RubyContainerFrame.

In HTML5 phrasing content should be ruby-base. (http://www.w3.org/TR/html5/text-level-semantics.html#the-ruby-element)
So, some nodes of <ruby>'s child should be wrapped with RubyCellFrame.
How could I get phrasing content node only?

(In reply to comment #67)
> > The latter would be a better implementation,
> 
> Sort of.  How would it handle styling of the containers that aren't present in
> the frame tree?  It seems to me that just dropping the extra containers
> altogether is the right thing to do...

I didn't think about handling the style of container...
Considering handling style, I think that it would be better to ignoring extra containers as well.

Regards,
Hajime.
Attached patch ruby-frame layout patch v2 (obsolete) — Splinter Review
I attached a new patch for <ruby> frame construction and its reflow.

Main changed parts.
1: some fixes about frame construction on Comment69 patch.
2: changing reflow code for new frame construction.
3: solving ruby-overhang problem with complex-ruby on Comment31.
4: add ruby-property for html.css.

This patch has same function of the first patch that I submitted.
Do you think that my frame construction code is going the right way?

Regards,
Hajime.
Attachment #460494 - Attachment is obsolete: true
Attachment #462335 - Attachment is obsolete: true
I'm still not sure we decided on what the behavior should be for the frame construction.  As in, what frame tree should it create in various circumstances?  Unless I missed that somewhere?  It's really hard to tell whether the attached code is what we want given that....
Another question is what should happen to ruby-container and ruby-cell elements whose parent is not a ruby element.
for Boris Zbarsky,

OK. I understand what you said.
So I thought about some invalid situations.
I will show my idea about the construction in these situations.
However some construction is still not implemented...

+++ There no <ruby> tag. ++++
1: <p><rb>base1</rb></p> => <p>base1</p>
2: <p><rt>text1</rt></p> => <p>text1</p>
3: <p><rb>base1</rb><rt>text1</rt></p> => <p>base1text1</p>
4: <p><rbc><rb>base1</rb></rbc></p>    => <p>base1</p>
5: <p><rtc><rt>text1</rt></rtc></p>    => <p>text1</p>
6:
<p>
  <rbc><rb>base1</rb></rbc>
  <rtc><rt>text1</rt></rtc>
</p> 
=> <p>base1text1</p>

+++ Incomplete <ruby> tag +++
6: <ruby><rb>base1</rb></ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
     +--[RubyCell]---[base1]

Rendering:
+-------++-----++-------+
|xxxxxxx||base1||xxxxxxx|
+-------++-----++-------+

7: <ruby><rt>text1</rt></ruby>
Frames:
[Ruby]
 +--[RubyBaseContainer(before)]
     +--[RubyCell]---[text1]

Rendering:
         +-----+
         |text1|
+-------++-----++-------+
|xxxxxxx||     ||xxxxxxx|
+-------++-----++-------+

8: <ruby>base1</ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
     +--[RubyCell]---[base1]

Rendering:
+-------++-----++-------+
|xxxxxxx||base1||xxxxxxx|
+-------++-----++-------+

9: <ruby><rbc><rb>base1</rb></rbc><rbt>text1</rbt></ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text1]

Rendering:
         +-----+
         |text1|
+-------++-----++-------+
|xxxxxxx||base1||xxxxxxx|
+-------++-----++-------+

10: <ruby><rbc>base1</rbc><rbt><rt>text1</rt></rbt></ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text1]

Rendering:
         +-----+
         |text1|
+-------++-----++-------+
|xxxxxxx||base1||xxxxxxx|
+-------++-----++-------+

11:
<ruby>
  <rbc><rt>base1</rt><rb>base2</rb></rbc>
  <rtc><rb>text1</rb><rt>text2</rt></rtc>
</ruby>
Frames
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base2]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text2]

Rendering
         +-----+
         |text2|
+-------++-----++-------+
|xxxxxxx||base2||xxxxxxx|
+-------++-----++-------+
+++ The number of <rt> and <rb> is different +++
12:
<ruby>
  <rbc><rb>base1</rb></rbc>
  <rtc><rt>text1</rt><rt>text2</rt></rtc>
</ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text1]
     +--[RubyCell]---[text2]

Rendering
         +-----+-----+
         |text1|text2|
+-------++-----+-----++-------+
|xxxxxxx||base1|     ||xxxxxxx|
+-------++-----+-----++-------+

13:
<ruby>
  <rbc><rb>base1</rb><rt>base2</rt></rbc>
  <rtc><rt>text1</rt></rtc>
</ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 |   +--[RubyCell]---[base2]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text1]

Rendering
         +-----+-----+
         |text1|     |
+-------++-----+-----++-------+
|xxxxxxx||base1|base2||xxxxxxx|
+-------++-----+-----++-------+

14: <ruby>base1<rt>text1</rt><rt>text2</rt></ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
     +--[RubyCell]---[text1]
     +--[RubyCell]---[text2]

Rendering
         +-----+-----+
         |text1|text2|
+-------++-----+-----++-------+
|xxxxxxx||base1|     ||xxxxxxx|
+-------++-----+-----++-------+


+++ <rb> and <rbc>, or <rt> and <rtc> coexists +++
15:
<ruby>
  <rbc><rb>base1</rb><rb>base2</rb></rbc>
  <rt>text1</rt>
  <rtc><rt>text2</rt></rtc>
</ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
 |   +--[RubyCell]---[text1]
 +--[RubyContainerFrame(after)]
     +--[RubyCell]---[text2]

Rendering
         +-----+
         |text1|
+-------++-----+-----++-------+
|xxxxxxx||base1|base2||xxxxxxx|
+-------++-----+-----++-------+
         |text2|
         +-----+

16:
<ruby>
  <rb>base1</rb><rt>text1</rt>
  <rbc><rb>base2</rb><rb>base3</rb></rbc>
  <rtc><rt>text2</rt><rt>text3</rt></rtc>
</ruby>
Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 +--[RubyContainerFrame(before)]
 |   +--[RubyCell]---[text1]
 +--[RubyContainerFrame(after)]
     +--[RubyCell]---[text2]
     +--[RubyCell]---[text3]

Rendering
         +-----+-----+
         |text1|     |
+-------++-----+-----++-------+
|xxxxxxx||base1|     ||xxxxxxx|
+-------++-----+-----++-------+
         |text2|text3|
         +-----+-----+

The reason for this occurance is:
1: first <rb> is wraped with RubyContainerFrame(base).
2: first <rt> is wraped with RubyContainerFrame(before).
3: first <rbc> is ignored because RubyContainerFrame(base) already exists.
4: first <rtc> became RubyContainerFrame(after) because RubyContainerFrame(before) already exists.

17: 
<ruby>
  <rb>base1</rb>
  <rt>text1</rt>
  <rbc><rb>base2</rb><rb>base3</rb></rbc>
  <rb>base4</rb>
  <rt>text2</rt>
  <rtc><rt>text3</rt><rt>text4</rt></rtc>
</ruby>
Frames
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 |   +--[RubyCell]---[base4]
 +--[RubyContainerFrame(before)]
 |   +--[RubyCell]---[text1]
 |   +--[RubyCell]---[text2]
 +--[RubyContainerFrame(after)]
     +--[RubyCell]---[text3]
     +--[RubyCell]---[text4]

Rendering
         +-----+-----+
         |text1|text2|
+-------++-----+-----++-------+
|xxxxxxx||base1|base4||xxxxxxx|
+-------++-----+-----++-------+
         |text3|text4|
         +-----+-----+

The reason for this occurance is:
1: first <rb> is wraped with RubyContainerFrame(base).
2: first <rt> is wraped with RubyContainerFrame(before).
3: first <rbc> is ignored because RubyContainerFrame(base) already exists.
4: second <rb> is added for RubyContainerFrame(base).
5: second <rt> is added for RubyContainerFrame(before).
6: second <rtc> became RubyContainerFrame(after) because RubyContainerFrame(before) already exists.


And about font-size problem, I'm waiting for a replay from the Japanese Layout Taskforce.
Do you have any other situations that we should consider about?

Regards,
Hajime.
(In reply to comment #74)
> 15:
> <ruby>
>   <rbc><rb>base1</rb><rb>base2</rb></rbc>
>   <rt>text1</rt>
>   <rtc><rt>text2</rt></rtc>
> </ruby>
> Frames:
> [Ruby]
>  +--[RubyContainerFrame(base)]
>  |   +--[RubyCell]---[base1]
>  +--[RubyContainerFrame(before)]
>  |   +--[RubyCell]---[text1]
>  +--[RubyContainerFrame(after)]
>      +--[RubyCell]---[text2]
> 
> Rendering
>          +-----+
>          |text1|
> +-------++-----+-----++-------+
> |xxxxxxx||base1|base2||xxxxxxx|
> +-------++-----+-----++-------+
>          |text2|
>          +-----+

This figure was wrong.
The one below is correct.

Frames:
[Ruby]
 +--[RubyContainerFrame(base)]
 |   +--[RubyCell]---[base1]
 |   +--[RubyCell]---[base2]
 +--[RubyContainerFrame(before)]
 |   +--[RubyCell]---[text1]
 +--[RubyContainerFrame(after)]
     +--[RubyCell]---[text2]

Rendering
         +-----+-----+
         |text1|     |
+-------++-----+-----++-------+
|xxxxxxx||base1|base2||xxxxxxx|
+-------++-----+-----++-------+
         |text2|     |
         +-----+-----+
>+++ There no <ruby> tag. ++++
>=> <p>base1text1</p>

This will lose styles on the <rb> and whatnot, no?  Or will it just create inline frames for the various ruby stuff?

Thank you for explaining the plan.

Given that, is there a reason to do the special processing in ConstructRuby instead of adding a pass where we currently do CreateNeededTablePseudos?  That would have the benefit of sharing more code and, for example, correctly handling things like :before and :after, which this patch doesn't as far as I can tell....
(In reply to comment #76)
I'm sorry for late reply.

> This will lose styles on the <rb> and whatnot, no?  Or will it just create
> inline frames for the various ruby stuff?
I think that this will lose style on the <rb>.
In the below code, the style on the <td> is lost on Firefox 3.6.
<p><td style='color:red;'>text</td></p>
In this case font-color of 'text' is black.

> Given that, is there a reason to do the special processing in ConstructRuby
> instead of adding a pass where we currently do CreateNeededTablePseudos?  That
> would have the benefit of sharing more code and, for example, correctly
> handling things like :before and :after, which this patch doesn't as far as I
> can tell....
If we can add a new processing into CreateNeededTablePseudos, this would be
better.
I thought that CreateNeededTablePseudos was only for the <table> tag. 
So, I added a wrapping process into ConstructRuby. 
If the processing is inside CreateNeededTablePseudos, can I add FCData of around <ruby> into sPseudoParentData?

Regards,
Hajime.
> In the below code, the style on the <td> is lost on Firefox 3.6.

In that code, there's no <td> node in the DOM at all, due to parser fixups.  The frame constructor doesn't lose anything.  If you create that DOM tree using DOM API calls, or use an XHTML document, it will render as expected.

> If we can add a new processing into CreateNeededTablePseudos, this would be
> better.

Or just create a new function called right before or after CreateNeededTablePseudos.  I think this would work better.

> I thought that CreateNeededTablePseudos was only for the <table> tag. 

It's for handling missing internal table elements (rows, cells, tables, colgroups, etc) in general.
(In reply to comment #78)
> > In the below code, the style on the <td> is lost on Firefox 3.6.
>
> In that code, there's no <td> node in the DOM at all, due to parser fixups.
> The frame constructor doesn't lose anything.  If you create that DOM tree using
> DOM API calls, or use an XHTML document, it will render as expected.
Thank you for your information.
I'll check that it will render as expected if I use the DOM API calls or the use of the XHTML document.

> > If we can add a new processing into CreateNeededTablePseudos, this would be
> > better.
>
> Or just create a new function called right before or after
> CreateNeededTablePseudos.  I think this would work better.
OK. I agree with you.
I'm going to start implementation the new function CreateNeededRubyPuseudos.
Is that fine with you?

> > I thought that CreateNeededTablePseudos was only for the <table> tag.
>
> It's for handling missing internal table elements (rows, cells, tables,
> colgroups, etc) in general.
I understand it.

Regards,
Hajime.
> Is that fine with you?

CreateNeededRubyPseudos, please.

But yes, that sounds fine.  Or maybe even better EnsureValidRubyStructure, since it'll do more than just creating pseudos?
I'm working on implementing the EnsureValidRubyStructure() at the moment. 
But I've come across a problem. 

I've added eTypeRuby, eTypeRubyContainer and eTypeRubyCell into ParentType.
And I've also added the FCData into sPseudoParentData using the FCDATA_DESIRED_PARENT_TYPE_TO_BITS option.
But in this implementation, valid ruby processing and table pseudo processing gets mixed.
For example, when calling SkipItemsWantingParentType() in CreateNeededTablePseudos(), around ruby FCItem that is needed to wrap is not skipped.

So, Now I have two implementation in mind.
1: creating RubyParentType
2: implementing EnsureValidStructure() without using ParentType.
However these implementations needs alot of new codes and we can't share codes...

Are there any good solutions?


Regards,
Hajime.
Boris is going to be away for a week, and he's the best person to answer your question, sorry!

It seems to me that renaming nsCSSFrameConstructor::CreateNeededTablePseudos to CreateNeededPseudos and integrating ruby processing into it could work, but it's probably simpler in the end to create a separate CreateNeededRubyPseudos and call it from ConstructFramesFromItemList after we've called CreateNeededTablePseudos. You'll have to duplicate some of the structures used to process table pseudos. I think that's "implementation 1" in your comment. It will add code but it's probably better to keep tables and ruby cleanly separated.
How much code would you be able to share with the table thing anyway?  Not using parent type seems like a better idea to me at first glance, but I haven't tried implementing it.

Really gone for a week now, sorry.  :(
Thank you for your reply.

I've attached a patch that implements EnsureValidRubyStructure.
This patch only includes the changes I've made to nsCSSFrameConstructor.cpp.

In this patch, I didn't implement any new ParentType (e.g. RubyParentType).
Reconsidering about the code that is shared, I found out that there wouldn't be many code that would be shared by implementing the new ParentType.
I think that it is true by implementing the new ParentType, I could implement EnsureValidRubyStructure by referencing TablePseudoProcess.
Although EnsureValidRubyStructure is slightly verbose, there is no any additional code elsewhere in this patch.

EnsureValidRubyStructure run processes as below.
1: If parent frame is <ruby>, wrap text node item with <rb> for HTML5.
2: If parent frame is <ruby>, wrap ruby-cell item with <rbc> or <rtc>.
3: If parent frame is NOT <ruby>, wrap around ruby items with <ruby>.

Also, there are some changes with its behavior that is written in Comment 73 and Comment 74.
If there are no <ruby> tag, the browser is able to display ruby.
1: <p><rb>base1</rb></p>
    -> <p><ruby><rbc><rb>base1</rb></rbc></ruby></p>
2: <p><rt>text1</rt></p>
    -> <p><ruby><rtc><rt>text1</rt></rtc></ruby></p>
3: <p><rb>base1</rb><rt>text1</rt></p>
    -> <p><ruby><rbc><rb>base1</rb></rbc>
                <rtc><rt>text1</rt></rtc></ruby></p>
4: <p><rbc><rb>base1</rb></rbc></p>
    -> <p><ruby><rbc><rb>base1</rb></rbc></ruby></p>
5: <p><rtc><rt>text1</rt></rtc></p>
    -> <p><ruby><rtc><rt>text1</rt></rtc></ruby></p>

And <rt> in <rbc> or <rb> in <rtc> are *not* ignored.
11:
<ruby>
 <rbc><rt>base1</rt><rb>base2</rb></rbc>
 <rtc><rb>text1</rb><rt>text2</rt></rtc>
</ruby>
Frames
[Ruby]
+--[RubyContainerFrame(base)]
|   +--[RubyCell]---[base1]
|   +--[RubyCell]---[base2]
+--[RubyContainerFrame(before)]
    +--[RubyCell]---[text1]
    +--[RubyCell]---[text2]

Rendering
        +-----+-----+
        |text1|text2|
+-------++-----+-----++-------+
|xxxxxxx||base1|base2||xxxxxxx|
+-------++-----+-----++-------+


Regards,
Hajime.
That looks pretty good. One comment: EnsureValidRubyStructure currently always iterates through the entire child list looking for ruby child frames. That is likely to hurt performance. You should probably add a field to FrameConstructionItemList that counts the number of ruby child frame items. That way, for lists that contain no ruby child frame items (the common case), you don't need to iterate through the list.

+  if (aParentFrame->GetType() == nsGkAtoms::rubyFrame ||
+      aParentFrame->GetType() == nsGkAtoms::rubyContainerFrame) {

It would also be nice to not call GetType() here, if there's a way to pass down whether the parent frame is a ruby or ruby container frame.
So is Mozilla planning on supporting more of ruby than the other browsers? I.e. more than HTML5 requires? I am asking because comment 84 mentions various concepts not present in HTML5 or implementations of WebKit/Trident.
But, concepts present in HTML 1.1 et W3C Ruby Annotation : http://www.w3.org/TR/ruby/
> So is Mozilla planning on supporting more of ruby than the other browsers? 

Unclear.  The current patches propose to do that, yes.

I'm aiming to look at the comment 84 patch once I dig out a bit from the blocker stuff that accumulated over the last week; figure a few days.
I've attached a patch that counts the number of the ruby children.

So, I added some variables.
1: mRubyChildCount (FrameConstructionItemList).
2: mIsRubyChild (FrameConstructionItem).
3: FCDATA_IS_RUBY_PART (MACRO).

> +  if (aParentFrame->GetType() == nsGkAtoms::rubyFrame ||
> +      aParentFrame->GetType() == nsGkAtoms::rubyContainerFrame) {
>
> It would also be nice to not call GetType() here, if there's a way to pass down
> whether the parent frame is a ruby or ruby container frame.

I dont quite understand why I shouldn't call GetType(). 
Why is it nice to not call GetType()?
I thought of ways to pass it down, but I couldn't think of any good solution.
Attachment #468576 - Attachment is obsolete: true
I fixed some bugs, this is the new patch.

1: It can render the markup below.

<ruby><span>AAA</sapn><rt>aaa</rt></ruby>


2: I fixied the bug which occured in the case of the makrup below.
   This bug couldn't render AAA.
   So I added the line, "newItem->mIsRubyChild = PR_TRUE;".

   <ruby>AAA<rt>aaa</rt></ruby>


I'm trying to rewrite around Reflow code of Ruby Frame for new construction method.
Are there any things that I should consider about FrameConstruction?

Regards,
Hajime.
Attachment #470674 - Attachment is obsolete: true
Attached patch css-ruby property patch v4 (obsolete) — Splinter Review
This is the latest patch. (css-ruby property and ruby-frame layout)

Now I've tried to implement the handling of dynamic DOM mutations around ruby-module.
The following are the implementations I've made.

1: Implementing AppendFrames(), InsertFrames() and RemoveFrame() in nsRubyFrame and nsRubyContainerFrame.
   In these functions, nsRubyFrame is intentionally set to NS_FRAME_IS_DIRTY.
2: In MaybeRecreateContainer(), if parentFrame is ruby-pseudo frame then RecreateFramesForContent() is called like table-pseudo.
3: In WipeContainingBlock(), if aItems has any ruby child or parentFrame is rubyFrame or rubyContainerFrame then RecreateFramesForContent() is called.

I didn't understand all about the handling dynamic DOM mutation.
Please give me advice if you see anything in mind.


Other changes:
1: Ruby-module css property moved to nsIDOMCSS2Properties.idl
2: In EnsureValidRubyStructure(), checking the number of ruby container and removing them if necessary.
3: Added new function FCItemList::Iterator::DeleteItemonItem(), it removes one item in FCItemList.
4: Implemented RubyFrame::IdentifyRubyContainerFrames to check whether container frame is base frame or text frame.
5: Added rp { display: none; } in html.css, because inline frame that is other than <rp> must be displayed.

Regards,
Hajime.
Attachment #462334 - Attachment is obsolete: true
Attached patch ruby-frame layout patch v3 (obsolete) — Splinter Review
This is the patch of ruby-frame layout.
Attachment #462688 - Attachment is obsolete: true
Attachment #481790 - Attachment is obsolete: true
Attachment #488437 - Flags: review?(bzbarsky)
Attachment #488438 - Flags: review?(bzbarsky)
Attached patch ruby reftest patch v1 (obsolete) — Splinter Review
This is the patch for ruby Reftest.

I added some tests in '/layout/reftests/ruby/'.

Especially I wrote some invalid cases (invalidRuby*.html) and the
expected behavior for this case (invalidRuby*-ref.html).
I would be glad if you could check these files and give me any comments.

Also I have a problem referring Comment 51.

I couldn't solve this problem, so I use <xx> tag instead of <rb>.
Concretely in the case of the markup below, generated contents are like below 'Genereted Content List'.
Should I add this case as a new bug?

***Markup***
<body>
 <ruby>
   <rbc><rb>b1</rb><rb>b2</rb><rb>b3</rb></rbc>
   <rtc><rt>t1</rt><rt>t2</rt><rt>t3</rt></rtc>
 </ruby>
</body>


***Generated Content List***
body@06260B88 intrinsicstate=[10000] flags=[02200001]
primaryframe=06276A70 refcount=2<
 Text@0613B750 intrinsicstate=[      10000] flags=[0c000001]
primaryframe=00000000 refcount=1<\u000a    >
 ruby@06277698 intrinsicstate=[10000] flags=[02200001]
primaryframe=06276C10refcount=3<
   Text@06277710 intrinsicstate=[      10000] flags=[0c000001]
primaryframe=00000000 refcount=1<\u000a      >
   rbc@06277838 intrinsicstate=[10000] flags=[02200001]
primaryframe=06248F08 refcount=2<
     rb@06277918 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249050 refcount=3<
       Text@062779F0 intrinsicstate=[      10000] flags=[00000001]
primaryframe=062491B8 refcount=2<b1>
     >
     rb@06277AF0 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249238 refcount=3<
       Text@0623B640 intrinsicstate=[      10000] flags=[00000001]
primaryframe=062492F0 refcount=2<b2>
     >
     rb@06277C50 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249370 refcount=3<
       Text@06277CC8 intrinsicstate=[      10000] flags=[00000001]
primaryframe=06249428 refcount=2<b3>
     >
   >
   Text@06277DA0 intrinsicstate=[      10000] flags=[0c000001]
primaryframe=00000000 refcount=1<\u000a      >
   rtc@06277E68 intrinsicstate=[10000] flags=[02200001]
primaryframe=062494A8 refcount=2<>
   rt@06277A68 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249538refcount=3<
     Text@06279018 intrinsicstate=[      10000] flags=[00000001]
primaryframe=062496A0 refcount=2<t1>
   >
   rt@062790F0 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249720refcount=3<
     Text@062791E8 intrinsicstate=[      10000] flags=[00000001]
primaryframe=062497D8 refcount=2<t2>
   >
   rt@06279260 intrinsicstate=[10000] flags=[02200001]
primaryframe=06249858refcount=3<
     Text@062792D8 intrinsicstate=[      10000] flags=[00000001]
primaryframe=06249910 refcount=2<t3>
   >
   Text@062793B0 intrinsicstate=[      10000] flags=[0c000001]
primaryframe=00000000 refcount=1<\u000a    >
 >
 Text@06279428 intrinsicstate=[      10000] flags=[0c000001]
primaryframe=00000000 refcount=1<\u000a  \u000a\u000a>
>
Attachment #492909 - Flags: review?(bzbarsky)
Attached patch ruby-frame layout patch v4 (obsolete) — Splinter Review
This is the patch of ruby-frame layout.

I fixed some parts referring the result of some tests I made.

Especially, I changed the way to construct frames when there are more than two <rbc> or three <rtc>.

BEFORE: The number of <rbc> or <rtc> is checked in the EnsureValidRubyStructure.
        RubyContainerFrames that is not expected are not constructed.
NOW: The number of <rbc> or <rtc> is NOT checked in the EnsureValidRubyStructre.
     The number of RubyContainerFrame is checked in RubyFrame::Reflow().
     RubyContainerFrames that is not expected are constructed like the expected one, but these are NOT into Reflow();

Now when removing first <rbc> in the markup below, second <rbc> is visible as new ruby-base.

<ruby>
 <rbc><rb>AA</rb><rb>BB</rb><rb>CC</rb></rbc>
 <rbc><rb>xx</rb><rb>xx</rb><rb>xx</rb></rbc>
 <rtc><rt>aa</rt><rt>bb</rt><rt>cc</rt></rtc>
<ruby>



Regards,
Hajime.
Attachment #488438 - Attachment is obsolete: true
Attachment #492910 - Flags: review?(bzbarsky)
Attachment #488438 - Flags: review?(bzbarsky)
Comment on attachment 488437 [details] [diff] [review]
css-ruby property patch v4

>+++ b/dom/interfaces/css/nsIDOMCSS2Properties.idl

You need to change the interface id here, since you're adding things to the interface.

>+++ b/layout/style/html.css
>+ruby { 

>+  margin: 0;
>+  border: 0;
>+  padding: 0;

Those are the default values.  Why do you need to set them explicitly here?

>+  line-height: 1;

Why?  Wouldn't normal line-height make more sense?  This should probably be at least commented explaining why it's needed if it's really needed.

>+  vertical-align: text-bottom;

Again, please explain in a comment why this is needed.

>+++ b/layout/style/nsCSSAnonBoxList.h
>+CSS_ANON_BOX(ruby, ":-moz-ruby")
>+CSS_ANON_BOX(rubyBase, ":-moz-ruby-base")
>+CSS_ANON_BOX(rubyBaseContainer, ":-moz-ruby-base-container")
>+CSS_ANON_BOX(rubyTextContainer, ":-moz-ruby-text-container")
>+CSS_ANON_BOX(rubyCellBlock, ":-moz-ruby-cell-block")

There should be corresponding display styles for these in ua.css, right?  Please add those or explain why they're not needed.

>+++ b/layout/style/nsCSSKeywordList.h
>+CSS_KEY(ruby-align, ruby_align)
>+CSS_KEY(ruby-position, ruby_position)
>+CSS_KEY(ruby-overhang, ruby_overhang)

You shouldn't need those.  Please take them out.

We're implementing "distribute-letter" but not "distribute-space" or "line-edge" for 'ruby-align', I assume?  That's fine by me, but just wanted to make sure that's the idea.

>+++ b/layout/style/nsCSSPropList.h
>+CSS_PROP_VISIBILITY(

These should probably go right after the css prop declaration for "right", not at the very end after all the SVG stuff.

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+nsComputedDOMStyle::DoGetRubyPosition(nsIDOMCSSValue** aValue)

This will need merging on either your part or Craig's, depending on whether this lands before bug 585867.  Probably better to have him land first; merging this on top of his changes should be easy...  But check with Craig on his timeframe if you have reviews and the tree is open and he hasn't landed yet?  Not worth holding this for more than a day or so waiting on the other bug.

>+++ b/layout/style/nsRuleNode.cpp
>+  // ruby overhang
>+  SetDiscrete(displayData.mRubyOverhang, visibility->mRubyOverhang,
>+              canStoreInRuleTree,
>+              SETDSC_ENUMERATED, parentVisibility->mRubyOverhang,
>+              NS_STYLE_RUBY_OVERHANG_AUTO, 0, 0, 0, 0);

The initial value should be NS_STYLE_RUBY_OVERHANG_NONE, per the spec draft I see.

>+++ b/layout/style/nsStyleStruct.cpp
>+  mRubyOverhang = NS_STYLE_RUBY_OVERHANG_AUTO;

Likewise here.

>+++ b/layout/style/nsStyleStruct.h
>   PRUint8 mDirection;                  // [inherited] see nsStyleConsts.h NS_STYLE_DIRECTION_*
...
>+  PRUint8       mRubyPosition;         // [inherited]
>+  PRUint8       mRubyAlign;            // [inherited]
>+  PRUint8       mRubyOverhang;         // [inherited]

There should be "see nsStyleConsts.h NS_STYLE_...." comments on those three members.

>+++ b/layout/style/test/property_database.js
>+	"ruby-overhang": {
>+		initial_values: [ "auto" ],

Again, should probably be "none" unless the spec has changed.

>+		other_values: [ "block", "list-item", "inline-block", "ruby", "ruby-text-container", "ruby-base-container", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row", "table-column-group", "table-column", "table-cell", "table-caption", "none"],

Should have "ruby-base" and "ruby-text" in that list as well.

Should the ruby display types be added to any of the things like IsBlockInside(), IsBlockOutside(), IsInlineOutside()?

For that matter, do we want to have separate ruby and inline-ruby (or block-ruby?) display types to deal with <ruby> being positioned or floated?  (hyatt brought this up on the www-style list a while back, as I recall.)  Or should we push that off to a separate bug?

One thing I'd like to see in this patch is additions to nsCSSFrameConstructor::FindDisplayData that make it return _something_ for the new display types.  Just the same thing it returns for NS_STYLE_DISPLAY_INLINE should do (esp. if we treat display:ruby as inline-outside).  I realize the later patches add that, but if we add some sort of fallback here then we can run a build with just this patch applied (e.g. while bisecting) without worrying about crashes due to failing the "FindDisplayData always returns something" assertion.

r-, but apart from the question about adding to Is*Inside/Outside there's nothing here that's non-mechanical, I think.
Attachment #488437 - Flags: review?(bzbarsky) → review-
Comment on attachment 492909 [details] [diff] [review]
ruby reftest patch v1

The dynamic append tests should probably be called dynamicAppendRuby1.html and so forth. ("Append" instead of "Appendant".)  Also you want to flush out layout (e.g. by getting document.body.offsetWidth) before doing the append.  You also want to flush for the other dynamic tests.

You need to add your new list to layout/reftests/reftest.list, right?  As things stand, those tests won't get run.

r=me with those changes.
Attachment #492909 - Flags: review?(bzbarsky) → review+
I'm partway through the layout patch, but not to the point where I'm happy with my understanding yet.  I should have comments on that tomorrow.
> For that matter, do we want to have separate ruby and inline-ruby (or
> block-ruby?) display types to deal with <ruby> being positioned or floated? 
> (hyatt brought this up on the www-style list a while back, as I recall.)  Or
> should we push that off to a separate bug?

I don't think that's necessary. Ruby is fundamentally an inline concept, and the anonymous box generation rules should be able to take care of a blockified <ruby> element if that's wanted. They should just wrap the ruby base and ruby text in an intermediary ruby container, and <ruby> will be a normal block. For floats and abspos, if we specify that 'display' computes to 'block' in those cases, then that's taken care of as well.

> ruby reftest patch v1

If these tests could be written to match the CSS template <http://wiki.csswg.org/test/css2.1/format> that would make it much easier to incorporate into the W3C test suites.

Also, there's been some discussion of the ruby-position values and whether the names are really appropriate for vertical text. That might be in flux therefore.
Oh, that reminds me.  We should probably moz-prefix all this stuff.  That means the new values of the display property, the three new property names, and probably the nsIDOMCSS2Properties property names.  We don't need to prefix the values of the three new properties.
Attached patch ruby reftest patch v2 (obsolete) — Splinter Review
Thank you for reviewing with some comments.
I have submitted a new patch for ruby test case. 

(In reply to comment #96)
> The dynamic append tests should probably be called dynamicAppendRuby1.html and
> so forth. ("Append" instead of "Appendant".)  Also you want to flush out layout
> (e.g. by getting document.body.offsetWidth) before doing the append.  You also
> want to flush for the other dynamic tests.
> You need to add your new list to layout/reftests/reftest.list, right?  As
> things stand, those tests won't get run.
> r=me with those changes.
I've already done that.

(In reply to comment #98)
> >ruby reftest patch v1
> If these tests could be written to match the CSS template
> <http://wiki.csswg.org/test/css2.1/format> that would make it much easier to
> incorporate into the W3C test suites.
I changed the test file style.
I added '//' right before <![CDATA[,  because Firefox interprets JavaScript code without '//'.
Is this the correct way?

Regards,
Hajime.
Attachment #492909 - Attachment is obsolete: true
Attachment #513375 - Flags: review?(bzbarsky)
(In reply to comment #95)

> > +++ b/dom/interfaces/css/nsIDOMCSS2Properties.idl
> You need to change the interface id here, since you're adding things to the
> interface.
I've changed that using http://mozilla.pettay.fi/cgi-bin/mozuuid.pl

> >+++ b/layout/style/html.css
> >+  margin: 0;
> >+  border: 0;
> >+  padding: 0;
> >+  line-height: 1;
I have erased it as you have said it to.

> >+  vertical-align: text-bottom;
This is for aligning the bottom of line and the bottom of RubyFrame.
As RubyFrame doesn't have baseline, when using vertical-align: baseline the baseline of line and the bottom of RubyFrame are aligned together.
This is wrong.
Also there is a need for different vertical-align proeprty for upside ruby, bottom side ruby and both side ruby in my implementaion.
I've been thinking if I should implement the baseline of *Ruby Frame* to solve this problem.
Do you think this has any feasibility?

> >+++ b/layout/style/nsCSSKeywordList.h
> >+CSS_KEY(ruby-align, ruby_align)
> >+CSS_KEY(ruby-position, ruby_position)
> >+CSS_KEY(ruby-overhang, ruby_overhang)
> You shouldn't need those.  Please take them out.
I understand.

> We're implementing "distribute-letter" but not "distribute-space" or
> "line-edge" for 'ruby-align', I assume?  That's fine by me, but just wanted to
> make sure that's the idea.
I reconsidered about the ruby-align property.
I think properties that is needed is as below.

auto 
left
center
right
distribute-space
(start)
(end)

I think distribute-letter is no need when thinking in the position of Japanese typography, but distribute-space is essential.
I think line-edge should not be in ruby-align as refered in http://dev.w3.org/csswg/css3-ruby/#rubyalign.
start and end property is important considering vertical layout and bidi text.
I would like to talk about this later on...
So I think at the moment we should implementing these five properties.

When implementing distribute-space, there is a need to insert the appropriate distance between each glyphs like text-justify: distribute.
Is it possible to do this in TextThebes?

> >+++ b/layout/style/nsCSSPropList.h
> >+CSS_PROP_VISIBILITY(
> These should probably go right after the css prop declaration for "right", not
> at the very end after all the SVG stuff.
I understand.
> >+++ b/layout/style/nsComputedDOMStyle.cpp
> >+nsComputedDOMStyle::DoGetRubyPosition(nsIDOMCSSValue** aValue)
> This will need merging on either your part or Craig's, depending on whether
> this lands before bug 585867.  Probably better to have him land first; merging
> this on top of his changes should be easy...  But check with Craig on his
> timeframe if you have reviews and the tree is open and he hasn't landed yet? 
> Not worth holding this for more than a day or so waiting on the other bug.
Should these parts be commented out or erased from this patch?

> >+++ b/layout/style/nsRuleNode.cpp
> >+  // ruby overhang
> >+  SetDiscrete(displayData.mRubyOverhang, visibility->mRubyOverhang,
> >+              canStoreInRuleTree,
> >+              SETDSC_ENUMERATED, parentVisibility->mRubyOverhang,
> >+              NS_STYLE_RUBY_OVERHANG_AUTO, 0, 0, 0, 0);
> The initial value should be NS_STYLE_RUBY_OVERHANG_NONE, per the spec draft I
> see.
> +++ b/layout/style/nsStyleStruct.cpp
> +  mRubyOverhang = NS_STYLE_RUBY_OVERHANG_AUTO;
> Likewise here.
> >+++ b/layout/style/test/property_database.js
> >+	"ruby-overhang": {
> >+		initial_values: [ "auto" ],
> Again, should probably be "none" unless the spec has changed.
I think there is no need of left and right, also I never seen these in any Japanese books and so on.
So we should implement two properties, none and auto.
About auto property, we should implement overhang width calculation depending on each character.
I would like to do this later on.

> >+++ b/layout/style/nsStyleStruct.h
> >   PRUint8 mDirection;                  // [inherited] see nsStyleConsts.h NS_STYLE_DIRECTION_*
> ...
> >+  PRUint8       mRubyPosition;         // [inherited]
> >+  PRUint8       mRubyAlign;            // [inherited]
> >+  PRUint8       mRubyOverhang;         // [inherited]
ok.

> There should be "see nsStyleConsts.h NS_STYLE_...." comments on those three
> members.
> >+		other_values: [ "block", "list-item", "inline-block", "ruby", "ruby-text-container", "ruby-base-container", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row", "table-column-group", "table-column", "table-cell", "table-caption", "none"],
> Should have "ruby-base" and "ruby-text" in that list as well.
I see.

> We should probably moz-prefix all this stuff.  That means
> the new values of the display property, the three new property names, and
> probably the nsIDOMCSS2Properties property names.  We don't need to prefix the
> values of the three new properties.  
I've done that.

Others that have been pointed out, I will answer them later.

Regards,
Hajime.
Comment on attachment 513375 [details] [diff] [review]
ruby reftest patch v2

r=me
Attachment #513375 - Flags: review?(bzbarsky) → review+
Depends on: 585867
> I've been thinking if I should implement the baseline of *Ruby Frame* to solve
> this problem.

That seems like a better approach, yes.  Let's leave the vertical-align:text-bottom for now, file a new bug on the baseline thing, and add a comment on the vertical-align style pointing to that bug.  That way we can land the initial implementation without getting bogged down in the baseline issues.

> So I think at the moment we should implementing these five properties.

OK, sounds good.  It sounds like distribute-space should also go in a followup bug, by the way.

> Is it possible to do this in TextThebes?

Probably, but you'll want to check with roc and it might take some work...

> Should these parts be commented out or erased from this patch?

No, just leave them as is.  That comment about merging was just for your information, not a request to change any code.

> So we should implement two properties, none and auto.

Sounds good.

> I would like to do this later on.

Also sounds good.  :)
Attached patch css-ruby property patch v5 (obsolete) — Splinter Review
I've attatched a new patch for css props.

> >+++ b/layout/style/nsCSSAnonBoxList.h
> >+CSS_ANON_BOX(ruby, ":-moz-ruby")
> >+CSS_ANON_BOX(rubyBase, ":-moz-ruby-base")
> >+CSS_ANON_BOX(rubyBaseContainer, ":-moz-ruby-base-container")
> >+CSS_ANON_BOX(rubyTextContainer, ":-moz-ruby-text-container")
> >+CSS_ANON_BOX(rubyCellBlock, ":-moz-ruby-cell-block")
> There should be corresponding display styles for these in ua.css, right? 
> Please add those or explain why they're not needed.
Could you check again to see if I have added it correctly?

> Should the ruby display types be added to any of the things like
> IsBlockInside(), IsBlockOutside(), IsInlineOutside()?
I'm sorry, I could not understand these three functions.
Could you tell me the role of each of these functions?

> For that matter, do we want to have separate ruby and inline-ruby (or
> block-ruby?) display types to deal with <ruby> being positioned or floated? 
> (hyatt brought this up on the www-style list a while back, as I recall.)  Or
> should we push that off to a separate bug?
>>[fantasai]
> I don't think that's necessary. Ruby is fundamentally an inline concept, and
> the anonymous box generation rules should be able to take care of a blockified
> <ruby> element if that's wanted. They should just wrap the ruby base and ruby
> text in an intermediary ruby container, and <ruby> will be a normal block. For
> floats and abspos, if we specify that 'display' computes to 'block' in those
> cases, then that's taken care of as well.
I agree to the fact that block-ruby isn't necessary.
But I can't really say whether that will work, 
because I'm not reawlly an expert on box generations and it's formatting model...

> One thing I'd like to see in this patch is additions to
> nsCSSFrameConstructor::FindDisplayData that make it return _something_ for the
> new display types.  Just the same thing it returns for NS_STYLE_DISPLAY_INLINE
> should do (esp. if we treat display:ruby as inline-outside).  I realize the
> later patches add that, but if we add some sort of fallback here then we can
> run a build with just this patch applied (e.g. while bisecting) without
> worrying about crashes due to failing the "FindDisplayData always returns
> something" assertion.
O.K.
I added some codes so it will work like NS_STYLE_DISPLAY_INLINE.

And I've implemented ruby-span so I added ruby-span property.

Regards,
Hajime.
Attachment #488437 - Attachment is obsolete: true
Attachment #516536 - Flags: review?(bzbarsky)
Attached patch css-ruby property patch v5.1 (obsolete) — Splinter Review
I'm sorry, I've attached the old path instead of new pacth.
This is new patch includes changes in ua.css

Regards,
Hajime.
Attachment #516536 - Attachment is obsolete: true
Attachment #516826 - Flags: review?(bzbarsky)
Attachment #516536 - Flags: review?(bzbarsky)
Comment on attachment 516826 [details] [diff] [review]
css-ruby property patch v5.1

>+++ b/layout/style/html.css
>+xx { 
>+  font-size: 60%;
>+  display: -moz-ruby-text;
>+}

Please take that out.

It looks like you took out the computed style stuff after all.  That's probably ok if we add it in a followup, but there was no real reason to take it out....

>+++ b/layout/style/test/property_database.js
>-		other_values: [ "block", "list-item", "inline-block", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row", "table-column-group", "table-column", "table-cell", "table-caption", "none" ],
>+		other_values: [ "block", "list-item", "inline-block", "ruby", "ruby-base", "ruby-text", "ruby-text-container", "ruby-base-container", "table", "inline-table", "table-row-group", "table-header-group", "table-footer-group", "table-row", "table-column-group", "table-column", "table-cell", "table-caption", "none"],

The added values there should be prefixed with "-moz-".

> Could you check again to see if I have added it correctly?

Looks good.

> Could you tell me the role of each of these functions?

The idea for the *Outside functions is to test what the display type looks like "from the outside", while IsBlockInside tests for what it looks like "from the inside".  For example, inline-block tests true for IsInlineOutside(), false for IsBlockOutside(), and true for IsBlockInside().

My guesses based on how this should work are:

1) IsInlineOutside() should be true if and only if a frame with that display type is allowed as a direct child
of an inline.  It sounds like NS_STYLE_DISPLAY_RUBY should be added here, probably, but not the others.

2) IsBlockOutside() and IsBlockInside() should be false for all the new display types.

> I agree to the fact that block-ruby isn't necessary.

OK, good.

> I added some codes so it will work like NS_STYLE_DISPLAY_INLINE.

Looks great.

The ruby-span bit is fine, but let's not add more stuff to these patches before landing, ok?  Followup bugs are good for that!  You do need to add -moz-ruby-span to property_database.js.

So action items:
1)  Fix the two nits in property_database.js
2)  Add NS_STYLE_DISPLAY_RUBY to IsInlineOutside()
3)  Take out the "xx" thing from html.css

r=me with those three changes made.
Attachment #516826 - Flags: review?(bzbarsky) → review+
Er, and the other thing with ruby-span is that the layout patch doesn't have it.  Could you please post a version that does (and in general one updated to your latest property patch changes), or take it out of the css property patch?

I'll make sure to finish up the layout bits on Monday if you post an updated version.  I'm really sorry it's taking so long.  :(
Attached patch ruby-frame layout patch v5 (obsolete) — Splinter Review
This is the new layout patch.

I implemented ruby-span referring the column span of Table. (mainly changed on nsRubyFrame.cpp)

I'm having the problem of corresponding -moz-ruby-cell-block and 'display:block'.
The problem is that BlockFrame that is generated on ConstructRubyCell() could not calculate the BlockFrame width correctly.
The width is very big than what is expected.
Do you have any idea to solve this?

If the correspoinding works well, I will remove nsCSSAnonBoxes::rubyCellBlock and alternately use nsCSSAnonBoxes::cellContent.
And I removed that stuff on the css prop patch.

> So action items:
> 1)  Fix the two nits in property_database.js
> 2)  Add NS_STYLE_DISPLAY_RUBY to IsInlineOutside()
O.K. I understand.

> 3)  Take out the "xx" thing from html.css
Probably there is still the problem that I refered on comment #93,
Should I remove xx on html.css and <xx> on some reftests?

Regards,
Hajime.
Attachment #492910 - Attachment is obsolete: true
Attachment #517990 - Flags: review?(bzbarsky)
Attachment #492910 - Flags: review?(bzbarsky)
> Probably there is still the problem that I refered on comment #93,

I'm not sure I follow what the problem is...  You're using "xx" instead of "rb" in the tests because of some issue with the layout part of the patch it sounds like.  We can sort that out, but even if the tests do need to use "xx" for some reason whatever styling goes with that needs to be in the _test_, not in html.css.

> Should I remove xx on html.css

Yes, no matter what.

> and <xx> on some reftests?

Why is the "xx" there in the first place?
Comment on attachment 517990 [details] [diff] [review]
ruby-frame layout patch v5

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -4436,17 +4452,31 @@ nsCSSFrameConstructor::FindDisplayData(c

It looks like this is not written on top of the css property patch, right?  How was this tested, then?

>+nsCSSFrameConstructor::EnsureValidRubyStructure(nsFrameConstructorState& aState,

Given that there is no specification for the thing this method is trying to implement, I think there needs to be a big comment right before the method that explains the invariants it's trying to ensure, and the general approach it takes to ensuring them.

>+  if (aItems.IsEmpty() ||
>+      !aItems.HasRubyChild() &&
>+      parentFrameType != nsGkAtoms::rubyFrame &&
>+      parentFrameType != nsGkAtoms::rubyContainerFrame) {

Please put in parens around the part after "||" so those of us who can't recall whether || has higher or lower precedence than && don't get confused.

>+  static const FrameConstructionData contFCData =

I'd prefer we call this "containerFCData".

>+  static const FrameConstructionData rubyFCData =
>+    FULL_CTOR_FCDATA(FCDATA_IS_RUBY_PART | FCDATA_SKIP_FRAMESET |
>+                     FCDATA_USE_CHILD_ITEMS,
>+                     &nsCSSFrameConstructor::ConstructRuby);

That shouldn't have FCDATA_IS_RUBY_PART, right?  The fcdata for containers and cells should, but not for the toplevel ruby frame itself.

>+    // wrapping for text and inline node when parent frame is <ruby>, <rbc> or <rtc>

What about other kids?  Block kids of a <ruby>, say?  Are any of the reftests testing that?  What did we decide the rendering should be in those cases?

>+      if ( item.mContent->IsNodeOfType(nsINode::eTEXT) &&
>+           !(item.mContent->TextIsOnlyWhitespace()) ||
>+           item.mIsAllInline )

Again, please parenthesize the part before the "||".  Also, you don't need the parens around |item.mContent->TextIsOnlyWhitespace()|.

But more imporrantly, shouldn't this use something like item.IsWhitespace()?  Otherwise, it looks like the wrong things will happen if the data in a textnode is changed dynamically after this process has run.  Something else we should add tests for.

This code will create one wrapper per child whitespace item.  Is that desirable?  For example, consider this markup:  <ruby>ABC<rt>abc</rt></ruby> where the "AB" is in one textnode but the "C" is in another one.  Your code will create two ruby-base boxes; one around the "AB" and one around the "C", whereas I think there should be only one such box here.

After this code runs, we should probably assert that aItems.HasRubyChild(), right?  I don't see how it can possibly be false at this point.

>+    if (!item.mIsRubyChild) {
>+      iter.Next();
>+    }
>+    else {

I think we should just |continue| after calling Next() in the |if| block there, and outdent the code in the |else| branch, since the |else| will no longer be needed.

That said, I'm still not sure why we want to leave block kids as-is here instead of wrapping them....  Is that on purpose?

>+      if (parentFrameType == nsGkAtoms::rubyFrame) {

So if parentFrameType == nsGkAtoms::rubyFrame, then we always walk the whole list, look for all the cells, wrap them, and return.  It seems like this |if| could just come before the whole current "wrap" loop.  So structure the code like this:

  if (parentFrameType == nsGkAtoms::rubyFrame) {
    // Loop to find and wrap the cells
    return NS_OK;
  }

  // The rest of the current wrap loop

I think that would be clearer.  Also it looks like if parentFrameType == nsGkAtoms::rubyContainerFrame then the wrap loop does nothing at all.  So that could become another check and early return right after the |parentFrameType == rubyFrame| block.  And then you'd just have a loop for the case of a non-ruby parent containing ruby kids.

>+        FrameConstructionItem* newBaseContItem = 

newBaseContainerItem

>+        FrameConstructionItem* newTextContItem = 

newTextContainerItem

>+          FrameConstructionItem& itemCell = cellIter.item();

I'd call the variable cellItem.

>+          else if (cellDisplay->mDisplay == NS_STYLE_DISPLAY_RUBY_BASE || 
>+                   itemCell.mTag == nsCSSAnonBoxes::rubyBase) {

You shouldn't need the tag check there now that we have the right styles for those anon boxes in ua.css.

>+        // insert wrapping container into last.

  "Insert the wrapping containers at the end of the child list"

>+        (newBaseContItem->mChildItems.IsEmpty()) ? 
>+          delete newBaseContItem: iter.InsertItem(newBaseContItem);
>+        (newTextContItem->mChildItems.IsEmpty()) ? 
>+          delete newTextContItem: iter.InsertItem(newTextContItem);

I would prefer actual if/then/else here to make the control flow clearer.

>+          if (endIter.item().mIsRubyChild) {
>+            endIter.Next();
>+
>+          }
>+          else {
>+            break;
>+          }

I would reverse the test like so:

  if (!endIter.item().mIsRubyChild) {
    break;
  }
  endIter.Next();

>+        FrameConstructionItem* newRubyItem = 
>+          new FrameConstructionItem(&rubyFCData, parentContent,
>+                                    nsCSSAnonBoxes::ruby, item.mNameSpaceID,
>+                                    nsnull, rubyStyle.forget(), PR_TRUE);
>+        iter.AppendItemsToList(endIter, newRubyItem->mChildItems);
>+        iter.InsertItem(newRubyItem);

First of all, newRubyItem needs to set things like mIsAllInline, mHasInlineEnds, and so forth.

Second, I'm not sure whether this does the right thing with whitespace.  Consider this markup:

  ABC<rb>D</rb> <rt>d</rt>EFG.

The code here will turn that into:

  ABC<ruby><rb>D</rb></ruby> <ruby><rt>d</rt></ruby>EFG.

whereas I'd think we want a single <ruby> containing both the <rb> and the <rt> here, right?  In practice, I would expect a newline there, by the way, not an actual space character, for practical uses of this stuff.  So I do expect this situation to appear.

>+nsCSSFrameConstructor::ConstructRuby(nsFrameConstructorState& aState,
>+nsCSSFrameConstructor::ConstructRubyContainer(nsFrameConstructorState& aState,

These are completely identical except for the exact NS_New* function they call to create the frame right?

Please file a followup bug on me to think about how to support this without quite so much code duplication.

Both of these need a ConstructViewForFrame call after the InitAndRestoreFrame, I think.

>+nsCSSFrameConstructor::ConstructRubyCell(nsFrameConstructorState& aState,

Again, need ConstructViewForFrame in here for the ruby cell itself.

>+  cellInnerFrame = NS_NewBlockFrame(mPresShell, innerPseudoStyle);

This should probably be NS_NewBlockFormattingContext.

>@@ -5292,16 +5588,19 @@ nsCSSFrameConstructor::AddFrameConstruct
>+  } else if (bits & FCDATA_IS_RUBY_PART) {
>+    item->mIsRubyChild = PR_TRUE;
>+    item->mIsBlock = PR_FALSE;

You need to set item->mHasInlineEnds and item->mIsAllInline correctly here as well.  I assume that a <ruby> inside a <span> shouldn't do anything weird to the <span>'s rendering?  If so, you probably want to just set both mHasInlineEnds and mIsAllInline to true.

>   if (item->mIsAllInline) {
>     aItems.InlineItemAdded();
>   } else if (item->mIsBlock) {
>     aItems.BlockItemAdded();
>-  }
>+  } else if (item->mIsRubyChild) {
>+    aItems.RubyChildItemAdded();
>+  }

This looks wrong.  The ruby bit here needs to be unconditional, since mIsAllInline should be true for ruby kids per above.

>@@ -11187,16 +11493,27 @@ nsCSSFrameConstructor::WipeContainingBlo
>+  // Situation #4 is a case when aItems has ruby children,

This is situation #3, I think.  There is no existing situation #3.

>+  // or parentFrameType is ruby frame or container frame
>+  // because text node might be wrapped <rb>.

  "or the parent is a ruby frame or ruby container frame"

This is a bit more eager to reframe than it _really_ needs to be, but it's nice and simple and I would expect that it would be very rare to have long text runs inside <ruby>, right?

>+nsCSSFrameConstructor::FrameConstructionItemList::
>+  return (mRubyChildCount)? PR_TRUE: PR_FALSE;

How about |return mRubyChildCount != 0;|?

>+nsCSSFrameConstructor::FrameConstructionItemList::
>+Iterator::DeleteItem()

This is unused.  Please don't add it.

>+++ b/layout/base/nsCSSFrameConstructor.h
>+#define FCDATA_IS_RUBY_PART 0x80000

This needs to be documented.

>+    inline PRBool HasRubyChild();

Just put the implementation of that method right here; there's no reason to put it in the C++.

>+      void DeleteItem();

Again, not used.

>+    PRUint32 mRubyChildCount;

This should probably be documented as well; it's not obvious what "ruby child" means here..

>+    // Whether this item is ruby child frame item.
>+    PRPackedBool mIsRubyChild;

Again, worth clarifying what "ruby child" means (just means FCATA_IS_RUBY_PART is set, right?).

I'm still reading over the frame implementations, but one question I had up front.  It looks like ruby can't wrap across lines, right?  So if the text inside it is wider than the block it's in we'll just get weird wrapping inside the cells?

That's probably OK (and is what webkit does), but I wanted to make sure that was the plan, not an accident.
Attached patch css-ruby property patch v6 (obsolete) — Splinter Review
Hello, 

I've been having a rough time with the earthquake, but I'm fine. 
Just have to hope the people in 東北 (Tohoku) are O.K.

For now, I'm attaching the new css prop patch.
Thank you for reviewing the layout code.
I will work on it now.

Regards,
Hajime.
Attachment #516826 - Attachment is obsolete: true
Comment on attachment 520159 [details] [diff] [review]
css-ruby property patch v6

Good to hear you're ok!  If there's anything I can do to help (other than the obvious issue of finishing up review of the layout patch), please let me know!

This patch is still missing -moz-ruby-span in property_database.js....
Attached patch css-ruby property patch v6.1 (obsolete) — Splinter Review
I've attached the css prop patch fixed where you pointed out.
I'm sorry, I misunderstood where you said "You do need to", for "You do not need to"...

Regards,
Hajime.
Attachment #520159 - Attachment is obsolete: true
Attachment #520420 - Flags: review?(bzbarsky)
(In reply to comment #111)
Hello,

> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> >@@ -4436,17 +4452,31 @@ nsCSSFrameConstructor::FindDisplayData(c
> It looks like this is not written on top of the css property patch, right?
> How was this tested, then?
I'm sorry, I couldn't understand what the problem is. 
I added the fallback code on css patch for testing only css patch.
There is original code of FindDisplayData for Ruby on the layout patch instead.
What should I do?

> >+// wrapping for text and inline node when parent frame is <ruby>, <rbc> or <rtc>
> What about other kids?  Block kids of a <ruby>, say?  Are any of the reftests
> testing that?  What did we decide the rendering should be in those cases?
I think that it is better to ignore block element inside <ruby>.
But other browsers don't ignore it and IE and Webkit has different rendering in this case.
I would like to send mail to www-style about this problem.

> But more imporrantly, shouldn't this use something like item.IsWhitespace()? 
> Otherwise, it looks like the wrong things will happen if the data in a textnode
> is changed dynamically after this process has run.  Something else we should
> add tests for.
Could you give me any specific case when such a thing occur?

> This code will create one wrapper per child whitespace item.  Is that
> desirable?  For example, consider this markup:  <ruby>ABC<rt>abc</rt></ruby>
> where the "AB" is in one textnode but the "C" is in another one.  Your code
> will create two ruby-base boxes; one around the "AB" and one around the "C",
> whereas I think there should be only one such box here.
It seems like not dividing "AB" and "C" in the case that you said.
Oh, do you mean when dynamically appending "C"? 

> That said, I'm still not sure why we want to leave block kids as-is here
> instead of wrapping them....  Is that on purpose?
It is for ignoring block elements.
So If we don't need to ignore block element, this leaving wouldn't be needed.

> >+nsCSSFrameConstructor::ConstructRuby(nsFrameConstructorState& aState,
> >+nsCSSFrameConstructor::ConstructRubyContainer(nsFrameConstructorState& aState,
> These are completely identical except for the exact NS_New* function they call
> to create the frame right?
> Please file a followup bug on me to think about how to support this without
> quite so much code duplication.
Should I make new bug for solving this duplication?

> >   if (item->mIsAllInline) {
> >     aItems.InlineItemAdded();
> >   } else if (item->mIsBlock) {
> >     aItems.BlockItemAdded();
> >-  }
> >+  } else if (item->mIsRubyChild) {
> >+    aItems.RubyChildItemAdded();
> >+  }
> This looks wrong.  The ruby bit here needs to be unconditional, since
> mIsAllInline should be true for ruby kids per above.
Is the code written below how you want it?
if (item->mIsAllInline) {
  aItems.InlineItemAdded();
} else if (item->mIsBlock) {
  aItems.BlockItemAdded();
}
if (item->mIsRubyChild) {
  aItems.RubyChildItemAdded();
}

> This is a bit more eager to reframe than it _really_ needs to be, but it's nice
> and simple and I would expect that it would be very rare to have long text runs
> inside <ruby>, right?
Yes, in my experience <ruby> has 5~6 characters at most.

> Again, worth clarifying what "ruby child" means (just means FCATA_IS_RUBY_PART
> is set, right?).
I think "RubyPart" is better and more clear than "RubyChild".
I'm changing some stuff about "RubyChid" and added some comments to explain it.

> I'm still reading over the frame implementations, but one question I had up
> front.  It looks like ruby can't wrap across lines, right?  So if the text
> inside it is wider than the block it's in we'll just get weird wrapping inside
> the cells?
Ruby could wrap across these lines, but I heard from fantasai that it is very difficult to implement this function.
What do you think...?

And I post mail to www-style about wrapping inside cells.


Regards,
Hajime.
> What should I do?

Make it so that the layout patch can be applied to a tree which already has the CSS patch in it.  That is, the part of the layout patch that handles the FCData for the new display types would modify the lines added in the CSS patch.  The simplest way to do this is to commit the css patch locally and then apply the layout patch on top (and fix merge issues).  Using mq can make this a bit easier if you want to and aren't using it already.

> I would like to send mail to www-style about this problem.

Sounds good.  Please do.  For our initial implementation, do we want to just hide them, then?

> Could you give me any specific case when such a thing occur?

Sure.  Say I have markup like this (notice the space inside the <span>):

  <span style="display: -moz-ruby" id="x"> </span>
  <script>
    document.body.offsetWidth;
    document.getElementById("x").firstChild.data = "Now there is text";
  </script>

> Oh, do you mean when dynamically appending "C"? 

Yes.

> Should I make new bug for solving this duplication?

Yes, please.  That's what "followup bug" means.

> Is the code written below how you want it?

Yes.

> I'm changing some stuff about "RubyChid" and added some comments to explain
> it.

Sounds good.

> What do you think...?

I agree with fantasai.  We can worry about it later if we want; for now what you have is fine.

Sorry for the lag on the rest of the review; trying to finish it today.
Comment on attachment 520420 [details] [diff] [review]
css-ruby property patch v6.1

r=me.  dbaron wanted to give this a once-over.
Attachment #520420 - Flags: superreview?(dbaron)
Attachment #520420 - Flags: review?(bzbarsky)
Attachment #520420 - Flags: review+
Comment on attachment 520420 [details] [diff] [review]
css-ruby property patch v6.1

Actually, there is one more problem, based on reading the layout patch.  Dynamic changes to the new properties aren't handled correctly, since there are no changes to nsStyleVisibility::CalcDifference, right?
Attachment #520420 - Flags: review+ → review-
Comment on attachment 517990 [details] [diff] [review]
ruby-frame layout patch v5

>+++ b/layout/generic/nsRubyCellFrame.cpp
>+nsRubyCellFrame::AlignCell(nscoord x,

There should be some documentation for this method.  What do x and width mean?

>+  if (!childBlock) {

That can't happen, right?  Just take the check out.

>+  SetRect(newRect);

There should be invalidation happening somewhere here, right?  Or is that handled elsewhere?

>+  childBlock->SetPosition(blockPoint);

Or maybe this is where we need the invalidation?  In any case, I'd think we
need to invalidate _somewhere_ here.

>+nsRubyCellFrame::Reflow(nsPresContext*           aPresContext,

>+  nsresult rv = NS_OK;

This is unused except for returning it at the end.  Just return NS_OK at the
end instead?

>+  nsHTMLReflowMetrics childMS(aMetrics.mFlags);

childMetrics is a better variable name here.

>+  aMetrics.width = childMS.width;
>+  aMetrics.height = childMS.height;

Need to set aMetrics.ascent as well.  Also, don't we need to set overflow areas as needed?

nsRubyCellFrame::Reflow should be NS_IMETHODIMP, right?  And NS_IMETHOD in the header.

>+nsRubyCellFrame::DestroyFrom(nsIFrame* aDestructRoot)
>+{
>+  nsHTMLContainerFrame::DestroyFrom(aDestructRoot);

Why do you need to implenent this at all, then?

>+++ b/layout/generic/nsRubyCellFrame.h
>+class nsRubyCellFrame : public nsHTMLContainerFrame

I don't see any provisions here for implementing shrink-wrap sizing (GetPrefWidth or GetMinWidth).  There's also no ComputeSize or ComputeAutoSize implementation or anything, but maybe the one you're inheriting is ok...

How does a cell figure out how wide it should be, exactly?

The explicit positioning in the AlignCell code makes it look like relative positioning on ruby cells doesn't work.  Is that correct?

This frame should probably implement GetContentInsertionFrame, right?  I'm a little surprised that dynamic insertion tests passed without that.

Why are ruby cells excluding ignorable whitespace?  That seems very wrong to me.  There should be a test for this; something with three text nodes under a ruby cell, containing the text "a", " ", "b" respectively.  If I read this right, the space won't show up with your patch.

>diff --git a/layout/generic/nsRubyContainerFrame.cpp b/layout/generic/nsRubyContainerFrame.cpp
>+nsRubyContainerFrame::Reflow(nsPresContext*           aPresContext,

Yuu need to set aStatus somewhere in here.  Also, need to set ascent on the metrics.  And there's not point having a separate ReflowChildren is there?  Just inline it into Reflow.

Again, Reflow should be NS_IMETHODIMP.

>+nsRubyContainerFrame::ReflowChildren(nsPresContext*           aPresContext,
>+  for (; childFrame; ) {

  while (childFrame) {

Or just use:

  for (nsIFrame* childFrame = mFrames.FirstChild();
       childFrame;
       childFrame = childFrame->GetNextSibling())

>+    nsHTMLReflowState childRS = 
>+      nsHTMLReflowState(aPresContext, aReflowState, childFrame, availSize);
>+    nsHTMLReflowMetrics childMS = 
>+      nsHTMLReflowMetrics(aMetrics.mFlags);

Shouldn't have those assignments; just construct the things you want directly.  Again, childMetrics makes more sense than childMS, in my opinion.

>+    ReflowChild(childFrame, aPresContext, childMS, childRS,
>+                aMetrics.width, childPt.y, NS_FRAME_NO_MOVE_FRAME, aStatus);

Why is this using NS_FRAME_NO_MOVE_FRAME?

>+    FinishReflowChild(childFrame, aPresContext, &childRS, childMS,
>+                      aMetrics.width, childPt.y, 0);

But this doesn't?

>+nsRubyContainerFrame::GetFirstCell()
>+nsRubyContainerFrame::GetLastCell()

These don't seem to be called anywhere.  Take them out?

>+nsRubyContainerFrame::GetCellAt(PRInt32 index)
>+  int i=0;

Spaces around the '=' please.

>+    const nsStyleDisplay* styleDisp = cell->GetStyleDisplay();
>+    PRInt32 span = styleDisp->mRubySpan;
>+
>+    if (i==index) 

This if/else could happen before the GetStyleDisplay, right?  That seems faster to me...

>+nsRubyContainerFrame::AppendFrames(nsIAtom*        aListName,

Why do you need to implement this, exactly?

>+    FrameNeedsReflow(rubyFrame, nsIPresShell::eTreeChange,
>+                     NS_FRAME_IS_DIRTY);

And in particular, why do you need to do that?  Why doesn't just asking for a DIRTY_CHILDREN reflow on yourself work?

>+nsRubyContainerFrame::InsertFrames(nsIAtom*        aListName,

Likewise.

>+nsRubyContainerFrame::RemoveFrame(nsIAtom*        aListName,

And likewise.

>+nsRubyContainerFrame::DestroyFrom(nsIFrame* aDestructRoot)

Why does this need to be implemented?

>+++ b/layout/generic/nsRubyContainerFrame.h
>+  NS_METHOD Reflow(nsPresContext*           aPresContext,

NS_IMETHOD.

I still need to sort through nsRubyFrame, but it looks like it doesn't really do intrinsic sizing (and it would need to implement the inline API for this, note).  How does something like an auto-width float with just <ruby> inside it behave?  It should end up the same width as the <ruby>, but I doubt that happens...
Actually, no.  You don't need to implement the inline intrinsic sizing API; nsFrame handles that for you if you implement GetMinWidth/GetPrefWidth.  But I think you do need to implement GetMinWidth/GetPrefWidth....

A question.  Should width/height styles apply to ruby cells and ruby containers?  (Note that they don't apply to normal inline frames, but do apply to table cells/rows.)
Comment on attachment 520420 [details] [diff] [review]
css-ruby property patch v6.1

>+  case eCSSProperty_ruby_span:
>+    return ParseVariant(aValue, VARIANT_INTEGER, nsnull);

Presumably this should be VARIANT_HI, and the error should cause you to fail a bunch of tests in layout/style/test/test_inherit_storage.html and test_initial_storage.html (and probably also test_inherit/initial_computation).

That said, the nsCSSParser.cpp and nsCSSPropList.cpp changes will look a good bit different on current trunk; I'd sort of like to see the merged version.
Comment on attachment 520420 [details] [diff] [review]
css-ruby property patch v6.1


>@@ -1374,16 +1378,17 @@ struct nsStyleDisplay {
>   PRUint8 mFloats;              // [reset] see nsStyleConsts.h NS_STYLE_FLOAT_*
>   PRUint8 mBreakType;           // [reset] see nsStyleConsts.h NS_STYLE_CLEAR_*
>   PRPackedBool mBreakBefore;    // [reset]
>   PRPackedBool mBreakAfter;     // [reset]
>   PRUint8 mOverflowX;           // [reset] see nsStyleConsts.h
>   PRUint8 mOverflowY;           // [reset] see nsStyleConsts.h
>   PRUint8 mResize;              // [reset] see nsStyleConsts.h
>   PRUint8   mClipFlags;         // [reset] see nsStyleConsts.h
>+  PRInt32 mRubySpan;

Also, please add the "// [reset]" comment as for other properties.
Comment on attachment 520420 [details] [diff] [review]
css-ruby property patch v6.1

You also need to implement the properties in nsComputedDOMStyle.cpp.  This patch has no changes to that file, but needs some.

Other than that (including my two comments above), and Boris's comments (particularly changes to the CalcDifference method), this looks fine to me, but I'm disturbed that it contains two mistakes that cause it to fail tests.  (There might be other pieces missing that I didn't notice.)

Could you merge on top of current mozilla-central, address these issues, and then make sure that all the mochitests in layout/style/test pass?  (You can do this by running "TEST_PATH=layout/style/test/ make mochitest-plain" in your objdir.)

Since I'd like to look again at the patch after those revisions, marking superreview-.
Attachment #520420 - Flags: superreview?(dbaron) → superreview-
Attached patch css-ruby property patch v7 (obsolete) — Splinter Review
Hello,

I fixed some stuff with css prop patch and merged with the top of mozilla-central. (DoGetMozRuby***() and CalcDifference())
And I tried to pass the test, but there were some errors.

1: First of all, there is a problem with ruby-span value.
Now ruby-span has "none" and Integer(1, 2, ...) value, I think this is causing these errors.
Is it possible to mix these values, "none" and Integer?
Or should I fixed "none" value to merely 1?
However in css3 ruby spec[1] ruby-span should use 'attr(x)' which is not supported on mozilla.
I think it is better for mozilla to support both "none", attr(x) and Integer values.
what is your opinion about this?

*** test_compute_data_with_start_struct.html
failed | initial and other values of -moz-ruby-span are different -
didn't expect "1", but got it
failed | initial and other values of -moz-ruby-span are different -
didn't expect "1", but got it

*** test_inherit_computation.html
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it

*** test_initial_computation.html
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it

*** test_value_cloning.html
failed | serialization should be nonempty for -moz-ruby-span: none -
didn't expect "", but got it

*** test_value_computing.html
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it
failed | should be testing with values that compute to different
things for '-moz-ruby-span' - didn't expect "1", but got it
failed | should not get initial value for '-moz-ruby-span:1' on
elementn. - didn't expect "1", but got it
failed | should not get initial value for '-moz-ruby-span:1' on
elementf. - didn't expect "1", but got it

*** test_value_strage
failed | setting 'none' on '-moz-ruby-span' - didn't expect "", but got it



2: There were errors like below especially around font properties.
I don't think css patch has anything to do with this error.
Do you have any idea to solve this problem?

*** test_bug401046.html
failed | at min font size 7, 4px should compute to 7px - got "4px",
expected "7px"
failed | at min font size 18, 4px should compute to 18px - got "4px",
expected "18px"
failed | at min font size 18, 12px should compute to 18px - got
"12px", expected "18px"

*** test_ident_escaping.html
failed | escaping done only where needed - got "-font-family_72756",
expected "Ha*kon\\ Lie"

***test_initial_computation.html
failed | -moz-initial should cause initial value for 'font' - got
"normal ; normal ; 400 ; 16px ; 20px ; serif ; normal ; none ; normal
; normal", expected "normal ; normal ; 400 ; 16px ; 19.2px ;
sans-serif ; normal ; none ; normal ; normal"
failed | -moz-initial should cause initial value for 'font' - got
"normal ; normal ; 400 ; 16px ; 20px ; serif ; normal ; none ; normal
; normal", expected "normal ; normal ; 400 ; 16px ; 19.2px ;
sans-serif ; normal ; none ; normal ; normal"
failed | should be testing with values that compute to different
things for 'font-family' - didn't expect "sans-serif", but got it
failed | should be testing with values that compute to different
things for 'font-family' - didn't expect "sans-serif", but got it
failed | -moz-initial should cause initial value for 'font-family' -
got "serif", expected "sans-serif"
failed | -moz-initial should cause initial value for 'font-family' -
got "serif", expected "sans-serif"
failed | -moz-initial should cause initial value for 'line-height' -
got "22px", expected "22.8px"
failed | -moz-initial should cause initial value for 'line-height' -
got "22px", expected "22.8px"

*** test_parse_ident.html ***
failed | 'a`' is a valid CSS identifier
... (slimilar errors) ...

*** test_value_computing.html
failed | should get initial value for 'font:medium serif' - got
"normal ; normal ; 400 ; 16px ; 20px ; serif ; normal ; none ; normal
; normal", expected "normal ; normal ; 400 ; 16px ; 19.2px ;
sans-serif ; normal ; none ; normal ; normal"
failed | should get initial value for 'font:medium serif' - got
"normal ; normal ; 400 ; 16px ; 20px ; serif ; normal ; none ; normal
; normal", expected "normal ; normal ; 400 ; 16px ; 19.2px ;
sans-serif ; normal ; none ; normal ; normal"
failed | should be testing with values that compute to different
things for 'font-family' - didn't expect "sans-serif", but got it
failed | should be testing with values that compute to different
things for 'font-family' - didn't expect "sans-serif", but got it
failed | should get initial value for 'font-family:serif' - got
"serif", expected "sans-serif"
failed | should get initial value for 'font-family:serif' - got
"serif", expected "sans-serif"
failed | should not get initial value for 'font-family:sans-serif' on
elementn. - didn't expect "sans-serif", but got it
failed | should not get initial value for 'font-family:sans-serif' on
elementf. - didn't expect "sans-serif", but got it
failed | should get initial value for 'line-height:normal' - got
"22px", expected "22.8px"
failed | should get initial value for 'line-height:normal' - got
"22px", expected "22.8px"

*** test_transitions_per_property.html
Passed: 33748
Failed: 354
Todo: 0
failed | transitions not supported for property -moz-ruby-span value
none - got "3", expected "1"
failed | radius-valued property border-bottom-left-radius: computed
value before transition - got "0px", expected "3px"
failed | radius-valued property border-bottom-left-radius:
interpolation of radius - got "0px", expected "6px"
....(similar errors)...


Regards,
Hajime.
Attachment #520420 - Attachment is obsolete: true
Attachment #523293 - Flags: superreview?(dbaron)
Attachment #523293 - Flags: review?(bzbarsky)
> and merged with the top of mozilla-central.

As of several few days ago, right?  You're going to need to merge again with the patch from bug 645620, which removed nsCSSStruct 3 days ago....

Luckily, this shouldn't be too bad.

> failed | initial and other values of -moz-ruby-span are different -
> didn't expect "1", but got it

That's because the test says |initial_values: [ "none" ]| for -moz-ruby-span, while the code in rulenode and style struct has "1" as the initial value.  Further, the parser will only parse integer ruby-span values (should it limit to _positive_ integer values, by the way?), given the code in the patch.  This explains the "" vs "none" error you got in test_value_cloning, say.

I don't know what the best thing to support for ruby-span is, but I would suggest just making integer work for now and deferring the rest of the work, whatever it is, to a followup bug.  In particular, making attr() work will be a nontrivial undertaking.

> 2: There were errors like below especially around font properties.

That's .... odd.  Do you see those without this patch applied?
One additional comment (in addition to Boris's) on attachment 523293 [details] [diff] [review]:


>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h

>+CSS_PROP_VISIBILITY(
>+    -moz-ruby-align,
>+    _moz_ruby_align,
>+    MozRubyAlign,
>+    CSS_PROPERTY_PARSE_VALUE,
>+    Display,
>+    mRubyAlign,
>+    VARIANT_HK,
>+    kRubyAlignKTable,
>+    CSS_PROP_NO_OFFSET,
>+    eStyleAnimType_None)

The third parameter to this macro, instead of being:
+    MozRubyAlign,
should be:
+    CSS_PROP_DOMPROP_PREFIXED(RubyAlign),

Same for the other properties.

Then, in nsRuleNode.cpp, you can drop the "Moz" from the GetValueFor*
function names.


I see a bunch of bugs in ruby-span handling that are likely related to the test failures:

First, you need the parser to accept 'none'; so you need VARIANT_HI | VARIANT_NONE rather than just VARIANT_HI (or you could write it out as VARIANT_INHERIT | VARIANT_INTEGER | VARIANT_NONE).

Second, you should have both "none" and "1" listed in the initial_values section for ruby-span in property-database.js.  The initial_values line should contain all values that compute to the initial value.

Third, I think your call to SetDiscrete for ruby-span is incorrect:  you should be passing 1 for aInitialValue and for aNoneValue, and 0 for the other parameters, which don't matter.
When I run mochitests with LC_ALL=ja_JP.UTF-8 LANG=ja_JP.UTF8, I see the errors from comment 125 in test_bug401046.html, test_initial_computation.html, and test_value_computation.html.  We should get a bug filed on that.  I don't see the others, though.  (Other than test_ident_escaping, the remaining errors lead me to wonder if you've recompiled (with the corresponding source) since you updated the tests.)
(In reply to comment #128)
> When I run mochitests with LC_ALL=ja_JP.UTF-8 LANG=ja_JP.UTF8, I see the errors
> from comment 125 in test_bug401046.html, test_initial_computation.html, and
> test_value_computation.html.  We should get a bug filed on that.  I don't see
> the others, though.  (Other than test_ident_escaping, the remaining errors lead
> me to wonder if you've recompiled (with the corresponding source) since you
> updated the tests.)

Bug 416581 for test_bug401046.html
Bug 646882 for test_initial_computation.html and test_value_computation.html
Hello,

> should it limit to _positive_ integer values, by the way?
Yes, I think that it should be limited to only _positive_ integers. 
I would like to give something like VARIANT_POSITIVE to
-moz-ruby-span, do you think it's possible?

If this limitation is *impossible*, I think we should erase '-1' as
invalid value on property_database.js.
And we would check the value whether it is positive or negative in ruby-span calculation.
What do you think?

Regards,
Hajime.
> I would like to give something like VARIANT_POSITIVE 

Just change CSS_PROPERTY_PARSE_VALUE to:

  CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_POSITIVE_NONZERO

in the relevant entry in nsCSSPropList.h.  Then you can add 0 and -1 as invalid values in property_database.js.

If 0 should be a valid value, then you want CSS_PROPERTY_VALUE_NONNEGATIVE instead of CSS_PROPERTY_VALUE_POSITIVE_NONZERO.
Attached patch css-ruby property patch v8 (obsolete) — Splinter Review
Hello,
I've attached the new css prop patch.

I tried to pass tests on /layout/style/test
I think that there are no errors around this patch, but I have some errors like below in the test.

Confirmed bug (thank you David-san)
* bug401046.html, 3 failed.
* test_initial_computation.html, 8 failed.
* test_value_computation.html, 10 failed.

Unconfirmed bug 
* test_ident_escaping.html, 1 failed.
---- failed | escaping done only where needed - got "-font-family_72756", expected ""Håkon\\ Lie"
* test_parse.ident.html, 31 failed.
failed | 'à' is a valid CSS identifier
failed | 'á' is a valid CSS identifier
.... (same error like the above) ....

I think these bug have something to do with character composition.


Regards,
Hajime.
Attachment #523293 - Attachment is obsolete: true
Attachment #524945 - Flags: superreview?(dbaron)
Attachment #524945 - Flags: review?(bzbarsky)
Attachment #523293 - Flags: superreview?(dbaron)
Attachment #523293 - Flags: review?(bzbarsky)
Hello,
I'm sorry for my late reply.

(In reply to comment #116)
> > I would like to send mail to www-style about this problem.
> Sounds good.  Please do.  For our initial implementation,
> do we want to just hide them, then?
Yes, I agree with you but we should think that later on...

> Could you give me any specific case when such a thing occur?
> Sure.  Say I have markup like this (notice the space inside the <span>):
>   <span style="display: -moz-ruby" id="x"> </span>
>  <script>
>     document.body.offsetWidth;
>    document.getElementById("x").firstChild.data = "Now there is text";
>  </script>
In this case, two <rb> is generated in my patches you pointed out.
I agree that this behavior isn't good.
So I'm going to change it to merge two text nodes and make it generate
only one <rb>

(In reply to comment #116)
> >+  SetRect(newRect);
> There should be invalidation happening somewhere here, right?  Or is that
> handled elsewhere?
> >+  childBlock->SetPosition(blockPoint);
> Or maybe this is where we need the invalidation?  In any case, I'd think we
> need to invalidate _somewhere_ here.
I don't understand 'invalidate' which you mentioned.
I found some code like nsTableFrame::InvalidateFrame(), but I don't quite get it.
What do you mean by _invalidate_ in this context?

> Also, don't we need to set overflow areas as needed?
How do I know whether overflow is needed or not?
Should I check all elements of metrics.overflowAreas?

> I don't see any provisions here for implementing shrink-wrap sizing
> (GetPrefWidth or GetMinWidth).  There's also no ComputeSize or ComputeAutoSize
> implementation or anything, but maybe the one you're inheriting is ok...
Does this have anything to do with Comment 121?

> How does a cell figure out how wide it should be, exactly?
In my implementation,
Firstly, the width of the ruby cell is same value with the width of child text node.
Secondly, the width is expanded in nsRubyFrame::AlignRubyCellFrames() because the width should be equal to max width of cell in same column.
Have I answered your question?

> The explicit positioning in the AlignCell code makes it look like relative
> positioning on ruby cells doesn't work.  Is that correct?
Yes, what are the possibilities of relative position in ruby cell?

> Why are ruby cells excluding ignorable whitespace?  That seems very wrong to
> me.  There should be a test for this; something with three text nodes under a
> ruby cell, containing the text "a", " ", "b" respectively.  If I read this
> right, the space won't show up with your patch.
Yes, you are right.

<ruby>
 <rb>AAA</rb>
 <rt>aaa</rt>
</ruby>

In the case above, if not ignoring the white space, I think that frames
are constructed like below.
I think the below behavior is unpreferable, so I ignored the white space.
What do you think about this?

RubyFrame
 RubyContainer
   RubyCell[   (space)]
   RubyCell[AAA]
   RubyCell[   (space)]
 RubyContainer
   RubyCell[aaa]

(In reply to comment #121)
> A question.  Should width/height styles apply to ruby cells and ruby
> containers?  (Note that they don't apply to normal inline frames, but do apply
> to table cells/rows.)
Please give me some time to think about it.


Regards,
Hajime.
> What do you mean by _invalidate_ in this context?

Tell the painting system that it needs to repaint an area.  Whenever frames are moved or resized something needs to tell the painting stuff that the difference between the old and new area needs to be repainted...

> How do I know whether overflow is needed or not?

Well, any time you have kids with overflow areas sticking out of your rect, right?  I think we have some utility functions for doing the right things here, in particular ConsiderChildOverflow.

> Does this have anything to do with Comment 121?

Yes, comment 121 was a followup to this part of my comments.

> Firstly, the width of the ruby cell is same value with the width of child
> text node.

Defined how?  Note that the child textnode can wrap.

> Have I answered your question?

Not quite...

> Yes, what are the possibilities of relative position in ruby cell?

Well, it can just be relatively positioned by any offset.  It looks like the current implementation just doesn't support relative positioning of ruby cells.

> In the case above, if not ignoring the white space, I think that frames
> are constructed like below.

That's a different situation, right?  It seems to me that the key is to only skip whitespace at the start and end of the cell.  That's what tables do.
> Should width/height styles apply to ruby cells and ruby containers?

I would say no.
What do other UAs do there?
Blocks: 650616
Hello,

1:
About dynamically changing around text node.
I agree with you that there is some strange behavior which the two adjacent text nodes creating two <rb>, and skipping the white-space node between two text nodes.
I'm going change the part where wrapping text node like CreateNeededTablePseudos().
And after the above work, I will attach the new layout patch.

2:
About applying and defining width/height and width/height of ruby cell.
Ruby cell and ruby container should not be applied to width/height.
Ruby cell should never be divided (wrapped) into two lines.
The width/height of ruby cell is equals to child text node that doesn't allow into two lines, and ruby cell is expanded in
nsRubyFrame::AlignRubyCellFrames().
I think that these behavior is fine for ruby layout but I can't decide that it is possible in the view of the implementation and the
specification.


> What do other UAs do there?
I've tried to display the below markup in two browsers and attached
displayed image.
<ruby>
 <rb style='width: 100px; border:solid 1px black;'>AAA</rb>
 <rt style='width: 100px; border:solid 1px black;'>aaa</rt>
</ruby>

Regards,
Hajime.
(In reply to comment #132)
> * test_ident_escaping.html, 1 failed.
> * test_parse.ident.html, 31 failed.
That's bug 650653.
Comment on attachment 524945 [details] [diff] [review]
css-ruby property patch v8

nsCSSPropList.h:

>+    CSS_PROPERTY_PARSE_VALUE | CSS_PROPERTY_VALUE_POSITIVE_NONZERO,

Wrap this to two lines to match the style of the rest of the lines
that use |.

In all four macros, drop the _moz_ from the second parameter of the 
macro (and make the required changes elsewhere to eCSSProperty_* enum 
names).

nsStyleStruct.cpp:

nsStyleDisplay::CalcDifference needs to handle changes
in mRubySpan.


sr=dbaron with those changes
Attachment #524945 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #139)

> In all four macros, drop the _moz_ from the second parameter of the 
> macro (and make the required changes elsewhere to eCSSProperty_* enum 
> names).

Should I drop the _moz_ from _moz_ruby, _moz_ruby_base and so on?

Regards,
Hajime.
(In reply to comment #140)
> (In reply to comment #139)
> 
> > In all four macros, drop the _moz_ from the second parameter of the 
> > macro (and make the required changes elsewhere to eCSSProperty_* enum 
> > names).
> 
> Should I drop the _moz_ from _moz_ruby, _moz_ruby_base and so on?

Yes.  Remove the _moz_ from the values that are for internal use, and leave the -moz- for what gets parsed and serialized.
Comment on attachment 524945 [details] [diff] [review]
css-ruby property patch v8

With the issues from comment 139 addressed, r=me.

I'm still waiting on the next iteration of the layout patch, right?  Or are you waiting on me at this point?
Attachment #524945 - Flags: review?(bzbarsky) → review+
I'm Sorry for the delay submitting the new layout patch.
Now I'm implementing the text wrapping mentioned on Comment 137

I will update the layout patch by the end of this week.

Regards,
Hajime.
All good.  I wasn't trying to rush you; just wanted to make sure you weren't blocked on anything on my end!
Attached patch ruby reftest patch v3 (obsolete) — Splinter Review
I added some tests for checking whitespace processing on ruby.
In whitespace3.html I couldn't think of any code or logic which can
generate text nodes that get separated first.
On onload=doTest(), wouldn't text nodes get added one by one after Reflow?
Attachment #513375 - Attachment is obsolete: true
Attached patch css-ruby property patch v8.1 (obsolete) — Splinter Review
This is the new css patch fixed on Comment 139.
Attachment #524945 - Attachment is obsolete: true
Attached patch ruby frame layout patch v6 (obsolete) — Splinter Review
I fixed where was pointed out and implemented wrapping more than one text node.
I think that what you pointed out about dynamic appending/inserting
the text node is already solved.
Attachment #517990 - Attachment is obsolete: true
Attachment #517990 - Flags: review?(bzbarsky)
> In whitespace3.html I couldn't think of any code or logic which can
> generate text nodes that get separated first.

I'm not sure what you mean.

> On onload=doTest(), wouldn't text nodes get added one by one after Reflow?

To the DOM, yes.  To the frame tree they would be added all together, I think.  Depending on how things end up working exactly with lazy frame construction...

Should I be working on reviewing v6 of the layout patch, or is that still a work-in-progress?
Comment on attachment 530896 [details] [diff] [review]
css-ruby property patch v8.1

r=me on this; looks good!
Attachment #530896 - Flags: review+
Is there any chance of this making it for Firefox 6 before it branches?
> Should I be working on reviewing v6 of the layout patch, or is that still a
> work-in-progress?

Sorry, I forgot to check the review flag.
Could you review the new layout patch?

Regards,
Hajime.
Attachment #530897 - Flags: review?(bzbarsky)
Attached patch css-ruby property patch v8.2 (obsolete) — Splinter Review
After discussion with bz on IRC this is a version of css-ruby property patch v8.1 designed to land after my patch for bug 657143.  Only changes are updating the removed uuid in nsIDOMCSS2Properties.idl, and where the new CSS properties are listed in nsComputedDOMStyle.cpp (and some of the line #s changed).
Attachment #532790 - Flags: review?(bzbarsky)
Comment on attachment 532790 [details] [diff] [review]
css-ruby property patch v8.2

clear review flag on my patch since http://hg.mozilla.org/mozilla-central/rev/a756a95aaa31 means it no longer applies to the nsStyleStruct files cleanly, will post updated patch later
Attachment #532790 - Flags: review?(bzbarsky)
Attached patch css-ruby property patch v8.2.1 (obsolete) — Splinter Review
Updated for nsStyleStruct changes (-moz-orient)
Attachment #532790 - Attachment is obsolete: true
Attachment #533458 - Flags: review?(bzbarsky)
(In reply to comment #154)
Thank you for fixing my patch :-)

Regards,
Hajime.
Attachment #533458 - Flags: review?(bzbarsky) → review+
Comment on attachment 530897 [details] [diff] [review]
ruby frame layout patch v6

>+ * This function will check and wrap the child list (aItems) that is related to

I was actually looking for a specific description of the exact things that we validate in this function, not just non-exhaustive examples.  It's really hard to review code when I can't tell what it's _trying_ to do, so I've basically ended up having to try to reverse-engineer the desired behavior from the code.

I will post an interdiff against this patch of my comments on this code based on what I understood of it.  Please let me know whether those comments are correct.

>+nsCSSFrameConstructor::EnsureValidRubyStructure(nsFrameConstructorState& aState,
>+      FCItemIterator endIter(rbIter);

What is that the "end" of?

For that matter, what is rbIter?  It's really just our overall list iterator; just call it iter.

>+          if (isStart || trailingSpaces) {
>+            endIter.DeleteItemsTo(spaceEndIter);
>+          }

So now rbIter is dangling....

>+          if (isStart) {
>+            rbIter = endIter;
>+          }

And this only updates it sometimes.  Was the codepath when trailingSpaces && !isStart tested?

>+        FCItemIterator nextIter(endIter);
>+        nextIter.Next();
>+        if (endIter.item().mIsRubyPart ||
>+            nextIter.item().mIsRubyPart) {

And here nextIter may have run off the end of the list.  Was _that_ codepath tested?

Why does it matter whether nextIter is a ruby part at all?

>+      if (endIter.item().mIsRubyPart) {

And here endIter may be off the end of the list (in fact, that's the non-break loop termination condition for the previous loop!).

I'd really like to talk to you about this code in a more synchronous way; I think we can get it into shape pretty quickly if we don't have to deal with multi-week latencies on my part.  :(  Assuming you're in Tokyo time, that puts you 13 hours ahead of me.  So sometime either in the evening for me (after 10pm, so around mid-morning for you) or morning for me (so evening for you) would work best.  Can you do irc?  Skype?  Something else?  It's about to be a 3-day weekend here, but I would be happy to talk on Tuesday morning (your Tuesday evening) if that works for you.

If not, we can do e-mail, I guess, and just mail back and forth for a few days...

>+  FCItemIterator iter(aItems);

Please move this below the parentFrameType == nsGkAtoms::rubyFrame block, since we don't need it until them (see below).

>+  if (parentFrameType == nsGkAtoms::rubyFrame) {
...
>+    do {	
>+      FrameConstructionItem& item = iter.item();

There's really no point in this outer loop.  All this will do is advance |iter| to the first ruby part and then start looking for cells from there to the end of the list.... why not just look for cells to start with?  That is, remove |iter| altogether, just create the wrapper items, then initialize cellIter with aItems and start looking for cells.

>+        FrameConstructionItem& cellItem = cellIter.item();
>+        const nsStyleDisplay* cellDisplay = cellItem.mStyleContext->GetStyleDisplay();

One thing I should have noticed last time.  If !cellItem->mIsRubyPart we should skip it here, I think, because the frame it will get is NOT a ruby-cell frame.  Think something like <html:img style="display: ruby-text">....

>+        if (cellDisplay->mDisplay == NS_STYLE_DISPLAY_RUBY_TEXT) {
>+          cellIter.AppendItemToList(newTextContainerItem->mChildItems);

So this part makes sense: ruby-text goes into a ruby-text-container.

>+        else if (cellDisplay->mDisplay == NS_STYLE_DISPLAY_RUBY_BASE) {
>+          nsRefPtr<nsStyleContext> newBaseCellStyle =
>+            mPresShell->StyleSet()->
>+            ResolveAnonymousBoxStyle(nsCSSAnonBoxes::rubyBase, 
>+                                     newBaseContainerItem->mStyleContext);
>+          cellItem.mStyleContext = newBaseCellStyle;
>+
>+          cellIter.AppendItemToList(newBaseContainerItem->mChildItems);

But this part I don't get.  Why do you need that style context munging?  Is that a carryover from when we could have had things with the wrong display type here because the anonymous box style was wrong?  As far as I can tell, yes; just take it out.

>+  if (parentFrameType == nsGkAtoms::rubyContainerFrame) {
>+    return NS_OK;
>+  }
>+  else {

Just drop the else and outdent the body of it, since we've already returned if parentFrame is a ruby-container.

>+    // wrapping with ruby frame 

>+      FCItemIterator endIter(iter);
>+      do {
>+        if (!endIter.item().mIsRubyPart &&
>+            !endIter.item().IsWhitespace(aState)) {
>+          break;
>+        }
>+        endIter.Next();
>+      } while(!endIter.IsDone());

This is, unfortunately, not right.  The problem is that you could have a ruby-part followed by whitespace and then followed by some text, say.  In that situation, the whitespace should in fact render, but this code would wrap it in a ruby frame and then drop it when ensuring a valid ruby structure for that frame's kids later.  We should add a test for this.

If the goal of this code matches item 3 in the comment above this function in the interdiff I'll attach, then I can suggest some code for doing that.  Please let me know whether it does.

One last nit: EnsureValidRubyStructure always returns NS_OK; it should just return void instead of returning an nsresult.

>+++ b/layout/base/nsCSSFrameConstructor.h
>+  /* If FCDATA_IS_RUBY_PART is set, then the frame is related to ruby thing.
>+   * But this flag is not include nsRubyFrame itself. */

How about

  /* If FCDATA_IS_RUBY_PART is set, then the frame is an internal part of a
   * ruby frame (a ruby cell or ruby cell container).  The ruby frame itself
   * does not have this bit set. */

>+    // Whether this item is descendant frame of RubyFrame.
>+    PRPackedBool mIsRubyPart;

How about:

  // Whether this item is an item for an internal part of a ruby frame.

>+++ b/layout/generic/nsRubyCellFrame.cpp
>+nsRubyCellFrame::AlignCell(nscoord x,

This still needs documentation.

>+nsRubyCellFrame::Reflow(nsPresContext*           aPresContext,

Once you set aMetrics.width and aMetrics.height you need to do:

  ConsiderChildOverflow(aDesiredSize.mOverflowAreas, childFrame);
  FinishAndStoreOverflow(&aDesiredSize);

>+++ b/layout/generic/nsRubyContainerFrame.cpp
>+nsRubyContainerFrame::Reflow(nsPresContext*           aPresContext,

Again, you don't really need the separate ReflowChildren method....  If you do keep it, the return value should be void, since it never returns anything but NS_OK.

>+  ReflowChildren(aPresContext, aMetrics, aReflowState, aStatus);

You need to FinishAndStoreOverflow(&aMetrics) after that call.

>+nsRubyContainerFrame::ReflowChildren(nsPresContext*           aPresContext,

>+  while (childFrame) {
>+    FinishReflowChild(childFrame, aPresContext, &childRS, childMetrics,
>+                      aMetrics.width, childPt.y, 0);

You need a ConsiderChildOverflow call after this.

>+    aMetrics.ascent = childMetrics.ascent;

Wouldn't it make more sense to do:

  if (aMetrics.ascent < childMetrics.ascent)
    aMetrics.ascent = childMetrics.ascent;

and initialize aMetrics.ascent to 0 up front in Reflow()?  Otherwise your ascent will end up as the ascent of your last kid; using the max of the ascents makes more sense to me.

I'd still like to get my question about the use of NS_FRAME_NO_MOVE_FRAME for ReflowChild but not FinishReflowChild here answered.

>+nsRubyContainerFrame::GetCellAt(PRInt32 index)
>+    PRInt32 span = styleDisp->mRubySpan;

You don't really need the |span| temporary.

>+++ b/layout/generic/nsRubyFrame.cpp

I'd really like to see the first paragraph of comment 121 addressed (and a test of the auto-width float I mention at the end of comment 120 added).

More comments on the rest on Tuesday....
Attachment #535826 - Flags: feedback?(hajime.shiozawa)
These tests show the shrink-wrapping problem I brought up (the float in the first test should be the intrinsic width of the <ruby>, not 0px wide).  They also show another problem: a <ruby> child of a 0-width block doesn't show up at all.  In particular, the second and third tests should look identical, I think, but they don't.
Comment on attachment 530897 [details] [diff] [review]
ruby frame layout patch v6

>+++ b/layout/generic/nsRubyFrame.cpp
>+AddChild(nsIFrame* aParent, nsIFrame* aFrame)

This is unused.

>+  mIsNeedOverhang(PR_FALSE),

So is this.

>+nsRubyFrame::Reflow(nsPresContext*           aPresContext,

This needs  FinishAndStoreOverflow and aMetrics.UnionOverflowAreasWithDesiredBounds() near the end.

Also, this should be NS_IMETHODIMP

>+nsRubyFrame::ReflowContainerFrame(nsIFrame*                containerFrame,

This never returns anything other than NS_OK, so should be a void method.

There should be a ConsiderChildOverflow call near the end here.

>+nsRubyFrame::IdentifyRubyContainerFrames()

This is called on every reflow; I think it would be better if it cached whether ie needs to relookup the containers.

>+  nsIFrame* childFrame = mFrames.FirstChild();
>+  for (; childFrame; ) {

Better to use a FrameList::Enumerator and have it advance in the for header itself, so you can use continue in the loop.

>+    if (styleDisplay->mDisplay == NS_STYLE_DISPLAY_RUBY_BASE_CONTAINER ||
>+        pseudo == nsCSSAnonBoxes::rubyBaseContainer) {

The pseudo check is not needed, but what _is_ needed is a check that childFrame is a ruby container frame.

And the members should be nsRubyContainerFrame*.

Also, in nsRubyContainerFrame::GetCellAt a check is needed that |cell| is in fact an nsRubyCellFrame, and GetCellAt should return nsRubyCellFrame*

>+nsRubyFrame::AlignRubyCellFrames(nsHTMLReflowMetrics& aMetrics)

Again, NS_ARRAY_LENGTH.  And those nsTArrays should probably be nsAutoTArray.

And as I said earlier, this stuff needs to implement the intrinsic width API.

>+nsRubyFrame::GetColCount()
>+  for (int i = 0; i < 3 ; i++) {

  NS_ARRAY_LENGTH(containers)

I think this would be cleaner to implement by asking each container for its col count.

Also, this method should probably return PRUint32.  And it should check that it's really working with ruby cells.

>+nsRubyFrame::GetMaxCellWidth(PRInt32 colIdx)

With the better types for mRuby*ContFrame and GetCellAt this can be simplified some.  Also, that if-cascade near the end and the dead |return 0| can be simplified with NS_MAX.

>+nsRubyFrame::DestroyFrom(nsIFrame* aDestructRoot)

Not needed.

>+nsRubyFrame::AppendFrames(nsIAtom*        aListName,
>+nsRubyFrame::InsertFrames(nsIAtom*        aListName,
>+nsRubyFrame::RemoveFrame(nsIAtom*        aListName,

These should just delegate to the superclass and invalidate our cached container frames.

I'll go ahead and just make the above changes locally....
Attachment #530897 - Flags: review?(bzbarsky) → review-
Two more issues:

1)  <rtc> is not actually usable in HTML.  I'm just going to nix the html.css styles for rtc, rbc, rb (which are not in HTML5) for now.  I'll change the reftests where explicit containers and base cells are really needed to use <span> with styles.

2)  Lots of DOS newlines.  I'll just remove those.
Don't we also use html.css for XHTML? In which case <rtc> would be usable with the Ruby Annotation module? http://www.w3.org/TR/ruby/
> Don't we also use html.css for XHTML?

Yes.

> In which case <rtc> would be usable with the Ruby Annotation module?

Does anyone else plan to ever implement that?  If not, I'd rather not create HTML elements that behave differently from other UAs for the moment.
Personally, I use <rtc> in XHTML 1.1 with CSS hack, is it going to continue to operate?
My current plan is for this patch to not affect the behavior of <rtc> in any way whatsoever: it will continue to be just an unknown element.
I guess the best answer for rtc/rbc would depend on the answer to Célian's question. On principle, though, I don't think we should choose not to support something *because* other browsers don't support it yet. :) There may be other reasons not to support things, but that shouldn't be one of them.

Célian, can you show us an example of your CSS hack?

I think we should definitely include styles for <rb>, and if we're doing anonymous box generation correctly the following should work: <ruby><rb>東</rb><rb>京</rb><rt>とう</rt><rt>きょう</rt></ruby>

See http://www.w3.org/Bugs/Public/show_bug.cgi?id=10830#c9 wrt <rb> and the above markup. HTML5 markup handles the simplest cases, but misses some reasonably common ones in Japanese.

I'll do a proper writeup on this later, but the shortcomings in the simplified HTML5 model are issues with fallback, line breaking, and the content/style distinction--all of which are relatively subtle. (Some of the design decisions in Ruby Annotation were made to account for these, but got lost in the translation to HTML5.)
I use it for this case :
<ruby><rbc><rb>kanji</rb></rbc><rtc><rp>(</rp><rt>furigana</rt><rp>)</rp></rtc><rtc><rp>(</rp><rt>romaji</rt><rp>)</rp></rtc></ruby>

But it's only work with Firefox 4 in XML (application/xhtml+xml) with the new parser.
There's a difference between "don't support it yet" and "have said they never plan to support it".

> I think we should definitely include styles for <rb>

Why?

> and if we're doing anonymous box generation correctly the following should work:

If we include styles for <rb> that will give two ruby base cells.  If we do not, then that gives one ruby base cell.  In WebKit, for what it's worth, there's one ruby base cell in that situation.  So we would immediately be non-interoperable on markup like that if we included those styles.  What does IE do with that markup, if I might ask?  The W3C bug you cite makes it sound like it also has only one ruby base cell in that situation..

Note that I'm happy to change things back if people really think it's a good idea to do something non-interoperable with other browsers here.  The change is trivial.  But there's really nothing stopping a page that wants to use the above markup from styling |rb { display: -moz-ruby-base; }| itself if it really wants that behavior...

As far as rtc goes, the situation is even worse than rbc/rb.  For those, we'd just be non-interoperable with WebKit and Trident.  For rtc, XHTML and HTML have totally different behavior.  I'd really rather not go there for now; if and when we do, we may want a new tagname to replace "rt" that's safe to use in "rtc"....
> But it's only work with Firefox 4 in XML (application/xhtml+xml) with the new
> parser.

Right, and that's not changing....
Attached patch Part 1 merged to tip (obsolete) — Splinter Review
Attachment #530895 - Attachment is obsolete: true
Attachment #530896 - Attachment is obsolete: true
Attachment #530897 - Attachment is obsolete: true
Attachment #533458 - Attachment is obsolete: true
Attachment #535826 - Attachment is obsolete: true
Attachment #535826 - Flags: feedback?(hajime.shiozawa)
Attachment #536875 - Attachment is obsolete: true
David, could you look at the reflow code?  Timothy, could you look at the frame construction code?
Attached patch Part 3: tests (obsolete) — Splinter Review
> Why?

We have the capability. Why should we /not/ implement it correctly? I do believe supporting <rb> is the right thing to do going forward.

FWIW, I don't accept arguments about bugwards compatibility unless there is a *content* compatibility problem. And for ruby I don't think we have one yet. WebKit's ruby behavior has quite a few bugs and isn't entirely compatible with IE either. Also neither they nor IE support the CSS ruby model, which we will after this lands.

> What does IE do with that markup, if I might ask?

IE treats consecutive <rb>s as a single ruby base and consecutive <rt>s as a single ruby text. Such a rendering would be close to what we'd get by implementing <rb>, and is a valid rendering of jukugo ruby, which is what that markup pattern would be appropriate for. (Both renderings are correct for this kind of ruby: it's just a stylistic variation whether the text is glommed together as in IE or each ruby is aligned to each base individually.)

> There's a difference between "don't support it yet" and "have said
> they never plan to support it".

I'd take any claims that WebKit will never support complex ruby with a grain of salt. They're getting into the Japanese ebook business, and when they get to dealing with textbooks, it's entirely possible they'll change their mind. ;)
> We have the capability. Why should we /not/ implement it correctly?

Well, the question is what "correctly" means here!  I understand that you think that supporting <rb> is it; I'm just saying that there seems to not be universal agreement on that as far as I can tell.  Do you have a plan for convincing WebKit and Trident to follow here?  Or will they implement something different and then we'll have to do both that and what you're advocating for now?  Or is the plan something else?

> And for ruby I don't think we have one yet.

Well, for <rtc> we sort of do, right?

But ok, I think I'm convinced we should restore the styles for rb/rbc.  What about rtc?
> Or is the plan something else?

The plan is that I will add an anonymous box generation section to CSS Ruby such that it can handle both HTML5 Ruby and XHTML Ruby Annotation and everything in between. (This sounds ambitious, but is actually straightforward inasmuch as anonymous box generation can be, and would be roughly what Shiozawa-san has implemented here.) If at any point WebKit or Trident implement the CSS Ruby model, I would expect them to follow that model. Trident in any case is functionally compatible with us wrt valid <rb>, even if their rendering interpretation is slightly different.

> Well, for <rtc> we sort of do, right?

Do we? Who is using <rtc> in a way that's incompatible with its intended rendering?
> If at any point WebKit or Trident implement the CSS Ruby model

My question was what your plan is for getting them to do this.

> Who is using <rtc>

IE, Webkit, and Gecko are all _parsing_ <rtc> (or more precisely <rt>) in a way that's incompatible with its intended rendering.  So in practice <rt> inside <rtc> is not usable in HTML on the web.
> My question was what your plan is for getting them to do this.

No plan for that.

Why are you pushing us to implement broken behavior, just because WebKit currently does? I don't understand what principles you're using to make your decisions here. Mine are

  1. Implement what's correct per spec / going forward
  2. Don't break existing content
  3. Prioritize work reasonably

At no point does bugwards compatibility enter that list. Insofar as content is written to bugs, it influences #2, but it is *not the same as* #2. If there is no content written to bugs, those bugs don't matter.

> IE, Webkit, and Gecko are all _parsing_ <rtc> (or more precisely <rt>)
> in a way that's incompatible with its intended rendering.

IE and Opera parse <rtc> correctly afaict. WebKit and Gecko do weird things when parsing ruby markup. I'm guessing this is one of those cases where Hixie put IE's parsing bugs in his spec and then IE fixed the bugs? Bugwards compatibility FTW?
> I don't understand what principles you're using to make your decisions here.

They're pretty simple:

1)  Implement something that allows web authors to solve their problems.
2)  Make sure that other implementations can converge on what we implement (and
    it doesn't matter whether the things preventing convergence are technical
    or political if they prevent convergence).

You seem to think there's no value in #2, but implementing behavior that we then can't standardize because it fails the 2-implementation test is pointless.

> IE and Opera parse <rtc> correctly afaict

Very interesting.  IE9 and IE10 in standards mode don't close <rtc> on <rt>.  IE8 and earlier close it.  The spec was almost certainly written based on IE8.

Given that, I think we should just get the HTML5 spec changed here.  Henri?
Though of course you still won't be able to use <rtc> in HTML until IE8- market share is gone...  figure 3-5 years at current rates.
> You seem to think there's no value in #2, but implementing behavior that we
> then can't standardize because it fails the 2-implementation test is pointless.

By accepting to implement CSS Ruby at all we're doing that already. So by your logic we should reject this patch and.. do what, reverse-engineer IE10 to come up with a CSS model that mimics their current behavior?
> By accepting to implement CSS Ruby at all we're doing that already.

I have pretty serious reservations about us implementing the underspecified and as far as I can tell overdesigned mess that is CSS Ruby, but the patch is here, it gets us ruby support in the common cases, and I don't think we should wait a few more years waiting on said reverse-engineering effort.

But in the interests of full disclosure, my best guess at what will happen here is that CSS Ruby in anything resembling its current state will fail at standardization and we will probably need to significantly change the code being created now to implement whatever the final thing getting standardized is.  That's life.

A key difference between that and issues with HTML elements, from my point of view, is that we're vendor-prefixing all the CSS Ruby gunk so that we won't create a legacy problem when the final spec doesn't look anything like the current wishlist (I can't really call the current state of CSS Ruby a spec, sorry).  Sadly, we can't (or at least don't) vendor-prefix HTML elements.  So we have less room for failed experiments there.

In any case, I'm convinced that we should in fact change the HTML5 spec here as far as <rt> handling goes and restore the rb,rbc,rtc styles...  thank you for explaining the IE compat situation more clearly.
I agree the CSS spec is overly complicated, but that nonetheless we should get the patch out.

The contest between the HTML5 and CSS standards for Ruby, and the evolution of each, will be driven by reality better than we can do so at this point, after such great input from many v. qualified parties. Will the formal folk like children's educational writers and newspapers be the main proponents, or will bloggers find it fun for some sort of gimmick? What tricks will CSS pros use most commonly to achieve cross-browser visual similarity? This information will naturally settle some arguments when it's available. Release this patch, and in a few years we'll have it.

As this patch handles the simple cases correctly I can't feel that we're launching into future doom by not sorting out the issues of <rtc> etc. If Joe Blogger wants to stick a hiragana reading under his unusual kanji surname, he won't ever go past the simplest <ruby><rb>XX</rb><rt>YY</rt></ruby> form. I suspect it will cover 99% of cases, just as > 99% of <table>s never appear with colgroup elements, thead/tbody/tfoot elements, the "dir" attribute, etc.
(In reply to comment #177)
> Given that, I think we should just get the HTML5 spec changed here.  Henri?

It looks like this has happened, per:

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12935

http://html5.org/r/6214-6216
Here's a dump of my thoughts on ruby markup:
  http://fantasai.inkedblade.net/weblog/2011/ruby/
Shiozawa-san, if there's anywhere I'm wrong, please correct me. :)
Attachment #538209 - Flags: review?(tnikkel)
Attachment #538209 - Flags: review?(dbaron)
Comment on attachment 538209 [details] [diff] [review]
Part 2 with review comments addressed

+  nsHTMLContainerFrame::CreateViewForFrame(newFrame, PR_FALSE);

We don't need to add any of these calls, these frames can never have views.

+static PRBool
+IsRubyPseudo(nsIFrame* aFrame)
+{
+  nsIAtom* pseudoType = aFrame->GetStyleContext()->GetPseudo();
+  return pseudoType &&
+    (pseudoType == nsCSSAnonBoxes::ruby ||
+     pseudoType == nsCSSAnonBoxes::rubyBaseContainer ||
+     pseudoType == nsCSSAnonBoxes::rubyTextContainer ||
+     pseudoType == nsCSSAnonBoxes::rubyBase ||
+     (pseudoType == nsCSSAnonBoxes::rubyCellBlock &&
+      aFrame->GetParent()->GetStyleContext()->GetPseudo() ==
+      nsCSSAnonBoxes::rubyBase));
+}

Why do we check that rubyCellBlock's are children of rubyBase and not rubyText?

Why is rubyText not in this list?


A list like:
A ruby frame can have these children ... with these restrictions ...
A ruby frame may be the child of any frame that ...
A ruby base frame can have these children ... and it can be the child of ...
for ruby, ruby base, ruby text, ruby base container, and ruby text container I think would be helpful.

+      // Allow any kids that are ruby parts or not inline.  We will fix up the
+      // ruby parts later in this function, and as for the non-inline kids
+      // we'll construct frames for them but just not lay them out.

So its just easier to construct frames for the non-inline kids then not to?

+    // wrapping for text and inline node when parent frame is <ruby>, <rbc> or <rtc>
+    // 3: <ruby><span>ABC</span><rb>abcd</rb></ruby>
+    //    -> <ruby><rb><span>ABC</span></rb><rt>abc</rt></ruby>

Number 3 doesn't seem right.

+nsCSSFrameConstructor::EnsureValidRubyStructure(nsFrameConstructorState& aState,
+  NS_ASSERTION(aItems.HasRubyPart(), "aItems don't have ruby child");

Why is this assertion true at this point?

+    nsRefPtr<nsStyleContext> textStyle = mPresShell->StyleSet()->
+      ResolveAnonymousBoxStyle(nsCSSAnonBoxes::rubyTextContainer, parentStyle);

Call it textContainerStyle.

+  if (parentFrameType == nsGkAtoms::rubyContainerFrame) {
+    return;
+  }

We can put this before the "if (parentFrameType == nsGkAtoms::rubyFrame)" block above.
> We don't need to add any of these calls, these frames can never have views.

Ok, removed.

> Why do we check that rubyCellBlock's are children of rubyBase and not rubyText?

Good catch.  We need to check both.  Fixed.

> Why is rubyText not in this list?

Needs to be there.  Added.

> A list like:
...
> I think would be helpful.

OK.  Added.  It looks like this, right before EnsureValidRubyStructure, after the comment that's already there in the patch:

 * This guarantees the following invariants:
 *
 * 1) The children of a ruby frame are all either block-level or
 *    ruby-base-container or ruby-text-container.
 * 2) The children of a ruby-base-container are all either block-level or
 *    ruby-base.
 * 3) The children of a ruby-text-container are all either block-level or
 *    ruby-text.
 * 4) The parent of a ruby-base is a ruby-base-container.
 * 5) The parent of a ruby-text is a ruby-text-container.
 * 6) The parent of a ruby-base-container or ruby-text-container is a ruby
 *    frame.

> So its just easier to construct frames for the non-inline kids then not to?

That's a good question.  We could add them to the undisplayed map and suppress frame construction for them.  Would you prefer that?

> Number 3 doesn't seem right.

Indeed.  Fixed.  New comment:

    // 3: <ruby><span>ABC</span><rb>abcd</rb></ruby>
    //    -> <ruby><rb><span>ABC</span></rb><rb>abcd</rb></ruby>

> Why is this assertion true at this point?

Hmm.  It's actually not, now that you ask.  A <ruby> with only block kids would trigger it.  I can remove it if we decide to keep the block frames.

If we just drop them instead, it would be true because at this point the list is nonempty.  And it either had ruby items already or the parent is ruby or ruby-container (see if check at the beginning of the function).  Since we didn't drop any ruby items, and all kids of a ruby or ruby-container are either dropped, ruby items, or get wrapped in rb, the assertion holds.

> Call it textContainerStyle.

Done.

> We can put this before the "if (parentFrameType == nsGkAtoms::rubyFrame)"
> block above.

Done.
(In reply to comment #185)
> > So its just easier to construct frames for the non-inline kids then not to?
> 
> That's a good question.  We could add them to the undisplayed map and
> suppress frame construction for them.  Would you prefer that?

It just seemed like not the greatest idea to have these lifeless frames floating around on the frame tree. Which way do you think is best?
> Which way do you think is best?

I guess he undisplayed map thing is somewhat better.  I'll change things accordingly.
Comment on attachment 538209 [details] [diff] [review]
Part 2 with review comments addressed

I've only gotten through part of the reflow parts of the patch so far, but I thought I'd share what I have so far, since there are some rather large issues, I think:

The new files all need license headers, and they should also have
one-line descriptions of what they're for.  The single-line comments at
the top (before any #ifdefs/#includes, in the format that shows up in
http://mxr.mozilla.org/mozilla-central/source/layout/generic/ ) should
mention the 'display' values and elements that the frame is for.

In general, I think it's good practice for each .cpp file to include
"it's" header file first, so that we ensure that each header file
compiles without prerequisites.  That is, the first include in X.cpp
should be X.h.

nsRubyCellFrame::AlignCell shouldn't have an empty comment above it.
The comment should either say something or be removed.

NEED TO REVIEW AlignCell MORE

So it's not clear to me how margin, border, and padding should behave
on the various ruby elements, though I'm inclined to think (as is
fantasai) that they should act more like margin/border/padding on
inlines than like margin/border/padding on inline-blocks and replaced
elements.  This patch mostly appears to treat border and padding like
replaced elements / inline-blocks and ignore margins.  (But we
probably should support something for adjusting the separation of the
ruby-text from the ruby-base, which fantasai thinks should be
collapsing margins on the ruby-text elements.)

nsRubyCellFrame::Reflow:
 - should not get childPt; the old position isn't useful (in this case
   it's just always preserving 0,0, except in the case of relative
   positioning, which I believe will be broken by increasing the
   relative position on each incremental reflow; please add a test for
   relatively-positioned ruby cells)
 - should instead either just pass 0,0 as coordinates for the child and
   assert that the child has no used margin, or should use the child's
   used margin as its initial position and add it to aMetrics.width
   (but for vertical margin/border/padding, see above)
 - should use a separate status for the child (or, for the other classes
   for which I quote this comment, each child) rather than reusing
   aStatus, should assert that it is never reflowed with a constrained
   height (should be guaranteed by nsLineLayout::ReflowFrame, and
   I presume the frame construction code guarantees it's always within a
   line), and should assert that the child never returns a reflow status
   other than NS_FRAME_COMPLETE (and should still set aStatus to
   NS_FRAME_COMPLETE)

It would probably be good for Reflow, GetMinWidth, and GetPrefWidth to
assert that the child does not have a next-sibling and perhaps also
assert about the anon box type (perhaps just as a form of
documentation).

nsRubyContainerFrame::Reflow:
 - same three comments as nsRubyCellFrame::Reflow, except must
   consider margin on child
 - same issue with what to do about vertical margin/border/padding
 - should not have the |rv| variable (just return NS_OK)
 - NEED TO LOOK AT ALIGNMENT (and GetBaseline?)
I'll fix the license headers and such.

> This patch mostly appears to treat border and padding like replaced elements /
> inline-blocks and ignore margins. 

Yes, with my changes.  The original patch ignored them all, and had some pretty bizarre behavior when they were specified....

> please add a test for relatively-positioned ruby cells

Will do.

Will make the other changes you want to nsRubyCell::Reflow.

> It would probably be good for Reflow, GetMinWidth, and GetPrefWidth to
> assert that the child does not have a next-sibling and perhaps also
> assert about the anon box type

Will do.

I'd be tempted to ignore vertical margin/border/padding issues for the moment, past the current behavior of making sure they don't overlap the text, honestly. Or do you think we need to sort those out before landing this?
What do other browsers do for margin/border/padding?

I definitely don't like the idea of honoring border and padding and dropping margin on the floor.
> What do other browsers do for margin/border/padding?

WebKit doesn't have a ruby-base element or any ruby-container elements that can be styled at all.  It treats margin/border/padding on <ruby> the way it treats inlines.  For ruby-text, it's... weird.  The horizontal margin affects the width of the <ruby> and the placement of the ruby base but not the placement of the ruby-text border box.  The vertical margin seems to be ignored.  Past that, looks like padding and border are treated as for an inline, sort of.

Opera doesn't have any ruby support.

IE9 again only supports ruby and ruby-text.  The former seems to be treated as an inline for our purposes.  The latter seems to treat border and padding as on inlines.  Vertical margins are ignored.  Horizontal margins are taken into account when determining the width of the <ruby>, but then ignored for actual horizontal positioning: the ruby-text is centered over the ruby-base.

Note that in both IE9 and WebKit putting border/padding/background on the ruby text makes the ruby base invisible if they're big enough (the background and border cover it up).
> nsRubyCellFrame::AlignCell shouldn't have an empty comment above it.

Oops.  I failed to merge in the text of that comment.  Will do that.
So dbaron wants more inline-like handling for <ruby>.

*dbaron* Also, other things to think about while you're there:
*dbaron*  - check vertical alignment
*dbaron*  - check line height handling
*dbaron*  - look at handling of multiple ruby-text, including position
*dbaron*  - check painting order
*dbaron* there are two possible options for line-height:
*dbaron*  (1) completely ignore the ruby-text
*dbaron*  (2) include half of the ruby-text (up to its midpoint)

That last bit is in terms of what should happen in VerticalAlignFrames.

In particular, we don't want to change the line height for the ruby text.  

It's not clear whether the <ruby> box needs to be tall enough for the ruby-text to be in it.  One option is to just size ruby and the container boxes as inlines.
Attachment #538208 - Attachment is obsolete: true
Attachment #538209 - Attachment is obsolete: true
Attachment #538209 - Flags: review?(tnikkel)
Attachment #538209 - Flags: review?(dbaron)
Attachment #538210 - Attachment is obsolete: true
Comment on attachment 611345 [details] [diff] [review]
Part 1 merged to tip

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

::: layout/style/html.css
@@ +785,5 @@
> +rb {
> +  display: -moz-ruby-base;
> +}
> +
> +rbc {

Do we really want to support rbc/rtc?
Good question.  Henri, what did you and Ian finally decide on re: HTML?
There has been no decision. The spec bug lingers open.  The latest is http://lists.w3.org/Archives/Public/public-i18n-cjk/2012JanMar/0063.html
There's a bunch of news on the ruby front lately.  There's a CSS spec (which has been accepted by the CSS working group) that defines how ruby layout should work in a more detailed way:
  http://dev.w3.org/csswg/css-ruby/
and there's also a proposed HTML spec to fix some of the issues with the HTML5 ruby spec (see bug 33339 comment 106) which will hopefully be accepted by the HTML working group.


I think any work to continue the patches in this bug should be sure to revise them to take an architectural approach that will correctly handle:
 * line-breaking within ruby
 * inline formatting
as described in the current draft of the CSS ruby module.  This means doing something much more like 'display: inline' and much less like 'display: inline-block'.
Depends on: 1021952
There's been a bunch of progress on ruby lately thanks to Susanna Bowen's internship.  She landed code, behind a preference, for frame construction (including whitespace handling and anonymous box creation) and for significant parts of reflow (dealing with the horizontal positioning and sizing).  I've filed dependencies of this bug on work that I know is remaining.  Things that I believe block shipping CSS ruby block bug 1039006, while other parts of ruby that I don't believe block shipping the feature block this bug directly.
Assignee: hajime.shiozawa → nobody
Target Milestone: mozilla2.0 → ---
No longer depends on: ruby-align
Depends on: 1041711
Depends on: 1104373
Depends on: 1105051
Depends on: 1098257
No longer depends on: 1098257
Depends on: 1107701
Depends on: 1112474
Depends on: 1115249
Depends on: 1115262
No longer depends on: 1115262
Depends on: 1119636
Depends on: 1130891
Depends on: 1055662
Depends on: 1133624
Depends on: 1134069
Depends on: 1134432
Depends on: 1135306
No longer depends on: 1135306
Depends on: 1135361
Depends on: 1135954
Depends on: 1136067
Depends on: 1138527
Depends on: 1140264
Depends on: 1141928
Depends on: 1141931
Depends on: 1143558
Depends on: 1144465
Depends on: 1149009
Depends on: 1164279
Depends on: 1224997
Depends on: 1226108
Depends on: 1228168
Depends on: 1242344
Depends on: 1267515
We should probably change this bug report into a tracking one, a meta one and then file/create bug reports for individual, single code scenarios.

"
meta 	A placeholder bug for tracking the progress of other bugs. Meta bugs are made dependent on other bugs so that interested parties can be kept up-to-date with status via one bug, without having to receive all the mails related to all the bugs related to the development of a particular area.
"
Keywords: meta
Depends on: 1294951
Summary: Implement CSS ruby module → [META] Implement CSS ruby module
Depends on: 1364732
Depends on: 1395766
Depends on: 1395774
Depends on: 1389286
Component: Layout → Layout: Ruby
Type: defect → enhancement
Depends on: 1691243
Depends on: 1694748
Depends on: 1697529
Depends on: 1722215
Depends on: 1746562
Depends on: 1557825
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.