Last Comment Bug 346485 - Implement output element
: Implement output element
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on:
Blocks: html5forms 559463 558036 561635 566348
  Show dependency treegraph
 
Reported: 2006-07-29 20:03 PDT by Alex Vincent [:WeirdAl]
Modified: 2010-12-17 07:02 PST (History)
19 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests - WIP (3.81 KB, patch)
2010-04-02 08:59 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (23.35 KB, patch)
2010-04-02 09:06 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v1 (8.76 KB, patch)
2010-04-05 06:15 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Patch v1 (40.60 KB, patch)
2010-04-05 06:17 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Tests v2 (9.60 KB, patch)
2010-04-08 03:42 PDT, Mounir Lamouri (:mounir)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (39.93 KB, patch)
2010-04-08 03:43 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3 (15.29 KB, patch)
2010-04-08 06:03 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.1 (39.90 KB, patch)
2010-04-08 06:04 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3.1 (15.20 KB, patch)
2010-04-08 15:42 PDT, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
Tests v3.2 (15.19 KB, patch)
2010-04-09 11:04 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Tests v3.3 (14.41 KB, patch)
2010-04-11 06:26 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.2 (39.95 KB, patch)
2010-04-11 06:30 PDT, Mounir Lamouri (:mounir)
bugs: review+
hsivonen: review+
mrbkap: review+
Details | Diff | Splinter Review
Patch v2.3 (39.94 KB, patch)
2010-04-20 08:34 PDT, Mounir Lamouri (:mounir)
jst: superreview+
Details | Diff | Splinter Review
Tests v3.4 (14.42 KB, patch)
2010-04-22 07:03 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.4 (39.93 KB, patch)
2010-04-22 07:25 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.5 (43.48 KB, patch)
2010-04-25 17:34 PDT, Mounir Lamouri (:mounir)
timeless: review+
roc: review+
Details | Diff | Splinter Review
Patch v2.6 (43.46 KB, patch)
2010-04-26 03:42 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2006-07-29 20:03:29 PDT
For new Web Forms 2 controls, the general consensus is they should be implemented as XBL-based widgets.  I'm not sure exactly where to put the files for that, though...

My initial thinking is content/html/content/src/webforms2.xml for the new bindings, and layout/style/html.css for the binding style rule.
Comment 1 Alex Vincent [:WeirdAl] 2006-08-02 23:33:23 PDT
I won't be able to xbl-ize this unless we can 100% guarantee the binding will apply... and per bz we can't do that for a number of reasons (display: none being one of them).
Comment 2 Mounir Lamouri (:mounir) 2010-04-02 08:59:32 PDT
Created attachment 436693 [details] [diff] [review]
Tests - WIP
Comment 3 Mounir Lamouri (:mounir) 2010-04-02 09:06:49 PDT
Created attachment 436694 [details] [diff] [review]
Patch - WIP

This patch is nearly ready. Still two things missing:
- htmlFor IDL attribute and for content attribute with DOMSettableTokenList
- this sentence too: "Whenever the element's descendants are changed in any way, if the value mode flag is in mode default, the element's default value must be set to the value of the element's textContent IDL attribute."

Constraint validation stuff and labels IDL attribute will not be implemented with this bug but with bug 345624 and bug 556743.
Comment 4 Mounir Lamouri (:mounir) 2010-04-05 06:15:05 PDT
Created attachment 437011 [details] [diff] [review]
Tests v1
Comment 5 Mounir Lamouri (:mounir) 2010-04-05 06:17:05 PDT
Created attachment 437012 [details] [diff] [review]
Patch v1

This patch is not implementing [PutForwards=value] for htmlFor IDL attribute because it doesn't sound needed. A message has been posted to whatwg ml to know why it needed.
Comment 6 Olli Pettay [:smaug] 2010-04-07 05:37:20 PDT
Comment on attachment 437012 [details] [diff] [review]
Patch v1


>+class nsDOMSettableTokenList : public nsDOMTokenList,
>+                               public nsIDOMDOMSettableTokenList
>+{
>+public:
>+  NS_DECL_ISUPPORTS
_INHERITED

>+nsHTMLOutputElement::~nsHTMLOutputElement()
>+{
>+  mTokenList->DropReference();
>+}
This crashes if mTokenList is null.


>+NS_IMETHODIMP
>+nsHTMLOutputElement::SubmitNamesValues(nsFormSubmission* aFormSubmission,
>+                                       nsIContent* aSubmitElement)
>+{
>+  return NS_OK;
>+}
Could you add a comment here that output isn't submittable.



>+void nsHTMLOutputElement::AttributeChanged(nsIDocument* aDocument,
>+                                           nsIContent*  aContent,
>+                                           PRInt32      aNameSpaceID,
>+                                           nsIAtom*     aAttribute,
>+                                           PRInt32      aModType)
>+{
>+  if (aContent != this) {
>+    DescendantsChanged();
>+  }
>+}
Why is this needed?


>+++ b/dom/base/nsDOMClassInfoID.h	Mon Apr 05 15:13:07 2010 +0200
>@@ -60,16 +60,17 @@ enum nsDOMClassInfoID {
>   eDOMClassInfo_DOMConstructor_id,
> 
>   // Core classes
>   eDOMClassInfo_XMLDocument_id,
>   eDOMClassInfo_DocumentType_id,
>   eDOMClassInfo_DOMImplementation_id,
>   eDOMClassInfo_DOMException_id,
>   eDOMClassInfo_DOMTokenList_id,
>+  eDOMClassInfo_DOMSettableTokenList_id,
IIRC, this all changed yesterday or so, so you need
have DOMCI_CLASS and DOMCI_DATA somewhere.
Comment 7 Olli Pettay [:smaug] 2010-04-07 05:46:47 PDT
Comment on attachment 437011 [details] [diff] [review]
Tests v1

Would it be possible to test also submission handling; output shouldn't be submitted.
Comment 8 Olli Pettay [:smaug] 2010-04-07 05:53:23 PDT
Comment on attachment 437011 [details] [diff] [review]
Tests v1

Using form.formData, testing submission should be really easy.
Comment 9 Mounir Lamouri (:mounir) 2010-04-07 15:30:08 PDT
getFormData() returns an object I can only use to append elements. I should QI to nsIXHRSendable but it is failing. If someone as a hint about what I should check ?
Comment 10 Mounir Lamouri (:mounir) 2010-04-07 15:30:58 PDT
CCing David for a11y.
Comment 11 alexander :surkov 2010-04-08 02:47:02 PDT
I have couple question to understand what we need to do for a11y here.

1) What frame is used for HTML output element?
2) Can HTML output contain arbitrary HTML or its content is always defined by @value attribute?
Comment 12 Mounir Lamouri (:mounir) 2010-04-08 03:42:51 PDT
Created attachment 437809 [details] [diff] [review]
Tests v2

The tests are now checking for the form submission and deeply checking the textContent value because there was a bug.
Comment 13 Mounir Lamouri (:mounir) 2010-04-08 03:43:41 PDT
Created attachment 437810 [details] [diff] [review]
Patch v2
Comment 14 Mounir Lamouri (:mounir) 2010-04-08 03:54:17 PDT
(In reply to comment #11)
> I have couple question to understand what we need to do for a11y here.
> 
> 1) What frame is used for HTML output element?

The output element has no specific frame.

> 2) Can HTML output contain arbitrary HTML or its content is always defined by
> @value attribute?

The output element can contain arbitrary HTML. But it's @value will only reflect the text from this html.

You can consider the output element like a span element. It has to be used for web developers to show something depending on other fields value and it adds the ability to check the element value validity. Even if the constraint validity API is not landed yet.
Comment 15 Olli Pettay [:smaug] 2010-04-08 04:07:21 PDT
Comment on attachment 437809 [details] [diff] [review]
Tests v2


>+<iframe name="submit_frame" onLoad="checkFormSubmission();" style="visibility: hidden;"></iframe>
Hmm, shouldn't you set onload="..." afterwards? Otherwise you
may handle also the load event for about:blank document.

>+function checkFormSubmission()
>+{
>+  // The output element value is 'bar' at this moment.
>+  // It should not appear in the submit url.
Could you actually check before submission that the value is 'bar'

>+  is(frames['submit_frame'].location.href,
>+    'http://mochi.test:8888/tests/content/html/content/test/foo',
>+     "The output element value should not be submitted");
>+  SimpleTest.finish();
Could you add at least one input element to the submission so that
we know that the submission works in general.

>+}
>+
>+// <output> has to work only with the html5 parser.
>+// Do not test if this parser is not enabled.
>+netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+            .getService(Components.interfaces.nsIPrefBranch);
>+if (prefs.getBoolPref("html5.enable"))
Is it actually possible enable html5 parser for these tests?
Something like enable tests in parent document and then load tests in subframe, and
after testing set html5.enable to whatever its value was originally?
I sure hsivonen knows whether that is possible.

And for a new element implementation reftests would be great.
Comment 16 Olli Pettay [:smaug] 2010-04-08 04:27:37 PDT
Comment on attachment 437810 [details] [diff] [review]
Patch v2


>+void nsHTMLOutputElement::CharacterDataChanged(nsIDocument* aDocument,
>+                                               nsIContent* aContent,
>+                                               CharacterDataChangeInfo* aInfo)
>+{
>+  if (aContent != this) {
>+    DescendantsChanged();
>+  }
>+}

So when would aContent be this?
Comment 17 alexander :surkov 2010-04-08 05:24:27 PDT
I filed bug 558036 for HTML output accessibility support. Let's move a11y discussion there.
Comment 18 Mounir Lamouri (:mounir) 2010-04-08 05:30:25 PDT
(In reply to comment #15)
> (From update of attachment 437809 [details] [diff] [review])
> 
> >+<iframe name="submit_frame" onLoad="checkFormSubmission();" style="visibility: hidden;"></iframe>
> Hmm, shouldn't you set onload="..." afterwards? Otherwise you
> may handle also the load event for about:blank document.

Actually, this is a typo ;)

> >+function checkFormSubmission()
> >+{
> >+  // The output element value is 'bar' at this moment.
> >+  // It should not appear in the submit url.
> Could you actually check before submission that the value is 'bar'
> 
> >+  is(frames['submit_frame'].location.href,
> >+    'http://mochi.test:8888/tests/content/html/content/test/foo',
> >+     "The output element value should not be submitted");
> >+  SimpleTest.finish();
> Could you add at least one input element to the submission so that
> we know that the submission works in general.

I will check input elements and the output element values before submit() so I'm sure the output element value is valid.

> >+// <output> has to work only with the html5 parser.
> >+// Do not test if this parser is not enabled.
> >+netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> >+var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+            .getService(Components.interfaces.nsIPrefBranch);
> >+if (prefs.getBoolPref("html5.enable"))
> Is it actually possible enable html5 parser for these tests?
> Something like enable tests in parent document and then load tests in subframe,
> and
> after testing set html5.enable to whatever its value was originally?
> I sure hsivonen knows whether that is possible.

It is doable. I tried that for the datalist element tests but I got a probably not so important issue. Actually, I'm wondering if it is needed to add a subframe and make the test more complex when we are planning to set the html5 parser as the default parser in a near future ?

> And for a new element implementation reftests would be great.

I forget them, sorry.
Comment 19 Mounir Lamouri (:mounir) 2010-04-08 06:03:09 PDT
Created attachment 437828 [details] [diff] [review]
Tests v3

I've added reftests and changes you've requested except enabling the html5 parser. Let me know if you really think it is needed.
By the way, if the reftests do not use html5 parser it will fail so I'm wondering how this should be managed for the tests servers...
Comment 20 Mounir Lamouri (:mounir) 2010-04-08 06:04:24 PDT
Created attachment 437829 [details] [diff] [review]
Patch v2.1

Fixing issue from comment 16.
Comment 21 Mounir Lamouri (:mounir) 2010-04-08 06:06:51 PDT
Henri, could you review the patch regarding the parser changes.
And if you have any comment about reftests/mochitests and html5 parser.
Comment 22 Mounir Lamouri (:mounir) 2010-04-08 15:42:41 PDT
Created attachment 437976 [details] [diff] [review]
Tests v3.1

There were typos in this patch.
Comment 23 Henri Sivonen (:hsivonen) 2010-04-09 05:28:28 PDT
> +    /*parent,incl,exclgroups*/          kNone, kNone, kNone,
> +    /*special props, prop-range*/       0,0,

How did you arrive at these values? Specifically, why aren't these values copied from the entry for the span element?

Other than this bit, which I don't know whether to mark as r+ or r-, the parser changes look good to me. However, I'm not qualified to review changes to the old parser. I suggest asking mrbkap to review the parser changes.

Comments about the tests:

> +  o.htmlFor.add('a');
> +  is(o.htmlFor, 'a b c', "'c' token should have been added");

The the failure explanation could be better. E.g. "Nothing should have changed".

English nits: in the mochitest s/reseted/reset/ and in the reftests s/it's/its/
Comment 24 Mounir Lamouri (:mounir) 2010-04-09 11:04:29 PDT
Created attachment 438113 [details] [diff] [review]
Tests v3.2

Fixing nits mentioned by Henri.
Comment 25 Mounir Lamouri (:mounir) 2010-04-09 11:13:15 PDT
(In reply to comment #23)
> > +    /*parent,incl,exclgroups*/          kNone, kNone, kNone,
> > +    /*special props, prop-range*/       0,0,
> 
> How did you arrive at these values? Specifically, why aren't these values
> copied from the entry for the span element?
> 
> Other than this bit, which I don't know whether to mark as r+ or r-, the parser
> changes look good to me. However, I'm not qualified to review changes to the
> old parser. I suggest asking mrbkap to review the parser changes.

I just puts default values. I did not try to have the same values than <span>. Maybe I should actually.
Could you review the html5 parser part ? I mean, just set r+ or r- depending on that. I will ask mrbkap to review the old parser part.

And thank you for your comments :)
Comment 26 Mounir Lamouri (:mounir) 2010-04-11 06:26:20 PDT
Created attachment 438354 [details] [diff] [review]
Tests v3.3

r=smaug

With the changes recommended by Henri, the tests now passed with the old parser. I've removed the check for the html5 parser in the tests.
Comment 27 Mounir Lamouri (:mounir) 2010-04-11 06:30:16 PDT
Created attachment 438355 [details] [diff] [review]
Patch v2.2

I've done the changed recommended by Henri and now the output element is working with the old parser.
I've to admit I did not try that before because I was unable to make the datalist element working with the old parser so I thought I was probably missing something... Looks like with a more simple element it's working easily.

Asking :smaug to review the content, :hsivonen to review the new parser change and :mrbkap to review the old parser change.
Comment 28 Henri Sivonen (:hsivonen) 2010-04-11 23:38:58 PDT
(In reply to comment #25)
> Could you review the html5 parser part ? I mean, just set r+ or r- depending on
> that.

What part do you mean? I see no changes to the HTML5 parser in attachment 438355 [details] [diff] [review].
Comment 29 Mounir Lamouri (:mounir) 2010-04-12 04:53:33 PDT
(In reply to comment #28)
> (In reply to comment #25)
> > Could you review the html5 parser part ? I mean, just set r+ or r- depending on
> > that.
> 
> What part do you mean? I see no changes to the HTML5 parser in attachment
> 438355 [details].

Indeed, there is only a change in the shared header file, AFAIK, which is pretty trivial. I just wanted you to say if what had to be done regarding html5 parser has been done. If you think that's useless just cancel the review request, I will understand ;)
Comment 30 Henri Sivonen (:hsivonen) 2010-04-12 06:26:27 PDT
Comment on attachment 438355 [details] [diff] [review]
Patch v2.2

r=hsivonen on the HTML5 parser-relevant node creation parts.
Comment 31 Olli Pettay [:smaug] 2010-04-14 14:47:54 PDT
Comment on attachment 438355 [details] [diff] [review]
Patch v2.2

If HTML5 doesn't change you need to implement the PutForwards nonsense. Could you file a followup bug for that. It shouldn't be hard to implement.

>+NS_IMETHODIMP
>+nsHTMLOutputElement::Reset()
>+{
>+  nsresult rv = nsContentUtils::SetNodeTextContent(this, mDefaultValue,
>+                                                   PR_TRUE);
>+  mValueModeFlag = eModeDefault;
>+  return rv;
>+}

this is right per the HTML5 draft, but I wonder why the mode is set after updating the value.
The problem is that SetNodeTextContent may fire mutation events which may set .value.
And setting .value sets mode to 'value', but Reset sets mode to default after that.
If also you see this as a problem, could you bring this up in whatwg mailing list. No need to change the code unless the draft is changed.
Comment 32 Mounir Lamouri (:mounir) 2010-04-14 16:29:37 PDT
(In reply to comment #31)
> (From update of attachment 438355 [details] [diff] [review])
> If HTML5 doesn't change you need to implement the PutForwards nonsense. Could
> you file a followup bug for that. It shouldn't be hard to implement.

bug 559463

> >+NS_IMETHODIMP
> >+nsHTMLOutputElement::Reset()
> >+{
> >+  nsresult rv = nsContentUtils::SetNodeTextContent(this, mDefaultValue,
> >+                                                   PR_TRUE);
> >+  mValueModeFlag = eModeDefault;
> >+  return rv;
> >+}
> 
> this is right per the HTML5 draft, but I wonder why the mode is set after
> updating the value.
> The problem is that SetNodeTextContent may fire mutation events which may set
> .value.
> And setting .value sets mode to 'value', but Reset sets mode to default after
> that.
> If also you see this as a problem, could you bring this up in whatwg mailing
> list. No need to change the code unless the draft is changed.

AFAICT, Reset will call SetNodeTextContent which will probably fire an event which may call DescendantsChanged. DescendantsChanged sets the default value value to text content value if the element is in default mode.
I think the mode is changed to default _after_ SetNodeTextContent to prevent setting the node text content with the default value _then_ setting the defalut value with the node text content. I see it like an optimization.
Comment 33 Olli Pettay [:smaug] 2010-04-14 23:45:33 PDT
DescendantsChanged isn't called because of an event.
It is called when DOM is mutated.
But after (or actually also before) DOM mutation a mutation event is fired.
And that event may change the value.

But yeah, I see the problem that Reset sets textContent which then causes
mDefaultValue to be taken from it.
Perhaps the solution is to have intermediate value for mode.
"will set default value", and if after SetNodeTextContent the mode is still that,
make mode to be eModeDefault.
Comment 34 Mounir Lamouri (:mounir) 2010-04-15 05:34:09 PDT
A bug has been open in the w3c bugzilla: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9529

If this is accepted, I will open a follow-up.
Comment 35 Blake Kaplan (:mrbkap) 2010-04-19 18:29:44 PDT
Comment on attachment 438355 [details] [diff] [review]
Patch v2.2

>diff -r 1b4cd3761376 parser/htmlparser/src/nsElementTable.cpp
>+    /*tag*/                             eHTMLTag_output,
>+    /*requiredAncestor*/                eHTMLTag_unknown,eHTMLTag_unknown,
>+    /*rootnodes,endrootnodes*/          &gRootTags,&gRootTags,
>+    /*autoclose starttags and endtags*/ 0,0,0,0,
>+    /*parent,incl,exclgroups*/          kSpecial, (kInlineEntity|kSelf|kFlowEntity), kNone,

What does it mean to have nested <output> elements? i.e. do you need kSelf here? Also, kInlineElement is a part of kFlowEntity, so you don't need it.

r=mrbkap with kSelf addressed one way or the other.
Comment 36 Henri Sivonen (:hsivonen) 2010-04-20 01:55:53 PDT
(In reply to comment #35)
> What does it mean to have nested <output> elements?

The HTML5 parsing algorithm makes nested output elements possible, so the changes to the old parser should, too. What it *means* is a different question.
Comment 37 Mounir Lamouri (:mounir) 2010-04-20 05:18:03 PDT
So I should keep kSelf and remove kFlowEntity ?

Olli, for the reset algorithm, the proposition has been accepted so we can now set the mode to the default mode before setting .textControl. In my opinion, we do not _need_ to prevent the set/get because it will only happen when resetting the form which doesn't happen so much and is punctual.
If you think it's needed, I can add a mIsInReset boolean to prevent the useless |GetNodeTextContent| call.
Comment 38 Olli Pettay [:smaug] 2010-04-20 05:26:43 PDT
yeah, probably mIsInReset isn't needed. If someone uses mutation events, 
all the dom modifications will be slow anyway.
Comment 39 Mounir Lamouri (:mounir) 2010-04-20 08:34:33 PDT
Created attachment 440228 [details] [diff] [review]
Patch v2.3

r=smaug,hsivonen,mrbkap

Changes requested in comment 34 (no follow-up needed actually) and comment 35 are in this patch.
Comment 40 Mounir Lamouri (:mounir) 2010-04-20 13:27:18 PDT
Checkin-needed for tests and patch.
Please ping me on IRC if you have issues while applying this patch.
Comment 41 Mounir Lamouri (:mounir) 2010-04-20 13:27:59 PDT
r=smaug,hsivonen,mrbkap
sr=jst
Comment 42 Mounir Lamouri (:mounir) 2010-04-22 07:03:21 PDT
Created attachment 440752 [details] [diff] [review]
Tests v3.4

r=smaug

This patch has been done against the current tree to make sure it can be applied easily.
Comment 43 Mounir Lamouri (:mounir) 2010-04-22 07:25:18 PDT
Created attachment 440757 [details] [diff] [review]
Patch v2.4

r=smaug,hsivonen,mrbkap sr=jst

This patch has been done against the current tree to make sure it can be
applied easily.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-04-22 22:21:04 PDT
I checked this in but backed it out because it caused numerous test failures related to the editor. I confirmed by backing it out locally that this patch was responsible. E.g.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271994893.1271995195.18608.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271994893.1271995559.19350.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271995866.1271997305.23141.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271994893.1271995424.19125.gz
Comment 45 Mounir Lamouri (:mounir) 2010-04-25 17:34:56 PDT
Created attachment 441410 [details] [diff] [review]
Patch v2.5

This patch is fixing the mentioned errors. It was because the output tag wasn't known by the editor.
Comment 46 Mounir Lamouri (:mounir) 2010-04-25 17:36:58 PDT
Comment on attachment 441410 [details] [diff] [review]
Patch v2.5

Josh, could you review the editor changes in this patch ? They are minor: 5 or 6 lines.
Comment 47 timeless 2010-04-26 01:15:18 PDT
Comment on attachment 441410 [details] [diff] [review]
Patch v2.5

presumably someone else is reviewing class nsDOMSettableTokenList

this insertion is kinda interesting:
>-#define NS_FORM_LEGEND         18
>-#define NS_FORM_SELECT         19
>-#define NS_FORM_TEXTAREA       20
>-#define NS_FORM_OBJECT         21
>+#define NS_FORM_OUTPUT         18
>+#define NS_FORM_LEGEND         19
>+#define NS_FORM_SELECT         20
>+#define NS_FORM_TEXTAREA       21
>+#define NS_FORM_OBJECT         22

> nsGenericHTMLFormElement::CanBeDisabled() const
should this match the order of the values of the defines, or are we favoring things based on current usage? (if so, i'd bet that fieldset or legend are going to be less common..)
>   return
>     type != NS_FORM_LABEL &&
>     type != NS_FORM_LEGEND &&
>     type != NS_FORM_FIELDSET &&
>-    type != NS_FORM_OBJECT;
>+    type != NS_FORM_OBJECT &&
>+    type != NS_FORM_OUTPUT;

I don't think we usually use this style for whitespace:
>+  void CharacterDataChanged(nsIDocument* aDocument,
>+  void ContentAppended     (nsIDocument* aDocument,
>+  void ContentInserted     (nsIDocument* aDocument,
>+  void ContentRemoved      (nsIDocument* aDocument,

and why not align all of these:
>+  ValueModeFlag  mValueModeFlag;
>+  nsString       mDefaultValue;
>+  nsRefPtr<nsDOMSettableTokenList> mTokenList;
>+};

it's interesting to see that this is a void (not your problem, it just seems wrong):
>+  AddMutationObserver(this);

i don't think this comment is needed:
>+// QueryInterface implementation for nsHTMLOutputElement

if we haven't turned on infallible malloc, then there should be an ifaddref marker to indicate oom handling:
>+nsHTMLOutputElement::GetHtmlFor(nsIDOMDOMSettableTokenList** aResult)
>+    mTokenList = new nsDOMSettableTokenList(this, nsGkAtoms::_for);
>+  NS_ADDREF(*aResult = mTokenList);

>+interface nsIDOMHTMLOutputElement : nsIDOMHTMLElement
>+   * The next attributes depends on the constraint validation API.

'depend'

you formally probably need roc to review this:
>@@ -230,16 +230,18 @@ EmbedContextMenuInfo::SetFormControlType

the editor changes are fine
Comment 48 Mounir Lamouri (:mounir) 2010-04-26 02:45:29 PDT
Comment on attachment 441410 [details] [diff] [review]
Patch v2.5

Asking roc to review the |EmbedContextMenuInfo::SetFormControlType| changes per previous comment.
Comment 49 Mounir Lamouri (:mounir) 2010-04-26 02:55:05 PDT
(In reply to comment #47)
> this insertion is kinda interesting:
> >-#define NS_FORM_LEGEND         18
> >-#define NS_FORM_SELECT         19
> >-#define NS_FORM_TEXTAREA       20
> >-#define NS_FORM_OBJECT         21
> >+#define NS_FORM_OUTPUT         18
> >+#define NS_FORM_LEGEND         19
> >+#define NS_FORM_SELECT         20
> >+#define NS_FORM_TEXTAREA       21
> >+#define NS_FORM_OBJECT         22

I prefer having elements in alphabetic order. It makes the change more verbose but it makes it easier to found something in the list after.

> > nsGenericHTMLFormElement::CanBeDisabled() const
> should this match the order of the values of the defines, or are we favoring
> things based on current usage? (if so, i'd bet that fieldset or legend are
> going to be less common..)
> >   return
> >     type != NS_FORM_LABEL &&
> >     type != NS_FORM_LEGEND &&
> >     type != NS_FORM_FIELDSET &&
> >-    type != NS_FORM_OBJECT;
> >+    type != NS_FORM_OBJECT &&
> >+    type != NS_FORM_OUTPUT;

I don't know if there is a convention.

> I don't think we usually use this style for whitespace:
> >+  void CharacterDataChanged(nsIDocument* aDocument,
> >+  void ContentAppended     (nsIDocument* aDocument,
> >+  void ContentInserted     (nsIDocument* aDocument,
> >+  void ContentRemoved      (nsIDocument* aDocument,
> 
> and why not align all of these:
> >+  ValueModeFlag  mValueModeFlag;
> >+  nsString       mDefaultValue;
> >+  nsRefPtr<nsDOMSettableTokenList> mTokenList;
> >+};
> 
> it's interesting to see that this is a void (not your problem, it just seems
> wrong):
> >+  AddMutationObserver(this);
> 
> i don't think this comment is needed:
> >+// QueryInterface implementation for nsHTMLOutputElement
> 
> if we haven't turned on infallible malloc, then there should be an ifaddref
> marker to indicate oom handling:
> >+nsHTMLOutputElement::GetHtmlFor(nsIDOMDOMSettableTokenList** aResult)
> >+    mTokenList = new nsDOMSettableTokenList(this, nsGkAtoms::_for);
> >+  NS_ADDREF(*aResult = mTokenList);
> 
> >+interface nsIDOMHTMLOutputElement : nsIDOMHTMLElement
> >+   * The next attributes depends on the constraint validation API.
> 
> 'depend'

I will fix what need to be fixed in this list.

> you formally probably need roc to review this:
> >@@ -230,16 +230,18 @@ EmbedContextMenuInfo::SetFormControlType
> 
> the editor changes are fine

Thank you for your quick review :)
Comment 50 Mounir Lamouri (:mounir) 2010-04-26 02:58:07 PDT
For information, I've sent the attached patch to the try server and it went fine.
Comment 51 Mounir Lamouri (:mounir) 2010-04-26 03:42:24 PDT
Created attachment 441457 [details] [diff] [review]
Patch v2.6

r=smaug,hsivonen,mrbkap,timeless,roc sr=jst

Tests and patch are ready to be landed.
Comment 53 Eric Shepherd [:sheppy] 2010-06-15 13:14:25 PDT
Documented:

https://developer.mozilla.org/en/HTML/Element/Output
Comment 54 Nochum Sossonko [:Natch] 2010-09-21 10:33:29 PDT
What is the element intended for? I'm trying to see what it does, but all I can find wherever I go is: "The output (<output>) element represents the result of a calculation.". I have no idea what that's supposed to mean, and MDC's docs on this are not very descriptive, nor do they contain any demos/samples.

Any help here would be greatly appreciated.
Comment 55 Unknown W. Brackets 2010-09-21 21:12:32 PDT
Basically, the element is mostly semantic, and form-associated so it resets when a form is reset.

http://dev.w3.org/html5/spec/Overview.html#the-output-element

It has a value and defaultValue like other form things.  Which is all doc'd on mdc here:

https://developer.mozilla.org/en/DOM/HTMLOutputElement

But the two docs aren't interlinked for some reason?  Hope that explains things.

-[Unknown]

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