Closed
Bug 514437
Opened 16 years ago
Closed 14 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•16 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•16 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•15 years ago
|
Assignee | ||
Comment 3•15 years ago
|
||
Shouldn't that be in "DOM: Core & HTML" component ?
Comment 5•15 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•15 years ago
|
||
Attachment #447188 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 7•15 years ago
|
||
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #447189 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 8•15 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•15 years ago
|
Attachment #447189 -
Flags: review?(timeless)
Assignee | ||
Comment 9•15 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•15 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•15 years ago
|
Keywords: helpwanted
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → mounir.lamouri
Assignee | ||
Updated•15 years ago
|
Summary: Implement HTML 5 progress element → Implement progress element (content part)
Comment 11•15 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•15 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•15 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•15 years ago
|
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+
Assignee | ||
Comment 15•15 years ago
|
||
r=timeless,smaug
Attachment #447227 -
Attachment is obsolete: true
Attachment #447552 -
Flags: review?(mrbkap)
Attachment #447227 -
Flags: review?(mrbkap)
Updated•15 years ago
|
Attachment #447552 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•15 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•15 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•15 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•14 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•14 years ago
|
||
Assignee | ||
Comment 21•14 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•14 years ago
|
||
Assignee | ||
Comment 23•14 years ago
|
||
Attachment #508764 -
Attachment is obsolete: true
Attachment #508766 -
Flags: review?(Olli.Pettay)
Attachment #508764 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
No longer blocks: 344614
Summary: Implement progress element (content part) → Implement content part of the progress element
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 24•14 years ago
|
||
Another small change in the tests.
Assignee | ||
Comment 25•14 years ago
|
||
The progress element isn't a listed element...
Assignee | ||
Comment 26•14 years ago
|
||
Attachment #508766 -
Attachment is obsolete: true
Attachment #512437 -
Flags: review?(Olli.Pettay)
Attachment #508766 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 27•14 years ago
|
||
Attachment #508745 -
Attachment is obsolete: true
Attachment #512438 -
Flags: review?(Olli.Pettay)
Attachment #508745 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
Attachment #508765 -
Attachment description: Inter diff for tests → Inter diff for tests (v1 to v2)
Assignee | ||
Comment 28•14 years ago
|
||
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #512437 -
Attachment is obsolete: true
Attachment #512473 -
Flags: review?(Olli.Pettay)
Attachment #512437 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 30•14 years ago
|
||
Assignee | ||
Comment 31•14 years ago
|
||
Removing some todos, thanks to bug 636750.
Attachment #512473 -
Attachment is obsolete: true
Attachment #512473 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 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•14 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)
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?
Assignee | ||
Updated•14 years ago
|
Attachment #512472 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #508765 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #512435 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #515497 -
Attachment is obsolete: true
Assignee | ||
Comment 35•14 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•14 years ago
|
Attachment #508746 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #512436 -
Attachment is obsolete: true
Assignee | ||
Comment 38•14 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•14 years ago
|
Whiteboard: [needs review] → [ready to land][waits for dependencies]
Assignee | ||
Comment 39•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6
Assignee | ||
Comment 40•14 years ago
|
||
Comment 41•14 years ago
|
||
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 42•14 years ago
|
||
Backed out the other part in http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f
Assignee | ||
Comment 43•14 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: 14 years ago → 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 44•14 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
•