45631 - {inc} text-indent: n% doesn't incrementally reflow correctly

Status: RESOLVED FIXED

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

Description

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

A block styled with "text-indent" set to some percentage value needs to reflow 
its first line whenever the block size changes. This doesn't happen; if the 
available width increases, then the optimizations in 
nsBlockFrame::PrepareResizeReflow will avoid marking the line dirty.

I have testcase and a patch. Open the testcase in Viewer and observe that, when 
you resize the window horizontally, the text doesn't move unless you force the 
line to be reflowed by moving the right margin across the right edge of the 
text.

The patch changes nsBlockFrame::PrepareResizeReflow, forcing the first line to 
be dirtied if "text-indent" is set to a percentage value. I considered other 
places to make the change (e.g. calling SetHasPercentageChild(PR_TRUE) when the 
first line is initially reflowed), but they turned out to be significantly more 
messy.

Comment 1

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

Attachment: HTML demonstrating failure to resize reflow line with "text-indent: 40%"

Comment 2

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

Attachment: HTML demonstrating failure to resize reflow line with "text-indent: 40%"

Comment 3

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

The first patch had the wrong MIME type, sorry.

BTW, I should point out that I have tested the patch and it works :-).

Comment 4

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

I mean the first *testcase* didn't have the right MIME type :-).

Comment 5

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

Attachment: Patch to nsBlockFrame::PrepareResizeReflow

Comment 6

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

This patch looks good to me (and low risk, but I have run into problems on low
risk changes recently...).  Considering the number of block/inline layout people
on sabbatical right now, I guess I should count as a reviewer (so r=dbaron), but
cc:ing waterson anyway so he can have a look.

Comment 7

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

I think we should put this on hold for now; the patch is not a complete 
solution. The immediate problem is that we actually need to reflow the first 
line whenever the block's *parent* size changes, because that's what the 
percentage depends on. This patch will not ensure that.

The bigger problem is that we need to inherit the computed value of text-indent, 
not the percentage. This implies that whenever we reflow a block which has a 
percentage "text-indent" style rule, we need to reflow all the (indirect) child 
frames that inherit the text-indent value.

Whose idea was it to inherit computed values of percentage attributes, anyway? 
Grrr.

Comment 8

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

Inheriting computed values is a good thing.  Try "div.legal { font-size: 80%; }"
and throw some markup inside it...

Comment 9

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

OK, that's harmless.

But the computed value depends on the layout of the parent box. So the size of 
some child box is linked non-locally to the size of some other box arbitrarily 
far up the box hierarchy. Mix that in with boxes whose layout depends on the 
sizes of their children, and BOOM!

Comment 10

[Marc Attinasi]

Triaging Clayton's list:

Handling off to buster, even though he is out. If this needs attention soon I'm 
CC'd so I'll see it.

Comment 11

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

I hope buster doesn't mind if I steal this.

Comment 12

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

Oops.

Comment 13

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

Changing the summary to reflect the bigger problem here. Note that as well as 
fixing the style system, I also have to fix reflow so that when the computed 
value changes, all the inheriting affected frames will be reflowed.

Comment 14

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

I have some doubts about the spec for "text-indent".

Consider the following example:
<div style="width: 200px;">
  <div style="width: 50px; text-indent: 50%;">
    Hello Kitty
  </div>
</div>

The spec says "the percentage refers to the width of the containing block". It 
only applies to block-level elements, and we inherit the computed value. Sooo... 
we compute 50% of 200px and apply a text-indent of 100px. Does that seem 
logical? I think not. In fact, there is no way to get the text-indent to be 50% 
of the inner div (short of inserting another div inside just to carry the 
"text-indent" property). I suspect that the "applies to containing block" text 
was copied and pasted from the other percentage properties (margins, width, etc) 
without thinking about it.

Suggestion: a percentage value for "text-indent" should be relative to the width 
of the block it applies to.

Comment 15

[Marc Attinasi]

Robert, I agree that it should apply to the element the property is specified on 
- in fact, I wonder if 'containing block' means just that, not the block that 
contains the element that the property is applied to, but the block that 
contains the property... It hardly seems useful to have the text-indent 
percentage be relative to the width of the block that contains the element that 
is being text-indented, as your example clearly shows.

Comment 16

[Hixie (not reading bugmail)]

I will check with the Working Group when I ask them about all the other things I
need to work out. I suggest we put this bug on hold until then.

Comment 17

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

I wonder if you could also bring up the issue that percentage text-indent is 
uniquely difficult to implement because it (alone among the CSS2 properties) has 
a troublesome combination of characteristics:
-- elements inherit the computed value
-- the computed value depends on the size of the box of the ancestor element 
carrying the property declaration (or, following the letter of the spec, its 
parent)

With other properties for which the computed value is inherited, the computed 
value depends only on the other styles applied to the element carrying the 
property declaration. In particular, it never depends on layout. "text-indent" 
is the only property where the actual value for a box depends on the dimensions 
of some box arbitrarily far away in the box hierarchy. This is particularly 
nasty for fixed-position boxes, because it means layout within the 
fixed-position box may depend on the dimensions of some box in a completely 
different subtree of the box tree. As far as I know, this is the only feature of 
CSS2 requiring such non-local constraints.

The evil example would be something like this:
<div>
  ... arbitrary deeply nested content ...
       <div id="evil" style="text-indent: 20%">
          <div id="sick" style="position: fixed; width: 100px; height: 100px;">
             ... arbitary content ...
          </div>
       </div>
  ...
</div>
The layout of content in the "sick" div depends on the dimensions of the "evil" 
div, but there's no relationship between their boxes.

My preferred fix is to have "text-indent" inherit the specified value rather 
than the computed value. This is how it currently works in Mozilla, but it's
also the only way we can avoid non-local layout constraints.

Comment 18

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

The computed value of 'width' is based on layout.  ('inherit' can be used to 
force inheritance of that value.)

Comment 19

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

Ah, you're right. I forgot about the properties that can be explicitly 
inherited.

Alright then, we're in for a world of pain.

Comment 20

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

OK, I have a patch that makes percentage text-indent work correctly, according 
to the letter of the spec. Changing it to the more sane alternative of applying 
the percentage to the block's own width is a one-line change.

Except there is one big problem: fixed-position elements inherit the property 
from the viewport, not from their true parents. In fact, this is currently true 
of all the inherited percentage styles that depend on layout (top, left, right, 
bottom, width, height, max-width, max-height, min-width, min-height, and maybe 
others). This is a serious bug that will not be easy to fix. It should be filed 
seperately if it isn't filed already.

Anyway, we can and should still fix the text-indent behaviour. I will submit the 
appropriate patch when Ian hears back from the working group.

Comment 21

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

Filed as bug 47613.

Comment 22

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 23

[Madhur Bhatia]

Attachment: testcase demonstarting that the %age value of text-indent is not inherited by the children

Here the parent has been given the styel "text-indent : 50%"
the children are inheriting the text-indent value from it's parent.

actual:
the text in the parent DIV is indented 50% of the viewport width 
the children DIVs are not inheritting the value (there is no indentation)

expected:
the children DIV's should have the txt indented by 50 % of it's contained
parent.

Comment 24

[Eric A. Meyer (dead account)]

Attachment 112924 [details] looks fine to me in 2003012213/Mac.  The paragraph has
'text-indent: 25%' and indeed its first line appears to have been indented 100
pixels, as the testcase text itself describes.  The description of the rules and
markup in comment 23 doesn't match the attachment.

Comment 25

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

The expected behavior here *may* be changed by CSS 2.1.

Comment 26

[Madhur Bhatia]

Attachment: actual testcase ..... demonstarting that the %age value of text-indent is not inherited by the children

whoops !!! sorry i attached the wrong testcase (attachment 112924 [details]).

Here is the correct testcase. Please refer to the description given in comment
23.
Thanks!!

Comment 27

[Eric A. Meyer (dead account)]

Okay, that testcase looks better, and it seems to show the value 'inherit' is
being evaluated as '0' for 'text-indent'.

Comment 28

[Hixie (not reading bugmail)]

This bug needs to be re-examined in light of the changes to the computed value 
definition in 2.1.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase that clearly shows inheritance of percent and absolute indents

This testcase (and code inspection of nsRuleNode.cpp) show that we do
inheritance of text-indent correctly per CSS2.1.

Comment 30

[Boris Zbarsky [:bzbarsky]]

The incremental reflow problem this was originally reported on is still present,
however, so setting summary back accordingly.

Comment 31

[Mats Palmgren (inactive)]

All testcases works for me, both with SeaMonkey 2006120701 (pre reflow branch) and SeaMonkey 2006120801 (post) on Linux.

Comment 32

[Mats Palmgren (inactive)]

I was mistaken, the incremental reflow bug is still present in attachment 11470 [details].

Comment 33

[Eli Friedman]

Attachment: Patch

Pretty straightforward to fix with CSS2.1 inheritance rules.  An alternative would be something like the original patch attached to this bug.

Comment 34

[Eli Friedman]

Checked in.