Closed
Bug 1083004
Opened 10 years ago
Closed 9 years ago
An anonymous ruby base container should be created if a ruby frame starts with a text container
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(2 files, 3 obsolete files)
7.29 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 4•10 years ago
|
||
Sorry, the old patch is not correct.
Attachment #8507654 -
Attachment is obsolete: true
Attachment #8507654 -
Flags: review?(bzbarsky)
Attachment #8508448 -
Flags: review?(bzbarsky)
Comment 5•10 years ago
|
||
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. :(
Comment 6•10 years ago
|
||
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?
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Comment 8•10 years ago
|
||
> 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.
Assignee | ||
Comment 9•10 years ago
|
||
(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 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
> 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.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8508448 -
Attachment is obsolete: true
Attachment #8512419 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Comment 18•10 years ago
|
||
> 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?
Assignee | ||
Comment 19•10 years ago
|
||
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bc86ed5af44 m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/a263c8cfb04f
Comment 20•10 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3754925&repo=mozilla-inbound
Assignee | ||
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
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.
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/357b7bb14462 https://hg.mozilla.org/integration/mozilla-inbound/rev/61fa2ff606b8
Comment 27•10 years ago
|
||
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)
Assignee | ||
Comment 28•10 years ago
|
||
Ah... OK, let's make it land with bug 1052924. I have no idea about the crash here...
Flags: needinfo?(quanxunzhen)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 29•9 years ago
|
||
with bug 1052924: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff07cb9adaf
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ff07cb9adaf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•