The default bug view has changed. See this FAQ.

Stop using the HTMLSpanElement interface for bgsound, multicol, image

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: annevk, Assigned: darkowlzz)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla22
dev-doc-complete, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=c++][good first bug])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
We should stop using HTMLSpanElement for everything. See bug 843881 comment 4.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 1

4 years ago
Can you please explain the bug in detail.
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.
The tests will be there: content/html/content/test/test_bug389797.html
(Reporter)

Comment 4

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

Updated

4 years ago
(Assignee)

Comment 6

4 years ago
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.
Attachment #721075 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 7

4 years ago
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.
Attachment #721075 - Attachment is obsolete: true
Attachment #721075 - Flags: feedback?(bzbarsky)
Attachment #721081 - Flags: feedback?(bzbarsky)
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", "");
(Assignee)

Comment 9

4 years ago
Created attachment 721118 [details] [diff] [review]
Replaced "Span" with "Unknown" and made changes in the test file

Made changes suggested by Mounir.
Attachment #721081 - Attachment is obsolete: true
Attachment #721081 - Flags: feedback?(bzbarsky)
Attachment #721118 - Flags: feedback?(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?
Attachment #721118 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 723541 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

Added test_bug844127.html
Attachment #721118 - Attachment is obsolete: true
Attachment #723541 - Flags: feedback?(bzbarsky)
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 ;)
(Assignee)

Comment 13

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

Comment 14

4 years ago
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)
Attachment #723541 - Attachment is obsolete: true
Attachment #723541 - Flags: feedback?(bzbarsky)
Attachment #723959 - Flags: feedback?(bzbarsky)
(Reporter)

Comment 15

4 years ago
I recommend updating the tests for the new pass condition and maybe adding a pointer to this bug.
> http://www.pastebin.mozilla.org/2212114 (mochitest errors)

Is that with attachment 723959 [details] [diff] [review]?  Because if so, something is broken....
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.
(Assignee)

Comment 18

4 years ago
Created attachment 724014 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

Made the suggested changes and resolved all mochitest errors.
Attachment #723959 - Attachment is obsolete: true
Attachment #724014 - Flags: feedback?(bzbarsky)
Attachment #723959 - Flags: feedback?(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.
Attachment #724014 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 20

4 years ago
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 :)
Attachment #724014 - Attachment is obsolete: true
Attachment #724060 - Flags: review?(bzbarsky)
Comment on attachment 724060 [details] [diff] [review]
Replaced "Span" with "Unknown" and added a test file

r=me
Attachment #724060 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a478fe3e06
Assignee: nobody → indiasuny000
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76a478fe3e06
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
So why isn't 'image' Unknown?
Because it's being dealt with in bug 843881.  No point thrashing it here....
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

Updated

4 years ago
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
<sigh>.  Fixed that just like I fixed the changes from comment 26...  Also fixing the summary to make it clearer what changed.
Summary: Stop defaulting to HTMLSpanElement → Stop defaulting to HTMLSpanElement for bgsound, multicol, image
Summary: Stop defaulting to HTMLSpanElement for bgsound, multicol, image → Stop usign the HTMLSpanElement interface for bgsound, multicol, image
Summary: Stop usign the HTMLSpanElement interface for bgsound, multicol, image → Stop using the HTMLSpanElement interface for bgsound, multicol, image

Updated

4 years ago
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.