Sync ruby annotations when base is justified

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Currently ruby annotation might not be synced with its base when the line is justified.
(Assignee)

Updated

4 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1055676
(Assignee)

Updated

4 years ago
Blocks: 1103832
(Assignee)

Updated

4 years ago
Depends on: 1107721
(Assignee)

Updated

4 years ago
Depends on: 1108429
(Assignee)

Updated

4 years ago
No longer depends on: 1055676
(Assignee)

Comment 3

4 years ago
Created attachment 8535273 [details] [diff] [review]
patch
Assignee: nobody → quanxunzhen
Attachment #8535273 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Blocks: 1108429
No longer depends on: 1108429
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
Attachment #8535273 - Flags: review?(dbaron) → review+
(Assignee)

Comment 5

4 years ago
(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.
Oh, I forgot that ruby-align had its own justification option.

Maybe ApplyLineJustificationToAnnotations?
(Assignee)

Comment 7

4 years ago
Created attachment 8536935 [details] [diff] [review]
patch

This patch additionally considers the case that writing-mode of annotation is different from that of the base line.
Attachment #8535273 - Attachment is obsolete: true
Attachment #8536935 - Flags: review?(dbaron)
Comment on attachment 8536935 [details] [diff] [review]
patch

r=dbaron
Attachment #8536935 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/cffd89b920e3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.