Bug 91228 - imagemap without a "name" attribute doesn't work (affects xkcd.com)
Summary: imagemap without a "name" attribute doesn't work (affects xkcd.com)
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://pioupioum.fr/sandbox/bugs/clie...
Keywords: EasyFix, InRadar
: 97204 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-07-13 06:08 PDT by Mehdi Kabab
Modified: 2017-01-19 01:42 PST (History)
16 users (show)

See Also:


Attachments
Patch (106.95 KB, patch)
2012-09-26 13:51 PDT, Akash Vaswani
no flags Details | Formatted Diff | Diff
Patch (31.37 KB, patch)
2012-09-27 11:35 PDT, Akash Vaswani
no flags Details | Formatted Diff | Diff
Patch (17.57 KB, patch)
2012-10-04 14:44 PDT, Akash Vaswani
no flags Details | Formatted Diff | Diff
Patch (17.57 KB, patch)
2012-10-04 16:29 PDT, Akash Vaswani
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mehdi Kabab 2012-07-13 06:08:10 PDT
According the DTD of XHTML Client-side Image Map Module loaded by the XHTML+RDFa DTD, an element map should not have to name attribute.
In a valid XHTML+RDFa page, WebKit ignores the map that not declare a `name` attribute (only a `id` attribute).

(Successfully tested on IE7+, Opera 12, Firefox 13).
Comment 1 Alexey Proskuryakov 2012-07-14 00:48:21 PDT
Ted, do you know if this is something we mean to support?
Comment 2 Theresa O'Connor 2012-07-14 15:01:08 PDT
I don't think it makes sense to change browser engines for RDFa.

That said, WebKit deviates from the HTML spec in this case:

§ 4.8.14.2 (which defines the map element's processing model) says that the "rules for parsing a hash-name reference to a map element must be followed."

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-map-element.html#processing-model

The rules for parsing a hash-name reference say to "[r]eturn the first element[…] that has an id attribute whose value is a case-sensitive match[…] or a name attribute whose value is a compatibility caseless match"

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-a-hash-name-reference

I don't think there would be a significant compat risk to such a change.
Comment 3 Alexey Proskuryakov 2012-09-20 10:13:47 PDT
As mentioned by Ted, this is not related to XHTML, and fails in HTML in exactly the same manner.

Per a bug I'm about to dupe, this affects xkcd.com.
Comment 4 Alexey Proskuryakov 2012-09-20 10:15:27 PDT
*** Bug 97204 has been marked as a duplicate of this bug. ***
Comment 5 Alexey Proskuryakov 2012-09-20 10:18:35 PDT
Duplicate has a suggested fix that may or may not be correct, I can't tell immediately.
Comment 6 vabr 2012-09-20 23:02:47 PDT
(In reply to comment #5)
> Duplicate has a suggested fix that may or may not be correct, I can't tell immediately.

I did not suggest it as a fix, as I'm not sure about the original purpose of the "if (document()->isHTMLDocument())". It was meant only as a pointer to the relevant part of code.
Thanks for looking into that!
Comment 7 Radar WebKit Bug Importer 2012-09-24 10:23:06 PDT
<rdar://problem/12359382>
Comment 8 Akash Vaswani 2012-09-26 13:51:17 PDT
Created attachment 165869 [details]
Patch
Comment 9 Alexey Proskuryakov 2012-09-26 14:03:24 PDT
Comment on attachment 165869 [details]
Patch

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

I think that this patch can use another iteration, primarily for ChangeLog, but also potentially for some of the questions.

> Source/WebCore/ChangeLog:13
> +        Bug: Imagemap only looks for maps with natching names and disregards maps with matching
> +             id attributes.
> +
> +        Fix: Run the mapping code for cases in which the attribute is id as well. Additionally,
> +             in the case that name and id of different maps match the usemap argument, we map 
> +             to the first one encountered. This is the same strategy used by Opera and Firefox.

I think that this explanation can be shorter, and go into per-funtiton comment section below.

You say that this matches Opera and Firefox - what about IE?

Is this a change in HTML5 from earlier HTML specs?

> Source/WebCore/html/HTMLMapElement.cpp:107
> -        if (isIdAttributeName(attribute.name())) {
> +        if (isIdAttributeName(attribute.name()))

You still need the braces, as there is more than one line inside. We do count the comments.

> LayoutTests/ChangeLog:10
> +        * fast/loader/resources/beach-scene.png: Added.

What is the license for this image?

> LayoutTests/fast/loader/map-images-to-id-and-name.html:12
> +                eventSender.mouseMoveTo(400, 130);
> +                eventSender.mouseDown();
> +                eventSender.mouseUp();

Do we have to use EventSender? The downside is that the test won't work in browser. Did you check existing image map tests for techniques?

If EventSender is a must, please add an explanation to the test about how to run it manually.
Comment 10 Brady Eidson 2012-09-26 14:44:09 PDT
(In reply to comment #9)
> (From update of attachment 165869 [details])
> 
> > LayoutTests/ChangeLog:10
> > +        * fast/loader/resources/beach-scene.png: Added.
> 
> What is the license for this image?

There are various pngs scattered throughout the LayoutTests directory - we have rights to use any of them, so you can substitute any of them.
> 
> > LayoutTests/fast/loader/map-images-to-id-and-name.html:12
> > +                eventSender.mouseMoveTo(400, 130);
> > +                eventSender.mouseDown();
> > +                eventSender.mouseUp();
> 
> Do we have to use EventSender? The downside is that the test won't work in browser. Did you check existing image map tests for techniques?
> 
> If EventSender is a must, please add an explanation to the test about how to run it manually.

Existing <map> tests that test similar functionality do use eventSender.  At least the ones that I could find.  I think simulating an actual user click is relevant here.

Akash, when Alexey mentions adding an explanation on how to run manually he simply means some text above the images that mentions the fact that if you're running the test manually you have to click on one map or the other, and what the expectation is when you do.
Comment 11 Akash Vaswani 2012-09-27 11:35:44 PDT
Created attachment 166037 [details]
Patch
Comment 12 WebKit Review Bot 2012-09-27 12:52:45 PDT
Comment on attachment 166037 [details]
Patch

Attachment 166037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036899

New failing tests:
fast/loader/map-images-to-id-and-name.html
fast/events/tabindex-focus-blur-all.html
Comment 13 WebKit Review Bot 2012-09-27 13:36:14 PDT
Comment on attachment 166037 [details]
Patch

Attachment 166037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14050373

New failing tests:
fast/loader/map-images-to-id-and-name.html
fast/events/tabindex-focus-blur-all.html
Comment 14 Build Bot 2012-09-28 07:31:36 PDT
Comment on attachment 166037 [details]
Patch

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

New failing tests:
fast/loader/map-images-to-id-and-name.html
fast/spatial-navigation/snav-imagemap-area-not-focusable.html
http/tests/workers/terminate-during-sync-operation.html
fast/spatial-navigation/snav-imagemap-overlapped-areas.html
fast/events/tabindex-focus-blur-all.html
fast/spatial-navigation/snav-imagemap-simple.html
Comment 15 Akash Vaswani 2012-10-04 14:44:03 PDT
Created attachment 167178 [details]
Patch
Comment 16 Sam Weinig 2012-10-04 15:32:42 PDT
Comment on attachment 167178 [details]
Patch

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

> Source/WebCore/html/HTMLMapElement.cpp:109
> +    cachedValue = newValue;
> +    
> +}

Extra newline.

> LayoutTests/ChangeLog:10
> +        * fast/loader/map-images-to-id-and-name-expected.txt: Added.
> +        * fast/loader/map-images-to-id-and-name.html: Added.
> +        * fast/loader/resources/circle-and-square.png: Added.

The test should go in fast/images
Comment 17 Akash Vaswani 2012-10-04 16:29:23 PDT
Created attachment 167200 [details]
Patch
Comment 18 Build Bot 2012-10-04 19:33:58 PDT
Comment on attachment 167200 [details]
Patch

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

New failing tests:
fast/images/image-map-multiple-xhtml.xhtml
Comment 19 Build Bot 2012-10-04 20:35:43 PDT
Comment on attachment 167200 [details]
Patch

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

New failing tests:
fast/images/image-map-multiple-xhtml.xhtml
Comment 20 WebKit Review Bot 2012-10-04 22:11:57 PDT
Comment on attachment 167200 [details]
Patch

Attachment 167200 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14172545

New failing tests:
fast/images/image-map-multiple-xhtml.xhtml
Comment 21 Alexey Proskuryakov 2012-10-04 22:16:51 PDT
Why did mac-ews try this patch twice?
Comment 22 Adam Barth 2012-10-04 22:28:10 PDT
> Why did mac-ews try this patch twice?

There's a one-hour timeout.  If we don't hear from the bot in an hour, we assume it died and will give the job to another bot.  We might need to raise the timeout.
Comment 23 Maciej Stachowiak 2013-02-16 22:26:46 PST
Should the patch here be committed or is the failing test meaningful?
Comment 24 Adam Barth 2013-02-16 23:08:37 PST
Seems likely to be a real failure since it came up twice and seems related to the patch.
Comment 25 Maciej Stachowiak 2013-02-17 01:32:04 PST
Comment on attachment 167200 [details]
Patch

Removing r+ since this appears to cause a material test failure. It would be nice to see a proper fix.
Comment 26 Chen Zhixiang 2013-07-09 23:09:06 PDT
Also broke http://car.driver.jp/
in My Window 7, FF 22、IE 8/9、Opera 12.14,all support this "ref by id" behavor, except WebKit(including Chrome 28)
Comment 27 Chen Zhixiang 2013-07-09 23:33:17 PDT
My understanding:

1、match name if <map> has name, or else match the id: this should mainly solve the problem!
2、if there are multiple <map> elements with the same name attribute, the spec say match the first?
   Well, i thought match depending on element's stroring order is not very good, the `match` procedure should be combined with 2 parts:

  (1)iterate over once within a (un-ordered?)container; 
  (2)match each element and give a score, the scores should be strict-linear order, that is to say, any 2 scores is not equal, since if permitting euqal, then we have to take position/order info into match consideration.
Comment 28 Chen Zhixiang 2013-07-11 02:19:15 PDT
http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#the-map-element

The name attribute gives the map a name so that it can be referenced. The attribute must be present and must have a non-empty value with no space characters. The value of the name attribute must not be a compatibility-caseless match for the value of the name attribute of another map element in the same document. If the id attribute is also specified, both attributes must have the same value.

From the draft spec, we can say it's too strict. This does not fit the spirit of HTML5 -- improve error compatibility.

If you say HTML5 allows more error-compatible parsing, then it should allows more loose name-id attribute matching, by the same principle?

So my thought:
(1)<img>'s usemap first matches <map>'s name, if there are multiple <map>'s with the same name, match the first;
(2)Then if no <img> with the matched name, match against id. if there are multiple <map>'s with the same id, match the first;
This would be more consistent and shows the clarity.

The first modification in patch will violate the above rule:
  <img usemap="#b" .../> 
  <map name="a" id="b" .../> 
  <map name="b" id="c" .../> 
The `keyMatchesMapNameOrId` will cause the first <map> matched, while it may be better the second <map>.

Or just modifier the parse? It'll be more simple: if <map> has no name but id, just add a name according to id.
Comment 29 Chen Zhixiang 2013-07-15 02:01:24 PDT
The patch has an error: it can not match upper-cased Id attribute.

inline bool keyMatchesLowercasedMapNameOrId(AtomicStringImpl* key, Element* element)
{
    if (!element->hasTagName(mapTag))
        return false;
    if (element->fastGetAttribute(nameAttr).lower().impl() == key)
        return true;
>    if (element->getIdAttribute().lower().impl() == key)
        return true;
    return false;
}
Comment 30 Chen Zhixiang 2013-07-15 02:09:04 PDT
The new case: //Test 2, 3 failed

<html>
    <head>
        <script>
            function log(msg)
            {
                document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
            }
            function test(testNumber, success)
            {
                log("Test " + testNumber + ":")
                if (success)
                    log("  PASS: Correct map selected.");
                else
                    log("  FAIL: Incorrect map selected.");
            }
        </script>
    </head>
    <body>
        <div style="display:inline-block">
            <p>Click on the blue squares for Tests 1 - 4. You should see "PASS" to the right for the first two, and "Hit me" for the next two</p>

            <p>Test 1</p>
            <img src="square-blue-100x100.png" usemap="#TEST1">
            <map id="TEST1-not-the-same" name="TEST1">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(1, true))">
            </map>
            <map name="TEST1">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(1, false))">
            </map>

            <p>Test 2</p>
            <img src="square-blue-100x100.png" usemap="#TEST2">
            <map id="TEST2" name="TEST2-not-the-same">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(2, true))">
            </map>
            <map name="TEST2">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(2, false))">
            </map>

            <p>Test 3</p>
            <img src="square-blue-100x100.png" usemap="#TEST3">
            <p>Test 4</p>
            <img src="square-blue-100x100.png" usemap="#TEST4">
            <map id="TEST3">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(3, true))">
            </map>
            <map name="TEST4">
                <area shape="rect" coords="0,0,100,100" href="javascript:void(test(4, true))">
            </map>
        </div>
        <div style="float:right">
            <pre style="margin-top:50px" id="console"></pre>
        </div>
    </body>
</hmtl>
Comment 31 Chen Zhixiang 2013-07-15 02:20:40 PDT
Fix:

 Node::InsertionNotificationRequest HTMLMapElement::insertedInto(ContainerNode* insertionPoint)
 {
-    if (insertionPoint->inDocument())
-        treeScope()->addImageMap(this);
+    if (insertionPoint->inDocument()) {
+        updateCachedValue(m_cachedId, document()->isHTMLDocument() ? getIdAttribute().lower() : getIdAttribute(), treeScope(), this);
+        updateCachedValue(m_cachedName, document()->isHTMLDocument() ? fastGetAttribute(nameAttr).lower() : fastGetAttribute(nameAttr), treeScope(), this);
+    }
     return HTMLElement::insertedInto(insertionPoint);
 }

When do match in ignoring case mode, the "Id" attribute also need to be lower()'ed.
In fact, better use equalsIgnoringCase...

This fixs http://car.driver.jp/ , :-)
Comment 32 Simon Pieters 2017-01-05 04:57:26 PST
Tests: https://github.com/w3c/web-platform-tests/pull/4511

(Note that the spec now requires case-sensitive matching, not compatibility caseless.)
Comment 33 Simon Pieters 2017-01-19 01:42:33 PST
Tests landed in web-platform-tests.