Last Comment Bug 844127 - Stop using the HTMLSpanElement interface for bgsound, multicol, image
: Stop using the HTMLSpanElement interface for bgsound, multicol, image
Status: RESOLVED FIXED
[mentor=bz][lang=c++][good first bug]
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Sunny [:darkowlzz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-22 07:56 PST by Anne (:annevk)
Modified: 2013-05-16 11:51 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replaced "Span" with "Unknown" and made changes in the test file (2.72 KB, patch)
2013-03-04 22:42 PST, Sunny [:darkowlzz]
no flags Details | Diff | Review
Replaced "Span" with "Unknown" and made changes in the test file (3.89 KB, patch)
2013-03-04 23:03 PST, Sunny [:darkowlzz]
no flags Details | Diff | Review
Replaced "Span" with "Unknown" and made changes in the test file (3.91 KB, patch)
2013-03-05 01:31 PST, Sunny [:darkowlzz]
bzbarsky: feedback+
Details | Diff | Review
Replaced "Span" with "Unknown" and added a test file (5.71 KB, patch)
2013-03-11 10:25 PDT, Sunny [:darkowlzz]
no flags Details | Diff | Review
Replaced "Span" with "Unknown" and added a test file (5.74 KB, patch)
2013-03-12 08:47 PDT, Sunny [:darkowlzz]
no flags Details | Diff | Review
Replaced "Span" with "Unknown" and added a test file (7.30 KB, patch)
2013-03-12 10:31 PDT, Sunny [:darkowlzz]
bzbarsky: feedback+
Details | Diff | Review
Replaced "Span" with "Unknown" and added a test file (12.05 KB, patch)
2013-03-12 11:45 PDT, Sunny [:darkowlzz]
bzbarsky: review+
Details | Diff | Review

Description Anne (:annevk) 2013-02-22 07:56:09 PST
We should stop using HTMLSpanElement for everything. See bug 843881 comment 4.
Comment 1 Amol Pol 2013-02-25 09:01:11 PST
Can you please explain the bug in detail.
Comment 2 Boris Zbarsky [:bz] 2013-02-26 12:20:38 PST
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) 2013-02-26 14:28:07 PST
The tests will be there: content/html/content/test/test_bug389797.html
Comment 4 Anne (:annevk) 2013-02-26 22:58:34 PST
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 [:bz] 2013-02-26 23:02:01 PST
> 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] 2013-03-04 22:42:10 PST
Created attachment 721075 [details] [diff] [review]
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] 2013-03-04 23:03:45 PST
Created attachment 721081 [details] [diff] [review]
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) 2013-03-05 00:43:25 PST
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] 2013-03-05 01:31:46 PST
Created attachment 721118 [details] [diff] [review]
Replaced "Span" with "Unknown" and made changes in the test file

Made changes suggested by Mounir.
Comment 10 Boris Zbarsky [:bz] 2013-03-05 05:28:44 PST
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] 2013-03-11 10:25:11 PDT
Created attachment 723541 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

Added test_bug844127.html
Comment 12 Mounir Lamouri (:mounir) 2013-03-11 18:08:30 PDT
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] 2013-03-12 05:57:16 PDT
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] 2013-03-12 08:47:56 PDT
Created attachment 723959 [details] [diff] [review]
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) 2013-03-12 09:13:02 PDT
I recommend updating the tests for the new pass condition and maybe adding a pointer to this bug.
Comment 16 Boris Zbarsky [:bz] 2013-03-12 09:35:54 PDT
> 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 [:bz] 2013-03-12 09:54:13 PDT
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] 2013-03-12 10:31:52 PDT
Created attachment 724014 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

Made the suggested changes and resolved all mochitest errors.
Comment 19 Boris Zbarsky [:bz] 2013-03-12 11:11:10 PDT
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] 2013-03-12 11:45:42 PDT
Created attachment 724060 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

There you go! Hope I didn't miss anything this time :)
Comment 21 Boris Zbarsky [:bz] 2013-03-12 13:08:30 PDT
Comment on attachment 724060 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

r=me
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-03-12 13:38:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a478fe3e06
Comment 23 Ed Morley [:emorley] 2013-03-13 05:38:32 PDT
https://hg.mozilla.org/mozilla-central/rev/76a478fe3e06
Comment 24 :Ms2ger 2013-03-14 05:39:48 PDT
So why isn't 'image' Unknown?
Comment 25 Boris Zbarsky [:bz] 2013-03-14 11:10:18 PDT
Because it's being dealt with in bug 843881.  No point thrashing it here....
Comment 26 Kohei Yoshino [:kohei] 2013-03-29 10:12:57 PDT
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 28 Boris Zbarsky [:bz] 2013-04-29 08:08:32 PDT
<sigh>.  Fixed that just like I fixed the changes from comment 26...  Also fixing the summary to make it clearer what changed.

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