844127 - Stop using the HTMLSpanElement interface for bgsound, multicol, image

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

We should stop using HTMLSpanElement for everything. See bug 843881 comment 4.

Comment 1

[Amol Pol]

Can you please explain the bug in detail.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sure.  Right now, when you create an HTML element via createElement or via the parser, it can end up with HTMLSpanElement as its DOM interface, even if it's not a <span>.

The elements this happens for are the ones whose entries in http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/public/nsHTMLTagList.h have "Span" as the second argument to HTML_TAG.

So what this bug is about is going through those, figuring out what interface they should be using, and fixing them accordingly.  For example, according to http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#other-elements,-attributes-and-apis :

  The bgsound, isindex, multicol, nextid, rb, and spacer elements must use the
  HTMLUnknownElement interface.

Presumably just replacing "Span" with "Unknown" for those elements will do the right thing.  Plus adding tests, of course.

For basefont the spec apparently calls for a separate interface; we'd need to create an interface and an implementation class for that..... or something.  Given that we don't do anything useful with basefont elements, you could just map them to HTMLUnknownElement for now too.

For "image" you probably want HTML_HTMLELEMENT_TAG() instead of HTML_TAG.

Don't worry about keygen for now.

That should cover all of them.

Comment 3

[Mounir Lamouri (:mounir)]

The tests will be there: content/html/content/test/test_bug389797.html

Comment 4

[Anne (:annevk)]

HTMLUnknownElement is also the default for unknown elements so maybe bgsound, insindex, et al can be handled that way.

We should use HTMLUnknownElement for basefont too. We removed it from the codebase at some point to get it removed from HTML eventually.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> HTMLUnknownElement is also the default for unknown elements so maybe bgsound, insindex,
> et al can be handled that way.

That's possible too; might be higher risk patch.

Good to hear about the basefont part of the spec being fiction!  Wish the spec said so.  ;)

Comment 6

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and made changes in the test file

I would like to work on this bug.

Submitting a patch.
Please check for any improvements.

Comment 7

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and made changes in the test file

Replaced HTML_TAG() for image with HTML_HTMLELEMENT_TAG() and removed the corresponding HTML_TAG() from the test file.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 721081 [details] [diff] [review]
Replaced "Span" with "Unknown" and made changes in the test file

Review of attachment 721081 [details] [diff] [review]:
-----------------------------------------------------------------

Quick drive-by comment.

::: content/html/content/test/test_bug389797.html
@@ -158,5 @@
>  HTML_TAG("html", "Html");
>  HTML_TAG("i", "");
>  HTML_TAG("iframe", "IFrame", [ "nsIDOMGetSVGDocument", "nsIDOMMozBrowserFrame" ],
>                               [ "nsIFrameLoaderOwner" ]);
> -HTML_TAG("image", "Span");

shouldn't that be:
HTML_TAG("image", "");

Comment 9

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and made changes in the test file

Made changes suggested by Mounir.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 721118 [details] [diff] [review]
Replaced "Span" with "Unknown" and made changes in the test file

This looks reasonable.  Can you also add a test that makes sure that those elements end up with the right prototype when parsed or created via createElement?

Comment 11

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and added a test file

Added test_bug844127.html

Comment 12

[Mounir Lamouri (:mounir)]

FWIW, I think it is preferable to not use bug numbers for test names but real names. The only cases where bug numbers could make sense is when this is really testing an edge case that was found in a bug report. I know most tests use bug numbers but they are most of the time wrong to do so ;)

Comment 13

[Sunny [:darkowlzz]]

After running the mochi-test on the build with all the changes, there seems to be many fail cases
http://www.pastebin.mozilla.org/2211811

Comment 14

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and added a test file

Changed test file name to |test_element_prototype.html| as suggested by mounir.

Made a few changes to |test_element_prototype.html| which resolved the fails on mochitest.
Now mochitest throws some errors which are based on this line and the lines below it
http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug389797.html?force=1#247

What can be done to resolve these errors? Remove those test code?
http://www.pastebin.mozilla.org/2212114 (mochitest errors)

Comment 15

[Anne (:annevk)]

I recommend updating the tests for the new pass condition and maybe adding a pointer to this bug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> http://www.pastebin.mozilla.org/2212114 (mochitest errors)

Is that with attachment 723959 [details] [diff] [review]?  Because if so, something is broken....

Comment 17

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/file/519c7cf5605b/content/html/content/test/test_bug389797.html#l247 needs to be adjusted for cases when classInfoString is "HTMLUnknownElement".

Also, the other two tests that use HTML_TAG should be fixed.

Comment 18

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and added a test file

Made the suggested changes and resolved all mochitest errors.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724014 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

Looks good.  What about the rest of comment 17?

Also, it's probably better to have the commit comment actually describe what you're doing and include the reviewer info.

Comment 20

[Sunny [:darkowlzz]]

Attachment: Replaced "Span" with "Unknown" and added a test file

There you go! Hope I didn't miss anything this time :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724060 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

r=me

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a478fe3e06

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76a478fe3e06

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So why isn't 'image' Unknown?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Because it's being dealt with in bug 843881.  No point thrashing it here....

Comment 26

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Also, please update the following doc:
https://developer.mozilla.org/en-US/docs/DOM/document.createElement

Comment 27

[Florian Scholz (Open Web Docs)]

Updated:
https://developer.mozilla.org/en-US/docs/DOM/document.createElement
Also mentioned on:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/22#DOM

Comment 28

[Boris Zbarsky [:bzbarsky]]

<sigh>.  Fixed that just like I fixed the changes from comment 26...  Also fixing the summary to make it clearer what changed.