HTMLProgressElement should not be form controls

RESOLVED FIXED in mozilla14

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mounir, Assigned: mjschranz)

Tracking

({dev-doc-complete})

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Thus, they should not have |.form| attributes.
Assignee: nobody → schranz.m
Quick fix!
Attachment #612436 - Flags: review?(mounir)
Comment on attachment 612436 [details] [diff] [review]
Patch V1 removing form attribute

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

The class shouldn't inherit from nsGenericHTMLFormElement but from nsGenericHTMLElement.

Could you update the patch with that change?
Attachment #612436 - Flags: review?(mounir)
I'm not sure if I missed anything here or not but the patch doesn't seem to be blowing up my try server test of it so that's a decent sign!

https://tbpl.mozilla.org/?tree=Try&rev=1112805beb34
Attachment #612436 - Attachment is obsolete: true
Attachment #613050 - Flags: review?
Accidentally didn't assign a reviewer!
Attachment #613050 - Attachment is obsolete: true
Attachment #613050 - Flags: review?
Attachment #613051 - Flags: review?(mounir)
Comment on attachment 613051 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

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

Sorry for not noticing that before but you have to remove NS_FORM_PROGRESS from content/html/content/public/nsIFormControl.h and all occurences of that.
You will also have to fix the tests that are failing.

Except that, your patch looks good.

::: content/html/content/src/nsHTMLProgressElement.cpp
@@ +165,1 @@
>                                                    aValue, aResult);

nit: could you re-align this line?
Attachment #613051 - Flags: review?(mounir)
Fixed up the tests I was causing to fail with this patch. Or at least, removed them since they were checking against a now non existent form attribute.

Try server results of this are here https://tbpl.mozilla.org/?tree=Try&rev=874fc96bb753

Let me know if there's something else I've missed.
Attachment #613051 - Attachment is obsolete: true
Attachment #616860 - Flags: review?(mounir)
Comment on attachment 616860 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

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

r=me with the two changes applied.

::: content/html/content/src/nsHTMLProgressElement.cpp
@@ -68,2 @@
>    NS_IMETHOD Reset();
>    NS_IMETHOD SubmitNamesValues(nsFormSubmission* aFormSubmission);

You should remove Reset() and SubmitNamesValues() too. nsHTMLProgressElement doesn't inherit from nsIFormControl anymore.

::: content/html/content/test/test_bug588683-1.html
@@ -453,5 @@
>            is(formsList[i].elements[j], elementsList[j],
>               "The form should contain " + elementsList[j]);
>          }
> -        is(elementsList[j].form, formsList[i],
> -           "The form owner should be the form associated to the list");

Keep that. You should only remove "progress" from the array |elementNames|.
Attachment #616860 - Flags: review?(mounir) → review+
Two minor changes made. Carrying over the review+ mentioned above by mounir.
Attachment #616860 - Attachment is obsolete: true
Attachment #616992 - Flags: review+
Keywords: checkin-needed
Comment on attachment 616992 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

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

For a few days, patches need to be approved to go to mozilla-central.
Attachment #616992 - Flags: review+
Attachment #616992 - Flags: approval-mozilla-central?
Attachment #616992 - Flags: approval-mozilla-central? → approval-mozilla-central+
https://hg.mozilla.org/integration/mozilla-inbound/rev/149a80dafcb8
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Backed out due to build failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f41d05f51c6

Logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096252&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096228&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=11096257&tree=Mozilla-Inbound

../../../../../content/html/content/src/nsHTMLProgressElement.cpp:124: error: no 'nsresult nsHTMLProgressElement::Reset()' member function declared in class 'nsHTMLProgressElement'
../../../../../content/html/content/src/nsHTMLProgressElement.cpp:131: error: no 'nsresult nsHTMLProgressElement::SubmitNamesValues(nsFormSubmission*)' member function declared in class 'nsHTMLProgressElement'
make[8]: *** [nsHTMLProgressElement.o] Error 1

http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound
Target Milestone: mozilla14 → ---
http://i0.kym-cdn.com/entries/icons/original/000/000/554/facepalm.jpg

Man do I feel dumb right now.

I pushed it up to try during, but didn't on the final one...

Anyway, fixed and going through try right now!
Could you attach the updated patch so I can push it to mozilla-inbound?
Yeah, last one was me just being silly. As you can see in this try run, it's actually just fine now!

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

Most of the fails I see are with fullscreen tests.
Attachment #616992 - Attachment is obsolete: true
Attachment #617458 - Flags: review+
No longer blocks: 555985
Summary: HTMLProgressElement and HTMLMeterElement should not be form controls → HTMLProgressElement should not be form controls
https://hg.mozilla.org/mozilla-central/rev/f946709d44e1
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite- → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Keywords: dev-doc-needed
Target Milestone: mozilla15 → mozilla14
Docs updated:

https://developer.mozilla.org/en/HTML/Element/progress

And listed on Firefox 14 for developers.
Depends on: 762047
Depends on: 762435
You need to log in before you can comment on or make changes to this bug.