275672 - Right-most character (that means end of line) should not have extra space for justify

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Bug 36322 is fixed.
But by the patch, the extra space for justify appear on most right charachter.
Therefore we cannot show the lines are aligned right edge of the box when the
line is layed out justify.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2481&action=view

On IE, when justifiable character is right most, the charachter doesn't have the
extra space. see this scrrenshot.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2482&action=view

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

By this patch, this problem is solved.
But it is not perfect.
When select the string, the right most charachter has extra space.
But I don't know how to fix it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patched build's screenshot

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Still problem...(the selection is overflow)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Still problem...(the selection is overflow)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I get the text frame that is last frame or not in the line.
The normal, this is success.
But when selection, I cannot get the text frame is last frame or not.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRInt32 justifiableRange = textLength;
+    if (mNextInFlow)
+      justifiableRange--;

I don't think this is correct. A frame with no next-in-flow can still be the end
of a justified line. Consider:

<b>XYZ</b>ABC

where XYZABC are Kanji and the line break occurs right after the </b>.

I'm not quite sure why this bug is occuring. I think it should be working
without any changes. The code here
http://lxr.mozilla.org/mozilla/source/layout/generic/nsLineLayout.cpp#2628
reduces pfd->mJustificationNumSpaces by 1 in the last non-empty frame on the
line so that the last justifiable character (either a space, or a Kanji
character) does not have extra justification space assigned to it. So if we have
a line
  XYZABC
where each character is 20px wide, and the available width is 200px, then we'll
say there are 80px of leftover space, *5* justifiable characters, so each gets
16px of extra space. Then the right edge of 'C' will be at 5*(16 + 20) + 20
which is exactly 200.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I'm not quite sure why this bug is occuring. I think it should be working
> without any changes. The code here
> http://lxr.mozilla.org/mozilla/source/layout/generic/nsLineLayout.cpp#2628
> reduces pfd->mJustificationNumSpaces by 1 in the last non-empty frame on the
> line so that the last justifiable character (either a space, or a Kanji
> character) does not have extra justification space assigned to it.

I think it it is not related.
nsLineLayout's |pfd->mJustificationNumSpaces| is not used when calculating the
extra space width and rendering the charachters.
We are using nsTextFrame::PrepareUnicodeText()'s return
value(aJustifiableCharCount).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right, I see. Then I think the right thing to do is to modify
nsTextFrame::TrimTrailingWhiteSpace to set a flag so that if a Kanji character
is the last character in the frame, we remember that its justification space
should be trimmed off (i.e. the character should be treated as not justifying).
Does that make sense?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Scrren shot of patched build(works fine)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screen shot of patched build(selection, buggy)

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I rewrote the patch by comment 8.
It works fine.
But still a bug, when select the text,
the extra spaces are lost arroud the selection.
This is other bug? or my patch is wrong?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://www.w3.org/Style/CSS/Test/CSS1/current/sec546.htm
test case of english.

Comment 14

[Jungshik Shin]

See also bug 265745. This patch may fix (or at least affect) it. 

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jungshik:

What? Is the bug-id right?

Comment 16

[Jungshik Shin]

oops. it's bug 275645.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think bug 275645 is not related this bug.
This patch incfluences only justify line.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the line is,

ABCDEF

if the 'CD' is selected,

AB CD EF
  |--|
    ^--- selected

we have *3* nsLineBox and *3* nsLineLayout.

 1st  2nd  3rd
| AB | CD | EF |

TrimTrailingWhiteSpace is called on every box.
But it is a bug(currentry), the function should be called only 3rd box.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: buggy patch

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> we have *3* nsLineBox and *3* nsLineLayout.

I don't think so.

I think what's happening with the last patch is that there's one text frame for
the whole line, which is marked as the "last text frame" (because it is), and
when there is text selected we make 3 calls to RenderString, and *each call*
ignores justification on its last character, but we really want to only ignore
justification on the last character *of the text frame*. Does that make sense?

+  PRBool    mIsLastTextFrame;

We must not add any new fields to nsTextFrame, there are many many of these
objects and their size is important. This flag can be stored as a bit in the
frame state.

I think you need to clear the "last text frame" flag in nsTextFrame::Reflow, in
case the frame gets moved somewhere where it's no longer the last text frame.
Also it's not exactly "is last text frame" because we don't always call
TrimTrailingWhitespace on the last text frame ... e.g. if the last frame on the
line is not a text frame. I'd prefer it to be called "hasTrailingWhitespace" or
something like that.

One thing I don't understand is how the sequencing of actions works. It looks to
me like we do things in this order:
1) Reflow text frames
2)    Compute justification weights for each frame
3) TrimTrailingWhitespace on the line
4)    Sets "last frame" flag on the last text frame
5) HorizontalAlignFrames using the weights computed in step 2, which aren't
correct because we didn't take the 'last frame' flag into account.

It might be working for you because currently your 'last frame' flag is sticky
and never gets cleared once it's set, so when we reflow the line again, step 2
will compute the right weights and everything works. But as I mentioned, the
'sticky flag' is a bug and we should clear the flag in step 1). I think the way
this should work is that after nsLineLayout calls
nsTextFrame::TrimTrailingWhitespace, if pfd->mJustificationNumSpaces > 0 then
nsLineLayout should request the text frame to recompute its justification weights.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Thank you Robert.
This patch has no problem.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 170442 [details] [diff] [review]
Patch rv1.0

+ nsTextFrame::IsEndOfLine()

This should be an inline function.

I'm still worried about the problem I mentioned in the second-to-last paragraph
of my previous comment. nsTextFrame::Reflow clears TEXT_IS_END_OF_LINE and then
later calls PrepareUnicodeText to count the number of justifiable characters.
This will always treat the last character as justifiable because
TrimTrailingWhitespace hasn't been called yet.

You should be able to see this in a testcase like this:
XYZABC <span>DEF </span>XYZABCDEF
where the line break occurs at the space in the span. The span will be the last
frame in the line, but it will report to line layout that it has one
justifiable character. So line layout will give half the justification space to
the first frame on the line and half to the second frame even though the second
frame isn't supposed to get any.

To fix this, nsLineLayout::TrimTrailingWhitespace will need to fix the pfd->
justifiable character counts for a frame after it's called
nsTextFrame::TrimTrailingWhitespace on the frame. I suggest adding a new
function to nsTextFrame that simply reports whether we need to reduce the
justifiable character count by one, or not.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 170442 [details] [diff] [review]
Patch rv1.0

You are right.
My patch cannot render the testcase.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 170720 [details] [diff] [review]
Patch rv1.1

GREAT! Thanks.

When this gets checked in, check to see if there is any Tp impact.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Robert.
1.8a6 is released. Please check-in the patch.

Comment 28

[Jungshik Shin]

checkined into trunk