Closed Bug 1087872 Opened 5 years ago Closed 5 years ago

Ruby pseudo frame is not removed properly

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(5 files, 7 obsolete files)

1.29 KB, patch
Details | Diff | Splinter Review
8.40 KB, patch
xidorn
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
2.63 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
18.46 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
1.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
Attached patch patch 1 - for frame removal case (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
Attachment #8510072 - Flags: review?(bzbarsky)
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!
Attachment #8510072 - Flags: review?(bzbarsky) → review+
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.
Depends on: 1088489
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.
Attached patch patch 1 - for frame removal case (obsolete) — Splinter Review
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.
Attachment #8510072 - Attachment is obsolete: true
Attachment #8511606 - Flags: review?(bzbarsky)
Attached patch patch 2 - Tests for patch 1 (obsolete) — Splinter Review
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.
Attachment #8511610 - Flags: feedback?(bzbarsky)
Attached patch patch 2 - Tests for patch 1 (obsolete) — Splinter Review
Add several items for testing merging ruby, ruby base, and ruby text pseudo elements.
Attachment #8511610 - Attachment is obsolete: true
Attachment #8511610 - Flags: feedback?(bzbarsky)
Attachment #8511810 - Flags: feedback?
Attachment #8511810 - Flags: feedback?
Depends on: 1090744
No longer depends on: 1090744
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.
Blocks: enable-css-ruby
No longer blocks: 1083004
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.
Attachment #8511810 - Flags: review?(bzbarsky)
Attached patch patch 2 - Tests for patch 1 (obsolete) — Splinter Review
Add workaround for the reflow bug.
Attachment #8511810 - Attachment is obsolete: true
Attachment #8511810 - Flags: review?(bzbarsky)
Attachment #8514050 - Flags: review?(bzbarsky)
Attachment #8514050 - Flags: review?(bzbarsky)
Attachment #8514718 - Flags: review?(bzbarsky)
Attached patch patch 3 - tests (obsolete) — Splinter Review
Attachment #8514050 - Attachment is obsolete: true
Attachment #8514720 - Flags: review?(bzbarsky)
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.
Attachment #8511606 - Flags: review?(bzbarsky) → review+
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
Attachment #8514718 - Flags: review?(bzbarsky) → review+
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.
Attachment #8514720 - Flags: review?(bzbarsky) → review+
Makes it blocked by bug 1052924. The patches will crash the tests before we get that landed.
Depends on: 1052924
Attachment #8511606 - Attachment is obsolete: true
Attachment #8520398 - Flags: review+
Attachment #8520398 - Flags: feedback?(bzbarsky)
Attachment #8514718 - Attachment is obsolete: true
Attachment #8520399 - Flags: review+
Comment on attachment 8520398 [details] [diff] [review]
patch 1 - for frame removal case, r=bz

Thank you for adding those comments.
Attachment #8520398 - Flags: feedback?(bzbarsky) → feedback+
(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.
Attachment #8514720 - Attachment is obsolete: true
Attachment #8520408 - Flags: review+
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?
Flags: needinfo?(dbaron)
Attachment #8536300 - Flags: review?(dbaron)
Backed out the test as per Xidorn's request on IRC for bustage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0586c2029d74
Keywords: leave-open
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.
Flags: needinfo?(dbaron)
Attachment #8536300 - Flags: review?(dbaron) → review+
Depends on: 1111891
https://hg.mozilla.org/mozilla-central/rev/bc6e0cee4326
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.