Closed Bug 1141928 Opened 5 years ago Closed 5 years ago

Ruby annotation is not placed correctly for RTL content

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Testcase:
data:text/html,<body dir="rtl"><ruby>base<rt>text</ruby>
Please note that the testcase in comment 0 could crash debug build because of violation of an assertion.
Blocks: 1141931
Assignee: nobody → quanxunzhen
Attachment #8575939 - Flags: review?(jfkthame)
Attached patch patch 2 - correct ruby text (obsolete) — Splinter Review
Attachment #8575940 - Flags: review?(jfkthame)
Attachment #8575942 - Flags: review?(jfkthame)
Actually, these patches are not enough for fixing ruby in RTL content. I plan to fix the left parts in bug 1141931.
Comment on attachment 8575942 [details] [diff] [review]
patch 3 - reftest

Review of attachment 8575942 [details] [diff] [review]:
-----------------------------------------------------------------

Look OK. I notice that without color:transparent, the "text" annotation is actually misplaced (doing Select All also reveals this, with the highlight being offset from the expected rect). I assume this is part of what you're expecting to handle in bug 1141931, and will add more tests at that time.
Attachment #8575942 - Flags: review?(jfkthame) → review+
Comment on attachment 8575939 [details] [diff] [review]
patch 1 - correct ruby text containers

Review of attachment 8575939 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with a minor suggested change (if you agree that it makes sense).

::: layout/generic/nsLineLayout.cpp
@@ +2982,5 @@
> +      // It is necessary to set the rect again because the container
> +      // width was unknown, and zero was used instead when we reflow
> +      // them. The corresponding base containers were repositioned in
> +      // VerticalAlignFrames and PlaceTopBottomFrames.
> +      annotation->mFrame->SetRect(lineWM, annotation->mBounds, aContainerWidth);

If I'm understanding things, this should only ever change the annotation frame's position, not size; so how about adding

  annotation->mFrame->GetLogicalSize(lineWM) ==
    annotation->mBounds.Size(lineWM)

to the assertion, and using SetPosition rather than SetRect.

(Or are there cases where the size might be changing as well?)
Attachment #8575939 - Flags: review?(jfkthame) → review+
Comment on attachment 8575940 [details] [diff] [review]
patch 2 - correct ruby text

Review of attachment 8575940 [details] [diff] [review]:
-----------------------------------------------------------------

Holding off on review here for now; the patch appears to work, but the end result feels a bit hacky to me. I've tried putting together an alternative approach that IMO results in cleaner code overall, which I'll post as a separate patch for you to consider, then let's decide how to go forward.
Here's my alternative suggestion for patch 2; seems to work in my local testing so far. See what you think...
Attachment #8576828 - Flags: review?(quanxunzhen)
Comment on attachment 8576828 [details] [diff] [review]
part 2 (alternative) - Correct the position of ruby text frames

Review of attachment 8576828 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I agree your patch is better than mine. But I wonder whether I can review this. This is not a trivial patch, and I'm not a peer. If I can, I'll grant it, though.
Attachment #8576828 - Flags: review?(quanxunzhen) → feedback+
Attachment #8575940 - Attachment is obsolete: true
Attachment #8575940 - Flags: review?(jfkthame)
Attachment #8576828 - Flags: review?(roc)
Attachment #8577081 - Flags: review?(jfkthame)
I just realized that I did the wrong change. We cannot get rid of line relative dir mapping code there, because logical side and line relative dir are different.
Attachment #8575939 - Attachment is obsolete: true
Attachment #8577082 - Flags: review?(jfkthame)
The previous one causes bustage on non-Win platforms
Attachment #8577081 - Attachment is obsolete: true
Attachment #8577081 - Flags: review?(jfkthame)
Attachment #8577090 - Flags: review?(jfkthame)
Attachment #8577082 - Attachment is obsolete: true
Attachment #8577082 - Flags: review?(jfkthame)
Attachment #8577091 - Flags: review?(jfkthame)
Attachment #8577090 - Flags: review?(jfkthame) → review+
Attachment #8577091 - Flags: review?(jfkthame) → review+
You need to log in before you can comment on or make changes to this bug.