Bug 76632 – [GTK] fast/forms/implicit-submission.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/forms/implicit-submission-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/forms/implicit-submission-actual.txt 
@@ -19,7 +19,7 @@
 Single checkbox with a submit should submit: PASS
 Single checkbox with a submit disabled should not submit: PASS
 Single select should not submit: PASS
-Select with a submit should submit: PASS
+Select with a submit should submit: FAIL
 Select with a disabled submit should not submit: PASS
 Multi-line select with a submit should submit: PASS
 Multi-line select with a disabled submit should not submit: PASS

[Kaustubh Atrawalkar]

In the changes - 105253 (http://trac.webkit.org/changeset/105253) there were changes made to switch from flag to RenderTheme for getting mode of operation in HTMLSelect element. There was change to submit the form on enter key press for Gtk. But later in change set 105286 (http://trac.webkit.org/changeset/105286) changes made to open popup menu in HTMLSelect element on enter key press instead of submitting the form. Need to fix the test case accordingly.

[Kaustubh Atrawalkar]

Created attachment 123710 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123710 [details]
Patch

Attachment 123710 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11336302

New failing tests:
fast/forms/implicit-submission.html

[Kaustubh Atrawalkar]

Created attachment 123715 [details]
Patch

Update chromium expected results as well

[WebKit Review Bot]

Comment on attachment 123715 [details]
Patch

Attachment 123715 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11337004

New failing tests:
media/audio-garbage-collect.html

[Kaustubh Atrawalkar]

(In reply to comment #5)
> (From update of attachment 123715 [details])
> Attachment 123715 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11337004
> 
> New failing tests:
> media/audio-garbage-collect.html

Seems to be unrelated. This is change in other layout test cases. Philippe can u check once? Thanks.

[Dimitri Glazkov (Google)]

You guys probably shouldn't change the test to fix it for one platform :)

[Kaustubh Atrawalkar]

(In reply to comment #7)
> You guys probably shouldn't change the test to fix it for one platform :)

I agree but in this case, there is change in the functionality. So need to fix the test accordingly. Earlier on HTMLSelectElement menu list element, pressing enter was causing submit form. Now it opens the pop up of list box.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (In reply to comment #7)
> > You guys probably shouldn't change the test to fix it for one platform :)
> 
> I agree but in this case, there is change in the functionality. So need to fix the test accordingly. Earlier on HTMLSelectElement menu list element, pressing enter was causing submit form. Now it opens the pop up of list box.

This change is Qt-only, right? Hitting Enter on a select will (and is expected to) submit form on Mac and Windows ports.

[Martin Robinson]

CCing our accessibility people here. There's likely a correct behavior with respect to how keyboards interact with select elements and we need to preserve or fix that.

[Kaustubh Atrawalkar]

Created attachment 126499 [details]
Updated Patch

Current behavior on Mac Safari is to submit form on enter key press. Gtk & Chromium port does not match with it. Fixed in the renderTheme port specific files. Please review. Thanks.

[WebKit Review Bot]

Comment on attachment 126499 [details]
Updated Patch

Attachment 126499 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11484662

New failing tests:
fast/forms/select/menulist-onchange-fired-with-key-up-down.html
fast/forms/select-popup-pagekeys.html

[Kaustubh Atrawalkar]

(In reply to comment #12)
> (From update of attachment 126499 [details])
> Attachment 126499 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11484662
> 
> New failing tests:
> fast/forms/select/menulist-onchange-fired-with-key-up-down.html
> fast/forms/select-popup-pagekeys.html

Seems like need to fix these as well. will work on it.

[Martin Robinson]

(In reply to comment #11)

> Current behavior on Mac Safari is to submit form on enter key press. Gtk & Chromium port does not match with it. Fixed in the renderTheme port specific files. Please review. Thanks.

It's important that these behaviors match the system default, rather than each other. That's why this is an option in RenderTheme. I also don't think it's right to change the way Chromium works here. If the behavior is correct then perhaps we just need to rebaseline the test on GTK+.

[Martin Robinson]

Comment on attachment 126499 [details]
Updated Patch

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

> LayoutTests/platform/chromium-win/fast/forms/implicit-submission-expected.txt:22
> -Select with a submit should submit: FAIL
> +Select with a submit should submit: PASS

Does the test pass if you keep this baseline intact? Perhaps we just need to unskip the test...

> LayoutTests/platform/chromium/test_expectations.txt:-2475
> -BUGCR43890 SLOW DEBUG : fast/forms/implicit-submission.html = PASS TEXT

This line doesn't actually mean the test is failing for Chromium. It just means that it can be slow sometimes. This is a message to the test harness not to time out.

[Kaustubh Atrawalkar]

(In reply to comment #15)
> (From update of attachment 126499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126499&action=review
> 
> > LayoutTests/platform/chromium-win/fast/forms/implicit-submission-expected.txt:22
> > -Select with a submit should submit: FAIL
> > +Select with a submit should submit: PASS
> 
> Does the test pass if you keep this baseline intact? Perhaps we just need to unskip the test...
>
No, this test does not pass. We need to change RenderThemeChromiumLinux.h as suggested.
> > LayoutTests/platform/chromium/test_expectations.txt:-2475
> > -BUGCR43890 SLOW DEBUG : fast/forms/implicit-submission.html = PASS TEXT
> 
> This line doesn't actually mean the test is failing for Chromium. It just means that it can be slow sometimes. This is a message to the test harness not to time out.

Yes, that's right. Just tried now and did not happened to see timeout. So removed the line.

[Martin Robinson]

(In reply to comment #16)

> > Does the test pass if you keep this baseline intact? Perhaps we just need to unskip the test...
> >
> No, this test does not pass. We need to change RenderThemeChromiumLinux.h as suggested.

If the test doesn't pass, perhaps we just need to rebaseline it. I don't think we want to modify the behavior be different than the GTK+ behavior.

[Kaustubh Atrawalkar]

Created attachment 126961 [details]
Rebaseline Patch

Chromium test expectation are already rebaselined. As per system expectation Select Submit button should not submit on enter. Fixing the expectation and rebaselined as Martin suggested.

[WebKit Review Bot]

Attachment 126961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kaustubh Atrawalkar]

Created attachment 126962 [details]
Rebaseline Patch

[WebKit Review Bot]

Attachment 126962 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 126962 [details]
Rebaseline Patch

Clearing flags on attachment: 126962

Committed r107713: <http://trac.webkit.org/changeset/107713>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.