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

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: darkowlzz)

Tracking

Trunk
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mounir][lang=c++], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
+++ 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

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 712255 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Hi,
I would like to work on this bug. 

Here is my first patch.
Attachment #712255 - Flags: review?(mounir)

Comment 3

5 years ago
(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

5 years ago
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 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
Attachment #712255 - Flags: review?(mounir) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 712674 [details] [diff] [review]
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.
Attachment #712255 - Attachment is obsolete: true
Attachment #712674 - Flags: review?(mounir)
(Assignee)

Comment 7

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #712674 - Attachment is obsolete: true
Attachment #712674 - Flags: review?(mounir)
(Reporter)

Comment 8

5 years ago
Are you sure you tested on the right build? The patch looks correct on first sight.
Flags: in-testsuite+
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
Attachment #712674 - Flags: review+
Comment on attachment 712674 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

As Ms2ger pointed out, your patch looks good.
Attachment #712674 - Attachment is obsolete: false
(Reporter)

Comment 11

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=692073e531e0
(Assignee)

Comment 12

5 years ago
Created attachment 712920 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Uncommented and removed the lines as suggested in Comment #9
Attachment #712674 - Attachment is obsolete: true
Attachment #712920 - Flags: review?(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 ;)
Attachment #712920 - Flags: review?(mounir) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 713134 [details] [diff] [review]
Applied the missed changes to button from Bug 787095

Deleted the test file and modified Makefile.in accordingly.
Attachment #712920 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
The bug has not been assigned to me yet. I can't add new keywords for check-in.
Assignee: nobody → indiasuny000
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.)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8479bdaf0c8
Keywords: checkin-needed
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"
(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...
(Assignee)

Comment 20

5 years ago
So what do we do now?
Er... what's not tested on linux?
(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.
(In reply to Boris Zbarsky (:bz) from comment #21)
> Er... what's not tested on linux?

devtools
Any idea why the heck not?  Can we get a bug filed on that?
I pushed that patch again with the devtools tests fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d999c65753ea
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla21
(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.
https://hg.mozilla.org/mozilla-central/rev/d999c65753ea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.