1217833 - CSS :first-line rule on RTL element, text placement is wrong

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[yonizaf]

Attachment: test case for rtl first-line

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150916094008

Steps to reproduce:

If you add a ::first-line CSS rule to a div that has dir=rtl and text-align:right, the text in the div get placed outside the element and far to the right. 

The ::first-line rule can be empty, it doesn't have to have any properties in it for this to happen.

A test file is attached to show the problem.

Comment 1

[Simon Montagu :smontagu]

This is a regression, but not a recent one. The initial regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd, though it seems to have morphed a bit since then.

A likely culprit is http://hg.mozilla.org/mozilla-central/rev/dab8e3865967 from bug 789096

Comment 2

[yonizaf]

Is there a reason why this bug still has unconfirmed status?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not surprisingly, the nsFirstLineFrame frames for both the first and following lines have basically the same x-offset as their contents, double-offsetting the contents.

I suspect that before the patch we always aligned first line frames to the left edge.  But I think that in a logical coordinate world it makes more sense to align them to the start-edge.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though another reasonable possibility would be to make the line frame fill the entire inline-size of the block (possibly excluding space taken by floats).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, it's not clear to me what's going wrong, given that we do call BeginSpan and EndSpan for the nsFirstLineFrame (which uses nsInlineFrame::ReflowFrames), so it's not clear to me why the use of ContainerSizeForSpan (and the calls to SetRect in VerticalAlignFrames) don't take care of this properly.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack showing the problem

The nsLineLayout code is doing the right thing, but then nsBidiPresUtils::RepositionFrame is moving it.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The thing that seems like the obvious patch:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/71b60ac7f7e4/remove-line-frame-exception
breaks the bidi reordering of the second of my tests (while fixing the positioning).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But adjusting aContainerSize there works.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c5c82835f0c&group_state=expanded

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Fix container width in exception for line frames in nsBidiPresUtils::ReorderFrames

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, I need to fix that commit message to be:

Bug 1217833 - Fix container width in exception for line frames in nsBidiPresUtils::ReorderFrames.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8691218 [details] [diff] [review]
Fix container width in exception for line frames in nsBidiPresUtils::ReorderFrames

Review of attachment 8691218 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense to me, thanks (with the corrected commit msg).

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac7ee2441e58d18ecac91a19a9a886b92794c5e
Bug 1217833 - Fix container width in exception for line frames in nsBidiPresUtils::ReorderFrames.  r=jfkthame

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1425df0a968&group_state=expanded

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e567a54aa60420657992d405823af24b2b2e7e70
Bug 1217833 followup - Mark one test as fuzzy(43,2) on a CLOSED TREE (though fuzzy amounts needed vary by platform).

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the fuzz was needed because the leftmost edge of the t was overlapping with the blue background, and the paint order of the two was different between test and reference.

I'll try a try run with adding a margin and removing the fuzz.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ac7ee2441e5
https://hg.mozilla.org/mozilla-central/rev/e567a54aa604

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5633db33bf&group_state=expanded

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a84fdee7bfbfe60f4bf2555519556e87d028dfe
Bug 1217833 followup - Add margin to avoid fuzz on reftest.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a84fdee7bfb