Last Comment Bug 484448 - [FIX]Rework whitespace handling in table pseudo frame construction
: [FIX]Rework whitespace handling in table pseudo frame construction
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
: 509477 519403 (view as bug list)
Depends on: 148810 487449
Blocks: 372641
  Show dependency treegraph
 
Reported: 2009-03-20 12:12 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2009-10-12 03:54 PDT (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (62.17 KB, patch)
2009-03-20 12:12 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Lots more tests, plus fixes for a few of the failures that resulted (76.21 KB, patch)
2009-03-25 20:39 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
With roc's issue fixed (77.53 KB, patch)
2009-03-27 19:05 PDT, Boris Zbarsky [:bz] (still a bit busy)
bernd_mozilla: review+
roc: superreview+
Details | Diff | Splinter Review
Previous patch merged to apply on top of the patch for bug 487449 (78.21 KB, patch)
2009-04-13 18:01 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Interdiff from the previous attachment (merged patch) to the patch I'd like to check in (13.22 KB, patch)
2009-04-13 18:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
What I'd like to land (75.55 KB, patch)
2009-04-13 18:05 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-20 12:12:07 PDT
Created attachment 368570 [details] [diff] [review]
Fix

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.
Comment 1 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-21 01:22:32 PDT
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.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-22 12:57:40 PDT
> 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.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-22 12:58:11 PDT
Oh, and I'll still be writing more tests here, for what it's worth.
Comment 4 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-22 14:41:08 PDT
(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.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-22 15:40:32 PDT
> 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.
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-22 16:53:58 PDT
> > 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.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-25 20:39:13 PDT
Created attachment 369436 [details] [diff] [review]
Lots more tests, plus fixes for a few of the failures that resulted
Comment 8 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-25 22:30:43 PDT
+  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.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-25 23:05:33 PDT
Hmm.  Good point.  I'll write tests for those two cases.
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-25 23:26:56 PDT
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.
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-27 18:43:37 PDT
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>.
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-27 19:05:47 PDT
Created attachment 369793 [details] [diff] [review]
With roc's issue fixed

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.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-03-29 14:26:12 PDT
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.
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-29 16:23:43 PDT
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.
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2009-03-29 16:24:33 PDT
Oh, and of course this does mean that dynamic changes of textnodes from whitespace to not in XUL and MathML are still buggy.
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-08 09:47:55 PDT
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.
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-08 09:57:25 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/0ea22856b5d9
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-08 10:06:40 PDT
Filed bug 487449 on finding the right prevSibling in ContentAppended.
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-08 12:58:56 PDT
Backed out; this caused a talos regression.

I guess I really need to fix bug 487449 first.
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-13 18:01:59 PDT
Created attachment 372512 [details] [diff] [review]
Previous patch merged to apply on top of the patch for bug 487449
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-13 18:03:36 PDT
Created attachment 372513 [details] [diff] [review]
Interdiff from the previous attachment (merged patch) to the patch I'd like to check in
Comment 22 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-13 18:05:06 PDT
Created attachment 372515 [details] [diff] [review]
What I'd like to land

You might want to review with an eye on the attached interdiff.
Comment 23 User image Boris Zbarsky [:bz] (still a bit busy) 2009-04-14 10:14:04 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/96e707a8f72a and this time Talos looks happy.
Comment 24 User image Boris Zbarsky [:bz] (still a bit busy) 2009-08-10 18:54:48 PDT
*** Bug 509477 has been marked as a duplicate of this bug. ***
Comment 25 User image Mardeg 2009-10-12 03:54:37 PDT
*** Bug 519403 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.