333560 - Missing space (or overlapping text) with nested RTL inlines and trailing collapsed whitespace

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In the URL above, in the items on the right hand side, there should be a space between the white text and the yellow link. That space is currently missing.

This is a regression from bug 299065.

Testcase coming soon.

Comment 1

[Uri Bernstein]

Attachment: testcase

Each line with red text should look like the line with green text below it:
Line #1 should have a space between the black and red texts.
Line #3 should not have a space, but the black and red texts should not overlap.

Comment 2

[Niro]

I reproduced it on Windows XP.
Please change OS and Hardware values to "All" (I am not allowed to do so)

Comment 3

[Uri Bernstein]

Attachment: patch

It appears that the frame holding the collapsed whitespace has width 0, but a bogus x coordinate. The patch ignores zero-width frames when looking for the last (rightmost) sibling (whose x-most coordinate is used as a base for laying out the siblings from right to left).

Comment 4

[Boris Zbarsky [:bzbarsky]]

So why is there a bogus x coord here?  Is that the real bug we should be fixing?

Comment 5

[Boris Zbarsky [:bzbarsky]]

And how does what that patch implements compare to sections 16.6.1 and 16.6.2 of CSS 2.1?

Comment 6

[Uri Bernstein]

(In reply to comment #4)
> So why is there a bogus x coord here?  Is that the real bug we should be
> fixing?
> 

Let me try to explain first what I meant by "bogus". The following is an LTR-only example:
Let's assume we're using a monospace font, where the character width is 100.
Suppose we have the following HTML:

<div><span><span>foo </span> </span></div>

We have two inline frames (for the two <span>s), and two text frames (one containing "foo ", and the other containing " ").
Both inline frames, as well as the first text frame, have width=300 (and x=0).
The second text frame has width=0 and x=400. That "400" is what I consider "bogus", as I would expect it to be 300.

Is this a real bug? I'm not sure I can answer this. I don't know of any code (other than the code I'm touching here) that cares about the x coordinate of zero-width frames, but there's a lot of code I don't know about.

Comment 7

[Uri Bernstein]

(In reply to comment #5)
> And how does what that patch implements compare to sections 16.6.1 and 16.6.2
> of CSS 2.1?
> 

Those sections deal with how whitespace is collapsed. The code I'm touching here does not do whitespace collapsing. It assumes that whitespace was already collapsed (and that the collapsing is reflected in the widths and positions of the frames). This assumption is valid thanks to the fact that 16.6.2 specifies that whitespace collapsing does not depend on bidi ordering of the frames - it's done on the logical level, and therefore can be done before reordering.

The only issue I'm really dealing with here is our implementation of "removing spaces" (as specified in 16.6.1). Specifically, for a whitespace-only text node, where the whitespace should be "removed" according to the spec, it seems that we do not actually remove the frame associated with that node, but simply set its its width to 0. The question then is whether the x coordinate of that frame is reliable. Currently it appears not to be so.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> That "400" is what I consider "bogus", as I would expect it to be 300.

Why?  The text before that frame is "foo " which has a width of 400, no?

Comment 9

[Uri Bernstein]

(In reply to comment #8)
> > That "400" is what I consider "bogus", as I would expect it to be 300.
> 
> Why?  The text before that frame is "foo " which has a width of 400, no?
> 

No. The text before it has a width of 300, as the trailing space isn't rendered (this can be vrified by setting a background or border on that text, or by doing a frame dump, of course).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

OK, I'll try to sort through this in the next day or two.  I need to finish the review for bug 332333 first, but after that....

Comment 11

[Uri Bernstein]

Attachment: testcase for underlying issue

This (LTR only) testcase illustrates comment #6. I had to change the example slightly by making the inner span wrap the trailing space instead of the "foo ", so I can apply a background and some horizontal padding to that space.
The first line in the testcase demonstartes the "problem" - the trailing space is not adjacent to the "foo", as if the first space was not collapsed.
The second line lacks the outer span - which makes the problem disappear.

Notice that it's not clear at all whether the background color should at all show up in this case. It vanishes if I make the padding width narrower than the width of a space. Safari doesn't show the colored backgrounds at all on this testcase. But this is probably an unrelated issue.

Comment 12

[Niro]

Attachment: another testcase

Overlapping text

Comment 13

[Uri Bernstein]

Nir's testcase is actually a bit different - it involves whitespace followed by a <br>, and it only regressed between 2006-04-20 and 2006-04-21, due to the fix for bug 132561.
Here the x coordinate of the (zero-width) BRFrame remains as if the space at the end of the line was not collapsed. The proposed patch therefore fixes this problem as well.

Note also that Nir's testcase represents a much more common situation than the original testcase, so this bug now affects a lot more pages.

Boris - did you have time to look at this since you wrote comment 10?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Uri, I did spend some time looking at this code, and I just don't know enough about our line layout and text reflow to review this in a timely manner.  :(  Could you maybe try roc or rbs?

Comment 15

[Uri Bernstein]

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

roc, can you weigh in on this?

BTW, if my approach is acceptable, I should still probably check for the case where all frames on the line are zero-width.

Comment 16

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

I think we should fix the underlying bug, which is that nsLineLayout::TrimTrailingWhitespace(In) should be fixing frame geometry to account for trimmed whitespace as Uri described very well in comment #6. In fact it tries to do this --- see http://lxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#2614 --- but clearly it's buggy.

Comment 17

[Uri Bernstein]

(In reply to comment #16)
> In fact it tries to do this --- see
> http://lxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#2614
> --- but clearly it's buggy.

That code, as far as I can tell using a debugger, correctly adjusts the x coordinate on the pfd - but this adjustment is never propagates to the frame itself.

Unfortunately I know nothing about PFDs, so I can't say whether the adjustment should be copied to the frame rect coordinates, and if so, where would be the correct place for that to happen.

Comment 18

[Uri Bernstein]

Attachment: patch, alternative B

This fixes the problem I described in comment 17, by copying some code (and comments) from the previous block (some 20 lines above).
With this patch, the original testcase renders correctly.

However, this does not change the behavior of the "underlying issue" testcase, and also, it doesn't fix Nir's testcase (which, as I mentioned above, is a more serious real-life problem).
So I suppose if we chose to take this route, we should file separate bugs for those two issues.

Comment 19

[Uri Bernstein]

Attachment: patch, alternative B

I should really pay more attention to what I'm actually attaching.

Comment 20

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

Comment on attachment 220567 [details] [diff] [review]
patch, alternative B

This is a good and correct fix no matter how we fix those other bugs.

Comment 21

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

Comment on attachment 220567 [details] [diff] [review]
patch, alternative B

Actually ... how about doing this at both of the places that this code occurs in TrimTrailingWhitespaceIn? Preferably via a small static helper function.

Comment 22

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

That might even fix these other testcases.

Comment 23

[Uri Bernstein]

Attachment: patch, alternative B, v2

> That might even fix these other testcases.
It does!

I couldn't pass pfd to the static function, because its type is protected. So I'm not sure whether it's really worth keeping the function for what it does. Also, feel free to suggest a better name for that function, if it is to be kept.

Comment 24

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

Comment on attachment 220623 [details] [diff] [review]
patch, alternative B, v2

That'll do

Comment 25

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsLineLayout.cpp;
/cvsroot/mozilla/layout/generic/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.236; previous revision: 3.235
done

Comment 26

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).