Bug 91228 – imagemap without a "name" attribute doesn't work (affects xkcd.com)

• Component: Images
• Status: NEW

[Mehdi Kabab]

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).

[Alexey Proskuryakov]

Ted, do you know if this is something we mean to support?

[Theresa O'Connor]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

*** Bug 97204 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Duplicate has a suggested fix that may or may not be correct, I can't tell immediately.

[vabr]

(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!

[Radar WebKit Bug Importer]

<rdar://problem/12359382>

[Akash Vaswani]

Created attachment 165869 [details]
Patch

[Alexey Proskuryakov]

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.

[Brady Eidson]

(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.

[Akash Vaswani]

Created attachment 166037 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Akash Vaswani]

Created attachment 167178 [details]
Patch

[Sam Weinig]

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

[Akash Vaswani]

Created attachment 167200 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Alexey Proskuryakov]

Why did mac-ews try this patch twice?

[Adam Barth]

> 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.

[Maciej Stachowiak]

Should the patch here be committed or is the failing test meaningful?

[Adam Barth]

Seems likely to be a real failure since it came up twice and seems related to the patch.

[Maciej Stachowiak]

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.

[Chen Zhixiang]

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)

[Chen Zhixiang]

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.

[Chen Zhixiang]

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.

[Chen Zhixiang]

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;
}

[Chen Zhixiang]

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>

[Chen Zhixiang]

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/ , :-)

[Simon Pieters (:zcorpan)]

Tests: https://github.com/w3c/web-platform-tests/pull/4511

(Note that the spec now requires case-sensitive matching, not compatibility caseless.)

[Simon Pieters (:zcorpan)]

Tests landed in web-platform-tests.

[Ahmad Saleem]

Safari Technology Preview 155 do still fail tests:

http://wpt.live/html/semantics/embedded-content/image-maps/image-map-processing-model/hash-name-reference.html

[Sam Sneddon [:gsnedders]]

https://wpt.fyi/results/html/semantics/embedded-content/image-maps/image-map-processing-model/hash-name-reference.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=subtest shows a variety of tests failing cross-browser.

[Ahmad Saleem]

I have local patch based on Blink patch and it make us pass WPT test cases.

Will try to do draft PR later today and see how it goes.

Meanwhile assigning it to myself.

[Ahmad Saleem]

Forgot to do it in Draft but anyway - https://github.com/WebKit/WebKit/pull/14268

^ Let's see. How it goes..