Bug 89539 - Fix a form restore test broken by HTML5 parser
Summary: Fix a form restore test broken by HTML5 parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 88685 89409
  Show dependency treegraph
 
Reported: 2012-06-19 18:37 PDT by Kent Tamura
Modified: 2012-06-19 21:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2012-06-19 18:48 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (4.62 KB, patch)
2012-06-19 19:24 PDT, Kent Tamura
haraken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-06-19 18:37:32 PDT
http://trac.webkit.org/changeset/66482 made fast/forms/restore-to-non-edited-controls.html nonsense. We fix the test in this bug.
Comment 1 Kent Tamura 2012-06-19 18:48:34 PDT
Created attachment 148481 [details]
Patch
Comment 2 Kentaro Hara 2012-06-19 19:22:02 PDT
Comment on attachment 148481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148481&action=review

> LayoutTests/ChangeLog:13
> +        After HTML5 parser, we don't attach renderes during innerHTML

Nit: renderes => renderers

> LayoutTests/ChangeLog:15
> +        built a form and the test haven't work since introducing HTML5 parser.

Just a confirmation: What happens if innerHTML is used and <!DOCTYPE html> is omitted?

I am asking this because I am not sure whether we should hide the difference by using document.write() (just like your patch) or we should test innerHTML for both <!DOCTYPE html> case and no-<!DOCTYPE html> case.
Comment 3 Kent Tamura 2012-06-19 19:23:57 PDT
Comment on attachment 148481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148481&action=review

>> LayoutTests/ChangeLog:15
>> +        built a form and the test haven't work since introducing HTML5 parser.
> 
> Just a confirmation: What happens if innerHTML is used and <!DOCTYPE html> is omitted?
> 
> I am asking this because I am not sure whether we should hide the difference by using document.write() (just like your patch) or we should test innerHTML for both <!DOCTYPE html> case and no-<!DOCTYPE html> case.

DOCTYPE doesn't matter. We always use HTML5 parser.
Comment 4 Kent Tamura 2012-06-19 19:24:57 PDT
Created attachment 148487 [details]
Patch 2

typo
Comment 5 Kentaro Hara 2012-06-19 19:28:27 PDT
Comment on attachment 148487 [details]
Patch 2

Thanks for the clarification.
Comment 6 Eric Seidel (no email) 2012-06-19 19:57:26 PDT
I'm confused.  If we changed innerHTML behavior, was this a regression?  Does our new innerHTML/form behavior match other browsers?

It's good that we fixed the test, but there may be an underlying bug here that we're pasting over?
Comment 7 Kent Tamura 2012-06-19 20:23:56 PDT
(In reply to comment #6)
> I'm confused.  If we changed innerHTML behavior, was this a regression?  Does our new innerHTML/form behavior match other browsers?
> 
> It's good that we fixed the test, but there may be an underlying bug here that we're pasting over?

Yeah, HTML5 parser made a regression.  We were able to restore form values for dynamically generated form with innerHTML, but now we can't.  However I haven't seen bug reports for this regression.  I guess this is not so important to users.

I think there are no ways to observe whether a node is attached in Node::finishParsingChildren() in innerHTML case on other browsers, and this is not a compatibility issue.

I have a plan to change the form restore timing.  The regression will be resolved then.
Comment 8 Eric Seidel (no email) 2012-06-19 20:31:46 PDT
(In reply to comment #7)
> (In reply to comment #6)
> I have a plan to change the form restore timing.  The regression will be resolved then.

Do we have a bug to track that change.  Seems useful to relate here.  Thanks!
Comment 9 Kent Tamura 2012-06-19 20:35:25 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > I have a plan to change the form restore timing.  The regression will be resolved then.
> 
> Do we have a bug to track that change.  Seems useful to relate here.  Thanks!

I explained the plan in https://bugs.webkit.org/show_bug.cgi?id=23346#c45 .  I'll add a new blocker bug for the timing change to Bug 23346.
Comment 10 Kent Tamura 2012-06-19 21:26:28 PDT
Committed r120796: <http://trac.webkit.org/changeset/120796>