514437 - Implement content part of the progress element

Status: RESOLVED FIXED

Description

[Eric Shepherd [:sheppy]]

HTML 5 includes a progress element for creating progress bars. This would be stunningly, remarkably, amazingly useful for a lot of people (myself included), so I figured I'd put my request in now. Obviously there are ways to do this now, but having support for this would make this much simpler.

Comment 1

[Henri Sivonen (:hsivonen)]

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

Comment 2

[Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-)]

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)]

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

Comment 4

[alegend45]

I believe so.

Comment 5

[Michael[tm] Smith]

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)]

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

Comment 7

[Mounir Lamouri (:mounir)]

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

Comment 8

[Mounir Lamouri (:mounir)]

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)]

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)]

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]

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)]

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

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

Comment 14

[Olli Pettay [:smaug]]

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)]

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

r=timeless,smaug

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

(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 19

[Mounir Lamouri (:mounir)]

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)]

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

Comment 21

[Mounir Lamouri (:mounir)]

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

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

Comment 22

[Mounir Lamouri (:mounir)]

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

Comment 23

[Mounir Lamouri (:mounir)]

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

Comment 24

[Mounir Lamouri (:mounir)]

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)]

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)]

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

Comment 27

[Mounir Lamouri (:mounir)]

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

Comment 28

[Mounir Lamouri (:mounir)]

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

Comment 29

[Mounir Lamouri (:mounir)]

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

Comment 30

[Mounir Lamouri (:mounir)]

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

Comment 31

[Mounir Lamouri (:mounir)]

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

Removing some todos, thanks to bug 636750.

Comment 32

[Mounir Lamouri (:mounir)]

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]]

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]]

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

Comment 35

[Mounir Lamouri (:mounir)]

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]]

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

Comment 37

[Olli Pettay [:smaug]]

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)]

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

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

Comment 40

[Mounir Lamouri (:mounir)]

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

Comment 41

[Shawn Wilsher :sdwilsh]

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

Comment 42

[Matt Brubeck (:mbrubeck)]

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

Comment 43

[Mounir Lamouri (:mounir)]

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]]

Updated documentation to indicate Firefox compatibility:

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

And mentioned on Firefox 6 for developers.