640947 - We should QI to nsIFormControl instead of checking the tag name in nsHTMLEditUtils::IsFormWidget

Status: RESOLVED WONTFIX

(Core :: DOM: Editor, defect)

Description

[Mounir Lamouri (:mounir)]

...and exclude <object>.

This has been proposed by Olli in bug 514437.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 518887 [details] [diff] [review]
Patch (v1)

>-PRBool 
>+PRBool
> nsHTMLEditUtils::IsFormWidget(nsIDOMNode *node)
> {
>   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::input);
>+  // Anything that is a form control is acceptable, unless it's an <object>
>+  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(node);
>+  nsCOMPtr<nsIDOMHTMLObjectElement> object = do_QueryInterface(node);
>+  return formControl && !object;

What about:
nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(node);
return formControl && formControl->GetType() != NS_FORM_OBJECT;

Comment 3

[(no longer active)]

(In reply to comment #2)
> Comment on attachment 518887 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=518887
> Patch (v1)
> 
> >-PRBool 
> >+PRBool
> > nsHTMLEditUtils::IsFormWidget(nsIDOMNode *node)
> > {
> >   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::input);
> >+  // Anything that is a form control is acceptable, unless it's an <object>
> >+  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(node);
> >+  nsCOMPtr<nsIDOMHTMLObjectElement> object = do_QueryInterface(node);
> >+  return formControl && !object;
> 
> What about:
> nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(node);
> return formControl && formControl->GetType() != NS_FORM_OBJECT;

I don't see the point, really.  I like this style better, but I won't object if you want to write the alternate version, unless roc r-minuses this.  :-)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's go with comment #2, it's slightly shorter and faster.

Comment 5

[(no longer active)]

Attachment: Patch (v2)

Comment 6

[Mounir Lamouri (:mounir)]

With this patch, this method is going to return true when the node is a fieldset or a label and false when the node is a keygen. Is that expected? I mean, according to the comment it is but I guess double-checking might be interesting.

Comment 7

[(no longer active)]

(In reply to comment #6)
> With this patch, this method is going to return true when the node is a
> fieldset or a label and false when the node is a keygen. Is that expected? I
> mean, according to the comment it is but I guess double-checking might be
> interesting.

Oh, good thing you mentioned this.  This method is used as part of series of checks to figure out if we can inject something as a child of a node or not.  The rule is that if this method returns true, then we shouldn't inject anything as a child of this method.  In that sense, this is bad if we're dealing with a fieldset node, and also bad if we're dealing with a keygen node.

Which brings me to a more general problem that I've seen in the editor code for a long time.  The general problem is that the editor tries to figure out the content model rules (for example, see the "content model" section here <http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fieldset-element>) on its own, rather than asking the content.  It should be the responsibility of the content node to determine whether it can include a given node with a tag name as is child, for example.  Basically, we need to encode the content model of the HTML5 section using an API on nsIContent rather than expecting the editor (me) to catch up on every new HTML5 element.  ;-)  (CCing some of the content folks to keep them in the loop about this.)

I'm now leaning towards WONTFIXing this bug, and asking you to just hack around this in bug 514437 until we have a real nsIContent API which the editor can use.

(We might have something to this effect in the HTML5 parser.  CCing Henri to lead me to it if that's the case.)

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to comment #7)
> (In reply to comment #6)
> > With this patch, this method is going to return true when the node is a
> > fieldset or a label and false when the node is a keygen. Is that expected? I
> > mean, according to the comment it is but I guess double-checking might be
> > interesting.
> 
> Oh, good thing you mentioned this.  This method is used as part of series of
> checks to figure out if we can inject something as a child of a node or not. 
> The rule is that if this method returns true, then we shouldn't inject anything
> as a child of this method.  In that sense, this is bad if we're dealing with a
> fieldset node, and also bad if we're dealing with a keygen node.

Surely at this point in time, we won't have keygen nodes, because bug 101019 remains unfixed. When bug 101019 is fixed, I'd expect keygen to quack like a form control.

> Basically, we need to encode the content model
> of the HTML5 section using an API on nsIContent rather than expecting the
> editor (me) to catch up on every new HTML5 element.  ;-)

Putting this on nsIContent makes sense to me.

> (We might have something to this effect in the HTML5 parser.  CCing Henri to
> lead me to it if that's the case.)

We don't. The HTML5 parser implements the tree builder as specced, and the spec doesn't try to generalize queryable traits of the elements. There are just big switch blocks in the spec, so there are big switch blocks in the parser.

The objects that represent well-known HTML element names in the parser have a bitfield with some trait bits, but at the moment, there aren't any containment-related bits there.

Also, in general, you can't infer validity containment rules from the parsing algorithm.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand why putting it on nsIContent makes sense.

Does any code other than editor care about this information?

It seems much harder to maintain this info by spreading it out over dozens of classes rather than keeping it in a central hash table or some such.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree that putting it in its own file would make sense.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(somewhere in content)

Comment 12

[(no longer active)]

To correct myself, I should have said "content API" rather than "nsIContent API".

(In reply to comment #9)
> Does any code other than editor care about this information?

Maybe not, but I think it should still be part of content/, rather than editor/.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok. I'm fine with having the implementation live in content/.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to comment #7)
> Which brings me to a more general problem that I've seen in the editor code for
> a long time.  The general problem is that the editor tries to figure out the
> content model rules (for example, see the "content model" section here
> <http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fieldset-element>)
> on its own, rather than asking the content.  It should be the responsibility of
> the content node to determine whether it can include a given node with a tag
> name as is child, for example.  Basically, we need to encode the content model
> of the HTML5 section using an API on nsIContent rather than expecting the
> editor (me) to catch up on every new HTML5 element.  ;-)  (CCing some of the
> content folks to keep them in the loop about this.)

Filed bug 651103.