Closed
Bug 514437
Opened 15 years ago
Closed 13 years ago
Implement content part of the progress element
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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.
Comment 1•15 years ago
|
||
Not a parser bug (the HTML5 parser is ready for this).
Component: HTML: Parser → Layout: Form Controls
QA Contact: parser → layout.form-controls
Comment 2•15 years ago
|
||
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•14 years ago
|
Assignee | ||
Comment 3•14 years ago
|
||
Shouldn't that be in "DOM: Core & HTML" component ?
Comment 5•14 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.
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #447188 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 7•14 years ago
|
||
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #447189 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 8•14 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•14 years ago
|
Attachment #447189 -
Flags: review?(timeless)
Assignee | ||
Comment 9•14 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•14 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•14 years ago
|
Keywords: helpwanted
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mounir.lamouri
Assignee | ||
Updated•14 years ago
|
Summary: Implement HTML 5 progress element → Implement progress element (content part)
Comment 11•14 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•14 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•14 years ago
|
||
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•14 years ago
|
Attachment #447227 -
Flags: review?(mrbkap)
Updated•14 years ago
|
Attachment #447188 -
Flags: review?(Olli.Pettay) → review+
Comment 14•14 years ago
|
||
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•14 years ago
|
||
r=timeless,smaug
Attachment #447227 -
Attachment is obsolete: true
Attachment #447552 -
Flags: review?(mrbkap)
Attachment #447227 -
Flags: review?(mrbkap)
Updated•14 years ago
|
Attachment #447552 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•14 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•14 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•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Comment 21•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #508764 -
Attachment is obsolete: true
Attachment #508766 -
Flags: review?(Olli.Pettay)
Attachment #508764 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
No longer blocks: 344614
Summary: Implement progress element (content part) → Implement content part of the progress element
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 24•13 years ago
|
||
Another small change in the tests.
Assignee | ||
Comment 25•13 years ago
|
||
The progress element isn't a listed element...
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #508766 -
Attachment is obsolete: true
Attachment #512437 -
Flags: review?(Olli.Pettay)
Attachment #508766 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 27•13 years ago
|
||
Attachment #508745 -
Attachment is obsolete: true
Attachment #512438 -
Flags: review?(Olli.Pettay)
Attachment #508745 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Attachment #508765 -
Attachment description: Inter diff for tests → Inter diff for tests (v1 to v2)
Assignee | ||
Comment 28•13 years ago
|
||
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #512437 -
Attachment is obsolete: true
Attachment #512473 -
Flags: review?(Olli.Pettay)
Attachment #512437 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 30•13 years ago
|
||
Assignee | ||
Comment 31•13 years ago
|
||
Removing some todos, thanks to bug 636750.
Attachment #512473 -
Attachment is obsolete: true
Attachment #512473 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 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•13 years ago
|
||
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•13 years ago
|
Attachment #515498 -
Flags: review?(Olli.Pettay) → review+
Comment 33•13 years ago
|
||
Comment on attachment 515499 [details] [diff] [review] Patch v2.2 Why is <progress> a form element? Why does it have .form?
Comment 34•13 years ago
|
||
I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12254
Assignee | ||
Updated•13 years ago
|
Attachment #512472 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #508765 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #512435 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #515497 -
Attachment is obsolete: true
Assignee | ||
Comment 35•13 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?
Comment 36•13 years ago
|
||
OK, I'll review. Depending on the spec changes nsHTMLProgressElement may or may not end up inheriting nsGenericHTMLFormElement.
Comment 37•13 years ago
|
||
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•13 years ago
|
Attachment #508746 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #512436 -
Attachment is obsolete: true
Assignee | ||
Comment 38•13 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•13 years ago
|
Whiteboard: [needs review] → [ready to land][waits for dependencies]
Assignee | ||
Comment 39•13 years ago
|
||
Pushed: http://hg.mozilla.org/mozilla-central/rev/bfb48178c8ec
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
Assignee | ||
Comment 40•13 years ago
|
||
And tests: http://hg.mozilla.org/mozilla-central/rev/6d42dd8b3d0d
Comment 41•13 years ago
|
||
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 42•13 years ago
|
||
Backed out the other part in http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f
Assignee | ||
Comment 43•13 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
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 44•13 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
You need to log in
before you can comment on or make changes to this bug.
Description
•