Bug 44781 - HTMLTreeBuilder needs to call HTMLFormElement::setDemoted
Summary: HTMLTreeBuilder needs to call HTMLFormElement::setDemoted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Adam Barth
URL: http://www.verkkokauppa.com
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2010-08-27 11:31 PDT by Anssi Kolehmainen
Modified: 2010-08-28 03:34 PDT (History)
5 users (show)

See Also:


Attachments
Incorrect rendering with chrome (110.98 KB, image/png)
2010-08-27 11:31 PDT, Anssi Kolehmainen
no flags Details
Correct rendering with firefox (122.52 KB, image/png)
2010-08-27 11:31 PDT, Anssi Kolehmainen
no flags Details
Patch (68.91 KB, patch)
2010-08-28 02:32 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (68.90 KB, patch)
2010-08-28 02:38 PDT, Adam Barth
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anssi Kolehmainen 2010-08-27 11:31:18 PDT
Created attachment 65741 [details]
Incorrect rendering with chrome

Verkkokauppa.com website is rendered incorrectly in latest builds of chrome and Webkit. The search bar ("pikahaku") is moved to right outside the page area. Attached screenshots should show the issue clearly.

Failing browsers: 
Linux, Google Chrome 7.0.503.1 (Official Build 57041) dev, WebKit 534.6
OS X, Webkit nightly, 5.0.1 (6533.17.8, r65398)

Working browsers:
Linux, Firefox 3.0.9
Linux, Konqueror 4.4.4
(Webkit and Chrome probably a month or two ago)
Comment 1 Anssi Kolehmainen 2010-08-27 11:31:41 PDT
Created attachment 65742 [details]
Correct rendering with firefox
Comment 2 Anssi Kolehmainen 2010-08-27 11:59:33 PDT
Did some tests with webkit nightly builds:

r66130	broken
r64816	broken
r64727	broken
r64636	works
r64583	works
r64451	works
Comment 3 Alexey Proskuryakov 2010-08-27 17:07:07 PDT
HTML5 tree builder (r64712) fits in this range, so it's a natural suspect.
Comment 4 Adam Barth 2010-08-27 17:11:11 PDT
Thanks for the report.  I can reproduce.  Will investigate.
Comment 5 Adam Barth 2010-08-27 17:28:28 PDT
Reduction:

<table>
  <tr><td>foobarbaz</td></tr>
  <tr><form><td>onetwothree</td></form></tr>
</table>
Comment 6 Adam Barth 2010-08-27 17:33:16 PDT
I need to verify, but it looks like we get the same DOM as Firefox4, we just render it differently.
Comment 7 Adam Barth 2010-08-28 01:11:56 PDT
Looks like the parsing of this case didn't change since Safari 5.  It might not be related to the parser or it might be some interaction between the parser and the rest of WebKit that we screwed up.
Comment 9 Adam Barth 2010-08-28 01:27:40 PDT
Apparently we're supposed to call HTMLFormElement::setDemoted.
Comment 10 Eric Seidel (no email) 2010-08-28 01:33:53 PDT
Since Hyatt created setDemoted/isDemoted, he should probably see this bug go by.
Comment 11 Adam Barth 2010-08-28 01:53:55 PDT
setDemoted has got be wrong.  If you build this same DOM with createElement/appendChild from script, I bet you get the wrong rendering.  We should probably use setDemoted in the first patch and then figure out the non-hacky solution in the second patch.
Comment 12 Adam Barth 2010-08-28 02:32:18 PDT
Created attachment 65816 [details]
Patch
Comment 13 Eric Seidel (no email) 2010-08-28 02:35:21 PDT
Comment on attachment 65816 [details]
Patch

WebCore/html/parser/HTMLConstructionSite.cpp:259
 +      ASSERT(element->tagQName() == formTag);
->hasName(formTag)

WebCore/html/parser/HTMLConstructionSite.cpp:256
 +  void HTMLConstructionSite::insertHTMLFormElement(AtomicHTMLToken& token, bool isDemoted)
isDemoted isn't very clear.  doNotAttach?  donno.

So wrong.  But better than what we had.
Comment 14 Adam Barth 2010-08-28 02:38:12 PDT
Created attachment 65817 [details]
Patch for landing
Comment 15 Adam Barth 2010-08-28 02:44:03 PDT
Comment on attachment 65817 [details]
Patch for landing

Clearing flags on attachment: 65817

Committed r66306: <http://trac.webkit.org/changeset/66306>
Comment 16 Adam Barth 2010-08-28 02:44:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Commit Bot 2010-08-28 03:34:55 PDT
Comment on attachment 65817 [details]
Patch for landing

Rejecting patch 65817 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
/parser/HTMLConstructionSite.cpp
Hunk #1 FAILED at 253.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/parser/HTMLConstructionSite.cpp.rej
patching file WebCore/html/parser/HTMLConstructionSite.h
Hunk #1 FAILED at 57.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/parser/HTMLConstructionSite.h.rej
patching file WebCore/html/parser/HTMLTreeBuilder.cpp
Hunk #1 FAILED at 1154.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/parser/HTMLTreeBuilder.cpp.rej

Full output: http://queues.webkit.org/results/3824056