Closed Bug 1083004 Opened 5 years ago Closed 5 years ago

An anonymous ruby base container should be created if a ruby frame starts with a text container

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files, 3 obsolete files)

Currently, ruby text containers without ruby base containers before won't be properly reflowed. According to the spec:

> If the first child of a ruby container is a ruby annotation container, an anonymous, empty ruby base container is assumed to exist before it.

We should insert an anonymous ruby base container at the beginning of ruby container if there is no such frame.
What is the best place to put the frame creation? Should I hack in the frame constructor, or can I just create the frame and insert it into the frame tree during the reflow of nsRubyFrame?
Flags: needinfo?(dbaron)
It should be in the frame constructor.  (There should already be some anonymous frame creation code for ruby in the frame constructor, although the spec has probably changed since it was written.)
Flags: needinfo?(dbaron)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
Attachment #8507654 - Flags: review?(bzbarsky)
Blocks: 1052924
No longer blocks: enable-css-ruby
Attached patch patch (obsolete) — Splinter Review
Sorry, the old patch is not correct.
Attachment #8507654 - Attachment is obsolete: true
Attachment #8507654 - Flags: review?(bzbarsky)
Attachment #8508448 - Flags: review?(bzbarsky)
I'm sorry for the lag here.  I'm not likely to get to this review until Monday; it involves me trying to make sense of the ruby spec again.  :(
But in the meantime....

If the text container at the start is removed, what makes sure that the anonymous base container that got created by this patch is also removed?
(In reply to Boris Zbarsky [:bz] from comment #5)
> I'm sorry for the lag here.  I'm not likely to get to this review until
> Monday; it involves me trying to make sense of the ruby spec again.  :(

No worries. The current version seems to still have some problems, and I'm fixing it.

(In reply to Boris Zbarsky [:bz] from comment #6)
> But in the meantime....
> 
> If the text container at the start is removed, what makes sure that the
> anonymous base container that got created by this patch is also removed?

I don't aware of any mechanism which does so. I guess it might also be a problem when a base container is insterted before? Do you have any suggestion about what should be done in these cases?

Anyway, an empty base container won't actually have any effect when it is still the first child of the ruby frame. But if it is not, then we have a problem.
> No worries. The current version seems to still have some problems, and I'm fixing it.

Version of the spec, or the patch?

> Do you have any suggestion about what should be done in these cases?

There's code in MaybeRecreateContainerForFrameRemoval that handles removal cases; presumably we'd need to add something there.

To handle insertion of a base before the anonymous base or between the anonymous base and the text, see WipeContainingBlock which handles rebuilding the frame tree on inserts as needed.
(In reply to Boris Zbarsky [:bz] from comment #8)
> > No worries. The current version seems to still have some problems, and I'm fixing it.
> 
> Version of the spec, or the patch?

The patch.
Comment on attachment 8508448 [details] [diff] [review]
patch

OK.  Can we hold off on the review request until things stabilize, then?
Attachment #8508448 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8508448 [details] [diff] [review]
> patch
> 
> OK.  Can we hold off on the review request until things stabilize, then?

Yes, sure.

(In reply to Boris Zbarsky [:bz] from comment #8)
> > No worries. The current version seems to still have some problems, and I'm fixing it.
> 
> Version of the spec, or the patch?
> 
> > Do you have any suggestion about what should be done in these cases?
> 
> There's code in MaybeRecreateContainerForFrameRemoval that handles removal
> cases; presumably we'd need to add something there.

It seems that we haven't handled any thing around the ruby in this case currently? My test tells me that the anonymous ruby text container even won't be removed when no ruby text frame is inside. Maybe this is a bug from the very beginning which we should solve now?

> To handle insertion of a base before the anonymous base or between the
> anonymous base and the text, see WipeContainingBlock which handles
> rebuilding the frame tree on inserts as needed.

My test shows that in this case everything just works as expected. It seems that such insertions cause a recreate of frames. (I didn't read the code detailly, though)
> It seems that we haven't handled any thing around the ruby in this case currently?

Ugh.  I thought that worked, but yes, the relevant bit is table-pseudo-specific.  So yes, a preexisting bug that should ideally get solved.

> It seems that such insertions cause a recreate of frames

It would be good to understand why.
Depends on: 1087872
Attached patch patchSplinter Review
Attachment #8508448 - Attachment is obsolete: true
Attachment #8512419 - Flags: review?(bzbarsky)
No longer depends on: 1087872
Boris, could you review this patch regardless of the fact that the pseudo frame won't be removed properly? I would like to solve that issue along with all other cases in bug 1087872, which now blocks enable-css-ruby.
Flags: needinfo?(bzbarsky)
Yeah, I can do that.

Sorry for the lag here; I'm pretty behind on reviews, unfortunately.  I'll try to get to it before end of day Friday, but if that doesn't happen it won't be until Tuesday...
Flags: needinfo?(bzbarsky)
Comment on attachment 8512419 [details] [diff] [review]
patch

>+  auto creationFunc = reinterpret_cast<void*>(

I would prefer that we looked at the computed display of iter.item() instead.

>+    ResolveAnonymousBoxStyle(nsCSSAnonBoxes::rubyBaseContainer,

Why not use the pseudo from pseudoData?

r=me with those fixed, and again sorry for the lag.
Attachment #8512419 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 8512419 [details] [diff] [review]
> patch
> 
> >+  auto creationFunc = reinterpret_cast<void*>(
> 
> I would prefer that we looked at the computed display of iter.item() instead.

I prefer that as well, but currently it would cause the pseudo rbc frame always be created in ruby-related tests because of bug 1068477. It also adds an extra assertion in those tests. I guess it's not a big problem, though.
> but currently it would cause the pseudo rbc frame always be created in ruby-related tests
> because of bug 1068477

Hmm.  OK, that's a good reason not to do it.  But please add a comment and file a bug depending on bug 1068477 to switch to using the display?
Blocks: 1088489
Attached patch patch for fixing crash (obsolete) — Splinter Review
This is a temporary patch to fix crashes. It is trivial because this code will finally get removed in bug 1052924.
Attachment #8520564 - Flags: review?(dbaron)
Fix the compile error on B2G. It should work fine now.

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0974fb946f66
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d142cb22c225
Attachment #8520564 - Attachment is obsolete: true
Attachment #8520564 - Flags: review?(dbaron)
Attachment #8521022 - Flags: review?(dbaron)
No longer blocks: 1088489
Apparently an actual link to the test failure is:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3753864&repo=mozilla-inbound

which shows:

22:09:13 INFO - [1879] ###!!! ABORT: running past end: 'mCurrent != mListLink', file /builds/slave/m-in-l64-d-0000000000000000000/build/layout/base/../generic/nsLineBox.h, line 759
6928 22:09:14 INFO - 629 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_value_cloning.html | application terminated with exit code 11
6935 22:09:23 WARNING - PROCESS-CRASH | /tests/layout/style/test/test_value_cloning.html | application crashed [@ mozalloc_abort(char const*)]



Xidorn's explanation of the patch for fixing the crash:
[2014-11-11 22:10:30 -0800] <xidorn> dbaron: this happens when we have no linebox in a rtc, which, i guess, happens when we have an empty rtc
Comment on attachment 8521022 [details] [diff] [review]
patch for fixing crash

I guess given that this is turned off, I'm ok with landing this temporarily, although I think it would probably have made more sense to just make the function return early if mLines.empty().
Attachment #8521022 - Flags: review?(dbaron) → review+
Comment on attachment 8521022 [details] [diff] [review]
patch for fixing crash

But actually, please remove the firstLine variable and pass nullptr directly.
sorry had to back this out for failing tests like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3802263&repo=mozilla-inbound starting with this push
Flags: needinfo?(quanxunzhen)
Ah... OK, let's make it land with bug 1052924. I have no idea about the crash here...
Flags: needinfo?(quanxunzhen)
Depends on: 1068477, 1096808
https://hg.mozilla.org/mozilla-central/rev/1ff07cb9adaf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.