860666 - font metrics of ::first-line erroneously used to calculate intrinsic width of entire element

Status: NEW

(Core :: Layout: Block and Inline, defect)

Description

[Francesco]

Attachment: testcase.htm

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

I applied a different font size to the first line of a table cell with :first-line; no explicit width specified for table or cell (see the code in the attached test file)


Actual results:

The size of the td element seems to be based only on the font size specified for first line; the td may be unnecessarily wide or the text within may overflow (see the attached test file)


Expected results:

The size of the td element should be based on the font size of all text within it

Comment 1

[Boris Zbarsky [:bzbarsky]]

I see the problem in Firefox 20, but not in a nightly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ah, I can sometimes get this to reproduce in a nightly if I reload enough.

This is really weird.

Mats, do you have time to look into this?

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #2, DIV:first-line

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2, DIV:first-letter

Comment 5

[Mats Palmgren (inactive)]

The problem is that when creating continuations for :first-line/letter frames
during reflow it may affect the block's width, but it appears we have no code
to deal with that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

For this particular case, with a <br>, can we just not put the text after the <br> inside the firstline frame to start with?

Comment 7

[Mats Palmgren (inactive)]

Maybe I misunderstand, but that's what we do and that's the problem!
When we start the initial reflow we have this:

Line(div)(1)@0x7f99f3fc9ae8 {0,0,0,0} [state=0000000000000402] [content=0x7f99ffa30600] [sc=0x7f9a0084a178] pst=:first-line<
  Text(0)"Blah Blah Blah Blah"@0x7f99f3fc99b8 [run=0x7f99f040b4f0][0,19,T]  next=0x7f99f3fc9a28 {0,0,0,0} [state=0000000050000402] [content=0x7f99ffa30700] sc=0x7f9a00849b50 pst=:-moz-non-element
  Frame(br)(1)@0x7f99f3fc9a28 {0,0,0,0} [state=0000000000000602] [content=0x7f99ffa30800] [sc=0x7f99f3fc9b60]
  Text(2)"Blah Blah Blah Blah Blah Blah Blah Blah "@0x7f99f3fc9a78 [run=0x7f99ee753800][0,40,T]  {0,0,0,0} [state=0000000040000402] [content=0x7f99ffa30d00] sc=0x7f9a00849b50 pst=:-moz-non-element
>

And when we calculate the preferred width for that we use the :first-line
style for *all* of the text.  Then we reflow, break the line, create the
non-:first-line style context for the text after the <br>, which now
does not fit in the preferred width.

Hang on, I'll clarify with a patch...

Comment 8

[Mats Palmgren (inactive)]

Attachment: wip

This could be one solution - basically just abort the current block's
reflow and restart it since its width will change.

(the nsFirstLineFrame change should probably go in a separate bug
because that's an independent bug really)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Maybe I misunderstand, but that's what we do 

I think you misunderstand.

My proposal is that when creating the initial lineframe we only make it contain things up to the <br>.  So the frame dump you show would never happen.

Comment 10

[Mats Palmgren (inactive)]

Attachment: plan B

> My proposal is that when creating the initial lineframe we only make it
> contain things up to the <br>.  So the frame dump you show would never
> happen.

OK, that means splitting frames during frame construction though.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why do we need all that?

What I was figuring is that this bit:

  while (link.NextFrame() && link.NextFrame()->IsInlineOutside()) {

would just also check for brframes....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 737338 [details] [diff] [review]
plan B

I don't think we want to split frames like that during frame construction....

Comment 13

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Hmm.  Why do we need all that?

<p><span>A<br>B</span></p>

should be handled per spec as:
<p><p::first-line><span>A<br></span></p::first-line><span>B</span></p>

Are you saying the spec is wrong?
http://dev.w3.org/csswg/selectors3/#first-line

Comment 14

[Boris Zbarsky [:bzbarsky]]

No, the spec is correct.  But if we want to handle that case, not just <br> on toplevel, then I'm not at all sure frame construction is the right place to handle it, indeed.

Comment 15

[Mats Palmgren (inactive)]

Boris Zbarsky (:bz) from bug 861643 comment #6:
> Well, I guess the real question is whether anything actually calls
> InsertFrames/AppendFrames/SetInitialChildList on these frames... and if not,
> whether we can just assert that this continues to be the case.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Mats, I'm not following what you mean in comment 15...

Comment 17

[Mats Palmgren (inactive)]

I copied your comment here as a reminder to consider adding such an assertion in
this bug rather than bug 861643.  It kind of depends on how we solve this bug.

BTW, I don't really see any other solutions than the two outlined so far.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Hmm.  So I'd like to understand the plan A better.  Given multiple blocks with interdependent widths (e.g. multiple table cells), how does it work?

Comment 19

[Francesco]

Might this bug be related to bug 362880? For example, bug 593551, which has been marked as a duplicate of it, looks quite similar to this.

Comment 20

[Boris Zbarsky [:bzbarsky]]

It's similar, but first-line vs first-letter means very different code is involved.

Comment 21

[Francesco]

Comment 5 and attachment 736628 [details] DIV:first-letter seem to suggest that the problem here involves both first-line and first-letter; but I may be wrong, of course.

Comment 24

[Ksenia Berezina [:ksenia]]

The site in https://webcompat.com/issues/55625 is returning a 404 for the affected link now, so there is no active breakage and we can unset the Webcompat priority.
(Looking at the history from August 2021 https://web.archive.org/web/20210809181819/https://jeancharlot.org/20191119_stylesheet.css.html it is still an issue in Firefox and Chrome, but works fine in Safari).

The first 2 testcases in this bug are also giving similar results in Chrome and Firefox, but Safari is different, so does the test case linked in a bug1655799 (a dupe of this bug). Based on this, we probably should keep this open still.