Bug 162353 – Fix serialization of bgsound, keygen and track elements

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Fix serialization of keygen and track elements to match the specification:
- https://html.spec.whatwg.org/#serialising-html-fragments

They are not supposed to have an end tag. Firefox and Chrome agree with the specification.

[Chris Dumez]

Created attachment 289474 [details]
Patch

[Darin Adler]

Comment on attachment 289474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289474&action=review

Does this leave us with sufficient test coverage? I can’t easily see the difference between this list and the ieForbidsInsertHTML list.

> Source/WebCore/editing/MarkupAccumulator.cpp:615
> +    return element.hasTagName(areaTag)
> +        || element.hasTagName(baseTag)
> +        || element.hasTagName(basefontTag)
> +        || element.hasTagName(bgsoundTag)
> +        || element.hasTagName(brTag)
> +        || element.hasTagName(colTag)
> +        || element.hasTagName(embedTag)
> +        || element.hasTagName(frameTag)
> +        || element.hasTagName(hrTag)
> +        || element.hasTagName(imgTag)
> +        || element.hasTagName(inputTag)
> +        || element.hasTagName(keygenTag)
> +        || element.hasTagName(linkTag)
> +        || element.hasTagName(metaTag)
> +        || element.hasTagName(paramTag)
> +        || element.hasTagName(sourceTag)
> +        || element.hasTagName(trackTag)
> +        || element.hasTagName(wbrTag);

An unrolled set of checks like this might be less efficient than using an array of the tags with a loop when there are so many tags involved; it might even be worth considering a HashSet, but probably not.

[Chris Dumez]

Comment on attachment 289474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289474&action=review

>> Source/WebCore/editing/MarkupAccumulator.cpp:615
>> +        || element.hasTagName(wbrTag);
> 
> An unrolled set of checks like this might be less efficient than using an array of the tags with a loop when there are so many tags involved; it might even be worth considering a HashSet, but probably not.

Compared to ieForbidsInsertHTML(), this list adds bgsound, keygen, and track. It removes <image>.
-> We have newly passing tests for bgsound, keygen, and track.
-> For <image>, it is an SVGElement, not an HTMLElement. Therefore, it would have returned early at the beginning of this function.

I'll look into using an array / loop.

[Darin Adler]

Comment on attachment 289474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289474&action=review

>>> Source/WebCore/editing/MarkupAccumulator.cpp:615
>>> +        || element.hasTagName(wbrTag);
>> 
>> An unrolled set of checks like this might be less efficient than using an array of the tags with a loop when there are so many tags involved; it might even be worth considering a HashSet, but probably not.
> 
> Compared to ieForbidsInsertHTML(), this list adds bgsound, keygen, and track. It removes <image>.
> -> We have newly passing tests for bgsound, keygen, and track.
> -> For <image>, it is an SVGElement, not an HTMLElement. Therefore, it would have returned early at the beginning of this function.
> 
> I'll look into using an array / loop.

Sounds good.

I know it’s not normal, but are you sure you can’t create an element with the HTML namespace but with the tag "image"? I guess there is some trick that converts "image" into imgTag during normal parsing. And I suppose this is for HTML, not XML documents, but still wondering if there is some way to create such an element.

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 289474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289474&action=review
> 
> >>> Source/WebCore/editing/MarkupAccumulator.cpp:615
> >>> +        || element.hasTagName(wbrTag);
> >> 
> >> An unrolled set of checks like this might be less efficient than using an array of the tags with a loop when there are so many tags involved; it might even be worth considering a HashSet, but probably not.
> > 
> > Compared to ieForbidsInsertHTML(), this list adds bgsound, keygen, and track. It removes <image>.
> > -> We have newly passing tests for bgsound, keygen, and track.
> > -> For <image>, it is an SVGElement, not an HTMLElement. Therefore, it would have returned early at the beginning of this function.
> > 
> > I'll look into using an array / loop.
> 
> Sounds good.
> 
> I know it’s not normal, but are you sure you can’t create an element with
> the HTML namespace but with the tag "image"? I guess there is some trick
> that converts "image" into imgTag during normal parsing. And I suppose this
> is for HTML, not XML documents, but still wondering if there is some way to
> create such an element.

Sure, it is possible to do document.createElement("image") and get an HTMLUnknownElement that the tag image. I have likely changed the behavior in such case (although the behavior change is a good thing here). I'll add test coverage and make sure other browsers pass this test).

[Chris Dumez]

Created attachment 289510 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 289510 [details]:

fast/images/move-image-to-new-document.html bug 162372 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Build Bot]

Comment on attachment 289510 [details]
Patch

Attachment 289510 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2122770

New failing tests:
fast/images/move-image-to-new-document.html

[Build Bot]

Created attachment 289516 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Chris Dumez]

Comment on attachment 289510 [details]
Patch

Clearing flags on attachment: 289510

Committed r206246: <http://trac.webkit.org/changeset/206246>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.