Bug 106482

Summary: fast/forms/min-content-form-controls.html fails on some platforms
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, rniwa, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch eric: review+, rniwa: commit-queue-

Description Ojan Vafai 2013-01-09 12:14:13 PST
fast/forms/min-content-form-controls.html fails on some platforms
Comment 1 Ojan Vafai 2013-01-09 12:15:43 PST
Created attachment 181965 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-09 12:19:27 PST
Comment on attachment 181965 [details]
Patch

rs=me.
Comment 3 Tony Chang 2013-01-09 12:19:41 PST
Comment on attachment 181965 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        The text dump for some form controls (menulists?) is different
> +        on different platforms. Remove the form controls to avoid adding
> +        platform-specific expectations for this test.

How are they different?

> LayoutTests/fast/forms/min-content-form-controls.html:42
> +    // Remove these elements since some of the text dumps are different platforms.

What does "some of the text dumps are different platforms" mean?
Comment 4 Ryosuke Niwa 2013-01-09 12:20:16 PST
Comment on attachment 181965 [details]
Patch

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

> LayoutTests/fast/forms/min-content-form-controls.html:47
> +function remove(id) {
> +    var node = document.getElementById(id);
> +    node.parentNode.removeChild(node);
> +}
> +
> +if (window.testRunner) {
> +    // Remove these elements since some of the text dumps are different platforms.
> +    ids.forEach(function(id) {
> +        remove(id + '-a');
> +        remove(id + '-b');
> +    });
> +}

Why don’t we just delete all input and select elements or simply wrap them in div and set innerHTML on that?
Comment 5 Ryosuke Niwa 2013-01-09 12:20:52 PST
Comment on attachment 181965 [details]
Patch

Oops, I apparently overrode tony’s cq-.
Comment 6 Ojan Vafai 2013-01-09 12:25:23 PST
Committed r139221: <http://trac.webkit.org/changeset/139221>
Comment 7 Darin Adler 2013-01-09 12:51:51 PST
Comment on attachment 181965 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        The text dump for some form controls (menulists?) is different
>> +        on different platforms. Remove the form controls to avoid adding
>> +        platform-specific expectations for this test.
> 
> How are they different?

Why is the text dump for these form controls different on different platforms? This seems a fine workaround for that problem, but is it something we could easily fix in the form control code?

>> LayoutTests/fast/forms/min-content-form-controls.html:42
>> +    // Remove these elements since some of the text dumps are different platforms.
> 
> What does "some of the text dumps are different platforms" mean?

Missing words here. I think you mean "are different on different platforms".
Comment 8 Ojan Vafai 2013-01-09 14:46:25 PST
(In reply to comment #7)
> (From update of attachment 181965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181965&action=review
> 
> >> LayoutTests/ChangeLog:10
> >> +        The text dump for some form controls (menulists?) is different
> >> +        on different platforms. Remove the form controls to avoid adding
> >> +        platform-specific expectations for this test.
> > 
> > How are they different?
> 
> Why is the text dump for these form controls different on different platforms? This seems a fine workaround for that problem, but is it something we could easily fix in the form control code?

Yeah, sorry, I was being overly lazy. Looking closer, I was confused about what was going on. The issue is that there was line wrapping on some platforms due to differently sized form controls, which lead to a different number of spaces in the innerText output.

An alternate workaround for this test would have been to put a <br> after each form control.
Comment 9 Ojan Vafai 2013-01-09 14:47:32 PST
In fact, I like the explicit break solution better since we don't then have to do something different for being in the test runner. I'll commit a followup patch with that change. Thanks for calling me out on this.
Comment 10 Ojan Vafai 2013-01-09 15:02:18 PST
http://trac.webkit.org/changeset/139245