264624 - Some imagemaps are broken here but not in IE

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Sean Roach]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

Go to this page.  http://www.eatyourlipstick.com/LelainaChapterTwo.htm (A
webcomic.) and you'll notice that the imagemaps don't all work.  A perusal of
them showed that the ones that DO work, have a single declaration of the map
name.  The one that had a single working button, had a second declaration of the
map name, AFTER that one working,(actually two buttons, with the same
destination,) button.  The broken buttons, in the imagemaps, all follow a second
copy of the map name declaration.

I can only HOPE I sent this bug to the right spot.

My computing environment is a 1.4GHz AMD, running Windows XP Pro Version 2002,
with service pack 2.

I have also sent the website's Sysop an email informing her of the "broken"
links, and probable fix.

Reproducible: Always
Steps to Reproduce:
1. Visit a webpage with imagemaps, that have duplicate declarations of the map name
2. notice the lower bar never changes from coordinate numbers to a link address
3.  notice that the labeled spot on the picture, that SHOULD lead you to a
certain page, does nothing.

Actual Results:  
When the cursor is over ANY button in the top bar, EXCEPT the one leading to the
main page for that particular webcomic, the lower bar will show coordinates, and
clicking does nothing.  The one that it does work on, the declaration for that
button comes before a duplicate entry of the map name declaration

Expected Results:  
All four, (actually at least eight,) regions should have worked, leading to 4
different pages.  One for the main homepage, one for the webcomic's main page,
one for news, and one for portfolio.

Go to this page.  http://www.eatyourlipstick.com/LelainaChapterTwo.htm (A
webcomic.) and you'll notice that the imagemaps don't all work.  A perusal of
them showed that the ones that DO work, have a single declaration of the map
name.  The one that had a single working button, had a second declaration of the
map name, AFTER that one working,(actually two buttons, with the same
destination,) button.  The broken buttons, in the imagemaps, all follow a second
copy of the map name declaration.

I can only HOPE I sent this bug to the right spot.

My computing environment is a 1.4GHz AMD, running Windows XP Pro Version 2002,
with service pack 2.

I have also sent the website's Sysop an email informing her of the "broken"
links, and probable fix.

Initially, I posted this to mozdev by mistake.  #7654.  Since invalidated

Comment 1

[Sean Roach]

I blame some formatting issues on laziness, others on lack of sleep.  Initially
typed up for mozdev's bug tracker, which has slightly different fields.  I
didn't intend to paste the main body in twice, however.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 4

[Mats Palmgren (inactive)]

The parser auto-closes the first MAP when it sees the second MAP start tag
and when there are more than one element with the same NAME attribute we
use the first. I think this is the correct behaviour per the HTML spec.
However, the link works in Opera, Konqueror and Netscape 4.x so I'm probably
missing something...

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase (Quirks mode)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Something like this perhaps...

Comment 7

[Boris Zbarsky [:bzbarsky]]

So if you add an <area> to that first map then it breaks in the other browsers too?

Comment 8

[Mats Palmgren (inactive)]

Yes: IE6/win32 and Opera7.54/linux.
No: Konqueror3.1.4/linux.
The reason why Konqueror works seems to be that it prefers the last MAP with
a mathing NAME. I also tested MAPs using the same NAME with valid AREAs but
using different URLs - all browser used the first URL except Konqueror that
used the last URL.
So, the patch would make us compatible with IE6 and Opera7 in Quirks mode.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162304 [details] [diff] [review]
Patch rev. 1

>Index: content/html/document/src/nsHTMLDocument.cpp

>+        rv = map->GetAreas(getter_AddRefs(mapAreas));
>+        if (NS_SUCCEEDED(rv) && mapAreas) {
>+          PRUint32 length = 0;
>+          mapAreas->GetLength(&length);
>+          if (length == 0) {

Couldn't you replace this with:

  PRBool hasKids = PR_FALSE;
  map->HasChildNodes(&hasKids);
  if (!hasKids) {

That should be a good deal faster and equivalent, since I doubt the tag-soup
parser allows anything but an <area> as a child of <map> and for XML documents
we'll never hit this code.

Other than that, this looks reasonable.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> That should be a good deal faster and equivalent, since I doubt the tag-soup
> parser allows anything but an <area> as a child of <map> and for XML documents

The HTML4 spec allows a lot more than area as a child of map, so I sure hope our
parser does too.  And I think I've checked that it did in the past.  See, e.g.,
http://www.w3.org/TR/1999/REC-html401-19991224/struct/objects.html#h-13.6.1.1

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah, true (just tested it, and <p> is allowed in an imagemap).  How do IE and
Opera treat imagemaps with only non-<area> children here?

Comment 12

[Boris Zbarsky [:bzbarsky]]

And I guess there is the issue of child textnodes.... so the patch as posted may
be the best way to go.  :(

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

I have tested a lot more cases and this is the closest I can get to what
IE/Opera does without changing Parser/Frame stuff...  The way IE/Opera does
things is just weird - they allow MAP as a container and then all contained
AREAs seems to be associated with *all* currently open MAPs.

This patch is the same as rev. 1 except that I now keep looping while MAPs
are empty (as I said in the comment of the first patch but didn't do in the
code). For normal pages it should not be a performance impact by that
compared with the first patch.

I'm not sure if this is really worth fixing when there is a whole range of
other (tag soup) cases where we aren't compatible. Or maybe we should just
do what IE/Opera does in quirks mode?

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #3 - tag soup

Both left and right link areas work in IE6 and Opera7.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'd rather not break parsing for this.  That patch seems reasonable as a quirk, yes.

Comment 16

[Mats Palmgren (inactive)]

Taking so I don't forget it again...

Comment 17

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

As rev. 2 - just updated to trunk + removed an unused variable (unrelated)
to fix a compiler warning.

Comment 18

[Mats Palmgren (inactive)]

Checked in to trunk 2005-07-25 16:37 PDT

-> FIXED