368155 - text-indent needs to contribute to intrinsic width (e.g., of floats)

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

See the attached testcase. This is a reflow branch regression.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reduced testcase

Reduced testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So I figure we should add the text-indent to the first line for both min and pref width if it's a coord (and treat percentages as 0).

That said, if text-indent is negative (especially if it plus the pref width of the text is negative), what should happen?

Comment 3

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

Negative is fine, except that we need to fix InlineMinWidthData::Break to no-op if the current line so far is negative.

Min and pref width should be (and are already, I think) a max over a list of line min/pref widths and zero, so that they don't go below zero.

Comment 4

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

(In reply to comment #3)
> Negative is fine, except that we need to fix InlineMinWidthData::Break to no-op
> if the current line so far is negative.

... for optional breaks, but not forced ones.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm...  Why should Break() no-op in this case?

Comment 6

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

Because the negative indents absorb the ability of what follows them to widen the container.  (Likewise for negative margins.)  Nothing would ever need to break until it's at least into positive width.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Fair enough.

So we need to pass a boolean to Break() indicating whether it's forced or not, eh?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reftests for once this bug gets fixed

This is what I get for hanging out in #developers too much.

Comment 9

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

Comment on attachment 255750 [details]
Reftests for once this bug gets fixed

These tests look fine, except they'd need a "fails" at the beginning of the line in the manifest if they're checked in before the bug is fixed.

Comment 10

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

Oops, and, as jwalden pointed out, if they're XHTML they should have .xhtml filenames.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: Fixing minor filename issue

Sorry about that, forgot that the files should have .xhtml extensions

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 255751 [details] [diff] [review]
Fixing minor filename issue

Carrying over dbaron's previous r/sr+

Comment 13

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

But if we did the fancy break stuff, we'd need to go to even more trouble to get the spaces to contribute to min width.  So I think it's not worth the bother.

Comment 14

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

But it is worth the bother to make sure negative text-indent can't cause min width to be greater than pref width.

Comment 15

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

Attachment: patch

This fixes the bug, and fixes the accumulation issues for negative indents and margins, but I haven't written testcases for any of the negative stuff yet.

Comment 16

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

Writing the tests caught a bug in the patch:  In min-width calculation I need to force the first thing on the line to fit, since there is no break-opportunity after the text-indent.

Comment 17

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

Attachment: patch

The nsIFrame.h changes explain the basic idea of what's going on here.

roc, see my XXX comment in nsTextFrameThebes as well...

Comment 18

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

> // XXXldb Where is the equivalent code in GetInlineMinWidth?

here:

    if (i < flowEndInTextRun && !mTextRun->CanBreakLineBefore(i)) {
      PRBool preformattedNewline = !collapseWhitespace &&
        frag->CharAt(iter.ConvertSkippedToOriginal(i)) == '\n';
      if (!preformattedNewline) {
        // we can't break here
        continue;
      }
    }

the code after this block handles both optional and forced line-breaks. You probably need to set a flag in here (maybe just move preformattedNewline out) to indicate whether the break is forced or not.

Comment 19

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

Attachment: patch

Still an XXX comment, but nsTextFrameThebes passes my tests now...

Comment 20

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

+      frag->CharAt(iter.ConvertSkippedToOriginal(start) == '\n')) {

I don't think the parentheses are in the right place....

+    PRBool preformattedNewline;

You probably want to initialize this to prevent warnings.

+      // XXXldb Shouldn't we be including the newline as part of the
+      // segment that it ends rather than part of the segment that it
+      // starts (and avoid the extra chunk above)?

Probably the best way would be to just have i start at "start".

Comment 21

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

Attachment: patch

Comment 22

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

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

+    PRBool preformattedNewline;

I still think you should initialize this to avoid warnings.

+      // XXXldb Shouldn't we be including the newline as part of the
+      // segment that it ends rather than part of the segment that it
+      // starts (and avoid the extra chunk above)?

You want to change this comment now that there is no extra chunk above...

Comment 23

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

Checked in to trunk, with those comments addressed.

Also checked in Ryan's reftest -- thanks for the test.

Comment 24

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/bugs&command=DIFF_FRAMESET&file=reftest.list&rev1=1.93&rev2=1.94&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/text-indent&command=DIFF_FRAMESET&file=reftest.list&rev1=1.1&rev2=1.2&root=/cvsroot

Comment 25

[Samuel Sidler (old account; do not CC)]

v.fixed

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre