338993 - Bidi text inside a span with white-space:nowrap; can wrap between bidi runs

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When a span (or other inline) is styled with "white-space:nowrap;", and contains bidi text, the text can still wrap between bidi runs (i.e., between LTR and RTL parts).

This happens only if the white-space:nowrap; is specified on the inline element itself, but not if it is inherited from a containing block element.

This is a regression from bug 299065. The span is now split into multiple continuations (one per bidi run), each with white-space:nowrap, so no wrapping can occur within each run. However, there's nothing left to indicate that wrapping can't occur between these continuations.

Testcase coming up. Suggestions for how to fix this are most welcome.

Comment 1

[Uri Bernstein]

Attachment: testcase

All the text should appear on the same line.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we're basically wrapping between a frame and its GetNextContinuation()?  Would just adding a check for that be sufficient?

Comment 3

[Uri Bernstein]

(In reply to comment #2)
> So we're basically wrapping between a frame and its GetNextContinuation()? 

Yes.

> Would just adding a check for that be sufficient?

Probably. (We have to check we're not breaking between a frame and its continuation if the frame is white-space:nowrap).

I don't understand the line-breaking code well, but from what I saw, nsLineLayout deals with nowrap by setting psd->mNoWrap on any nowrap container, thus signaling that line breaking is not allowed between children of that container. This mechanism is no good for us, since we sometimes want to allow wrapping between some children of a container, but not others (a nowrap frame and its continuation).
 
So I'm not sure where would be the right place to stick the check you are proposing, and I have a feeling it would be somewhat ugly (requiring to examine the white-space property in an additional place). I plan to look more into this tomorrow, but I'll appreciate any pointers or help.

Comment 4

[Simon Montagu :smontagu]

Is this ultimately the same issue as bug 177147?

Comment 5

[Uri Bernstein]

(In reply to comment #4)
> Is this ultimately the same issue as bug 177147?
> 

They are somewhat similar, but I don't know the code well enough to say whether they are "ultimately the same issue".

Comment 6

[Uri Bernstein]

Attachment: hack

This is probably not the correct solution, but as far as I can tell, it works.
It doesn't fix bug 177147, of course.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 223154 [details] [diff] [review]
hack

I don't really know this code well enough to review it... try roc or dbaron?

Comment 8

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

I think this CanPlaceFrame function should really be removed or massively changed. In inline layout, the default should be to not allow breaking. Dynamic breaking should only be allowed at certain points ... namely, before and after replaced elements (for compatibility reasons), at <WBR>, and in text with whitespace:normal (in whitespace, at hyphens, before or after ideographs). Instead we're trying to do it all backwards with breaking between frames the default and then suppressed in certain cases.

(This is different from block layout where by default you *can* break between lines.)

In fact, what I proposed in bug 317278 is to reduce line breaking to three mechanisms:
1) during reflow, when an inline frame finds that it can't all fit in available width, return BREAK_BEFORE if none of the frame fit or BREAK_AFTER if some fit and the rest was pushed to a continuation.
2) return BREAK_AFTER when a hard break is hit (whitespace:pre newline, BR)
3) during reflow, record the last place where an optional break could be taken. If we overrun the available width, reflow the line again and force it to break at the last possible place.

That would make this bug and bug 177147 go away.

I'm actually hoping to do this as soon as dbaron reviews the patch in bug 317278 which starts things moving in this direction. So if you don't mind let's keep this patch here on ice for a while in case it's not needed.

Comment 9

[Uri Bernstein]

(In reply to comment #8)
> I'm actually hoping to do this as soon as dbaron reviews the patch in bug
> 317278 which starts things moving in this direction. So if you don't mind let's
> keep this patch here on ice for a while in case it's not needed.
> 

I don't mind at all. I'd love to see this (and bug 17747) fixed the right way, instead of doing this ugly hack.

Comment 10

[Simon Montagu :smontagu]

Fixed by bug 343445