Closed Bug 484448 Opened 16 years ago Closed 16 years ago

[FIX]Rework whitespace handling in table pseudo frame construction

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 3 obsolete files)

Previous patch merged to apply on top of the patch for bug 487449
(deleted), patch
Details | Diff | Splinter Review
Interdiff from the previous attachment (merged patch) to the patch I'd like to check in
(deleted), patch
Details | Diff | Splinter Review
What I'd like to land
(deleted), patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
Attached patch Fix (obsolete) (deleted) — — Splinter Review
We have a number of dynamic whitespace handling bugs, and at least one behavior that's sort of purposeful but suboptimal: <span style="display: table-row"> <span>One</span> <span>Two</span> </span> ends up looking like a single cell containing the text "OneTwo" with no space. There should really be a space there. Attached patch addresses these issues, and reworks some of the code involved to avoid reframing too much on appends now that whitespace is not stripped from frame construction item lists as eagerly. This patch depends on the one for bug 148810, and I think with this we have no outstanding dynamic mutation bugs where table anonymous objects are concerned.
Attachment #368570 - Flags: superreview?(roc)
Attachment #368570 - Flags: review?(bernd_mozilla)
Is there a reason you've used reftest-wait in some tests and an offsetWidth hack in others? I'd kind of prefer the offsetWidth hack in all of them. Wouldn't it be simpler for DeleteItemsTo to just do nothing if the iterator is already at aEnd? Similarly for SkipWhitespace, it should be able to do nothing. Is this just a performance thing? Otherwise looks good.
Depends on: 148810
Blocks: 372641
> Is there a reason you've used reftest-wait in some tests and an offsetWidth > hack in others? Yes. In some I just want to run some script that modifies the DOM after the load is done and layout is complete, whereas in others I am explicitly testing our ContentAppended codepath with multiple nodes being appended. The only way to do the latter is with offsetWidth and assumptions about how the parser notifies, now that we don't use ContentAppended with multiple nodes for document fragment insertions. If I could avoid using the offsetWidth thing altogether, I would. Is there a reason you'd prefer it (presumably right at the end of the <body>) to assuming the frame model is up-to-date when onload fires? > Wouldn't it be simpler for DeleteItemsTo to just do nothing if the iterator is > already at aEnd? Sure, but it's only called in cases when we really need to do something at the moment. I could add a check for iterator equality, but it'd just be checking an extra time. Same for SkipWhitespace.
Oh, and I'll still be writing more tests here, for what it's worth.
(In reply to comment #2) > > Is there a reason you've used reftest-wait in some tests and an offsetWidth > > hack in others? > > Yes. In some I just want to run some script that modifies the DOM after the > load is done and layout is complete, whereas in others I am explicitly testing > our ContentAppended codepath with multiple nodes being appended. The only way > to do the latter is with offsetWidth and assumptions about how the parser > notifies, now that we don't use ContentAppended with multiple nodes for > document fragment insertions. If I could avoid using the offsetWidth thing > altogether, I would. Is there a reason you'd prefer it (presumably right at > the end of the <body>) to assuming the frame model is up-to-date when onload > fires? One reason I prefer it is that it's slightly faster (I presume) because we should only have to paint once instead of twice. Another reason is that if you want to test multiple changes it's neater with a chain of .offsetWidth calls in a single script than with a chain of setTimeouts. I've used this in crashtests a bit. I also like being able to break on the .offsetWidth call in a debugger when I want to inspect frame trees. It feels more deterministic, although maybe that's completely irrational. > > Wouldn't it be simpler for DeleteItemsTo to just do nothing if the iterator > > is already at aEnd? > > Sure, but it's only called in cases when we really need to do something at the > moment. I could add a check for iterator equality, but it'd just be checking > an extra time. Same for SkipWhitespace. OK, so it's a performance thing ... that's OK I guess.
> One reason I prefer it is that it's slightly faster (I presume) because we > should only have to paint once instead of twice. Hmm. I suppose that's true, yeah (though in that case it's testing our invalidation code too, right? ;)). I'm not using setTimeouts here, nor chaining multiple changes. Agreed on the debugger thing. I think the deterministic feeling is irrational, possibly even more so cross-browser. I'll look into changing this, though. > OK, so it's a performance thing Well, sorta. It's a cheap check to do; I could just do it and have safer code and I doubt performance would really suffer in any measurable way.
> > OK, so it's a performance thing > > Well, sorta. It's a cheap check to do; I could just do it and have safer code > and I doubt performance would really suffer in any measurable way. OK. I don't mind either way.
Attachment #368570 - Attachment is obsolete: true
Attachment #369436 - Flags: superreview?(roc)
Attachment #369436 - Flags: review?(bernd_mozilla)
Attachment #368570 - Flags: superreview?(roc)
Attachment #368570 - Flags: review?(bernd_mozilla)
+ do { + if (curParent->GetFirstChild(nsnull) != curAfterChild) { + isInsertAtStart = PR_FALSE; + break; + } + curAfterChild = curParent; + curParent = curParent->GetParent(); + } while (curAfterChild != origParentFrame); Should that be curParent->GetFirstContinuation()->GetFirstChild(nsnull)? As is, I think you might set isInsertAtStart to true when appending to an element whose :after frame is the only thing in its last-continuation. if (WipeContainingBlock(state, containingBlock, parentFrame, items, - isAppend && !appendAfterFrame, prevSibling)) + isAppend && !appendAfterFrame, + prevSibling == nsnull, + prevSibling)) You might need something more here too for a similar situation. Otherwise looks good.
Hmm. Good point. I'll write tests for those two cases.
The ContentInserted case doesn't need anything more, since prevSibling is always the right previous sibling frame if there's one to be had at all. That is, if it's null that really does mean that we're being inserted at the beginning of the frame list for the parent (possibly not in the parent's first continuation, but that doesn't matter). In the ContentAppended case, though, the GetFirstContinuation() call could get pretty expensive as we do a bunch of appends to an inline... I'll need to think about that a tad.
So I guess with white-space:pre-wrap I can in fact construct a testcase where the whitespace is filtered out incorrectly with this patch. <sigh>.
Attached patch With roc's issue fixed (obsolete) (deleted) — — Splinter Review
I decided that GetFirstContinuation() might be slow, but GetPrevContinuation() is fast. And if GetPrevContinuation(), we should just pass false for aIsInsertAtStart. Maybe there are super-edge-cases when the parent of the ::after has a prev continuation but none of its prev continuations have kids (e.g. they've all been removed without reflowing after that), but I'm not going to worry about those here. Passing false is safe; it just might make us reframe when we don't have to. The goal is to have the common case right. I still worry about possible reframes here when appending rows to a table-row-group from the parser if the first thing being appended is whitespace. I don't see a way to fix that without having a correct prevSibling in ContentAppended, though. :( I haven't been able to get a talos run on try server in days, so I don't actually have data on this patch with the leading whitespace thing performance-wise; the last version I tried would lose leading whitespace when it shouldn't have by assuming it was always OK to drop... So before checking it in I'll certainly run it on try server; if it causes problems there I'll need to think harder about changing ContentAppended around.
Attachment #369436 - Attachment is obsolete: true
Attachment #369793 - Flags: superreview?(roc)
Attachment #369793 - Flags: review?(bernd_mozilla)
Attachment #369436 - Flags: superreview?(roc)
Attachment #369436 - Flags: review?(bernd_mozilla)
Attachment #369793 - Flags: superreview?(roc) → superreview+
Comment on attachment 369793 [details] [diff] [review] With roc's issue fixed + // We could handle all this in CreateNeededTablePseudos, but that would + // involve creating frame construction items for whitespace kids of + // eExcludesIgnorableWhitespace frames, where we know we'll be dropping them + // all anyway. I think you should change this comment. IMHO the benefit of NeedFrameFor is not that we avoid creating frame construction items --- if that was the point, it would not be worth having, since whitespace inside tables is a lot less common than "everything else", so paying overhead for "everything else" is unlikely to be a win. I think the real benefit is that we avoid adding FRAMETREE_DEPENDS_ON_CHARS to every text node.
We'd only need to add this flag to textnodes that are kids of eExcludesIgnorableWhitespace frames (so boxes and MathML). But yes, I can mention that benefit in the comment too.
Oh, and of course this does mean that dynamic changes of textnodes from whitespace to not in XUL and MathML are still buggy.
Attachment #369793 - Flags: review?(bernd_mozilla) → review+
And upon reading it again, I think my comment is correct: the only benefits of this check up front are avoiding an extra pass down the frame construction item list (added this to the comment) and avoiding creating items for whitespace inside xul boxes and and MathML. There is no dynamic change issue, since I do flag textnodes inside these as depending on chars.
Pushed http://hg.mozilla.org/mozilla-central/rev/0ea22856b5d9
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Filed bug 487449 on finding the right prevSibling in ContentAppended.
Backed out; this caused a talos regression. I guess I really need to fix bug 487449 first.
Status: RESOLVED → REOPENED
Depends on: 487449
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
Attached patch What I'd like to land (deleted) — — Splinter Review
You might want to review with an eye on the attached interdiff.
Attachment #372515 - Flags: superreview?(roc)
Attachment #372515 - Flags: review?(roc)
Attachment #372515 - Flags: superreview?(roc)
Attachment #372515 - Flags: superreview+
Attachment #372515 - Flags: review?(roc)
Attachment #372515 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/96e707a8f72a and this time Talos looks happy.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: