Closed
Bug 540574
Opened 15 years ago
Closed 15 years ago
[HTML5][Patch] Trailing WS: docshell/test/navigation/test_bug386782.html fails
Categories
(Core :: DOM: HTML Parser, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: hsivonen, Assigned: hsivonen)
References
Details
Attachments
(1 file, 1 obsolete file)
|
5.76 KB,
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
docshell/test/navigation/test_bug386782.html fails due to trailing whitespace in HTML5 parsing getting hoisted into body.
|
Assignee | |
Updated•15 years ago
|
Summary: [HTML5] docshell/test/navigation/test_bug386782.html fails → [HTML5] Trailing WS: docshell/test/navigation/test_bug386782.html fails
|
|
Assignee | |
Comment 1•15 years ago
|
||
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 2•15 years ago
|
||
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?
|
||
Comment 3•15 years ago
|
||
(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.
|
|
||
Comment 4•15 years ago
|
||
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.)
|
|
Assignee | |
Comment 5•15 years ago
|
||
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.
|
|
Assignee | |
Comment 6•15 years ago
|
||
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)
|
|
Assignee | |
Updated•15 years ago
|
Summary: [HTML5] Trailing WS: docshell/test/navigation/test_bug386782.html fails → [HTML5][Patch] Trailing WS: docshell/test/navigation/test_bug386782.html fails
|
||
Comment 8•15 years ago
|
||
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+
|
|
Assignee | |
Comment 9•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•