Closed
Bug 45631
Opened 25 years ago
Closed 18 years ago
{inc} text-indent: n% doesn't incrementally reflow correctly
Categories
(Core :: Layout: Block and Inline, defect, P3)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: roc, Assigned: sharparrow1)
References
Details
(Keywords: css1, testcase, Whiteboard: [CSS1-5.4.7] (CSS2.1 Issue 234))
Attachments
(4 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•25 years ago
|
||
Reporter | ||
Comment 2•25 years ago
|
||
Reporter | ||
Comment 3•25 years ago
|
||
The first patch had the wrong MIME type, sorry. BTW, I should point out that I have tested the patch and it works :-).
Reporter | ||
Comment 4•25 years ago
|
||
I mean the first *testcase* didn't have the right MIME type :-).
Keywords: patch
Reporter | ||
Comment 5•25 years ago
|
||
Comment 6•25 years ago
|
||
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.
Reporter | ||
Comment 7•25 years ago
|
||
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•25 years ago
|
||
Inheriting computed values is a good thing. Try "div.legal { font-size: 80%; }" and throw some markup inside it...
Reporter | ||
Comment 9•25 years ago
|
||
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•25 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
Reporter | ||
Comment 11•25 years ago
|
||
I hope buster doesn't mind if I steal this.
Status: NEW → ASSIGNED
Reporter | ||
Comment 13•25 years ago
|
||
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
Reporter | ||
Comment 14•25 years ago
|
||
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•24 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.
Comment 16•24 years ago
|
||
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])
Reporter | ||
Comment 17•24 years ago
|
||
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•24 years ago
|
||
The computed value of 'width' is based on layout. ('inherit' can be used to force inheritance of that value.)
Reporter | ||
Comment 19•24 years ago
|
||
Ah, you're right. I forgot about the properties that can be explicitly inherited. Alright then, we're in for a world of pain.
Reporter | ||
Comment 20•24 years ago
|
||
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.
Updated•24 years ago
|
Summary: text-indent: n% doesn't inherit computed value → text-indent: n% doesn't inherit computed value [INLINE]
Reporter | ||
Updated•24 years ago
|
Priority: P3 → P4
Reporter | ||
Updated•24 years ago
|
Target Milestone: --- → mozilla1.0
Comment 22•23 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
Updated•23 years ago
|
Attachment #11469 -
Attachment is obsolete: true
Updated•23 years ago
|
Attachment #11471 -
Attachment is obsolete: true
Updated•23 years ago
|
Whiteboard: (py8ieh: check with CSS working group [wg]) → [CSS1-5.4.7](py8ieh: check with CSS working group [wg])
Reporter | ||
Updated•23 years ago
|
OS: Windows NT → All
Priority: P4 → P3
Hardware: PC → All
Target Milestone: mozilla1.0.1 → mozilla1.1
Reporter | ||
Updated•23 years ago
|
Target Milestone: mozilla1.1alpha → Future
Comment 23•22 years ago
|
||
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•22 years ago
|
||
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 26•22 years ago
|
||
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•22 years ago
|
Attachment #112924 -
Attachment is obsolete: true
Comment 27•22 years ago
|
||
Okay, that testcase looks better, and it seems to show the value 'inherit' is being evaluated as '0' for 'text-indent'.
Updated•22 years ago
|
QA Contact: petersen → ian
Comment 28•21 years ago
|
||
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)
![]() |
||
Comment 29•21 years ago
|
||
This testcase (and code inspection of nsRuleNode.cpp) show that we do inheritance of text-indent correctly per CSS2.1.
![]() |
||
Comment 30•21 years ago
|
||
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]
Updated•21 years ago
|
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
Comment 31•18 years ago
|
||
All testcases works for me, both with SeaMonkey 2006120701 (pre reflow branch) and SeaMonkey 2006120801 (post) on Linux.
Comment 32•18 years ago
|
||
I was mistaken, the incremental reflow bug is still present in attachment 11470 [details].
Assignee | ||
Comment 33•18 years ago
|
||
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)
Reporter | ||
Updated•18 years ago
|
Attachment #265484 -
Flags: superreview+
Attachment #265484 -
Flags: review?(roc)
Attachment #265484 -
Flags: review+
Assignee | ||
Comment 34•18 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•