Closed Bug 514437 Opened 11 years ago Closed 9 years ago

Implement content part of the progress element

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: sheppy, Assigned: mounir)

References

()

Details

(Keywords: dev-doc-complete, html5)

Attachments

(2 files, 16 obsolete files)

9.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.71 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Not a parser bug (the HTML5 parser is ready for this).
Component: HTML: Parser → Layout: Form Controls
QA Contact: parser → layout.form-controls
Not really a form control either..

Is there a good reason we can't just use a xul:progressmeter here?
Component: Layout: Form Controls → Layout
Keywords: helpwanted
QA Contact: layout.form-controls → layout
Version: 1.9.1 Branch → Trunk
Blocks: 344614
Keywords: html5
Shouldn't that be in "DOM: Core & HTML" component ?
I believe so.
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.
Blocks: 566348
Blocks: 567740
Attached patch Tests v1 (obsolete) — Splinter Review
Attachment #447188 - Flags: review?(Olli.Pettay)
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #447189 - Flags: review?(Olli.Pettay)
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.
Assignee: mounir.lamouri → nobody
Component: Layout → DOM: Core & HTML
QA Contact: layout → general
Attachment #447189 - Flags: review?(timeless)
Comment on attachment 447189 [details] [diff] [review]
Patch v1

Josh, Blake and Olli, could you review the editor, old parser and content changes respectively ?
Attachment #447189 - Flags: review?(mrbkap)
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.
Keywords: helpwanted
Assignee: nobody → mounir.lamouri
Summary: Implement HTML 5 progress element → Implement progress element (content part)
Blocks: 567872
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.
Attachment #447189 - Flags: review?(timeless) → review+
(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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #447189 - Attachment is obsolete: true
Attachment #447227 - Flags: review?(Olli.Pettay)
Attachment #447189 - Flags: review?(mrbkap)
Attachment #447189 - Flags: review?(Olli.Pettay)
Attachment #447227 - Flags: review?(mrbkap)
Attachment #447188 - Flags: review?(Olli.Pettay) → review+
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?
Attachment #447227 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v1.2 (obsolete) — Splinter Review
r=timeless,smaug
Attachment #447227 - Attachment is obsolete: true
Attachment #447552 - Flags: review?(mrbkap)
Attachment #447227 - Flags: review?(mrbkap)
Attachment #447552 - Flags: review?(mrbkap) → review+
Attachment #447552 - Flags: superreview?(jonas)
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.
Attachment #447552 - Flags: superreview?(jonas) → superreview+
(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).
Keywords: dev-doc-needed
No longer blocks: 566348
Duplicate of this bug: 602782
Attached patch Patch v2 (obsolete) — Splinter Review
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
Attachment #447552 - Attachment is obsolete: true
Attachment #508745 - Flags: review?(Olli.Pettay)
Attached patch Inter diff (v1.2 to v2.0) (obsolete) — Splinter Review
Attached patch Tests v2 (obsolete) — Splinter Review
Updated tests with todo's when we suck with floating points.
Attachment #447188 - Attachment is obsolete: true
Attachment #508764 - Flags: review?(Olli.Pettay)
Attached patch Inter diff for tests (v1 to v2) (obsolete) — Splinter Review
Attached patch Tests v2 (obsolete) — Splinter Review
Attachment #508764 - Attachment is obsolete: true
Attachment #508766 - Flags: review?(Olli.Pettay)
Attachment #508764 - Flags: review?(Olli.Pettay)
Blocks: 633207
No longer blocks: 344614
Summary: Implement progress element (content part) → Implement content part of the progress element
Whiteboard: [needs review]
Another small change in the tests.
Attached patch Inter diff (v2 to v2.1) (obsolete) — Splinter Review
The progress element isn't a listed element...
Attached patch Tests v2.1 (obsolete) — Splinter Review
Attachment #508766 - Attachment is obsolete: true
Attachment #512437 - Flags: review?(Olli.Pettay)
Attachment #508766 - Flags: review?(Olli.Pettay)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #508745 - Attachment is obsolete: true
Attachment #512438 - Flags: review?(Olli.Pettay)
Attachment #508745 - Flags: review?(Olli.Pettay)
Attachment #508765 - Attachment description: Inter diff for tests → Inter diff for tests (v1 to v2)
Attached patch Tests v2.2 (obsolete) — Splinter Review
Attachment #512437 - Attachment is obsolete: true
Attachment #512473 - Flags: review?(Olli.Pettay)
Attachment #512437 - Flags: review?(Olli.Pettay)
Blocks: 634549
Depends on: 636750
Attached patch Tests v2.3Splinter Review
Removing some todos, thanks to bug 636750.
Attachment #512473 - Attachment is obsolete: true
Attachment #512473 - Flags: review?(Olli.Pettay)
Attachment #515498 - Attachment is patch: true
Attachment #515498 - Attachment mime type: application/octet-stream → text/plain
Attachment #515498 - Flags: review?(Olli.Pettay)
Attached patch Patch v2.2Splinter Review
I've no inter diff (forgot...) for 2.1 to 2.2 but this is basically moving everything from float to double in nsHTMLProgressElement.
Attachment #512438 - Attachment is obsolete: true
Attachment #515499 - Flags: review?(Olli.Pettay)
Attachment #512438 - Flags: review?(Olli.Pettay)
Attachment #515498 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 515499 [details] [diff] [review]
Patch v2.2

Why is <progress> a form element? Why does it have .form?
Attachment #512472 - Attachment is obsolete: true
Attachment #508765 - Attachment is obsolete: true
Attachment #512435 - Attachment is obsolete: true
Attachment #515497 - Attachment is obsolete: true
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?
OK, I'll review.
Depending on the spec changes nsHTMLProgressElement may or may not end up
inheriting nsGenericHTMLFormElement.
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.
Attachment #515499 - Flags: review?(Olli.Pettay) → review+
Attachment #508746 - Attachment is obsolete: true
Attachment #512436 - Attachment is obsolete: true
(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
Whiteboard: [needs review] → [ready to land][waits for dependencies]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/bfb48178c8ec
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
No longer depends on: 655860
Updated documentation to indicate Firefox compatibility:

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

And mentioned on Firefox 6 for developers.
Depends on: 660200
You need to log in before you can comment on or make changes to this bug.