CSS borders and backgrounds incorrectly rendered when element is broken across lines in vertical mode

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
See testcase. In horizontal mode, the blue (left) and red (right) borders on the <b> element are correctly rendered only at the beginning and end of the entire element.

However, in vertical mode, the equivalent blue (top) and red (bottom) borders are repeated on each fragment of the <b> element when it breaks across lines. The spurious renderings do not have any space allowed for them, however, so they overlap the glyphs; only the (correct) borders at the beginning and end actually occupy space in the line.
Assignee

Comment 1

5 years ago
The problem here occurs because InlineBackgroundData assumes lines are horizontal; it needs to be aware of vertical writing mode. For now, it looked simplest to keep working with physical coordinates here, and just test IsVertical as needed and choose x or y fields of various rects, rather than converting everything to logical-coordinate geometry, though that would be a possible alternative approach. If we were to change all the frames' mRects to be logical some day, then we'd want to rework this too.
Attachment #8506363 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Blocks: 1079125
No longer blocks: writing-mode
Assignee

Comment 2

5 years ago
A reftest based on the testcase here.
Comment on attachment 8506363 [details] [diff] [review]
Make InlineBackgroundData aware of vertical writing mode.

I'm curious about the terminology here:
 * why "Measure" rather than "ISize"?
   (I thought we were avoiding Extent and Measure terminology;
   css-writing-modes certainly is now.)
 * why "VisualStart" rather than "IStart"?  (I don't understand what
   distinction there is, if any.)

Perhaps rename these?

Also, presumably VisualStartBorderData's SetX method should also
be renamed SetCoord.

Otherwise this seems fine; r=dbaron with that, or happy to discuss
naming if you think the names you have are better.
Attachment #8506363 - Flags: review?(dbaron) → review+
Comment on attachment 8520169 [details] [diff] [review]
Reftest for borders at start/end of an inline element that is split across lines.

What about a reftest for background drawing (esp. with images), which is what InlineBackgroundData is supposed to handle.  (Also see the background-break property.)
Oh, I guess it handles both backgrounds and borders since bug 613659 or bug 988653.  And I meant box-decoration-break rather than background-break.  Perhaps still worth testing backgrounds too?
Assignee

Comment 6

5 years ago
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #3)
> Comment on attachment 8506363 [details] [diff] [review]
> Make InlineBackgroundData aware of vertical writing mode.
> 
> I'm curious about the terminology here:
>  * why "Measure" rather than "ISize"?
>    (I thought we were avoiding Extent and Measure terminology;
>    css-writing-modes certainly is now.)
>  * why "VisualStart" rather than "IStart"?  (I don't understand what
>    distinction there is, if any.)

This isn't an inline-dir coordinate, which is why I named it differently. It's a physical coordinate in either the horizontal (LTR) or vertical (TTB) direction, depending whether we're dealing with horizontal or vertical writing mode; but it always measures from left/top towards right/bottom, regardless of bidi directionality. I should add some comments to clarify this, though.

I forget exactly what I was thinking with the use of "Measure" -- probably because of the connection with the VisualStart coordinate, which is not an IStart, I wasn't totally happy using ISize either. But that would be OK, actually; regardless of bidi direction, it's still the frame's ISize that is relevant.

> 
> Perhaps rename these?
> 
> Also, presumably VisualStartBorderData's SetX method should also
> be renamed SetCoord.

Indeed it should.

So, I'll do some renaming and post a fresh patch, but I'd like to keep VisualStart (or some other name that's distinct from IStart, if you have suggestions); I'll try to explain it better in comments.
I'm not crazy about "visual", though I'm ok with it if you prefer it.  Maybe ILow for the inline-direction low-numbered end, though that's a bit cryptic?
Assignee

Comment 8

5 years ago
Updated patch with a comment added; and calling this the "visual-direction inline start", or VIStart for short. Does that seem acceptable/understandable enough?
Attachment #8523831 - Flags: feedback?(dbaron)
Assignee

Updated

5 years ago
Attachment #8506363 - Attachment is obsolete: true
Assignee

Comment 9

5 years ago
I've added a reftest using a background-image; without the patch here, the (non-repeating) background is incorrectly rendered at the beginning of the second line as well as at the beginning of the <b> element.
Attachment #8523854 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8520169 - Attachment is obsolete: true
Comment on attachment 8523831 [details] [diff] [review]
Make InlineBackgroundData aware of vertical writing mode.

> Updated patch with a comment added; and calling this the "visual-direction
> inline start", or VIStart for short. Does that seem acceptable/understandable
> enough?

Hmmm.  I guess I just don't see what's "visual" about it.  positive-direction inline start?

I don't feel strongly enough to hold it up, though.
Attachment #8523831 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8523854 [details] [diff] [review]
Reftests for borders and backgrounds when inline element is split across lines in vertical writing-mode.

r=dbaron, although I would somewhat prefer:
 * tests to have useful names rather than bug numbers
 * not to use a red image as part of a pass condition for a test (any other color is fine, preferably not green, though, since that means the test passes by the presence of the image)

It also might not be a bad idea to test a repeating image that's nonuniform so that you're checking the tiling position.

(I didn't look that closely at the tests themselves; let me know if you wanted me to.)
Attachment #8523854 - Flags: review?(dbaron) → review+
Assignee

Comment 12

5 years ago
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #10)
> Hmmm.  I guess I just don't see what's "visual" about it. 
> positive-direction inline start?

That doesn't really help, IMO. It seems confusing because in a block with RTL direction, the positive direction for inline coordinates is leftwards, which isn't what we're dealing with here: this is a coordinate that is measured either LTR (if writing mode is horizontal) or TTB (if mode is vertical), not affected by bidi directionality.

I suppose I keep thinking of "visual" because of the traditional distinction between "logical" and "visual" when discussing bidi. So in a horizontal context, "visual" direction is always LTR, whereas the logical direction may be either LTR or RTL; and analogously, in vertical mode "visual" would be TTB, even if direction=rtl and the text is actually running BTT.

Looking at it another way, it's the inline-axis of the physical topLeft-to-bottomRight coordinate system. Call it PhysicalInline?
Flags: needinfo?(dbaron)
Assignee

Comment 13

5 years ago
I've added a test for a repeating background, to check its positioning/tiling; this fails on current trunk, and passes with the patch here.
Attachment #8527876 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8523854 - Attachment is obsolete: true
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> That doesn't really help, IMO. It seems confusing because in a block with
> RTL direction, the positive direction for inline coordinates is leftwards,
> which isn't what we're dealing with here: this is a coordinate that is
> measured either LTR (if writing mode is horizontal) or TTB (if mode is
> vertical), not affected by bidi directionality.

Well, it's the positive physical direction in the logical inline axis.

> I suppose I keep thinking of "visual" because of the traditional distinction
> between "logical" and "visual" when discussing bidi. So in a horizontal
> context, "visual" direction is always LTR, whereas the logical direction may
> be either LTR or RTL; and analogously, in vertical mode "visual" would be
> TTB, even if direction=rtl and the text is actually running BTT.

OK, now at least the term makes sense.

> Looking at it another way, it's the inline-axis of the physical
> topLeft-to-bottomRight coordinate system. Call it PhysicalInline?

Works for me.
Flags: needinfo?(dbaron)
Assignee

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8caca1d5c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1ef81301e4
Summary: CSS borders incorrectly rendered when element is broken across lines in vertical mode → CSS borders and backgrounds incorrectly rendered when element is broken across lines in vertical mode
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/fe8caca1d5c1
https://hg.mozilla.org/mozilla-central/rev/bc1ef81301e4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 1243125
No longer depends on: 1243125
You need to log in before you can comment on or make changes to this bug.