Comment 1 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-09-26) 2009-09-04 05:20:35 PDT

Not a parser bug (the HTML5 parser is ready for this).

Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2009-09-04 13:20:23 PDT

Not really a form control either..

Is there a good reason we can't just use a xul:progressmeter here?

Comment 3 Mounir Lamouri (:mounir) 2010-03-30 08:10:16 PDT

Shouldn't that be in "DOM: Core & HTML" component ?

Comment 4 alegend45 2010-05-13 09:39:54 PDT

I believe so.

Comment 5 Michael[tm] Smith 2010-05-17 00:06:06 PDT

FWIW, note that support for the HTML5 progress element recently landed in WebKit trunk, and it's now testable in WebKit nightlies; simple test case at http://trac.webkit.org/export/59461/trunk/LayoutTests/fast/dom/HTMLProgressElement/progress-element.html (and some others in the same dir). So it would great to have another implementation against which to do some cross-browser testing.

Comment 6 Mounir Lamouri (:mounir) 2010-05-24 15:02:21 PDT

Created attachment 447188 [details] [diff] [review]
Tests v1

Comment 7 Mounir Lamouri (:mounir) 2010-05-24 15:02:52 PDT

Created attachment 447189 [details] [diff] [review]
Patch v1

Comment 8 Mounir Lamouri (:mounir) 2010-05-24 15:03:57 PDT

I'm going to divide this bug in two: one for the content, one for the layout so patches are going to be smaller and easier to review.

Comment 9 Mounir Lamouri (:mounir) 2010-05-24 15:09:38 PDT

Comment on attachment 447189 [details] [diff] [review]
Patch v1

Josh, Blake and Olli, could you review the editor, old parser and content changes respectively ?

Comment 10 Mounir Lamouri (:mounir) 2010-05-24 15:13:08 PDT

This implementation is following the specifications even if I think .position should be a double and .value/.max should reflect the internal value instead of the the content attribute.

Olli, let me know if you want me to implement these changes.

Comment 11 timeless 2010-05-24 16:04:00 PDT

Comment on attachment 447189 [details] [diff] [review]
Patch v1

are there tests for this?

+NS_IMETHODIMP
>+nsHTMLProgressElement::GetForm(nsIDOMHTMLFormElement** aForm)
>+{
>+    return nsGenericHTMLFormElement::GetForm(aForm);

this ^ is overindented

compare:
>+nsHTMLProgressElement::GetPosition(float* aPosition)
>+{
>+  *aPosition = kDefaultPosition;

personally, i'd prefer early return for the false case:
>+  if (attrValue && attrValue->Type() == nsAttrValue::eFloatValue) {

then you wouldn't need to pre init *aPosition for this case, however I'm not sure about this module's style.

the editor bits seem fine.

Comment 12 Mounir Lamouri (:mounir) 2010-05-24 16:12:27 PDT

(In reply to comment #11)
> (From update of attachment 447189 [details] [diff] [review])
> are there tests for this?

Yes. There is a second file attached with the required tests.

> +NS_IMETHODIMP
> >+nsHTMLProgressElement::GetForm(nsIDOMHTMLFormElement** aForm)
> >+{
> >+    return nsGenericHTMLFormElement::GetForm(aForm);
> 
> this ^ is overindented

Thanks for noticing.

> personally, i'd prefer early return for the false case:
> >+  if (attrValue && attrValue->Type() == nsAttrValue::eFloatValue) {
> 
> then you wouldn't need to pre init *aPosition for this case, however I'm not
> sure about this module's style.

I've changed that.

> the editor bits seem fine.

Thank you for the review.

Comment 13 Mounir Lamouri (:mounir) 2010-05-24 16:21:43 PDT

Created attachment 447227 [details] [diff] [review]
Patch v1.1

Comment 14 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2010-05-25 08:07:44 PDT

Comment on attachment 447227 [details] [diff] [review]
Patch v1.1

>diff --git a/js/src/xpconnect/src/dom_quickstubs.qsconf b/js/src/xpconnect/src/dom_quickstubs.qsconf
>--- a/js/src/xpconnect/src/dom_quickstubs.qsconf
>+++ b/js/src/xpconnect/src/dom_quickstubs.qsconf
>@@ -278,16 +278,17 @@ members = [
>     'nsIDOMHTMLInputElement.form',
>     'nsIDOMHTMLInputElement.src',
>     'nsIDOMHTMLInputElement.name',
>     'nsIDOMHTMLInputElement.value',
>     'nsIDOMHTMLLinkElement.disabled',
>     'nsIDOMHTMLOptionElement.index',
>     'nsIDOMHTMLOptionElement.selected',
>     'nsIDOMHTMLOptionElement.form',
>+    'nsIDOMHTMLProgressElement.position',
Why only position?
'nsIDOMHTMLProgressElement.*',
would make more sense, no?

Comment 15 Mounir Lamouri (:mounir) 2010-05-26 09:44:26 PDT

Created attachment 447552 [details] [diff] [review]
Patch v1.2

r=timeless,smaug

Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-26 17:36:04 PDT

Comment on attachment 447552 [details] [diff] [review]
Patch v1.2

sr=me

Though note that I don't think we should land this until we have <progress> fully implemented.

At the very least we need to have rendering done. But really we need stylability too in order to ship it.

Comment 17 Mounir Lamouri (:mounir) 2010-05-26 17:44:45 PDT

(In reply to comment #16)
> (From update of attachment 447552 [details] [diff] [review])
> sr=me
> 
> Though note that I don't think we should land this until we have <progress>
> fully implemented.
> 
> At the very least we need to have rendering done. But really we need
> stylability too in order to ship it.

I divided the progress element in two parts to simplify the reviews but I don't want to have theme landed separately (iow, this part has to wait for the layout part).
For the stylability, on top of my head I would say everything should be okay except for the anonymous div (the progress-bar) which would have to be stylable with a pseudo-element but afaik, that's would not be easy to do (I've put the placeholder pseudo-element on hold because of that).

Comment 18 Svetlana A. Tkachenko 2010-10-07 22:33:46 PDT

*** Bug 602782 has been marked as a duplicate of this bug. ***

Comment 19 Mounir Lamouri (:mounir) 2011-02-01 06:16:40 PST

Created attachment 508745 [details] [diff] [review]
Patch v2

This is updating the patch to compile against current trunk and applies some specs change. I've also applied some changes I've asked. See:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11936
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11937
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11938
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11939

Comment 20 Mounir Lamouri (:mounir) 2011-02-01 06:17:13 PST

Created attachment 508746 [details] [diff] [review]
Inter diff (v1.2 to v2.0)

Comment 21 Mounir Lamouri (:mounir) 2011-02-01 07:44:19 PST

Created attachment 508764 [details] [diff] [review]
Tests v2

Updated tests with todo's when we suck with floating points.

Comment 22 Mounir Lamouri (:mounir) 2011-02-01 07:44:48 PST

Created attachment 508765 [details] [diff] [review]
Inter diff for tests (v1 to v2)

Comment 23 Mounir Lamouri (:mounir) 2011-02-01 07:45:55 PST

Created attachment 508766 [details] [diff] [review]
Tests v2

Comment 24 Mounir Lamouri (:mounir) 2011-02-15 02:49:08 PST

Created attachment 512435 [details] [diff] [review]
Inter diff for tests (v2.0 to v2.1)

Another small change in the tests.

Comment 25 Mounir Lamouri (:mounir) 2011-02-15 02:56:41 PST

Created attachment 512436 [details] [diff] [review]
Inter diff (v2 to v2.1)

The progress element isn't a listed element...

Comment 26 Mounir Lamouri (:mounir) 2011-02-15 02:57:17 PST

Created attachment 512437 [details] [diff] [review]
Tests v2.1

Comment 27 Mounir Lamouri (:mounir) 2011-02-15 02:57:50 PST

Created attachment 512438 [details] [diff] [review]
Patch v2.1

Comment 28 Mounir Lamouri (:mounir) 2011-02-15 06:25:31 PST

Created attachment 512472 [details] [diff] [review]
Inter diff for tests (v2.1 to v2.2)

Comment 29 Mounir Lamouri (:mounir) 2011-02-15 06:26:10 PST

Created attachment 512473 [details] [diff] [review]
Tests v2.2

Comment 30 Mounir Lamouri (:mounir) 2011-02-27 11:33:48 PST

Created attachment 515497 [details] [diff] [review]
Inter diff for tests (v2.2 to v2.3)

Comment 31 Mounir Lamouri (:mounir) 2011-02-27 11:34:46 PST

Created attachment 515498 [details] [diff] [review]
Tests v2.3

Removing some todos, thanks to bug 636750.

Comment 32 Mounir Lamouri (:mounir) 2011-02-27 11:36:25 PST

Created attachment 515499 [details] [diff] [review]
Patch v2.2

I've no inter diff (forgot...) for 2.1 to 2.2 but this is basically moving everything from float to double in nsHTMLProgressElement.

Comment 33 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-05 16:03:02 PST

Comment on attachment 515499 [details] [diff] [review]
Patch v2.2

Why is <progress> a form element? Why does it have .form?

Comment 34 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-06 02:28:54 PST

I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12254

Comment 35 Mounir Lamouri (:mounir) 2011-03-09 11:01:21 PST

Olli, would you be fine if I open a follow-up to apply the changes that might come from the spec bug and finish with reviewing this patch?

Comment 36 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-09 11:17:07 PST

OK, I'll review.
Depending on the spec changes nsHTMLProgressElement may or may not end up
inheriting nsGenericHTMLFormElement.

Comment 37 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-10 05:55:38 PST

Comment on attachment 515499 [details] [diff] [review]
Patch v2.2

>+++ b/content/base/src/nsContentAreaDragDrop.cpp
>@@ -440,17 +440,18 @@ DragDataProducer::Produce(nsDOMDataTrans
>   // aCanDrag to false however, as we still want to allow the drag.
>   nsCOMPtr<nsIContent> findFormNode = mSelectionTargetNode;
>   nsIContent* findFormParent = findFormNode->GetParent();
>   while (findFormParent) {
>     nsCOMPtr<nsIFormControl> form(do_QueryInterface(findFormParent));
>     if (form && form->GetType() != NS_FORM_OBJECT &&
>                 form->GetType() != NS_FORM_FIELDSET &&
>                 form->GetType() != NS_FORM_LABEL &&
>-                form->GetType() != NS_FORM_OUTPUT)
>+                form->GetType() != NS_FORM_OUTPUT &&
>+                form->GetType() != NS_FORM_PROGRESS)
Could you perhaps file a followup to add some method to nsIFormControl for this.
bool nsIFormControl::IsNativelyDraggable() or something like that.
Or, actually the method could be added to nsIContent.

>+   * @return whether the progress elemnet is in the indeterminate state.
'elemnet'


>+nsHTMLProgressElement::GetPosition(double* aPosition)
>+{
>+  if (IsIndeterminate()) {
>+    *aPosition = kIndeterminatePosition;
>+    return NS_OK;
>+  }
>+
>+  double value;
>+  double max;
>+  GetValue(&value);
>+  GetMax(&max);
>+
>+  *aPosition = (double)value / (double)max;
is the casting needed? If so, use C++ casting, not C.
max can't be 0, but I hope we test cases when someone tries to set it to 0.


>+ * For more information on this interface, please see
>+ * http://www.whatwg.org/specs/web-apps/current-work/#the-output-element
output?


>+++ b/editor/libeditor/html/nsHTMLEditUtils.cpp
>@@ -428,16 +428,17 @@ nsHTMLEditUtils::IsFormWidget(nsIDOMNode
> {
>   NS_PRECONDITION(node, "null node passed to nsHTMLEditUtils::IsFormWidget");
>   nsCOMPtr<nsIAtom> nodeAtom = nsEditor::GetTag(node);
>   return (nodeAtom == nsEditProperty::textarea)
>       || (nodeAtom == nsEditProperty::select)
>       || (nodeAtom == nsEditProperty::button)
>       || (nodeAtom == nsEditProperty::output)
>       || (nodeAtom == nsEditProperty::keygen)
>+      || (nodeAtom == nsEditProperty::progress)
>       || (nodeAtom == nsEditProperty::input);
> }
This should just QI node to nsIFormControl and perhaps check that 
it is not an <object>. Could be done in a followup.

Comment 38 Mounir Lamouri (:mounir) 2011-03-11 06:58:27 PST

(In reply to comment #37)
> Could you perhaps file a followup to add some method to nsIFormControl for
> this.
> bool nsIFormControl::IsNativelyDraggable() or something like that.
> Or, actually the method could be added to nsIContent.

bug 640946

> >+   * @return whether the progress elemnet is in the indeterminate state.
> 'elemnet'

Ok.

> >+nsHTMLProgressElement::GetPosition(double* aPosition)
> >+{
> >+  if (IsIndeterminate()) {
> >+    *aPosition = kIndeterminatePosition;
> >+    return NS_OK;
> >+  }
> >+
> >+  double value;
> >+  double max;
> >+  GetValue(&value);
> >+  GetMax(&max);
> >+
> >+  *aPosition = (double)value / (double)max;
> is the casting needed? If so, use C++ casting, not C.

No, I had to do that when .position was a double but .value and .max where floats. It was fixed locally. 

> max can't be 0, but I hope we test cases when someone tries to set it to 0.

It is tested.

> >+ * For more information on this interface, please see
> >+ * http://www.whatwg.org/specs/web-apps/current-work/#the-output-element
> output?

Eh, bad copy-paste :)

> >+++ b/editor/libeditor/html/nsHTMLEditUtils.cpp
> >@@ -428,16 +428,17 @@ nsHTMLEditUtils::IsFormWidget(nsIDOMNode
> > {
> >   NS_PRECONDITION(node, "null node passed to nsHTMLEditUtils::IsFormWidget");
> >   nsCOMPtr<nsIAtom> nodeAtom = nsEditor::GetTag(node);
> >   return (nodeAtom == nsEditProperty::textarea)
> >       || (nodeAtom == nsEditProperty::select)
> >       || (nodeAtom == nsEditProperty::button)
> >       || (nodeAtom == nsEditProperty::output)
> >       || (nodeAtom == nsEditProperty::keygen)
> >+      || (nodeAtom == nsEditProperty::progress)
> >       || (nodeAtom == nsEditProperty::input);
> > }
> This should just QI node to nsIFormControl and perhaps check that 
> it is not an <object>. Could be done in a followup.

bug 640947

Comment 39 Mounir Lamouri (:mounir) 2011-05-09 05:34:06 PDT

Pushed:
http://hg.mozilla.org/mozilla-central/rev/bfb48178c8ec

Comment 40 Mounir Lamouri (:mounir) 2011-05-09 05:35:43 PDT

And tests:
http://hg.mozilla.org/mozilla-central/rev/6d42dd8b3d0d

Comment 41 Shawn Wilsher :sdwilsh 2011-05-09 16:10:43 PDT

Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.

Comment 42 Matt Brubeck (:mbrubeck) 2011-05-09 20:05:07 PDT

Backed out the other part in http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f

Comment 43 Mounir Lamouri (:mounir) 2011-05-10 06:54:44 PDT

The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/d79de4b6b7f1
http://hg.mozilla.org/mozilla-central/rev/684cdf4a4212

Comment 44 Eric Shepherd [:sheppy] 2011-05-23 09:56:19 PDT

Updated documentation to indicate Firefox compatibility:

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

And mentioned on Firefox 6 for developers.