Currently we doesn't follow the Anonymous Ruby Box Generation in the spec for whitespace handling. We should implement that.

Fix a problem

Comment on attachment 8511605 [details] [diff] [review] patch Find some problems in this patch during testing bug 1087872.

Comment on attachment 8514790 [details] [diff] [review] patch >+ // We are on inter-segment whitespaces, which we want to >+ // create an independent ruby base container for them. s/for them/for/ r=me. Thank you for the excellent comments!

I wrap each ruby content in a <p> in ruby-whitespaces-1 to avoid touching bugs in our current reflow code. It should be safe. try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=72be6cfc431c

Comment on attachment 8521081 [details] [diff] [review] patch, r=bz Looks fine based on the Bugzilla interdiff.

Find some new problems. I should have written a more complete testcase.

Some fixes.

The tests are written in this way: there are five different elements may appear in a ruby container: rb, rt, rbc, rtc, and inline content. In each test file, there are two parts: all elements are wrapped inside a <ruby> in one part, they are directly contained by a <p> in the other part. In each part, all permutations of pairs of two adjacent elements are presented, so there are 5*5+1=26 elements. The five test files share the same structure, but with different element type order. Hopefully, those tests could cover all cases we may meet.

In addition, I don't think we can pass the tests safely without the new reflow code in bug 1052924.

Comment on attachment 8522677 [details] [diff] [review] patch 2 - misparented inline content & inter-segment whitespace Which patches does this need to be applied on top of? I want to check that things like spaceEndIter.item() are safe here, but don't have enough context in the diff to do that...

(In reply to Boris Zbarsky [:bz] from comment #15) > Comment on attachment 8522677 [details] [diff] [review] > patch 2 - misparented inline content & inter-segment whitespace > > Which patches does this need to be applied on top of? I want to check that > things like spaceEndIter.item() are safe here, but don't have enough context > in the diff to do that... Patch 1 of this bug has been landed, and AFAICS there is no other patch necessary to be applied on top of patch 2.

> Patch 1 of this bug has been landed Oh. Separate bug for the followup would have been a good idea, then.... In any case, this part of the patch: + // We are on a non-droppable whitespace, which might be an + // inter-segment whitespace. + if (groupingParentType == eTypeBlock) { + // We are on inter-segment whitespaces, which we want to + // create an independent ruby base container for. + endIter = spaceEndIter; + break; + } + ParentType nextParentType = spaceEndIter.item().DesiredParentType(); How do we know spaceEndIter is not done?

(In reply to Boris Zbarsky [:bz] from comment #17) > > Patch 1 of this bug has been landed > > Oh. Separate bug for the followup would have been a good idea, then.... > > In any case, this part of the patch: > > + // We are on a non-droppable whitespace, which might be an > + // inter-segment whitespace. > + if (groupingParentType == eTypeBlock) { > + // We are on inter-segment whitespaces, which we want to > + // create an independent ruby base container for. > + endIter = spaceEndIter; > + break; > + } > + ParentType nextParentType = > spaceEndIter.item().DesiredParentType(); > > How do we know spaceEndIter is not done? Ah... In this specific case, I think it is never done, because in a ruby container, trailing space should have been dropped in the loop above. But it might worth a comment here.

I hope to land the code patch first, and land the tests after bug 1052924. Is that ok?

Sorry for the lag here. This code is ... finicky. I'm going to try to finish this up tomorrow.

Sorry for repeatly updating the patches. I found some new problems...

(In reply to Please do not ask for reviews for a bit [:bz] from comment #20) > Sorry for the lag here. This code is ... finicky. I'm going to try to > finish this up tomorrow. That's fine. This patch is not that urgent.

Comment on attachment 8526606 [details] [diff] [review] patch 2 - misparented inline content & inter-segment whitespace >+ // Leading and trailing whitespaces of pseudo level containers >+ // are not trimmed, because according to Anonymous Ruby Box >+ // Generation of CSS Ruby, trimming happens before generation of >+ // those containers. Why is this not just a spec bug? It seems odd to me to have this behavior difference. Why is it there?

(In reply to Please do not ask for reviews for a bit [:bz] from comment #25) > Comment on attachment 8526606 [details] [diff] [review] > patch 2 - misparented inline content & inter-segment whitespace > > >+ // Leading and trailing whitespaces of pseudo level containers > >+ // are not trimmed, because according to Anonymous Ruby Box > >+ // Generation of CSS Ruby, trimming happens before generation of > >+ // those containers. > > Why is this not just a spec bug? It seems odd to me to have this behavior > difference. Why is it there? I'm not sure, but I don't think it is a spec bug. This case happens only when a whitespace is part of a "consecutive sequence of text and inline-level boxes" in ruby base container, and will be wrapped in an anonymous <rb> soon. For example, if we have code: <ruby><rt/> <span/> <rt/></ruby> The flow defined in spec does the following steps: <ruby><rt/><rb> <span/> </rb><rt/></ruby> <ruby><rtc/><rbc><rb> <span/> </rb></rbc><rtc/></ruby> (unrelated tags are omitted) But in our frame constructor, we do not wrap it from inner to outer, hence we will have an intermediate state: <ruby><rtc/><rbc> <span/> </rbc><rtc/></ruby> The leading and trailing whitespaces should have been wrapped in <rb> but it hasn't yet, so we shouldn't trim them. I say it is the only case because, if the whitespace is not part of such sequence, it would be either an inter-segment whitespace, which should have been splitted before it being wrapped in pseudo container, or an inter-level whitespace, which should have been trimmed by another rule. I agree that this code is finicky... Maybe we should refactor this code to make it more clear, but I'm not very confident in doing so... Ah... That reminds me another potential problem in the current code, that some boxes may bypass the inlinize mechanism (implement in bug 1098257) and cause block-in-inline split in ruby via pseudo box generation. Maybe the refactor is necessary then...

Comment on attachment 8526606 [details] [diff] [review] patch 2 - misparented inline content & inter-segment whitespace OK I decide to rewrite the frame generation code in a more clear manner.

I'm going to refactor the whole nsCSSFrameConstructor::CreateNeededPseudoContainers, and separate it into several methods, roughly one for each parent type, so that it could be easier to check. Does it sound good? If good, I'll start working on this. Maybe we should have another bug for this, I guess.

> I'm not sure, but I don't think it is a spec bug. It's ... unclear. Consider this markup: <ruby><rbc> <rb>Text</rb> </rbc><rtc><rt>Stuff</rt></rtc></ruby> The whitespace should get trimmed in this case, right? Now what about this case: <ruby> <rb>Text</rb> <rtc><rt>Stuff</rt></rtc></ruby> Should the whitespace around the <rb> be trimmed or not? As far as comment 28, that might be reasonable. The reason it was a single method for all the table stuff was that all of those behave pretty much identically. And the reason we added ruby bits to the same method was that it behaved _pretty_ similarly and we wanted to avoid multiple passes over the frame construction item list. I agree that things are now getting too complicated because each of the ruby bits wants slightly different behavior. So it might make sense to break things out, especially if we can avoid the multiple list walks in the common case.

Comment on attachment 8526607 [details] [diff] [review] patch for tests Writing new tests.

Comment on attachment 8526607 [details] [diff] [review] patch for tests Well, I guess these tests are still useful. I wanted to write some new tests, but I don't have a clear idea about how to construct a better test set for this yet.

(In reply to Boris Zbarsky [:bz] from comment #29) > > I'm not sure, but I don't think it is a spec bug. > > It's ... unclear. Consider this markup: > > <ruby><rbc> > <rb>Text</rb> > </rbc><rtc><rt>Stuff</rt></rtc></ruby> > > The whitespace should get trimmed in this case, right? Right, because it is leading space in non-pseudo ruby base container. > Now what about this case: > > <ruby> > <rb>Text</rb> > <rtc><rt>Stuff</rt></rtc></ruby> > > Should the whitespace around the <rb> be trimmed or not? Yes, it should, since it is leading whitespace in non-pseudo ruby container. And even in case like: <ruby> <span>Text</span></ruby> The whitespace should be trimmed according to the spec.

OK. So in what case would we have leading/trailing whitespace in a pseudo ruby container that does not get trimmed?

I mentioned that in the comments in patch 3: + // Normally, pseudo frames start from and end at some elements, + // which means they don't have leading and trailing whitespaces at + // all. But there are two cases where they have: + // 1. It is a inter-segment whitespace which in an individual ruby + // base container. + // 2. The pseudo frame start from or end at some consecutive inline + // content, which is not pure whitespace, but includes some. + // In either case, the whitespaces should not be trimmed.

Comment on attachment 8529515 [details] [diff] [review] patch 2 - separate wrapping into an independent method >+ IsRubyParentType(ourParentType) || This won't compile; there is no "ourParentType" declared in this function. The next patch removes this line, but it would be good to have this intermediate state actually compile and work. Probably by passing in the parent type as an argument, right? r=me with that fixed.

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #37) > Comment on attachment 8529515 [details] [diff] [review] > patch 2 - separate wrapping into an independent method > > >+ IsRubyParentType(ourParentType) || > > This won't compile; there is no "ourParentType" declared in this function. > > The next patch removes this line, but it would be good to have this > intermediate state actually compile and work. Probably by passing in the > parent type as an argument, right? Since even the current code guarantees that tables in a ruby parent would finally be put in a ruby base or ruby text when it is being constructed, I'd prefer remove that line in patch 2 directly instead of passing in the parent type. Sounds good?

Removing in patch 2 sounds fine. I think it won't give the right behavior, but I also doubt anyone will ever try to test this edge case with this partial patch stack, so that's ok.

Comment on attachment 8529516 [details] [diff] [review] patch 3 - implement ruby box generation >+nsCSSFrameConstructor::ComputeRubyWhitespaceType(uint_fast8_t aPrevDisplay, >+ } else if (aNextDisplay == NS_STYLE_DISPLAY_RUBY_TEXT || No else after return, generally. So: if (x) { return y; } if (z) { return whatever; } etc. >+ * This function check the content from |aStartIter| to |aEndIter|, s/check/checks/ >+ * determine whether it contains only whitespace, and if yes, interpret "determines", "interprets" >+ * the type of whitespace. This method does not change any of the iters. Maybe we should make the iter arguments const to make this more clear? We can add a const overload of item() as needed. >+nsCSSFrameConstructor::WrapItemsInPseudoRubyLeafBox( >+ NS_PRECONDITION(!aIter.IsDone(), "Shouldn't be done yet"); >+ NS_PRECONDITION(aIter.item().DesiredParentType() != parentType, >+ "Pointing to a level container?"); Please just make those MOZ_ASSERTs. And maybe move the !IsDone() assert into the implementation of item()? I'm not sure why I didn't put it there to start with! Also, the explanation string for the second assert doesn't make much sense to me. We're really asserting that aIter really needs wrapping in a pseudo, right? >+nsCSSFrameConstructor::WrapItemsInPseudoRubyLevelContainer( >+ NS_PRECONDITION(aIter.item().DesiredParentType() != eTypeRuby, >+ "Pointing to a level container?"); Again, MOZ_ASSERT. >+ RubyWhitespaceType whitespaceType = >+ InterpretRubyWhitespace(aState, endIter, contentEndIter); How do we know contentEndIter is not at the end of the list (which InterpretRubyWhitespace depends on)? The comments in InterpretRubyWhitespace talk about things being trimmed in a non-pseudo ruby box, but we can get here for pseudo boxes too via CreateNeededPseudoInternalRubyBoxes, right? I'd really like some clearer documentation somewhere in here on what the invariants are that we maintain around whitespace and why this call is safe. >+ } else if (whitespaceType == eRubyInterSegmentWhitespace) { Can we assert that wrapperType == eTypeRubyBaseContainer here? If not, why not? Per spec, inter-segment white space is supposed to be wrapped in a ruby-base box, so I'd be pretty surprised if we can be here with wrapperType == eTypeRubyTextContainer... >+ // Except inter-annotation whitespaces, misparented inline content >+ // doesn't belong to pseudo ruby text container. Break at endIter. I think this would be clearer: // Misparented inline content that's not inter-annotation whitespace doesn't // belong in a pseudo ruby text container. Break at endIter. >+ * This function trims leading an trailing whitespaces s/an/and/ >+ * This function walk through the child list (aItems) and create needed "walks", "creates". >+ * pseudo children to wrap misparented children. "pseudo ruby children"? >+nsCSSFrameConstructor::CreateNeededPseudoInternalRubyBoxes( >+ // all. But there are two cases where they have: But there are two case where they do have leading or trailing whitespace: >+ // 1. It is a inter-segment whitespace which in an individual ruby "an inter-segment" >+ // 2. The pseudo frame start from or end at some consecutive inline "starts", "ends". > nsCSSFrameConstructor::CreateNeededPseudoContainers( > if ((!trailingSpaces && > IsTableParentType(spaceEndIter.item().DesiredParentType())) || >- (trailingSpaces && IsTableParentType(ourParentType)) || >- isRubyLeading || isRubyTrailing || isRubyInterLevel) { >+ (trailingSpaces && IsTableParentType(ourParentType))) { Now that we know ourParentType is not ruby, IsTableParentType(ourParentType) is exactly equivalent to the (cheaper) ourParentType != eTypeBlock test that we used to have here before bug 1039017. Mind switching back to that and adjusting the comments for case (2) above? I guess we still need IsTableParentType() for the desired parent type of spaceEndIter.item(), unfortunately. >+ !IsRubyParentType(groupingParentType) || So what ruby types can groupingParentType be? I'd think we only want to be in this code if groupingParentType is eTypeRuby, with the new function you added handling the other cases (by first wrapping a ruby container frame around all the ruby stuff, per step 1 of <http://dev.w3.org/csswg/css-ruby/#box-fixup>). But what happens when you have a block containing some ruby-text elements? What will groupingParentType end up being here? Don't we end up with a grouping type of "ruby-text-container"? That part needs sorting out or more documentation, but r=me on the rest of this. This is looking pretty nice!

Comment on attachment 8526607 [details] [diff] [review] patch for tests Well, at least the tests suggest my question about ruby-base inside block works correctly.... ;) It might be nice to have comments in each test about what it is they're testing exactly; it's not obvious what the differences between the 5 tests are and what might have not gotten tested. r=me with those added.

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #40) > Comment on attachment 8529516 [details] [diff] [review] > patch 3 - implement ruby box generation > > >+ RubyWhitespaceType whitespaceType = > >+ InterpretRubyWhitespace(aState, endIter, contentEndIter); > > How do we know contentEndIter is not at the end of the list (which > InterpretRubyWhitespace depends on)? The comments in > InterpretRubyWhitespace talk about things being trimmed in a non-pseudo ruby > box, but we can get here for pseudo boxes too via > CreateNeededPseudoInternalRubyBoxes, right? Right, but any leading or trailing spaces should have been trimmed in CreateNeededPseudoInternalRubyBoxes, thus pseudo boxes never start or end with that type of spaces. > I'd really like some clearer documentation somewhere in here on what the > invariants are that we maintain around whitespace and why this call is safe. OK, I'll add some. > >+ } else if (whitespaceType == eRubyInterSegmentWhitespace) { > > Can we assert that wrapperType == eTypeRubyBaseContainer here? If not, why > not? Per spec, inter-segment white space is supposed to be wrapped in a > ruby-base box, so I'd be pretty surprised if we can be here with wrapperType > == eTypeRubyTextContainer... I think we can. > > nsCSSFrameConstructor::CreateNeededPseudoContainers( > > if ((!trailingSpaces && > > IsTableParentType(spaceEndIter.item().DesiredParentType())) || > >- (trailingSpaces && IsTableParentType(ourParentType)) || > >- isRubyLeading || isRubyTrailing || isRubyInterLevel) { > >+ (trailingSpaces && IsTableParentType(ourParentType))) { > > Now that we know ourParentType is not ruby, IsTableParentType(ourParentType) > is exactly equivalent to the (cheaper) ourParentType != eTypeBlock test that > we used to have here before bug 1039017. Mind switching back to that and > adjusting the comments for case (2) above? > > I guess we still need IsTableParentType() for the desired parent type of > spaceEndIter.item(), unfortunately. > > >+ !IsRubyParentType(groupingParentType) || > > So what ruby types can groupingParentType be? Any. If the first item is <rb> then it is <rbc>, if <rt> then <rtc>, if <rbc>/<rtc> then <ruby>. They still have their own parent type here. > I'd think we only want to be in this code if groupingParentType is > eTypeRuby, with the new function you added handling the other cases (by > first wrapping a ruby container frame around all the ruby stuff, per step 1 > of <http://dev.w3.org/csswg/css-ruby/#box-fixup>). But what happens when > you have a block containing some ruby-text elements? What will > groupingParentType end up being here? Don't we end up with a grouping type > of "ruby-text-container"? groupingParentType can be any of the ruby parent type here. The value might not be exactly correct to some extent, but we convert all of them into eTypeRuby at the end of that function.

> Right, but any leading or trailing spaces should have been trimmed Hmm. But only if our parent was not itself a pseudo, right? Are we basically depending on there being only one level of ruby pseudo containers to make this work out right? > They still have their own parent type here. OK. > but we convert all of them into eTypeRuby Where does that happen, exactly?

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #41) > Comment on attachment 8526607 [details] [diff] [review] > patch for tests > > Well, at least the tests suggest my question about ruby-base inside block > works correctly.... ;) > > It might be nice to have comments in each test about what it is they're > testing exactly; it's not obvious what the differences between the 5 tests > are and what might have not gotten tested. Please see comment 13 for how the tests are constructed. The structure of the test set means to check whether the frame constructor works properly for different start item and different combination of items. I feel that the tests are still insufficient, but I don't have idea how to build better ones. I guess that it might help to have some tests about nesting different type of ruby box...

Oh, nevermind. I see, right here: if (IsRubyParentType(groupingParentType)) { wrapperType = eTypeRuby; And I guess the other important bit is that we just keep going forward with any ruby parent type until we find something that wants either ourParentType or a non-ruby parent type, ok.

As far as the tests go, yeah, that would be the obvious thing to test. And whatever whitespace edge cases we're not already testing....

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #43) > > Right, but any leading or trailing spaces should have been trimmed > > Hmm. But only if our parent was not itself a pseudo, right? Are we > basically depending on there being only one level of ruby pseudo containers > to make this work out right? If we construct a pseudo <ruby>, we don't wrap leading or trailing space at the very begining. So does pseudo <rtc>&<rbc>. If there is a concrete ruby parent, those spaces are trimmed. I think that's all.

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #46) > As far as the tests go, yeah, that would be the obvious thing to test. And > whatever whitespace edge cases we're not already testing.... I feel that my tests have covered almost all the whitespace cases. I have no idea about what edge cases should be tested in addition.

Per comment 40, I add a const overload of item() and add assertions to them. I guess it's better to make it a separate patch.

Comment on attachment 8536099 [details] [diff] [review] patch 3.1 - add const overload & assertion r=me

(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #41) > Comment on attachment 8526607 [details] [diff] [review] > patch for tests > > Well, at least the tests suggest my question about ruby-base inside block > works correctly.... ;) > > It might be nice to have comments in each test about what it is they're > testing exactly; it's not obvious what the differences between the 5 tests > are and what might have not gotten tested. > > r=me with those added. May I simply add <link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1088489#c13"> to each of the tests?

> May I simply add Yes, sounds good.

Comment on attachment 8536105 [details] [diff] [review] patch 3.2 - implement ruby box generation Looks good based on the interdiff.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8e553415f8bb

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/77ad52c03cc9 https://hg.mozilla.org/integration/mozilla-inbound/rev/69eaa08a1f97 https://hg.mozilla.org/integration/mozilla-inbound/rev/247b87f9114e https://hg.mozilla.org/integration/mozilla-inbound/rev/b501eda706bf

https://hg.mozilla.org/mozilla-central/rev/77ad52c03cc9 https://hg.mozilla.org/mozilla-central/rev/69eaa08a1f97 https://hg.mozilla.org/mozilla-central/rev/247b87f9114e https://hg.mozilla.org/mozilla-central/rev/b501eda706bf