839171 - Update formMethod reflection to have the empty string as default value (and 'get' as invalid value)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

+++ This bug was initially created as a clone of Bug #787095 +++

See $URL for the HTML spec change. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=17185 for the reasons. I think it makes sense.

Bug 787095 changed the implementation of input.form{Enctype,Method}, but forgot to make the same changes to the button implementation.

Comment 1

[Julian Viereck]

Someone pinged me on IRC about how to fix this. I guess it's about chaning 

  https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLButtonElement.cpp#122

in the same way the marco was changed in

  https://bug787095.bugzilla.mozilla.org/attachment.cgi?id=705182

and update the tests.

Comment 2

[Sunny [:darkowlzz]]

Attachment: Applied the missed changes to button from Bug 787095

Hi,
I would like to work on this bug. 

Here is my first patch.

Comment 3

[Julian Viereck]

(In reply to darkowlzz from comment #2)
> Created attachment 712255 [details] [diff] [review]
> Applied the missed changes to button from Bug 787095

Have you run the test suite locally? There were some changes to the tests going with the patch in bug 787095 - that's why I assume some tests need to be adjusted for this patch as well.

Comment 4

[Benedict Singer]

Yes, you'll want to add tests to content/html/content/test/forms/test_button_attributes_reflection.html Also make sure to run the test suite, as this change may impact other tests.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 712255 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Review of attachment 712255 [details] [diff] [review]:
-----------------------------------------------------------------

Your changes you look but as Benedict pointed out, you need to update the tests. Could you attach a new patch with those tests updated. Also, run the tests in content/html/content/test/ (it should take a few minutes) to make sure everything is green.
The comment to run the test is, when you are inside the objdir:
TEST_PATH=content/html/content/test/ make mochitest-plain

Comment 6

[Sunny [:darkowlzz]]

Attachment: Applied the missed changes to button from Bug 787095

Updated tests.
Ran mochitest and got:
INFO Passed: 86100
INFO Failed: 0
INFO Todo: 247
everything was green.

Comment 7

[Sunny [:darkowlzz]]

Whoops!! sorry for the comment #6 where I ran mochitest without applying the patch.
After applying the patch results were like this:

INFO Passed: 86098
INFO Failed: 2
INFO Todo:   244

mochitest-plain failed:
266 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When no attribute is set, the value should be the default value. - got application/x-www-form-urlencoded, expected
305 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/forms/test_button_attributes_reflection.html | When no attribute is set, the value should be the default value. - got get, expected

What went wrong and how do I fix it?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Are you sure you tested on the right build? The patch looks correct on first sight.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Review of attachment 712674 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the tests cleaned-up.

::: content/html/content/test/forms/test_button_attributes_reflection.html
@@ +44,5 @@
>  //  defaultValue: {
>  //    invalid: "application/x-www-form-urlencoded",
>  //    missing: "",
>  //  }
> +  defaultValue: { invalid: "application/x-www-form-urlencoded", missing: "" }

Please, uncomment the lines above and remove this one.

@@ +59,5 @@
>  //  defaultValue: {
>  //    invalid: "get",
>  //    missing: "",
>  //  }
> +  defaultValue: { invalid: "get", missing: "" }

ditto

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

As Ms2ger pointed out, your patch looks good.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=692073e531e0

Comment 12

[Sunny [:darkowlzz]]

Attachment: Applied the missed changes to button from Bug 787095

Uncommented and removed the lines as suggested in Comment #9

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 712920 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Review of attachment 712920 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

Note that generally speaking, when we say "r=me with something changes", you don't have to ask for another review if you change what has been asked ;)

So, could you `hg rm content/html/content/test/test_bug585508.html` and modify content/html/content/test/Makefile.in appropriately? test_bug585508.html duplicates tests already in test_button_attributes_reflection.html and is buggy now ;)

Comment 14

[Sunny [:darkowlzz]]

Attachment: Applied the missed changes to button from Bug 787095

Deleted the test file and modified Makefile.in accordingly.

Comment 15

[Sunny [:darkowlzz]]

The bug has not been assigned to me yet. I can't add new keywords for check-in.

Comment 16

[Mounir Lamouri (:mounir)]

We have some people who regularly push patches with 'checkin-needed'. If your patch isn't push in the next 48 hours, comment here, I will push it myself. (It's better to have patches pushed in batches because it reduces the number of cycles we use to test them and I don't have any patch to push right now.)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8479bdaf0c8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-browser-chrome failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87f9a587e86

https://tbpl.mozilla.org/php/getParsedLog.php?id=19703949&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-11.js | 'formMethod' in buttonProtoNode should have the right value. - Got "", expected "get"
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-11.js | 'formMethod' in buttonAsProtoProtoProtoNode should have the right value. - Got "", expected "get"

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Backed out for mochitest-browser-chrome failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b87f9a587e86
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=19703949&tree=Mozilla-
> Inbound
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_propertyview-11.js | 'formMethod' in buttonProtoNode should have
> the right value. - Got "", expected "get"
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/debugger/test/
> browser_dbg_propertyview-11.js | 'formMethod' in buttonAsProtoProtoProtoNode
> should have the right value. - Got "", expected "get"

Gasp... this is not tested on linux. This is what happens when we want to reduce charge on our infrastructure and only run try on linux64...

Comment 20

[Sunny [:darkowlzz]]

So what do we do now?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er... what's not tested on linux?

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to darkowlzz from comment #20)
> So what do we do now?

I sent to try your patch with a fix for the failing test:
https://tbpl.mozilla.org/?tree=Try&rev=fa6468b19010

If it succeed, I will land those patches.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Er... what's not tested on linux?

devtools

Comment 24

[Boris Zbarsky [:bzbarsky]]

Any idea why the heck not?  Can we get a bug filed on that?

Comment 25

[Mounir Lamouri (:mounir)]

I pushed that patch again with the devtools tests fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d999c65753ea

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #24)
> Any idea why the heck not?  Can we get a bug filed on that?

I've opened bug 841731.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d999c65753ea

Comment 28

[Kohei Yoshino]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21