Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: WeirdAl, Assigned: mounir)

Tracking

(Blocks: 2 bugs, {dev-doc-complete, html5})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 15 obsolete attachments)

14.42 KB, patch
Details | Diff | Splinter Review
43.46 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

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

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
(Assignee)

Updated

8 years ago
Keywords: html5
OS: Windows XP → All
Hardware: x86 → All
Summary: Implement Web Forms 2 <html:output/> → Implement output element
(Assignee)

Comment 2

8 years ago
Created attachment 436693 [details] [diff] [review]
Tests - WIP
Assignee: ajvincent → mounir.lamouri
Status: NEW → ASSIGNED
(Assignee)

Comment 3

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

Comment 4

8 years ago
Created attachment 437011 [details] [diff] [review]
Tests v1
Attachment #436693 - Attachment is obsolete: true
Attachment #437011 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

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

Updated

8 years ago
Attachment #437012 - Flags: review?(Olli.Pettay) → review-

Comment 6

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

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

8 years ago
Comment on attachment 437011 [details] [diff] [review]
Tests v1

Using form.formData, testing submission should be really easy.
Attachment #437011 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 9

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

Comment 10

8 years ago
CCing David for a11y.
(Assignee)

Updated

8 years ago
Keywords: dev-doc-needed

Comment 11

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

Comment 12

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

Comment 13

8 years ago
Created attachment 437810 [details] [diff] [review]
Patch v2
Attachment #437012 - Attachment is obsolete: true
Attachment #437810 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 14

8 years ago
(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 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.
Attachment #437809 - Flags: review?(Olli.Pettay) → review-
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?

Updated

8 years ago
Blocks: 558036

Comment 17

8 years ago
I filed bug 558036 for HTML output accessibility support. Let's move a11y discussion there.
(Assignee)

Comment 18

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

Comment 19

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

Comment 20

8 years ago
Created attachment 437829 [details] [diff] [review]
Patch v2.1

Fixing issue from comment 16.
Attachment #437810 - Attachment is obsolete: true
Attachment #437829 - Flags: review?(Olli.Pettay)
Attachment #437810 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

8 years ago
Attachment #437829 - Flags: review?(hsivonen)
(Assignee)

Comment 21

8 years ago
Henri, could you review the patch regarding the parser changes.
And if you have any comment about reftests/mochitests and html5 parser.
(Assignee)

Comment 22

8 years ago
Created attachment 437976 [details] [diff] [review]
Tests v3.1

There were typos in this patch.
Attachment #437828 - Attachment is obsolete: true
Attachment #437976 - Flags: review?(Olli.Pettay)
Attachment #437828 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Attachment #437976 - Flags: review?(Olli.Pettay) → review+
> +    /*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/
(Assignee)

Comment 24

8 years ago
Created attachment 438113 [details] [diff] [review]
Tests v3.2

Fixing nits mentioned by Henri.
Attachment #437976 - Attachment is obsolete: true
(Assignee)

Comment 25

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

Comment 26

8 years ago
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.
Attachment #438113 - Attachment is obsolete: true
(Assignee)

Comment 27

8 years ago
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.
Attachment #437829 - Attachment is obsolete: true
Attachment #438355 - Flags: review?(Olli.Pettay)
Attachment #437829 - Flags: review?(hsivonen)
Attachment #437829 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

8 years ago
Attachment #438355 - Flags: review?(hsivonen)
(Assignee)

Updated

8 years ago
Attachment #438355 - Flags: review?(mrbkap)
(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].
(Assignee)

Comment 29

8 years ago
(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 on attachment 438355 [details] [diff] [review]
Patch v2.2

r=hsivonen on the HTML5 parser-relevant node creation parts.
Attachment #438355 - Flags: review?(hsivonen) → review+
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.
Attachment #438355 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

8 years ago
Blocks: 559463
(Assignee)

Comment 32

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

Updated

8 years ago
Attachment #438355 - Flags: superreview?(jst)
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.
(Assignee)

Comment 34

8 years ago
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 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.
Attachment #438355 - Flags: review?(mrbkap) → review+
(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.
(Assignee)

Comment 37

7 years ago
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.
yeah, probably mIsInReset isn't needed. If someone uses mutation events, 
all the dom modifications will be slow anyway.
(Assignee)

Comment 39

7 years ago
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.
Attachment #438355 - Attachment is obsolete: true
Attachment #440228 - Flags: superreview?(jst)
Attachment #438355 - Flags: superreview?(jst)

Updated

7 years ago
Attachment #440228 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 40

7 years ago
Checkin-needed for tests and patch.
Please ping me on IRC if you have issues while applying this patch.
Keywords: checkin-needed
(Assignee)

Comment 41

7 years ago
r=smaug,hsivonen,mrbkap
sr=jst
(Assignee)

Comment 42

7 years ago
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.
Attachment #438354 - Attachment is obsolete: true
(Assignee)

Comment 43

7 years ago
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.
Attachment #440228 - Attachment is obsolete: true
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
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Blocks: 561635
(Assignee)

Comment 45

7 years ago
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.
Attachment #440757 - Attachment is obsolete: true
(Assignee)

Comment 46

7 years ago
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.
Attachment #441410 - Flags: review?(timeless)

Comment 47

7 years ago
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
Attachment #441410 - Flags: review?(timeless) → review+
(Assignee)

Comment 48

7 years ago
Comment on attachment 441410 [details] [diff] [review]
Patch v2.5

Asking roc to review the |EmbedContextMenuInfo::SetFormControlType| changes per previous comment.
Attachment #441410 - Flags: review?(roc)
(Assignee)

Comment 49

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

Comment 50

7 years ago
For information, I've sent the attached patch to the try server and it went fine.
Attachment #441410 - Flags: review?(roc) → review+
(Assignee)

Comment 51

7 years ago
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.
Attachment #441410 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5c4ecf53b751
http://hg.mozilla.org/mozilla-central/rev/9e373d1c6707
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Flags: in-testsuite+

Updated

7 years ago
Blocks: 566348
Documented:

https://developer.mozilla.org/en/HTML/Element/Output
Keywords: dev-doc-needed → dev-doc-complete
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

7 years ago
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]
You need to log in before you can comment on or make changes to this bug.