Closed Bug 1021952 Opened 5 years ago Closed 5 years ago

Implement ruby "display" values with stub frame classes

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: sgbowen8, Assigned: sgbowen8)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 22 obsolete files)

43.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
18.39 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch patch v1 (obsolete) — Splinter Review
Patch that adds the ruby display values and stubs for the frame classes.
Attachment #8436087 - Flags: review?(dholbert)
Attachment #8436087 - Attachment description: ruby-frames.patch → patch v1
Comment on attachment 8436087 [details] [diff] [review]
patch v1

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

::: content/base/src/nsGkAtomList.h
@@ +1877,5 @@
> +GK_ATOM(rubyFrame, "RubyFrame")
> +GK_ATOM(rubyBaseFrame, "RubyBaseFrame")
> +GK_ATOM(rubyBaseContainerFrame, "RubyBaseContainerFrame")
> +GK_ATOM(rubyTextFrame, "RubyTextFrame")
> +GK_ATOM(rubyTextContainerFrame, "RubyTextContainerFrame")

Move each of the "Container" lines up by 1 line here, to preserve alphabetical order.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4529,5 @@
> +      FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewRubyBaseContainerFrame) },
> +    { NS_STYLE_DISPLAY_RUBY_TEXT,
> +      FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewRubyTextFrame) },
> +    { NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER,
> +      FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewRubyTextContainerFrame) },

Hmm. So since these are all inline-level display types, these probably don't want FCDATA_MAY_NEED_SCROLLFRAME -- I bet they don't want to be individually scrollable.

Really, these probably want to looks closer to the NS_STYLE_DISPLAY_INLINE entry. It uses nsCSSFrameConstructor::ConstructInline(), which is a bit complex so that it can handle blocks inside of inlines -- see the big comment at the beginning. Maybe we can just co-opt that method, and just swap out its NS_NewInlineFrame() call with something a bit more complex...? (not sure yet)

::: layout/generic/moz.build
@@ +84,5 @@
> +    'nsRubyBaseContainerFrame.cpp',
> +    'nsRubyBaseFrame.cpp',
> +    'nsRubyFrame.cpp',
> +    'nsRubyTextContainerFrame.cpp',
> +    'nsRubyTextFrame.cpp',

I don't see these (or their .h files) in the patch -- I think you need to 'hg add' them?

::: layout/generic/nsFrameIdList.h
@@ +118,5 @@
> +FRAME_ID(nsRubyFrame)
> +FRAME_ID(nsRubyBaseFrame)
> +FRAME_ID(nsRubyBaseContainerFrame)
> +FRAME_ID(nsRubyTextFrame)
> +FRAME_ID(nsRubyTextContainerFrame)

As in nsGkAtomList, the Container lines each need to be bumped up 1 line, to preserve alphabetical order.

::: layout/style/nsCSSProps.cpp
@@ +1004,5 @@
> +  eCSSKeyword_ruby,               NS_STYLE_DISPLAY_RUBY,
> +  eCSSKeyword_ruby_base,          NS_STYLE_DISPLAY_RUBY_BASE,
> +  eCSSKeyword_ruby_base_container,NS_STYLE_DISPLAY_RUBY_BASE_CONTAINER,
> +  eCSSKeyword_ruby_text,          NS_STYLE_DISPLAY_RUBY_TEXT,
> +  eCSSKeyword_ruby_text_container,NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER,

Add another space of indentation to the right half of this table, so that you can keep at least 1 space after the comma in your added lines here, for readability's sake.

::: layout/style/nsStyleConsts.h
@@ +418,5 @@
> +#define NS_STYLE_DISPLAY_RUBY                   33
> +#define NS_STYLE_DISPLAY_RUBY_BASE              33
> +#define NS_STYLE_DISPLAY_RUBY_BASE_CONTAINER    33
> +#define NS_STYLE_DISPLAY_RUBY_TEXT              33
> +#define NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER    33

These should not all be 33. :) They need different values.

(Does this not cause any test failures? If not, that's a bit concerning... we probably need an assertion to check that the same numeric value isn't listed twice in kDisplayKTable or something... Anyway, that doesn't need to happen here, though.)

::: layout/style/test/property_database.js
@@ +4797,5 @@
>  }
>  
> +if (SpecialPowers.getBoolPref("layout.css.ruby.enabled")) {
> +  gCSSProperties["display"].other_values.push("ruby", 
> +                                              "ruby-base", 

Delete space character @ end of "ruby" & "ruby-base" lines here.
Attached patch patch v2 (obsolete) — Splinter Review
Addressed the problems in the previous patch with the exception of the FCDATA confusion (since we're still not sure what to do there).
Attachment #8436131 - Flags: review?(dholbert)
Attachment #8436087 - Attachment is obsolete: true
Attachment #8436087 - Flags: review?(dholbert)
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8436131 [details] [diff] [review]
patch v2

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

We'll follow up on the frame constructor stuff on Monday, but here are a 2 nits in the frame classes (both of which might be easiest-addressed by editing the patch file directly, if you don't want to have to edit/save/quit, edit/save/quit over and over)

(No pressure to re-post the patch just to address these nits, though.)

::: layout/generic/nsRubyBaseContainerFrame.cpp
@@ +3,5 @@
> +/* This Source Code is subject to the terms of the Mozilla Public License
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */
> +
> +/* rendering object for CSS "display: grid | inline-grid" */

Nit: this ^ comment needs tweaking in the various frame files, to match the actual 'display' values being implemented.

@@ +24,5 @@
> +NS_IMPL_FRAMEARENA_HELPERS(nsRubyBaseContainerFrame)
> +
> +nsContainerFrame*
> +NS_NewRubyBaseContainerFrame(nsIPresShell* aPresShell,
> +                         nsStyleContext* aContext)

Nit: the nsStyleContext* arg needs an indentation-tweak to line up here. (in the .h file too) (and in the other frame class's files, too)
Is there an actual spec we're following this time?  If so, please add a link to that in the URL field.

Note that there's previous work along these lines in bug 256274, though it may no longer be relevant depending on how much the spec has changed.
Attached patch patch v3 (obsolete) — Splinter Review
Fixes for the problems from comment 4. Frame construction settings should be more reasonable.

I'm going to head into doing the anonymous box generation now. The existing work from the past bug doesn't look like it exactly follows the new spec, but I'll see if I can modify it so that it does.
Comment on attachment 8437146 [details] [diff] [review]
patch v3

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

Two nits noticed when skimming the latest patch:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4522,5 @@
>      { NS_STYLE_DISPLAY_INLINE_GRID,
>        FCDATA_DECL(FCDATA_MAY_NEED_SCROLLFRAME, NS_NewGridContainerFrame) },
> +    { NS_STYLE_DISPLAY_RUBY,
> +      FCDATA_DECL(FCDATA_IS_INLINE | FCDATA_IS_LINE_PARTICIPANT,
> +                       NS_NewRubyFrame) },

Nit: indentation is off for these NS_* lines here. (Should be aligned with FCDATA_IS_INLINE above it)

::: layout/generic/nsRubyBaseContainerFrame.cpp
@@ +4,5 @@
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */
> +
> +/* rendering object for CSS "display: ruby | ruby-base | ruby-base-container |
> + * ruby-text | ruby-text-container" */

Nit: Each chunk of documentation like this ^ in the frame classes should just mention a single display value (the one corresponding to this frame class), rather than listing all of the ruby display values.
Comment on attachment 8436131 [details] [diff] [review]
patch v2

[canceling review request on v2 & marking that version obsolete, since we now have v3, and since I think you're working on adding more]
Attachment #8436131 - Attachment is obsolete: true
Attachment #8436131 - Flags: review?(dholbert)
Attached patch ruby-anon.patch v1 (obsolete) — Splinter Review
This patch generates the anonymous ruby boxes according to the spec's section 2.2 (see URL). I appropriated the CreateNeededTablePseudos function for this, since it does basically the same thing. White space handling for Ruby still needs to be special cased within the function.
Attachment #8440029 - Flags: review?(dholbert)
Attached patch ruby-anon.patch v2 (obsolete) — Splinter Review
Attachment #8440029 - Attachment is obsolete: true
Attachment #8440029 - Flags: review?(dholbert)
Attachment #8440068 - Flags: review?(bzbarsky)
It'll take me a few days to get to this, unfortunately.  I have a bit of a review backlog right now.  :(
Depends on: 1025308
This patch goes on top of the other two. It applies style fixups to ruby elements to make sure all children of a ruby element are forced to be inline, as required by the spec.
Attachment #8441472 - Flags: review?(bzbarsky)
Attached patch ruby-pairing.patch v1 (obsolete) — Splinter Review
This patch is to ensure that the conditions in section 2.3 of the spec are met.
Attachment #8441724 - Flags: review?(bzbarsky)
I'm sorry about the terrible lag here.  Unfortunately, I'm out tomorrow and I haven't made my way through this stuff yet.  :(  This is at the top of my priority list for Monday morning.
Blocks: 1030993
Attached patch ruby-frames.patch v4 (part 1) (obsolete) — Splinter Review
Most of the changes here relate to changing ruby text container frames and ruby text frames to blocks.
Attachment #8437146 - Attachment is obsolete: true
Attachment #8447443 - Flags: review?(dholbert)
Attached patch ruby-anon.patch v3 (part 2) (obsolete) — Splinter Review
Attachment #8440068 - Attachment is obsolete: true
Attachment #8440068 - Flags: review?(bzbarsky)
Attachment #8447444 - Flags: review?(bzbarsky)
Attached patch ruby-pairing.patch v2 (part 4) (obsolete) — Splinter Review
You should be able to diff the two new versions of the patches. The changes are very minor and all relate to changing some of the ruby frames to blocks instead of containers.
Attachment #8441724 - Attachment is obsolete: true
Attachment #8441724 - Flags: review?(bzbarsky)
Attachment #8447445 - Flags: review?(bzbarsky)
Attachment #8447443 - Attachment description: ruby-frames.patch v4 → ruby-frames.patch v4 (part 1)
Attachment #8441472 - Attachment description: ruby-style-fixups.patch v1 → ruby-style-fixups.patch v1 (part 3)
Attachment #8447444 - Attachment description: ruby-anon.patch v3 → ruby-anon.patch v3 (part 2)
Attachment #8447445 - Attachment description: ruby-pairing.patch v2 → ruby-pairing.patch v2 (part 4)
Comment on attachment 8441472 [details] [diff] [review]
ruby-style-fixups.patch v1 (part 3)

>+//    then we set it to a valid inine display value

"inline".

>+  // see if the display value is already a block

Already an inline?

>+++ b/layout/style/nsStyleContext.cpp
>+    if ((parentDisp->IsFlexOrInlineStyle() ||

Shouldn't that be named IsFlexOrGridStyle()?  Or better yet IsFlexOrGridDisplayType()?

>+        parentDisp->IsRubyStyle()) &&

And IsRubyDisplayType()?

>         GetPseudo() != nsCSSAnonBoxes::mozNonElement) {

I'm not sure this makes sense to do like this.  For the ruby case, we don't care about non-elements, since those are inline anyway.  Furthermore, sicne your switch does not have a default case, you don't care about skipping over table parts as well: those will just get ignored by EnsureInlineDisplay (this is worth documenting in EnsureInlineDisplay).  And the table-part ignoring is the only part you're ending up sharing.  So I think this shoud just be a separate if statement, not combined with the flex-or-grid one.

>+          if (displayVal != disp->mDisplay && 
>+              !disp->IsAbsolutelyPositionedStyle() &&
>+            !disp->IsFloatingStyle()) {

Please fix the indent here.

Maybe it's worth adding an IsOutOfFlowStyle() helper...

>+++ b/layout/style/nsStyleStruct.h
>   bool IsRubyStyle() const {
>     return NS_STYLE_DISPLAY_RUBY == mPosition ||

This predates this patch, but should be mDisplay, not mPosition throughout this function.

>+  bool IsFlexOrInlineStyle() const {
>+    return NS_STYLE_DISPLAY_FLEX == mPosition ||

Same here.

r=me with those issues fixed, but I'd like to see the updated patch.
Attachment #8441472 - Flags: review?(bzbarsky) → review+
Comment on attachment 8447443 [details] [diff] [review]
ruby-frames.patch v4 (part 1)

Comment 7's nits still need addressing in this patch, it looks like.

>+++ b/layout/generic/nsRubyBaseContainerFrame.h
>+protected:
>+  friend nsContainerFrame* NS_NewRubyBaseContainerFrame(nsIPresShell* aPresShell,
>+                                                    nsStyleContext* aContext);

Each of the "friend...NS_NewRuby" chunks, like the above, need an indentation-tweak on the next line.

>+nsBlockFrame* NS_NewRubyTextFrame(nsIPresShell* aPresShell,
>+                                      nsStyleContext* aContext);

Each of the "nsBlockFrame* NS_NewRuby*" declarations should instead be returning nsContainerFrame* (which is still a superclass, just a more distant ancestor now.)  These NS_NewXYZFrame() functions are intentionally generic in their return value; until recently, they all just returned a pointer of type nsIFrame*.  Very recently, someone made the nsContainerFrame-subclass ones return nsContainerFrame*, presumably because now we use some container-specific behavior after creating them.  There's no reason they should return anything more specific than that, though. (so not nsBlockFrame)
Comment on attachment 8447444 [details] [diff] [review]
ruby-anon.patch v3 (part 2)

This is looking pretty good!  I'm sorry it took me so long, but now this code is finally fresh in my mind, as is the spec...

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+  if (aFrameType == nsGkAtoms::rubyBaseFrame) {
>+    return eTypeRubyBase;
>+  } 
>+  if (aFrameType == nsGkAtoms::rubyTextFrame) {
>+    return eTypeRubyText;
>+  } 

Why shouldn't those be eTypeBlock for purposes of this method?  I would think
they could, just like table cells.  More on this below.

>+nsCSSFrameConstructor::CreateNeededPseudos(nsFrameConstructorState& aState,
>+            //TODO: Add whitespace handling for ruby

Yeah.  I spent a while trying to figure out what that should look like, and decided that the spec is just confused.  Worse yet, I can't tell what the spec is _trying_ to do.

So this is probably OK for now, but file an explicit followup blocking enabling this stuff, and reference its bug number here, please.


>+        // Don't group ruby base boxes and ruby annotation boxes together
>+        if (ourParentType == eTypeRuby && (prevParentType != groupingParentType)) {
>+          break;

I don't think this is correct.

Say we have this:

  <div style="display: ruby">
    Some text
    <div style="display: ruby-base">
  </div>

In this case ourParentType is eTypeRuby, groupingParentType is eTypeBlock, and prevParentType is eTypeRubyBaseContainer.  But we want to group these two together.

I think you want something like:

       if (ourParentType == eTypeRuby &&
           (prevParentType == eTypeRubyTextContainer) !=
           (groupingParentType == eTypeRubyTextContainer)) {

which is what we do for the similar situation with colgroups vs rowgroups.

>+      case eTypeRuby:
>+        if (groupingParentType == eTypeRubyBaseContainer ||
>+            groupingParentType == eTypeRubyBase) {
>+          wrapperType = eTypeRubyBaseContainer;
>+        } else if (groupingParentType == eTypeRubyTextContainer ||
>+                   groupingParentType == eTypeRubyText) {
>+          wrapperType = eTypeRubyTextContainer;

Nothing has a eTypeRubyBase or eTypeRubyText desired parent type, as far as I can tell, so those can't be values of groupingParentType.  Furthermore, there can be kids of a eTypeRuby (like text, say) that cause groupingParentType to be set to eTypeBlock.

Again, this should probably be more like the table case: if groupingParentType is eTypeRubyTextContainer, then wrapperType should be eTypeRubyTextContainer, otherwise it should be eTypeRubyBaseContainer.

>+        } else {
>+          iter = endIter;
>+          continue;
>+        }

I'm not sure I follow this bit.  what is it trying to do?  At first glance what it does is skip wrapping things between iter and endIter altogether, which seems wrong to me.  I think this block is not needed at all...

>+      default: 

Assert that ourParentType == eTypeBlock?

>+        } else if (IsTableParentType(groupingParentType)) {

Can't you just assert IsTableParentType(groupingParentType) here if !IsRubyParentType(groupingParentType)?  Seems like you should be able to, at first glance.

>+        } else {
>+          iter = endIter;
>+          continue;

And then this block is not needed.

>     if (pseudoType == nsCSSAnonBoxes::table &&
>-        parentStyle->StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_INLINE) {
>+        (parentStyle->StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_INLINE ||
>+         IsRubyParentType(ourParentType))) {
>       pseudoType = nsCSSAnonBoxes::inlineTable;

Ah, this is why you had ruby-base and ruby-text as plausible return values from GetParentType(nsIFrame*)?

I think it makes more sense to just check for the relevant display values here.  Specifically, we only need to check for inline, ruby-base, and ruby-text display types, right?

> nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState& aState,
>-    NS_ASSERTION(iter.item().DesiredParentType() == GetParentType(aParentFrame),
>+    NS_ASSERTION(iter.item().DesiredParentType() == parentType ||
>+                 (iter.item().DesiredParentType() == eTypeBlock &&
>+                  IsRubyParentType(parentType)),
>                  "Needed pseudos didn't get created; expect bad things");

I don't think this change will be needed if you change GetParentType as I suggest.

>+++ b/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsRubyBaseContainerFrame.cpp
>+++ b/layout/generic/nsRubyBaseContainerFrame.h
>+++ b/layout/generic/nsRubyBaseFrame.cpp
>+++ b/layout/generic/nsRubyBaseFrame.h
>+++ b/layout/generic/nsRubyFrame.cpp
>+++ b/layout/generic/nsRubyFrame.h

These bits don't really seem related to the anon pseudos.  Do they really belong in this patch?  Or in part 1?

>+++ b/layout/style/nsStyleStruct.h
>+  bool IsRubyStyle() const {
>+    return NS_STYLE_DISPLAY_RUBY == mPosition ||

IsRubyDisplayType(), and mDisplay, not mPosition.

Again, r=me but I'd like to see an updated patch (ideally along with an interdiff) and I'm pretty sure the frame class changes don't belong in this changeset.
Attachment #8447444 - Flags: review?(bzbarsky) → review+
Comment on attachment 8447445 [details] [diff] [review]
ruby-pairing.patch v2 (part 4)

>+    ParentType parentType = GetParentType(aParentFrame);
>+    if (parentType == eTypeRuby) {

This seems like a convoluted way of saying aParentFrame->GetType() == nsGkAtoms::rubyFrame.

>+      EnsureValidRubySegments(aParentFrame, aState, aFrameItems);

What handles updating the ruby segment stuff on dynamic insertions/removals of frames?  We could really use testcases here; unfortunately it's hard to write tests for frame structure directly.  :(

Inserting an anonymous ruby base container before a ::before styled with display:ruby-text-container seems like it will confuse nsLayoutUtils::GetBeforeFrame.  Either we need to not insert this anonymous frame and handle the pairing at layout time instead of frame construction time, or we need to fix at least GetBeforeFrame.

My preference, honestly, would be to handle pairing at layout time if that's workable.  That would also address my concern above about dynamic insertions/removals.

I'm going to review the remaining code here, but I'm really not totally convinced this is the right approach.

Even if we do keep this approach, why are we not doing this on frame construction item lists instead of frame lists?  Just because we have to dig around in the ruby containers and might not have frame construction items for their kids yet?

>+nsIFrame* nsCSSFrameConstructor::InsertAnonymousRuby(ParentType aRubyType,

This is an odd name for the function, since it doesn't insert anything anywhere.  Perhaps CreateAnonymousRubyFrame or something?

>+                                                     nsIAtom* aAnonBoxType,

aAnonBoxType can be gotten from aRubyType via *sPseudoParentData[aRubyType].mPseudoType, no?  And in fact you do that later in this function, looks like.

>+  FrameConstructionItem* newItem =
...
>+  FrameConstructionItemList newItems;
>+  FCItemIterator iter(newItems);
>+  iter.InsertItem(newItem);

How about:

  FrameConstructionItemList newItems;
  newItems.AppendItem(...);

instead of those four lines?  And then you can ConstructFramesFromItemList() instead of creating an iterator.

>+void nsCSSFrameConstructor::EnsureValidRubySegment(nsFrameList::Enumerator& e,
>+  nsContainerFrame* baseContainer = do_QueryFrame(e.get());

Assert that this is a ruby base container (via GetType()), since I assume it is?

>+  int maxAnnotations = 1;

This variable could use a comment explaining what it means.

I think a bunch of this code might have ended up a bit simpler if CreateAnonymousRubyFrame put the new frame into a passed-in nsFrameList instead of returning it.

>+    newTextContainer->AppendFrames(kPrincipalList, textList);

Shouldn't this be SetInitialChildList?

>+  while(!e.AtEnd() && e.get()->GetType() != nsGkAtoms::rubyBaseContainerFrame) {

Space before '('.

>+    // Text frames which are direct children of the ruby box get left in-flow
>+    // as-is.

Why are there textframes there at all?

>+      int numChildren = 0;
>+      nsFrameList::Enumerator textEnum(e.get()->PrincipalChildList());
>+      while (!textEnum.AtEnd()) {
>+        numChildren++;
>+        textEnum.Next();
>+      }

  int32_t numChildren = e.get()->PrincipalChildList().GetLength();

>+  nsFrameList::Enumerator baseEnum(baseContainer->PrincipalChildList());
>+  while (!baseEnum.AtEnd()) {

Again, just use GetLength().

>+  if (numBases < maxAnnotations) {
>+    for (int i = 0; i < maxAnnotations - numBases; i++) {

  for (; numBases < maxAnnotations; numBases++)
Attached patch ruby-frames.patch v5 (obsolete) — Splinter Review
Attachment #8447443 - Attachment is obsolete: true
Attachment #8447443 - Flags: review?(dholbert)
Attached patch ruby-frames.patch v6 (part 1) (obsolete) — Splinter Review
Moving IsFrameOfType overrides from part 2 to part 1.
Attachment #8448090 - Attachment is obsolete: true
Attached patch ruby-anon.patch v4 (part 2) (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #20)
> Comment on attachment 8447444 [details] [diff] [review]
> ruby-anon.patch v3 (part 2)
> >+        // Don't group ruby base boxes and ruby annotation boxes together
> >+        if (ourParentType == eTypeRuby && (prevParentType != groupingParentType)) {
> >+          break;
> 
> I don't think this is correct.
> 
> Say we have this:
> 
>   <div style="display: ruby">
>     Some text
>     <div style="display: ruby-base">
>   </div>
> 
> In this case ourParentType is eTypeRuby, groupingParentType is eTypeBlock,
> and prevParentType is eTypeRubyBaseContainer.  But we want to group these
> two together.

I don't think the two should be grouped together. The spec doesn't say what to do with text frames that are direct children of ruby boxes. My intent was to leave them there and print them inline, just like ruby bases, but without wrapping them in a base. But maybe this is a weird approach, so I'll ask about it. There's definitely a bug with this conditional in either case. (Leaving it as is until I figure out what the behavior should actually be.)

> Nothing has a eTypeRubyBase or eTypeRubyText desired parent type, as far as
> I can tell, so those can't be values of groupingParentType.  Furthermore,
> there can be kids of a eTypeRuby (like text, say) that cause
> groupingParentType to be set to eTypeBlock.

You're right.

> >+        } else {
> >+          iter = endIter;
> >+          continue;
> >+        }
> 
> I'm not sure I follow this bit.  what is it trying to do?  At first glance
> what it does is skip wrapping things between iter and endIter altogether,
> which seems wrong to me.  I think this block is not needed at all...

This is because of my earlier mentioned intent to leave other types of frames inside a ruby box alone.

> >     if (pseudoType == nsCSSAnonBoxes::table &&
> >-        parentStyle->StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_INLINE) {
> >+        (parentStyle->StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_INLINE ||
> >+         IsRubyParentType(ourParentType))) {
> >       pseudoType = nsCSSAnonBoxes::inlineTable;
> 
> Ah, this is why you had ruby-base and ruby-text as plausible return values
> from GetParentType(nsIFrame*)?

Yes, that's why I needed it. But sure, I can change the conditional here instead.
 
> >+++ b/layout/generic/nsFrame.cpp
> >+++ b/layout/generic/nsRubyBaseContainerFrame.cpp
> >+++ b/layout/generic/nsRubyBaseContainerFrame.h
> >+++ b/layout/generic/nsRubyBaseFrame.cpp
> >+++ b/layout/generic/nsRubyBaseFrame.h
> >+++ b/layout/generic/nsRubyFrame.cpp
> >+++ b/layout/generic/nsRubyFrame.h
> 
> These bits don't really seem related to the anon pseudos.  Do they really
> belong in this patch?  Or in part 1?

Yep, they should go in part 1.
Attachment #8447444 - Attachment is obsolete: true
> The spec doesn't say what to do with text frames that are direct children of ruby boxes.

Yes, it does.  http://dev.w3.org/csswg/css-ruby/#box-fixup step 3:

  Any consecutive sequence of text and inline-level boxes directly parented by a ruby
  container or ruby base container is wrapped in an anonymous ruby base.
Attached patch ruby-anon.patch v5 (part 2) (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #25)
> > The spec doesn't say what to do with text frames that are direct children of ruby boxes.
> 
> Yes, it does.  http://dev.w3.org/csswg/css-ruby/#box-fixup step 3:
> 
>   Any consecutive sequence of text and inline-level boxes directly parented
> by a ruby
>   container or ruby base container is wrapped in an anonymous ruby base.

Ah, okay. That area of the spec has been updated since I last looked for that information. So, you're right about how to handle the grouping for that.
Attachment #8448131 - Attachment is obsolete: true
Attachment #8448187 - Flags: review?(bzbarsky)
Comment on attachment 8448187 [details] [diff] [review]
ruby-anon.patch v5 (part 2)

>+nsCSSFrameConstructor::CreateNeededPseudos(nsFrameConstructorState& aState,
>+      default: 
>+        if (IsRubyParentType(groupingParentType)) {

Again, please assert that ourParentType == eTypeBlock?

> nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState& aState,
>+#ifdef DEBUG

No need to add that around an NS_ASSERTION.

>+++ b/layout/style/nsStyleStruct.h
>+           NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER;

Missing " == mDisplay"?

r=me with those nits fixed.
Attachment #8448187 - Flags: review?(bzbarsky) → review+
Comment on attachment 8447445 [details] [diff] [review]
ruby-pairing.patch v2 (part 4)

Per IRC discussion, let's try doing this during reflow.
Attachment #8447445 - Flags: review?(bzbarsky)
Attached patch ruby-anon.patch v6 (part 2) (obsolete) — Splinter Review
Attachment #8448187 - Attachment is obsolete: true
Attachment #8448834 - Flags: review?(bzbarsky)
Needed to rebase this patch because of changes in ruby-anon.
Comment on attachment 8448838 [details] [diff] [review]
ruby-style-fixups.patch v2 (part 3)

This doesn't seem to have addressed my review comments so far, right?
Comment on attachment 8448834 [details] [diff] [review]
ruby-anon.patch v6 (part 2)

r=me
Attachment #8448834 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 8448838 [details] [diff] [review]
> ruby-style-fixups.patch v2 (part 3)
> 
> This doesn't seem to have addressed my review comments so far, right?

Oops, don't know how I missed those. I'll have a new patch version up in a second. But first can I get a clarification on this review comment:

>I'm not sure this makes sense to do like this.  For the ruby case, we don't care about
>non-elements, since those are inline anyway.  Furthermore, sicne your switch does not have a
>default case, you don't care about skipping over table parts as well: those will just get ignored
>by EnsureInlineDisplay (this is worth documenting in EnsureInlineDisplay).  And the table-part
>ignoring is the only part you're ending up sharing.  So I think this shoud just be a separate if
>statement, not combined with the flex-or-grid one.

I'm just not sure what you're saying here. What do you mean by a separate if statement? Do you think the GetPseudo() check is completely unnecessary?
> What do you mean by a separate if statement? 

I mean something like this:

    if ((parentDisp->mDisplay == NS_STYLE_DISPLAY_FLEX ||
         parentDisp->mDisplay == NS_STYLE_DISPLAY_INLINE_FLEX ||
         parentDisp->mDisplay == NS_STYLE_DISPLAY_GRID ||
         parentDisp->mDisplay == NS_STYLE_DISPLAY_INLINE_GRID) &&
        GetPseudo() != nsCSSAnonBoxes::mozNonElement) {
      /* current code */
    } else if (parentDisp->IsRubyDisplayType()) {
      /* Your new code */
    }

> Do you think the GetPseudo() check is completely unnecessary?

It's necessary in case when the display is being blockified, because we don't want to do that with text.  But in the case when display is being converted to inline, that's fine: text is already inline.
Okay, I think I see what you mean now. Here's the updated version with your review comments addressed.
Attachment #8441472 - Attachment is obsolete: true
Attachment #8448838 - Attachment is obsolete: true
Attachment #8448870 - Flags: review?(bzbarsky)
Comment on attachment 8448870 [details] [diff] [review]
ruby-style-fixups.patch v3 (part 3)

r=me
Attachment #8448870 - Flags: review?(bzbarsky) → review+
Comment on attachment 8448130 [details] [diff] [review]
ruby-frames.patch v6 (part 1)

One more nit on part 1: we should be adding a line for the new pref (setting it to false) in modules/libpref/src/init/all.js, as mentioned on the recently-created MDN documentation for new about:config preferences:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Preferences#How_to_add_new_preference

(I'd previously suggested leaving all.js untouched, in the equivalent patch for CSS grid, towards the end of bug 975501 comment 6.  But now that we have a more formalized policy, we should follow it. (and incidentally, the grid pref has now been added to all.js, in a different bug. You probably want to add your new pref line somewhere near that one.))

Also, while I'm here:
 * It looks like the indentation tweaks around the "friend" lines (per comment 19) are still needed, and so are the things mentioned in comment 7.
 * I'd suggest splitting out (& holding off on) this patch's changes to nsCSSFrameConstructor::CreateContinuingFrame() -- those changes slow down that method a tiny bit, and they only matter if a frame's Reflow() method can return a non-NS_FRAME_COMPLETE nsReflowStatus (which would indicate that the frame is asking its parent for a continuation, e.g. if it's being split across a line- or page-boundary).  That doesn't apply to ruby frames yet, as of the patches here & in bug 1030993. Once their reflow methods get advanced enough to return non-NS_FRAME_COMPLETE statuses, we can update CreateContinuingFrame() then -- but until then, it's probably better that we leave out these unneeded (for now) checks.

(With those nits addressed, this is probably r=me, but I'll hold off on granting official r+ until seeing the next patch version, to get a better idea of the final thing & to see any other changes that you have pending.)
(Actually, never mind about the CreateContinuingFrame bullet-point in previous comment -- I forgot that we're inheriting from nsBlockFrame now, and its reflow method can return NS_FRAME_COMPLETE & request a continuation.  So the CreateContinuingFrame clauses can potentially be exercised after all.)
(er, meant to say "...and its reflow can return *non*-NS_FRAME_COMPLETE & request a continuation")
Note that the ruby-anon patch doesn't produce the frame structure the spec currently expects (even ignoring the whitespace bits), but that it's also not clear that the spec is stable here....
Attached patch ruby-frames.patch v7 (part 1) (obsolete) — Splinter Review
Okay, those nits should finally be addressed. Also, nsRubyTextFrame is now inline again. The only block frame class is now nsRubyTextContainerFrame.

Worth noting that the last time I did a try run of this, it failed some mochitests. So I'm going to look into that next.
Attachment #8448130 - Attachment is obsolete: true
Attachment #8450350 - Flags: review?(dholbert)
bz: Could you give a test case or an example case where the spec and the patch differ? Last I heard, the spec for anonymous box generation should be fairly stable now.
Flags: needinfo?(bzbarsky)
You should really follow www-style.  See in particular http://lists.w3.org/Archives/Public/www-style/2014Jul/0030.html and item #1 in my response to it at http://lists.w3.org/Archives/Public/www-style/2014Jul/0037.html and the rest of that thread...
Flags: needinfo?(bzbarsky)
Comment on attachment 8450350 [details] [diff] [review]
ruby-frames.patch v7 (part 1)

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

The commit message currently has "r=dholbert,dbaron", but I don't think dbaron's reviewed/reviewing this, right?

(I'm also going to defer to bz on a few points about the FCDATA_DECL() expressions, so this probably should end up as "r=dholbert,bz".)

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4538,5 @@
> +      FCDATA_DECL(FCDATA_IS_INLINE | FCDATA_IS_LINE_PARTICIPANT,
> +                  NS_NewRubyFrame) },
> +    { NS_STYLE_DISPLAY_RUBY_BASE,
> +      FCDATA_DECL(FCDATA_IS_INLINE | FCDATA_IS_LINE_PARTICIPANT,
> +                  NS_NewRubyBaseFrame) },

I'm not 100% clear on how much it matters (and how appropriate it is) to have the IS_INLINE / IS_LINE_PARTICIPANT on the various intermediate ruby frames  (and the related IsFrameOfType flags), since these frames don't directly interact with any non-Ruby frames, and as fantasai notes on www-style, it's not clear that they're exactly "inline-level" ( http://lists.w3.org/Archives/Public/www-style/2014Jul/0032.html ).

But mimicking nsInlineFrame is a reasonable starting point, and I imagine nsLineLayout will still see these frames and expect them to behave like inlines. So this probably makes sense.

I'll defer to bz as a sanity-check that these flags make sense, though.

@@ +4546,5 @@
> +    { NS_STYLE_DISPLAY_RUBY_TEXT,
> +      FCDATA_DECL(FCDATA_IS_INLINE | FCDATA_IS_LINE_PARTICIPANT,
> +                  NS_NewRubyTextFrame) },
> +    { NS_STYLE_DISPLAY_RUBY_TEXT_CONTAINER,
> +      FCDATA_DECL(0, NS_NewRubyTextContainerFrame) },

Now that nsRubyTextContainerFrame inherits from nsBlockFrame, I think frame construction is a bit trickier for it, though I'm not 100% clear on how. I do know that we don't have any FCDATA_DECL lines that use NS_NewBlockFrame, and I know some blocks expect to have ConstructBlock() called on them for some post-processing, so that leads me to believe that we need something more complex here as well.

I'm going to defer to bz on this part, since I suspect he knows whether this would be sufficient or if we need something more complex.

@@ +8328,5 @@
>      newFrame = NS_NewFlexContainerFrame(shell, styleContext);
>      newFrame->Init(content, aParentFrame, aFrame);
> +    //TODO: Add conditionals for rubyFrame and rubyBaseContainerFrame
> +      // once their reflow methods are advanced enough to return
> +      // non-complete statuses

nit: indentation is a bit broken here. De-indent those last two lines.

::: layout/generic/nsRubyBaseContainerFrame.cpp
@@ +4,5 @@
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */
> +
> +/* rendering object for CSS "display: ruby | ruby-base | ruby-base-container |
> + * ruby-text | ruby-text-container" */

This comment still needs updating, in the .h & .cpp files for the new frame classes, per end of comment 7.

@@ +8,5 @@
> + * ruby-text | ruby-text-container" */
> +
> +#include "nsRubyBaseContainerFrame.h"
> +
> +#include "nsCSSAnonBoxes.h"

All of your new *Frame.cpp files have this #include for nsCSSAnonBoxes.h, but I don't think they actually need it.  Can it be removed?

@@ +45,5 @@
> +
> +bool 
> +nsRubyBaseContainerFrame::IsFrameOfType(uint32_t aFlags) const 
> +{
> +  return nsContainerFrame::IsFrameOfType(aFlags & 

Delete end-of-line whitespace after "bool", "const", and "&" there.

(I think this applies to all of the IsFrameOfType() impls in the various nsRuby*Frame.cpp methods -- they all have the same end-of-line whitespace.)

::: layout/generic/nsRubyTextContainerFrame.cpp
@@ +49,5 @@
> +{
> +  return MakeFrameName(NS_LITERAL_STRING("RubyTextContainer"), aResult);
> +}
> +#endif
> +

nit: there's an extra blank line at the end of this file. Remove it.

::: layout/style/nsCSSProps.cpp
@@ +1079,4 @@
>    // The next two entries are controlled by the layout.css.grid.enabled pref.
> +  eCSSKeyword_grid,                NS_STYLE_DISPLAY_GRID,
> +  eCSSKeyword_inline_grid,         NS_STYLE_DISPLAY_INLINE_GRID,
> +  eCSSKeyword_ruby,                NS_STYLE_DISPLAY_RUBY,

Add a comment mentioning the pref here, like the existing comment for grid. (so, "// The next five entries...")
Attachment #8450350 - Flags: review?(dholbert)
Attachment #8450350 - Flags: review?(bzbarsky)
Attachment #8450350 - Flags: review+
[bz, I'm punting review to you on two points that I'm not sure about; see instances of "defer to bz" in previous comment]
Here's the latest try run of the part 1 patch:
https://tbpl.mozilla.org/?tree=Try&rev=82975ffb6d5d
With the logcat of the failing mochi-test on Android:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/229079fd93162141328cb26733b4f558f3f7a7ddd7e01cbe8cfdc3795ffd7c74b2b1f8aee9fbbb12904bc00820ae6b547d7bf62a8c1514cbe603290a8be72d8c

bz: Do you have any ideas about these assertions and why they might be triggered? Neither I nor dholbert recognize them. And is there a way to get a backtrace for them?
The assertions in question are:
{
07-03 15:05:31.187 I/GeckoDump( 2242): 970 INFO TEST-START | /tests/layout/style/test/test_root_node_display.html
07-03 15:05:31.250 I/Gecko   ( 2242): ++DOMWINDOW == 29 (0x6eb67380) [pid = 2242] [serial = 1005] [outer = 0x664ba100]
07-03 15:05:31.750 I/Gecko   ( 2242): [2242] ###!!! ASSERTION: Unexpected document; this will lead to incorrect behavior!: 'aElement->GetCrossShadowCurrentDoc() == Document()', file /builds/slave/try-and-d-00000000000000000000/build/layout/base/RestyleTracker.cpp, line 246
07-03 15:05:31.750 I/Gecko   ( 2242): [2242] ###!!! ASSERTION: root of native anonymous subtree must have parent equal to binding parent: '!IsRootOfNativeAnonymousSubtree() || (GetParent() && GetBindingParent() == GetParent())', file ../../../dist/include/nsIContent.h, 
}

That pair of assertions appears to repeat 20 times.
I don't know how to get anything useful out of the android/b2g stuff.  :(

As far as what would trigger them, the most obvious thing would be trying to restyle elements that are not in a document, for the first one.  For the second one, could be various issues with anonymous content (not properly unbinding it, etc).  But I'm not sure why they are only relevant here.  And only on Android!
Question for bz:
Can you confirm the following idea/approach is good to implement in the CSS frame constructor code?

I'd like to handle whitespace as described in the spec (dev.w3.org/csswg/css-ruby/#box-fixup -- particularly points 4 and 5). Currently, CreateNeededPseudos will remove both the whitespace described in point 4 and point 5. I would like it to leave the whitespace described in point 5 alone, and I think the best way to do this is if such frames are treated exactly like any other frame with a desired parent type of RBC/RTC/Ruby, depending on whether it is inter-base, inter-annotation, or inter-segment respectively.

I think the best approach to this is a pre-processing step and a change to the way the desired parent type is stored. I would create a member variable in the FrameConstructionItem class called mDesiredParentType and have the corresponding FCdata bits stored there on construction. Then, before CreateNeededPseudos is called, I would walk through the items in the aItems list and set mDesiredParentType for these special white space cases (depending on the display type for the previous and next frame construction items). A few changes might be necessary for the logic in CreateNeededPseudos, but it should not be too difficult from there.

What are your thoughts on this approach?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #48)
> As far as what would trigger them, the most obvious thing would be trying to
> restyle elements that are not in a document, for the first one.

The mochitest (test_root_node_display.html) does restyle two elements, though they are in a document. It sets .style.display = $WHATEVER for document.documentElement, as well as for a floated element in a display:none subtree.  (where $WHATEVER is presumably one of the ruby display types, in this case)

Here's the mochitest's source, for the record / for convenience:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_root_node_display.html?force=1
Comment on attachment 8450350 [details] [diff] [review]
ruby-frames.patch v7 (part 1)

I'm sorry for the lag here.  :(  I had overestimated the amount of patch I'd need to read here.

So I think using FCDATA_IS_INLINE is actually wrong for all of these.  You don't need the eager child frame construction item creation that triggers, I believe, and you also don't need to be calling BuildInlineChildItems.  I _think_ it'll more or less do the right thing because of the display value munging we do on kids of a ruby-*, but we really shouldn't go into BuildInlineChildItems with anything other than a non-replaced display:inline.

FCDATA_IS_LINE_PARTICIPANT needs to be set if and only if IsFrameOfType(nsIFrame::eLineParticipant).  The other places (other than the assert that it matches IsFrameOfType()) we use the bit are:

1)  In NeedsAnonFlexOrGridItem().  This will only happen if the parent is a flex-or-grid and will happen after anonymous pseudo construction, so won't ever see internal ruby items, right?

2)  For maintaining the parent's mLineParticipantCount.  This is used in AnyItemsNeedBlockParent(), which is only used nsCSSFrameConstructor::ProcessChildren when the parent is a XUL box.  Again this happens after anonymous pseudo construction, so from a frame construction point of view it doesn't matter what this is set to on the internal items.

So we do need FCDATA_IS_LINE_PARTICIPANT on the NS_STYLE_DISPLAY_RUBY bit.  For the others, it depends on whether we want IsFrameOfType(nsIFrame::eLineParticipant) to be true in other parts of the codebase.

As far as blocks go, the main reason for ConstructBlock is to handle columnnation.  As long as we don't support columnation of nsRubyTextContainerFrame, I don't think we have to go through ConstructBlock for it.  An existing example of such a thing is NS_NewXULDescriptionFrame, which is used directly in an fcdata struct and just calls NS_NewBlockFrame.

That said, if you're going to inherit from nsBlockFrame you need to do some testing of floating kids of your frame to see what happens.  Especially since you're not setting your frame as a block formatting context root...  Should check the spec to see what behavior is really wanted here and whether this patch produces it.

Also, FCDATA_DECL(0, foo) is the same as SIMPLE_FCDATA(foo), and I'd somewhat prefer the latter version.

r=me on the frame constructor changes modulo the above.
Attachment #8450350 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #48)
> I don't know how to get anything useful out of the android/b2g stuff.  :(
> 
> As far as what would trigger them, the most obvious thing would be trying to
> restyle elements that are not in a document, for the first one.  For the
> second one, could be various issues with anonymous content (not properly
> unbinding it, etc).  But I'm not sure why they are only relevant here.  And
> only on Android!

Here's the stacktrace for the first assertion failure (the one in RestyleTracker.cpp): https://pastebin.mozilla.org/5555234. The element the assertion is failing on is a scrollbar, and GetCrossShadowDoc() is returning null (and, thus, not the same as this->Document()) since both IsInShadowTree() and IsInUncomposedDoc() are false. bz: Do you know what either of these mean?

Also note that there are 8 assertion failures for each ruby frame class. So each of the ruby frame classes are doing something identically wrong. The assertion failures appear to occur at this (http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_root_node_display.html?force=1&mark=34#34) line of the mochi-test.

I have the changes from your comment 51 (I don't think any were relevant to the Android bug), but I'd like to hold off on posting a new version until we have this bug fixed, if that's okay with you.
Flags: needinfo?(bzbarsky)
> bz: Do you know what either of these mean?

They mean, between the two of them, "actually in the tree of nodes we're trying to render".  This scrollbar sounds like it's been unbound from the document, but we're still trying to restyle it for some reason.

> layout/style/test/test_root_node_display.html

Aha!  That's useful info!  So there's a good chance these are viewport scrollbars or something.

So that raises a spec question: what should happen to root elements which have one of the ruby display types?  CSS2.1 says "others" are preserved, but I don't think it makes sense to do that.  Similarly for abspos/floated ruby boxes.  Elika?
Flags: needinfo?(bzbarsky) → needinfo?(fantasai.bugs)
I sent http://lists.w3.org/Archives/Public/www-style/2014Jul/0228.html

We should probably also add tests for floated/abspos/fixedpos ruby bits, if we don't have any already.
And more to the point, we should have a test for CSS 2.1 section 9.7 in general, but one that uses all the values of "display" we have in our property database.
And we should make EnsureBlockDisplay() in a debug build assert if it's handed an unknown display type.
The assertion failures don't appear to be unique to ruby elements. See bug 1038905. For now, my workaround is just to add the ruby display values to the front of the "other_values" list in property_database.js, rather than the back.

Regarding EnsureBlockDisplay(), are you saying we should add cases to the switch for all display values? The default case seems fine. Not sure what you're getting at.

For the tests we should add, I take it you'd like those to be in the ruby-frames patch? What's the process of adding those? I've made reftests before, but not any other kinds of tests.
> are you saying we should add cases to the switch for all display values?

#ifdef DEBUG, yes.  And assert when a new one comes in.

> The default case seems fine. Not sure what you're getting at.

It's fine for our purposes but only because it doesn't match the spec.  The default case in the spec is "do nothing".  Hence we want to detect when new display types are being added so we can check that their specs define what EnsureBlockDisplay should do on them, because our default behavior doesn't match the spec's.

Followup/separate bug is totally fine for this part.

> For the tests we should add, I take it you'd like those to be in the ruby-frames patch?

Nah, followup/separate bug for that is OK too.  That's assuming we don't already have them, though at first glance we do not.

You'd want a mochitest similar to test_root_node_display.html but basically testing that setting float:left/right and position:absolute/fixed blockify all display values.  And also expand test_flexbox_child_display_values.xhtml (which should perhaps be using  gCSSProperties["display"].other_values instead of hardcoding a list).
>It's fine for our purposes but only because it doesn't match the spec.  The default case in the spec >is "do nothing".  Hence we want to detect when new display types are being added so we can check that >their specs define what EnsureBlockDisplay should do on them, because our default behavior doesn't >match the spec's.

I'm confused...why is it fine if it /doesn't/ match the spec? Are we talking about the CSS 2.1 spec or the CSS ruby spec?

Also would we want the followup bugs for these tests to block this bug?

(In reply to Boris Zbarsky [:bz] from comment #51)
> That said, if you're going to inherit from nsBlockFrame you need to do some
> testing of floating kids of your frame to see what happens.  Especially
> since you're not setting your frame as a block formatting context root... 
> Should check the spec to see what behavior is really wanted here and whether
> this patch produces it.

To which spec are you referring? What kind of testing should I be doing? Are there examples?
Sorry for forgetting to make these changes in a new patch, as I just remembered you prefer that. I'll remember next time!

Anyway, this should address what you said in comment 51 and the Android bug I was seeing.
Attachment #8450350 - Attachment is obsolete: true
Attachment #8456439 - Flags: review?(bzbarsky)
> Are we talking about the CSS 2.1 spec or the CSS ruby spec?

"Yes".  Though I believe the ruby spec here is buggy; hence my post to www-style.  If that change is accepted, we'll just not match CSS2.1, which is ok in general because we'd only not match it for any _new_ display types that got added.  Which is why we should assert when a new type is added.

> Also would we want the followup bugs for these tests to block this bug?

No, just do them at some point, in parallel with this bug or after.

> To which spec are you referring?

The ruby spec.

> What kind of testing should I be doing?

Put a float inside a ruby-base or ruby-text.  Where does i float to, exactly?  What does it use as the containing block?

> Are there examples?

No, just need to write some HTML that would exercise floats inside the various ruby frame types.
Comment on attachment 8456439 [details] [diff] [review]
ruby-frames.patch v8 (part 1)

r=me

A followup for figuring out the story for layout of floats inside these frames is fine as long as it blocks turning this stuff on by default.  This bug is getting too long.  ;)
Attachment #8456439 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #61)
> > Are we talking about the CSS 2.1 spec or the CSS ruby spec?
> 
> "Yes".  Though I believe the ruby spec here is buggy; hence my post to
> www-style.  If that change is accepted, we'll just not match CSS2.1, which
> is ok in general because we'd only not match it for any _new_ display types
> that got added.  Which is why we should assert when a new type is added.
> 
> > Also would we want the followup bugs for these tests to block this bug?
> 
> No, just do them at some point, in parallel with this bug or after.
> 
> > To which spec are you referring?
> 
> The ruby spec.
> 
> > What kind of testing should I be doing?
> 
> Put a float inside a ruby-base or ruby-text.  Where does i float to,
> exactly?  What does it use as the containing block?
> 
> > Are there examples?
> 
> No, just need to write some HTML that would exercise floats inside the
> various ruby frame types.

The ruby spec doesn't mention anything about floating children.

Isn't where the child floats dependant on the implementation of reflow code for its parent? I.e., isn't it something that should be handled in a later patch? (some time after the patch in bug 1030993 for instance)
(In reply to sgbowen from comment #63)
> Isn't where the child floats dependant on the implementation of reflow code
> for its parent?

Not necessarily, if its parent isn't its containing block. For example, if an element is supposed to float out of its ruby parent & up to the nearest block, then (depending on how it's specced) its layout could be pretty independent from ruby layout. (similar to how an floated child of a <span> doesn't impact or participate in the <span>'s layout, really)  We use "PushFloatContainingBlock" in nsCSSFrameConstructor.cpp to establish float containing blocks for descendants.  If we knew the right floating behavior, we could hypothetically add some calls to that method (or rely on existing calls, if they do the right thing) to implement correct float behavior for ruby, without needing ruby reflow methods yet.

But in any case, given the length of this bug, & that this float stuff isn't really specced, & that we've got reasonable starting behavior here, & that this is disabled-by-default right now anyway: yes, let's punt on worrying about float layout to a followup bug. :)

Good work tracking down the source of the assertion failure! That was a nasty pitfall (particularly with the android-dependence); well done getting through it. :) Let's get this landed & close this out!
Blocks: 1039017
This is just rebased on top of the ruby-frames.patch (part 1). There should be nothing new here.
Attachment #8448834 - Attachment is obsolete: true
Attachment #8458233 - Flags: review?(bzbarsky)
This is also just a rebase.

Here's the try run showing the first 3 parts passing everything:
https://tbpl.mozilla.org/?tree=Try&rev=3d4eb76bb43d

I think this should be ready to check-in.
Attachment #8448870 - Attachment is obsolete: true
Attachment #8458234 - Flags: review?(bzbarsky)
Comment on attachment 8458233 [details] [diff] [review]
ruby-anon.patch v7 (part 2)

r=me based on diff of diffs.
Attachment #8458233 - Flags: review?(bzbarsky) → review+
Comment on attachment 8458234 [details] [diff] [review]
ruby-style-fixups.patch v4 (part 3)

Per IRC discussion, this is wrong, since it won't make a block kid of ruby-text-container be inline.  Need testcases.  ;)
Attachment #8458234 - Flags: review?(bzbarsky) → review-
Fix the problem with ruby-style-fixups.

(Note I'm obsoleting the previous part 4 patch here since that was obsolete a long time ago.)
Attachment #8447445 - Attachment is obsolete: true
Attachment #8458252 - Flags: review?(bzbarsky)
(Being bad at bugzilla. Patch should be non-empty now.)
Attachment #8458252 - Attachment is obsolete: true
Attachment #8458252 - Flags: review?(bzbarsky)
Attachment #8458255 - Flags: review?(bzbarsky)
Comment on attachment 8458255 [details] [diff] [review]
ruby-display-type.patch v1 (part 4)

Please do add the comments I asked for on IRC.

r=me with those added.
Attachment #8458255 - Flags: review?(bzbarsky) → review+
Added comments.
Attachment #8458255 - Attachment is obsolete: true
Attachment #8458281 - Flags: review?(bzbarsky)
Comment on attachment 8458281 [details] [diff] [review]
ruby-display-type.patch v2 (part 4)

r=me
Attachment #8458281 - Flags: review?(bzbarsky) → review+
bz: Would you mind changing the part 3 patch from review- to review+, since the issues with it were fixed by patch 4?
Flags: needinfo?(bzbarsky)
Comment on attachment 8458234 [details] [diff] [review]
ruby-style-fixups.patch v4 (part 3)

Sure.  Probably best to fold patch 4 into this one...
Attachment #8458234 - Flags: review- → review+
Folded in part 4, as suggested. So this is now just part 3 v4 + part 4 v2.
Attachment #8458234 - Attachment is obsolete: true
Attachment #8458281 - Attachment is obsolete: true
Attachment #8458848 - Flags: review?(bzbarsky)
Comment on attachment 8458848 [details] [diff] [review]
ruby-style-fixups.patch v5 (part 3)

r=me, but no real need to ask for review that qfold worked correctly....  I mostly trust it.  ;)
Attachment #8458848 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(bzbarsky)
This should be ready for #checkin-needed, but I'm waiting until after Monday's merge.
Flags: needinfo?(sgbowen8)
Flags: needinfo?(sgbowen8)
Keywords: checkin-needed
These patches don't seem to apply cleanly.
Flags: needinfo?(sgbowen8)
Keywords: checkin-needed
(In reply to Wes Kocher (:KWierso) from comment #79)
> These patches don't seem to apply cleanly.

They apply cleanly for both me and dholbert on the mozilla-inbound tip. Are you sure you applied them in order (by part number)? If so, which one is failing?
Flags: needinfo?(sgbowen8) → needinfo?(kwierso)
(In reply to sgbowen from comment #80)
> (In reply to Wes Kocher (:KWierso) from comment #79)
> > These patches don't seem to apply cleanly.
> 
> They apply cleanly for both me and dholbert on the mozilla-inbound tip. Are
> you sure you applied them in order (by part number)? If so, which one is
> failing?

Yeah, mq must've been trying to apply them backwards.
Flags: needinfo?(kwierso)
I did need to edit the commit messages for two of those patches to get past the commit hooks rejecting commits without bug numbers or reviewers, for the record.
Depends on: 1043706
[Clearing needinfo request, since the spec should have been clarified. Bug me if it's still a problem.]
Flags: needinfo?(fantasai.bugs)
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.