1081770 - Sync ruby annotations when base is justified

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Currently ruby annotation might not be synced with its base when the line is justified.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

We might finally need to move some of the reflow code into nsLineLayout, so that the line layout which reflows base frames could be aware of the text frames, and make it possible to sync them when TextAlignLine happens. We could investigate it later, but I do think it is the other hard part for ruby.

The current behavior of WebKit and Blink is that, rubies are not justified at all, which I don't think is correct.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

The way I plan to solve this is to make the line layout for annotations allocate PerFrameData and PerSpanData from the line layout of the base, and link the data together, so that ApplyFrameJustification is enabled to adjust the data of annotations as well.

I'm going to fix it after bug 1055676.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8535273 [details] [diff] [review]
patch

This should have a commit message before being posted for review.  I'd
suggest something like:
>Bug 1081770 - Move ruby annotation frames when text-align: justify is applied to ruby bases.  r=dbaron

SyncRectOfAnnotationFrames doesn't seem like the best name; "Sync Rect"
is rather generic (and this code is just the horizontal aspects of
applying text-align: justify).  I'd suggest instead calling it
ApplyJustificationToAnnotations.  I'd also suggest maybe naming aSpan
aContainingSpan, and adding a comment saying that aPFD must be one
of the frames of aContainingSpan.

>+  PerFrameData* pfd = aPFD->mNextAnnotation;

Is there a bit you can use to assert that aPFD isn't an annotation?
If so (and I thought there was), please add such an assertion.

>+    // It is possible that there are siblings which do not attached to
>+    // a ruby base-level frame. Their size will not be affected by the
>+    // justification, but we still have to move them so that they won't
>+    // overlap us.

Why does this happen?  Is it because of things like 'direction' changes
within the ruby?  And what guarantees that this doesn't affect things
that are also in some other ruby base's mAnnotations?

The comment should (briefly, at least) answer these questions.


>+  inline void SyncRectOfAnnotationFrames(PerFrameData* aPFD,

Drop the "inline".


r=dbaron with that

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #4)
> Comment on attachment 8535273 [details] [diff] [review]
> patch
> 
> This should have a commit message before being posted for review.  I'd
> suggest something like:
> >Bug 1081770 - Move ruby annotation frames when text-align: justify is applied to ruby bases.  r=dbaron
> 
> SyncRectOfAnnotationFrames doesn't seem like the best name; "Sync Rect"
> is rather generic (and this code is just the horizontal aspects of
> applying text-align: justify).  I'd suggest instead calling it
> ApplyJustificationToAnnotations.  

This patch doesn't actually apply any justification to annotations. It just sync the position. So this name might be more confusing. Annotations do have justification when ruby-align is applied (which I have implemented in my local repo for bug 1108429). Maybe SyncJustificationToAnnotations.

> >+    // It is possible that there are siblings which do not attached to
> >+    // a ruby base-level frame. Their size will not be affected by the
> >+    // justification, but we still have to move them so that they won't
> >+    // overlap us.
> 
> Why does this happen?  Is it because of things like 'direction' changes
> within the ruby?  And what guarantees that this doesn't affect things
> that are also in some other ruby base's mAnnotations?
> 
> The comment should (briefly, at least) answer these questions.

This happens in two cases:
1. intra-annotation whitespace not paired with intra-base whitespace (see bug 1099807);
2. extra annotations which do not have base to be paired with.

I'll add comment.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I forgot that ruby-align had its own justification option.

Maybe ApplyLineJustificationToAnnotations?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

This patch additionally considers the case that writing-mode of annotation is different from that of the base line.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8536935 [details] [diff] [review]
patch

r=dbaron

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cffd89b920e3