Closed Bug 540574 Opened 10 years ago Closed 10 years ago

[HTML5][Patch] Trailing WS: docshell/test/navigation/test_bug386782.html fails

Categories

(Core :: HTML: Parser, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file, 1 obsolete file)

docshell/test/navigation/test_bug386782.html fails due to trailing whitespace in HTML5 parsing getting hoisted into body.
Summary: [HTML5] docshell/test/navigation/test_bug386782.html fails → [HTML5] Trailing WS: docshell/test/navigation/test_bug386782.html fails
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
The attached patch removes the whitespace issue mentioned in the bug description.

However, the second test set expects a <br> element as a child of <body>. The HTML5 parser doesn't generate such an element. What generates it when the old parser is enabled? Is it important that it is generated?
(In reply to comment #2)
> However, the second test set expects a <br> element as a child of <body>. The
> HTML5 parser doesn't generate such an element. What generates it when the old
> parser is enabled? Is it important that it is generated?

This is probably done by the editor component.  We will remove the use of br's inside the editor at some point, but for now, yes, it is important, and the editor assumes that the br element exists.
Something that you can check is to see whether the br element has an attribute named _moz_editor_bogus_node set on it (the value will be true if it's set.)
Note to self: The likely cause here is that the HTML5 parser doesn't break out of a doc update immediately after adding the body element to the document.
Notes:
 * The data: URLs in the test had extra whitespace, which I removed.
 * The testInterval value needs to be adjusted. I don't know why exactly, but it seems the interval chosen to be long enough in the hope that stuff happen before the interval ends.
 * I made the StartLayout stuff copy the old parser more closely for the non-frameset case.
 * So far, I don't see a reason to copy the old parser exactly in the frameset case, so I simplified that case a bit instead of copying it exactly.
 * The editor doesn't create the bogus br unless the tree op executor breaks out of the doc update batch after the body element has been appended to the tree.
Attachment #424988 - Attachment is obsolete: true
Attachment #427314 - Flags: review?(bnewman)
Summary: [HTML5] Trailing WS: docshell/test/navigation/test_bug386782.html fails → [HTML5][Patch] Trailing WS: docshell/test/navigation/test_bug386782.html fails
Duplicate of this bug: 510802
Comment on attachment 427314 [details] [diff] [review]
Remove extra WS from test; copy the startlayout behavior of the old parser better

>diff --git a/docshell/test/navigation/test_bug386782.html b/docshell/test/navigation/test_bug386782.html
>--- a/docshell/test/navigation/test_bug386782.html
>+++ b/docshell/test/navigation/test_bug386782.html
>@@ -20,12 +20,12 @@
> 
>   
>     //var test.window = null;
>-    var testInterval = 2000;
>+    var testInterval = 3000;

The changes you've made to the parser to make this test pass are certainly worthwhile, but in all honesty the test itself is terribly written (obviously not your fault).  Waiting whole seconds already seems excessive, and even 3000ms wasn't enough when I ran the test over X11.

I played around with setting gTest.window.onload instead of using setTimeout, but the editor seems to take even longer to initialize.  Unless this test can be rewritten in a sound way, it just seems to be a big tinderbox-orange risk, so I'd really like to disable it.

>+void
>+nsHtml5TreeOpExecutor::StartLayout() {
>+  if (mLayoutStarted || !mDocument) {
>+   return;
>+  }

Indent by another space here?

>+  EndDocUpdate();
>+
>+  if(NS_UNLIKELY(!mParser)) {
>+    // got terminate
>+    return;
>+  }
>+
>+  nsContentSink::StartLayout(PR_FALSE);

What happened to FlushPendingAppendNotifications() before the nsContentSink::StartLayout call?

>+  BeginDocUpdate();
>+}

That is to say, did you intend a functional change between the version of nsHtml5TreeOpExecutor::StartLayout below and the version above?  If so, I didn't catch that change in your list in comment #6.

>-    void StartLayout() {
>-      nsIDocument* doc = GetDocument();
>-      if (doc) {
>-        FlushPendingAppendNotifications();
>-        nsContentSink::StartLayout(PR_FALSE);
>-      }
>-    }

r=me with those issues addressed.
Attachment #427314 - Flags: review?(bnewman) → review+
http://hg.mozilla.org/mozilla-central/rev/7fa9133d1fc6

(In reply to comment #8)
> (From update of attachment 427314 [details] [diff] [review])
> >diff --git a/docshell/test/navigation/test_bug386782.html b/docshell/test/navigation/test_bug386782.html
> >--- a/docshell/test/navigation/test_bug386782.html
> >+++ b/docshell/test/navigation/test_bug386782.html
> >@@ -20,12 +20,12 @@
> > 
> >   
> >     //var test.window = null;
> >-    var testInterval = 2000;
> >+    var testInterval = 3000;
> 
> The changes you've made to the parser to make this test pass are certainly
> worthwhile, but in all honesty the test itself is terribly written (obviously
> not your fault).  Waiting whole seconds already seems excessive, and even
> 3000ms wasn't enough when I ran the test over X11.
> 
> I played around with setting gTest.window.onload instead of using setTimeout,
> but the editor seems to take even longer to initialize.  Unless this test can
> be rewritten in a sound way, it just seems to be a big tinderbox-orange risk,
> so I'd really like to disable it.

My recollection is that waiting for a magic amount of time is a more general pattern in editor tests. :-(

> >+void
> >+nsHtml5TreeOpExecutor::StartLayout() {
> >+  if (mLayoutStarted || !mDocument) {
> >+   return;
> >+  }
> 
> Indent by another space here?
> 
> >+  EndDocUpdate();
> >+
> >+  if(NS_UNLIKELY(!mParser)) {
> >+    // got terminate
> >+    return;
> >+  }
> >+
> >+  nsContentSink::StartLayout(PR_FALSE);
> 
> What happened to FlushPendingAppendNotifications() before the
> nsContentSink::StartLayout call?

EndDocUpdate() above calls FlushPendingAppendNotifications().

> r=me with those issues addressed.

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.