Note: There are a few cases of duplicates in user autocompletion which are being worked on.

HTMLProgressElement should not be form controls

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: mounir, Assigned: mjschranz)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Thus, they should not have |.form| attributes.
(Assignee)

Updated

5 years ago
Assignee: nobody → schranz.m
(Assignee)

Comment 1

5 years ago
Created attachment 612436 [details] [diff] [review]
Patch V1 removing form attribute

Quick fix!
Attachment #612436 - Flags: review?(mounir)
(Reporter)

Comment 2

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

Comment 3

5 years ago
Created attachment 613050 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

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

Comment 4

5 years ago
Created attachment 613051 [details] [diff] [review]
Patch V2 Changing Inheritance from GenericFormElement to GenericElement

Accidentally didn't assign a reviewer!
Attachment #613050 - Attachment is obsolete: true
Attachment #613050 - Flags: review?
Attachment #613051 - Flags: review?(mounir)
(Reporter)

Comment 5

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

Comment 6

5 years ago
Created attachment 616860 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

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)
(Reporter)

Comment 7

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

Comment 8

5 years ago
Created attachment 616992 [details] [diff] [review]
Patch V3 Changing Inheritance from GenericFormElement to GenericElement

Two minor changes made. Carrying over the review+ mentioned above by mounir.
Attachment #616860 - Attachment is obsolete: true
Attachment #616992 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 9

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

Comment 12

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

Comment 13

5 years ago
Could you attach the updated patch so I can push it to mozilla-inbound?
(Assignee)

Comment 14

5 years ago
Created attachment 617458 [details] [diff] [review]
Fixed up Patch from minor issues with last one

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+
(Reporter)

Updated

5 years ago
No longer blocks: 555985
(Reporter)

Updated

5 years ago
Summary: HTMLProgressElement and HTMLMeterElement should not be form controls → HTMLProgressElement should not be form controls
Duplicate of this bug: 748238
(Reporter)

Comment 16

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f946709d44e1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite- → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Reporter)

Updated

5 years ago
Keywords: dev-doc-needed

Updated

5 years ago
Target Milestone: mozilla15 → mozilla14
Docs updated:

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

And listed on Firefox 14 for developers.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

5 years ago
Depends on: 762047
(Reporter)

Updated

5 years ago
Depends on: 762435
You need to log in before you can comment on or make changes to this bug.