Implement content part of the progress element

RESOLVED FIXED in mozilla6

Status

()

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

People

(Reporter: sheppy, Assigned: mounir)

Tracking

({dev-doc-complete, html5})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 16 obsolete attachments)

9.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.71 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

7 years ago
Blocks: 344614
Keywords: html5
(Assignee)

Comment 3

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

Updated

7 years ago
Blocks: 559773

Comment 4

7 years ago
I believe so.

Comment 5

7 years ago
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.

Updated

7 years ago
Blocks: 566348
(Assignee)

Updated

7 years ago
Blocks: 567740
(Assignee)

Comment 6

7 years ago
Created attachment 447188 [details] [diff] [review]
Tests v1
Attachment #447188 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 7

7 years ago
Created attachment 447189 [details] [diff] [review]
Patch v1
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #447189 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 8

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

Updated

7 years ago
Attachment #447189 - Flags: review?(timeless)
(Assignee)

Comment 9

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

Comment 10

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

Updated

7 years ago
Keywords: helpwanted
(Assignee)

Updated

7 years ago
Assignee: nobody → mounir.lamouri
(Assignee)

Updated

7 years ago
Summary: Implement HTML 5 progress element → Implement progress element (content part)
(Assignee)

Updated

7 years ago
Blocks: 567872

Comment 11

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

Comment 12

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

Comment 13

7 years ago
Created attachment 447227 [details] [diff] [review]
Patch v1.1
Attachment #447189 - Attachment is obsolete: true
Attachment #447227 - Flags: review?(Olli.Pettay)
Attachment #447189 - Flags: review?(mrbkap)
Attachment #447189 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Attachment #447227 - Flags: review?(mrbkap)

Updated

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

Comment 15

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

r=timeless,smaug
Attachment #447227 - Attachment is obsolete: true
Attachment #447552 - Flags: review?(mrbkap)
Attachment #447227 - Flags: review?(mrbkap)

Updated

7 years ago
Attachment #447552 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

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

Comment 17

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

Updated

7 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

7 years ago
No longer blocks: 566348

Updated

7 years ago
Duplicate of this bug: 602782
(Assignee)

Comment 19

6 years ago
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
Attachment #447552 - Attachment is obsolete: true
Attachment #508745 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 20

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

Comment 21

6 years ago
Created attachment 508764 [details] [diff] [review]
Tests v2

Updated tests with todo's when we suck with floating points.
Attachment #447188 - Attachment is obsolete: true
Attachment #508764 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 22

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

Comment 23

6 years ago
Created attachment 508766 [details] [diff] [review]
Tests v2
Attachment #508764 - Attachment is obsolete: true
Attachment #508766 - Flags: review?(Olli.Pettay)
Attachment #508764 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Blocks: 633207
(Assignee)

Updated

6 years ago
No longer blocks: 344614
Summary: Implement progress element (content part) → Implement content part of the progress element
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Comment 24

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

Another small change in the tests.
(Assignee)

Comment 25

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

The progress element isn't a listed element...
(Assignee)

Comment 26

6 years ago
Created attachment 512437 [details] [diff] [review]
Tests v2.1
Attachment #508766 - Attachment is obsolete: true
Attachment #512437 - Flags: review?(Olli.Pettay)
Attachment #508766 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 27

6 years ago
Created attachment 512438 [details] [diff] [review]
Patch v2.1
Attachment #508745 - Attachment is obsolete: true
Attachment #512438 - Flags: review?(Olli.Pettay)
Attachment #508745 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #508765 - Attachment description: Inter diff for tests → Inter diff for tests (v1 to v2)
(Assignee)

Comment 28

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

Comment 29

6 years ago
Created attachment 512473 [details] [diff] [review]
Tests v2.2
Attachment #512437 - Attachment is obsolete: true
Attachment #512473 - Flags: review?(Olli.Pettay)
Attachment #512437 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Blocks: 634549
(Assignee)

Updated

6 years ago
Depends on: 636750
(Assignee)

Comment 30

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

Comment 31

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

Removing some todos, thanks to bug 636750.
Attachment #512473 - Attachment is obsolete: true
Attachment #512473 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #515498 - Attachment is patch: true
Attachment #515498 - Attachment mime type: application/octet-stream → text/plain
Attachment #515498 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 32

6 years ago
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.
Attachment #512438 - Attachment is obsolete: true
Attachment #515499 - Flags: review?(Olli.Pettay)
Attachment #512438 - Flags: review?(Olli.Pettay)

Updated

6 years ago
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?
I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12254
(Assignee)

Updated

6 years ago
Attachment #512472 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #508765 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #512435 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #515497 - Attachment is obsolete: true
(Assignee)

Comment 35

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

Updated

6 years ago
Attachment #508746 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #512436 - Attachment is obsolete: true
(Assignee)

Comment 38

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

Updated

6 years ago
Whiteboard: [needs review] → [ready to land][waits for dependencies]
(Assignee)

Comment 39

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/bfb48178c8ec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
(Assignee)

Comment 40

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

Updated

6 years ago
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out the other part in http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f
(Assignee)

Comment 43

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
No longer depends on: 655860
(Reporter)

Comment 44

6 years ago
Updated documentation to indicate Firefox compatibility:

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

And mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

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