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

RESOLVED FIXED in Future

Status

()

P3
normal
RESOLVED FIXED
18 years ago
4 years ago

People

(Reporter: roc, Assigned: sharparrow1)

Tracking

({css1, testcase})

Trunk
Future
css1, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSS1-5.4.7] (CSS2.1 Issue 234))

Attachments

(4 attachments, 3 obsolete attachments)

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.
Created attachment 11469 [details] [diff] [review]
HTML demonstrating failure to resize reflow line with "text-indent: 40%"
Created attachment 11470 [details]
HTML demonstrating failure to resize reflow line with "text-indent: 40%"
The first patch had the wrong MIME type, sorry.

BTW, I should point out that I have tested the patch and it works :-).
I mean the first *testcase* didn't have the right MIME type :-).
Keywords: patch
Created attachment 11471 [details] [diff] [review]
Patch to nsBlockFrame::PrepareResizeReflow
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.
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.
Inheriting computed values is a good thing.  Try "div.legal { font-size: 80%; }"
and throw some markup inside it...
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

18 years ago
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.
Assignee: clayton → buster
I hope buster doesn't mind if I steal this.
Status: NEW → ASSIGNED
Oops.
Assignee: buster → roc+moz
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
Summary: text-indent: n% doesn't incrementally reflow correctly → text-indent: n% doesn't inherit computed value
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

18 years ago
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.
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.
Whiteboard: (py8ieh: check with CSS working group [wg])
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.
The computed value of 'width' is based on layout.  ('inherit' can be used to 
force inheritance of that value.)
Ah, you're right. I forgot about the properties that can be explicitly 
inherited.

Alright then, we're in for a world of pain.
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.
Summary: text-indent: n% doesn't inherit computed value → text-indent: n% doesn't inherit computed value [INLINE]
Priority: P3 → P4
Target Milestone: --- → mozilla1.0

Comment 22

17 years ago
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)
Target Milestone: mozilla1.0 → mozilla1.0.1
Attachment #11469 - Attachment is obsolete: true
Attachment #11471 - Attachment is obsolete: true
Keywords: patch
Whiteboard: (py8ieh: check with CSS working group [wg]) → [CSS1-5.4.7](py8ieh: check with CSS working group [wg])
OS: Windows NT → All
Priority: P4 → P3
Hardware: PC → All
Target Milestone: mozilla1.0.1 → mozilla1.1
Target Milestone: mozilla1.1alpha → Future

Comment 23

16 years ago
Created attachment 112924 [details]
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.
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.
The expected behavior here *may* be changed by CSS 2.1.

Comment 26

16 years ago
Created attachment 113636 [details]
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!!

Updated

16 years ago
Attachment #112924 - Attachment is obsolete: true
Okay, that testcase looks better, and it seems to show the value 'inherit' is
being evaluated as '0' for 'text-indent'.
QA Contact: petersen → ian
This bug needs to be re-examined in light of the changes to the computed value 
definition in 2.1.
Whiteboard: [CSS1-5.4.7](py8ieh: check with CSS working group [wg]) → [CSS1-5.4.7] (CSS2.1 Issue 234)
Created attachment 140889 [details]
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.
The incremental reflow problem this was originally reported on is still present,
however, so setting summary back accordingly.
Summary: text-indent: n% doesn't inherit computed value [INLINE] → text-indent: n% doesn't incrementally reflow correctly [INLINE]
Component: Layout → Layout: Block and Inline
Summary: text-indent: n% doesn't incrementally reflow correctly [INLINE] → {inc} text-indent: n% doesn't incrementally reflow correctly

Updated

14 years ago
Keywords: testcase
All testcases works for me, both with SeaMonkey 2006120701 (pre reflow branch) and SeaMonkey 2006120801 (post) on Linux.
I was mistaken, the incremental reflow bug is still present in attachment 11470 [details].
(Assignee)

Comment 33

12 years ago
Created attachment 265484 [details] [diff] [review]
Patch

Pretty straightforward to fix with CSS2.1 inheritance rules.  An alternative would be something like the original patch attached to this bug.
Assignee: roc → sharparrow1
Attachment #265484 - Flags: review?(roc)
Attachment #265484 - Flags: superreview+
Attachment #265484 - Flags: review?(roc)
Attachment #265484 - Flags: review+
(Assignee)

Comment 34

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Flags: in-testsuite?
Duplicate of this bug: 148396
You need to log in before you can comment on or make changes to this bug.