Bug 133336 – Ignore usemap attributes without '#' in img element

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jinwoo Song]

HTML5 specification says we should ignore usemap attributes without #.
http://www.w3.org/TR/html5/infrastructure.html#valid-hash-name-reference

[Jinwoo Song]

Created attachment 232166 [details]
Patch

[Build Bot]

Comment on attachment 232166 [details]
Patch

Attachment 232166 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5169700786929664

New failing tests:
fast/images/image-map-multiple-xhtml.xhtml

[Build Bot]

Created attachment 232171 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232166 [details]
Patch

Attachment 232166 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4660278742482944

New failing tests:
fast/images/image-map-multiple-xhtml.xhtml

[Build Bot]

Created attachment 232174 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jinwoo Song]

Created attachment 232175 [details]
Patch

[Jinwoo Song]

Created attachment 232176 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 232176 [details]
Patch

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

> Source/WebCore/dom/TreeScope.cpp:210
> +    if (m_rootNode.document().isHTMLDocument() && hashPos == notFound)
> +        return nullptr;

What do other browsers do?  I'm very afraid that this is not a backwards compatible change.

[Jinwoo Song]

Comment on attachment 232176 [details]
Patch

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

>> Source/WebCore/dom/TreeScope.cpp:210
>> +        return nullptr;
> 
> What do other browsers do?  I'm very afraid that this is not a backwards compatible change.

FireFox and IE.10 does not support usemap attribute if it has not '#'. Blink works as WebKit as of now.

[Darin Adler]

Comment on attachment 232176 [details]
Patch

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

>>> Source/WebCore/dom/TreeScope.cpp:210
>>> +        return nullptr;
>> 
>> What do other browsers do?  I'm very afraid that this is not a backwards compatible change.
> 
> FireFox and IE.10 does not support usemap attribute if it has not '#'. Blink works as WebKit as of now.

Why specific to HTML documents? That part seems wrong to me.

> Source/WebCore/dom/TreeScope.cpp:211
>      String name = (hashPos == notFound ? url : url.substring(hashPos + 1)).impl();

If we’re exiting when hashPos is notFound, then we should be removing the ? : expression here.

[Darin Adler]

Comment on attachment 232176 [details]
Patch

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

>>>> Source/WebCore/dom/TreeScope.cpp:210
>>>> +        return nullptr;
>>> 
>>> What do other browsers do?  I'm very afraid that this is not a backwards compatible change.
>> 
>> FireFox and IE.10 does not support usemap attribute if it has not '#'. Blink works as WebKit as of now.
> 
> Why specific to HTML documents? That part seems wrong to me.

Given the many years of support for this in WebKit, it’s likely that some WebKit-specific content is accidentally depending on the old behavior. We’ll have to find some way of researching that question.

[Jinwoo Song]

Created attachment 232368 [details]
Patch

Applied Darin's comments.

[Jinwoo Song]

Comment on attachment 232176 [details]
Patch

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

>> Source/WebCore/dom/TreeScope.cpp:211
>>      String name = (hashPos == notFound ? url : url.substring(hashPos + 1)).impl();
> 
> If we’re exiting when hashPos is notFound, then we should be removing the ? : expression here.

I also agree that we need a way to handle this kind of issues. Specially, if there it is impossible to provide the legacy behavior like this, how can we apply the latest spec to the implementation?

[Ryosuke Niwa]

r=me if we believe websites don't rely on this feature.

[WebKit Commit Bot]

Comment on attachment 232368 [details]
Patch

Clearing flags on attachment: 232368

Committed r172796: <http://trac.webkit.org/changeset/172796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.