Last Comment Bug 514437 - Implement content part of the progress element
: Implement content part of the progress element
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
: 602782 (view as bug list)
Depends on: 636750 660200
Blocks: 567740 559773 567872 633207 634549
  Show dependency treegraph
 
Reported: 2009-09-03 09:26 PDT by Eric Shepherd [:sheppy]
Modified: 2011-05-27 03:57 PDT (History)
29 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests v1 (8.73 KB, patch)
2010-05-24 15:02 PDT, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v1 (26.49 KB, patch)
2010-05-24 15:02 PDT, Mounir Lamouri (:mounir)
timeless: review+
Details | Diff | Splinter Review
Patch v1.1 (26.53 KB, patch)
2010-05-24 16:21 PDT, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v1.2 (26.53 KB, patch)
2010-05-26 09:44 PDT, Mounir Lamouri (:mounir)
mrbkap: review+
jonas: superreview+
Details | Diff | Splinter Review
Patch v2 (27.78 KB, patch)
2011-02-01 06:16 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff (v1.2 to v2.0) (7.60 KB, patch)
2011-02-01 06:17 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v2 (8.91 KB, patch)
2011-02-01 07:44 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff for tests (v1 to v2) (9.39 KB, patch)
2011-02-01 07:44 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v2 (8.86 KB, patch)
2011-02-01 07:45 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff for tests (v2.0 to v2.1) (971 bytes, patch)
2011-02-15 02:49 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff (v2 to v2.1) (1.79 KB, patch)
2011-02-15 02:56 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v2.1 (9.81 KB, patch)
2011-02-15 02:57 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.1 (28.66 KB, patch)
2011-02-15 02:57 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff for tests (v2.1 to v2.2) (1.27 KB, patch)
2011-02-15 06:25 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v2.2 (9.70 KB, patch)
2011-02-15 06:26 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff for tests (v2.2 to v2.3) (3.31 KB, patch)
2011-02-27 11:33 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v2.3 (9.55 KB, patch)
2011-02-27 11:34 PST, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Patch v2.2 (28.71 KB, patch)
2011-02-27 11:36 PST, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review

Description Eric Shepherd [:sheppy] 2009-09-03 09:26:22 PDT
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 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.

Note You need to log in before you can comment on or make changes to this bug.