1021952 - Implement ruby "display" values with stub frame classes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[sgbowen]

Attachment: patch v1

Patch that adds the ruby display values and stubs for the frame classes.

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[sgbowen]

Attachment: patch v2

Addressed the problems in the previous patch with the exception of the FCDATA confusion (since we're still not sure what to do there).

Comment 4

[Daniel Holbert [:dholbert]]

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)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[sgbowen]

Attachment: patch v3

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 7

[Daniel Holbert [:dholbert]]

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 8

[Daniel Holbert [:dholbert]]

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]

Comment 9

[sgbowen]

Attachment: ruby-anon.patch v1

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.

Comment 10

[sgbowen]

Attachment: ruby-anon.patch v2

Comment 11

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days to get to this, unfortunately.  I have a bit of a review backlog right now.  :(

Comment 12

[sgbowen]

Attachment: ruby-style-fixups.patch v1 (part 3)

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.

Comment 13

[sgbowen]

Attachment: ruby-pairing.patch v1

This patch is to ensure that the conditions in section 2.3 of the spec are met.

Comment 14

[Boris Zbarsky [: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.

Comment 15

[sgbowen]

Attachment: ruby-frames.patch v4 (part 1)

Most of the changes here relate to changing ruby text container frames and ruby text frames to blocks.

Comment 16

[sgbowen]

Attachment: ruby-anon.patch v3 (part 2)

Comment 17

[sgbowen]

Attachment: ruby-pairing.patch v2 (part 4)

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Daniel Holbert [:dholbert]]

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 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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++)

Comment 22

[sgbowen]

Attachment: ruby-frames.patch v5

Comment 23

[sgbowen]

Attachment: ruby-frames.patch v6 (part 1)

Moving IsFrameOfType overrides from part 2 to part 1.

Comment 24

[sgbowen]

Attachment: ruby-anon.patch v4 (part 2)

(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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 26

[sgbowen]

Attachment: ruby-anon.patch v5 (part 2)

(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.

Comment 27

[Boris Zbarsky [: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.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447445 [details] [diff] [review]
ruby-pairing.patch v2 (part 4)

Per IRC discussion, let's try doing this during reflow.

Comment 29

[sgbowen]

Attachment: ruby-anon.patch v6 (part 2)

Comment 30

[sgbowen]

Attachment: ruby-style-fixups.patch v2 (part 3)

Needed to rebase this patch because of changes in ruby-anon.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8448834 [details] [diff] [review]
ruby-anon.patch v6 (part 2)

r=me

Comment 33

[sgbowen]

(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?

Comment 34

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 35

[sgbowen]

Attachment: ruby-style-fixups.patch v3 (part 3)

Okay, I think I see what you mean now. Here's the updated version with your review comments addressed.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8448870 [details] [diff] [review]
ruby-style-fixups.patch v3 (part 3)

r=me

Comment 37

[Daniel Holbert [:dholbert]]

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.)

Comment 38

[Daniel Holbert [:dholbert]]

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

Comment 39

[Daniel Holbert [:dholbert]]

(er, meant to say "...and its reflow can return *non*-NS_FRAME_COMPLETE & request a continuation")

Comment 40

[Boris Zbarsky [:bzbarsky]]

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....

Comment 41

[sgbowen]

Attachment: ruby-frames.patch v7 (part 1)

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.

Comment 42

[sgbowen]

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.

Comment 43

[Boris Zbarsky [: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...

Comment 44

[Daniel Holbert [:dholbert]]

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...")

Comment 45

[Daniel Holbert [:dholbert]]

[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]

Comment 46

[sgbowen]

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?

Comment 47

[Daniel Holbert [:dholbert]]

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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

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!

Comment 49

[sgbowen]

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?

Comment 50

[Daniel Holbert [:dholbert]]

(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 51

[Boris Zbarsky [:bzbarsky]]

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.

Comment 52

[sgbowen]

(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.

Comment 53

[Boris Zbarsky [: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?

Comment 54

[Boris Zbarsky [:bzbarsky]]

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.

Comment 55

[Boris Zbarsky [:bzbarsky]]

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.

Comment 56

[Boris Zbarsky [:bzbarsky]]

And we should make EnsureBlockDisplay() in a debug build assert if it's handed an unknown display type.

Comment 57

[sgbowen]

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.

Comment 58

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 59

[sgbowen]

>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?

Comment 60

[sgbowen]

Attachment: ruby-frames.patch v8 (part 1)

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.

Comment 61

[Boris Zbarsky [: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 62

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 63

[sgbowen]

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

Comment 64

[Daniel Holbert [:dholbert]]

(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!

Comment 65

[sgbowen]

Attachment: ruby-anon.patch v7 (part 2)

This is just rebased on top of the ruby-frames.patch (part 1). There should be nothing new here.

Comment 66

[sgbowen]

Attachment: ruby-style-fixups.patch v4 (part 3)

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.

Comment 67

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8458233 [details] [diff] [review]
ruby-anon.patch v7 (part 2)

r=me based on diff of diffs.

Comment 68

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 69

[sgbowen]

Attachment: ruby-display-type.patch v1 (part 4)

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.)

Comment 70

[sgbowen]

Attachment: ruby-display-type.patch v1 (part 4)

(Being bad at bugzilla. Patch should be non-empty now.)

Comment 71

[Boris Zbarsky [: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.

Comment 72

[sgbowen]

Attachment: ruby-display-type.patch v2 (part 4)

Added comments.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8458281 [details] [diff] [review]
ruby-display-type.patch v2 (part 4)

r=me

Comment 74

[sgbowen]

bz: Would you mind changing the part 3 patch from review- to review+, since the issues with it were fixed by patch 4?

Comment 75

[Boris Zbarsky [: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...

Comment 76

[sgbowen]

Attachment: ruby-style-fixups.patch v5 (part 3)

Folded in part 4, as suggested. So this is now just part 3 v4 + part 4 v2.

Comment 77

[Boris Zbarsky [: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.  ;)

Comment 78

[sgbowen]

This should be ready for #checkin-needed, but I'm waiting until after Monday's merge.

Comment 79

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

These patches don't seem to apply cleanly.

Comment 80

[sgbowen]

(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?

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/7dea1a15c29f
https://hg.mozilla.org/integration/b2g-inbound/rev/7589cc5c6787
https://hg.mozilla.org/integration/b2g-inbound/rev/a94b21d8308c

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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.

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7dea1a15c29f
https://hg.mozilla.org/mozilla-central/rev/7589cc5c6787
https://hg.mozilla.org/mozilla-central/rev/a94b21d8308c

Comment 85

[fantasai]

[Clearing needinfo request, since the spec should have been clarified. Bug me if it's still a problem.]