After removing all frames in a pseudo ruby text container, that pseudo frame does not get removed automatically. This may also happen on ruby base container.

I'm investigating the insertion part to figure out whether it is also necessary to add code there for ruby pseudo frames.

Comment on attachment 8510072 [details] [diff] [review] patch 1 - for frame removal case r=me. Thank you!

Oh, and maybe add a testcase?

(In reply to Boris Zbarsky [:bz] from comment #4) > Oh, and maybe add a testcase? Do you have any suggestion about how to make tests for this case? It seems that we doesn't process white spaces in ruby frame correctly, which makes it hard to do tricks on spaces. Maybe we should fix that first.

Do the empty frames left behind take up space? I guess maybe not... If you can't think of a way to write a test, then we can let it be for now, I guess.

(In reply to Boris Zbarsky [:bz] from comment #6) > Do the empty frames left behind take up space? I guess maybe not... > > If you can't think of a way to write a test, then we can let it be for now, > I guess. I'm now fixing the whitespace processing for ruby frames. After that done, we can use some space tricks for testing this. Maybe I should open another new bug.

It seems that given the behavior I implemented in bug 1088489, there are more cases we have to handle when frame removal happens. For example, removing the last rtc of a segment could covert the space before it to an inter-segment whitespace, which has its own pseudo rb and rbc. Maybe we can always do reframe when removal happens if it is in a ruby frame, since, unlike tables, it is not very often to have a large ruby structure.

How far up would we have to reframe? Just up to the ruby-container?

My plan is, if the parent of the removed frame is a ruby, rbc, or rtc, or a pseudo rb or rt, then reframe the content of the parent. I think it should be enough for ruby's cases. So yes, at most up to a ruby container if it is not pseudo.

Add some code to process cases added in bug 1088489. I'm not sure about the completeness of this patch, because it cannot fully pass all of tests I write. I printed the frame tree, and found that the tree correctly reflected what should happen, but the render result did not. Even worse, the render results of those failed parts are even not the expected results without this patch. I guess there are some problems with our current reflow code. If that is the case, we do not need to fix them now, as reflow code is likely to be rewrited in bug 1052924.

Since the tests here are not fully passed, I do not request review for now. In addition, this code could cause crash in the current reflow code. attachment 8511608 [details] [diff] [review] and attachment 8511609 [details] [diff] [review] need to be applied before viewing the tests.

Add several items for testing merging ruby, ruby base, and ruby text pseudo elements.

Since this problem has been existing before, I'd like to make it block enable-css-ruby instead of bug 1083004, because that would cause a cyclic dependency.

I can confirm that the tests in attachment 8511610 [details] [diff] [review] not being passed is because of some bugs in our current reflow code, which should be fixed in bug 1052924 later.

Comment on attachment 8511810 [details] [diff] [review] patch 2 - Tests for patch 1 As mentioned in comment 16, the test here cannot be passed is not because of problems in this bug.

Add workaround for the reflow bug.

Comment on attachment 8511606 [details] [diff] [review] patch 1 - for frame removal case So you don't actually need IsTableOrRubyPseudo(parent), right? You only want to check for ruby-base and ruby-text, because for the three container types the catchall "Check ruby containers" stuff will reframe anyway, no? I'm probably OK leaving this as-is, though. But might be worth a comment about the redundancy to make it clear. r=me, and sorry for the lag here; last week was somewhat abbreviated.

Comment on attachment 8514718 [details] [diff] [review] patch 2 - for frame insertion cases >+ // 1) There are effectly three types of white spaces in ruby frames we "effectively" r=me

Comment on attachment 8514720 [details] [diff] [review] patch 3 - tests >+ <!-- tailing white space --> "trailing", multiple places. >+ <p>'a' and 'b' should be paried with 'x' and 'y' respectively:</p> "paired", multiple places. >+ <!-- pseduo ruby text container --> "pseudo" >+window.onload = function() { It might be a good idea to flush out layout before running the rest of the script, because we don't really want to guarantee that layout has run before onload fires. This applies to both scripts. >+ // XXX Force a reflow. There are some bugs in the current reflow Just getting .clientWidth would work to force the reflow. Why are you setting the style width too? Is there a followup bug for those asserts? r=me with the above fixed; the actual tests look great.

Makes it blocked by bug 1052924. The patches will crash the tests before we get that landed.

Comment on attachment 8520398 [details] [diff] [review] patch 1 - for frame removal case, r=bz Thank you for adding those comments.

(In reply to Boris Zbarsky [:bz] from comment #23) > Comment on attachment 8514720 [details] [diff] [review] > patch 3 - tests > > >+window.onload = function() { > > It might be a good idea to flush out layout before running the rest of the > script, because we don't really want to guarantee that layout has run before > onload fires. This applies to both scripts. > > >+ // XXX Force a reflow. There are some bugs in the current reflow > > Just getting .clientWidth would work to force the reflow. Why are you > setting the style width too? I think my actual meaning here is force reflow twice. Anyway, I have marked this bug depend on bug 1052924, hence that hack could be removed. > Is there a followup bug for those asserts? I think those asserts can also be resolved by bug 1052924. I'll check them again after that bug lands.

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

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f017241da71 https://hg.mozilla.org/integration/mozilla-inbound/rev/c78048798c2a https://hg.mozilla.org/integration/mozilla-inbound/rev/39d19feaf2b2

To suppress reftest fail like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4637555&repo=mozilla-inbound I'm not sure about the real problem here though... Should I file another bug to investigate that?

Backed out the test as per Xidorn's request on IRC for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/0586c2029d74

https://hg.mozilla.org/mozilla-central/rev/1f017241da71 https://hg.mozilla.org/mozilla-central/rev/c78048798c2a

Comment on attachment 8536300 [details] [diff] [review] patch - fuzzy one reftest on windows r=dbaron. From the "open reftest analyzer" link in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=39d19feaf2b2 it looks like the one pixel that's off is at the upper right corner of the last "x", and only in the blue component. I think it probably is worth filing a followup bug, since it seems like something that shouldn't happen. You should add the bug number in a comment at the end of the line in reftest.list. (I suspect it may be a bug about failing to propagate overflow areas correctly.) Also, you should have just landed this rather than waiting for review, rather than have no tests in the tree for a landed patch.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6e0cee4326