233353 - <area shape="circle"> with negative radius in coords is ignored

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Jeremy Wells]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

Client side image map on page referenced above will not work properly in 
Firebird, but it does work in Internet Explorer. Only about 1/4 of the links 
work.

Reproducible: Always
Steps to Reproduce:
1. Go to http://www.oldtowncape.org/walking_tour/central.htm
2. Try clicking on number 33, 32, 1, etc.
3. Repeat the same thing using Internet Explorer

Actual Results:  
Image-mapped links don't work.

Expected Results:  
The links should have worked.

Comment 1

[Jeff Walden [:Waldo]]

Can circle coordinates for HTML image maps have negative radii?  33, 32, and 1
all have -10 as the value of their radii, and based on the conventional wisdom
of polar coordinates, negative radii are invalid.  I didn't test all of them to
see if only <area>s with a negative radius worked, tho.

Also reassigning to Browser product as that does layout, not Firebird.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Can circle coordinates for HTML image maps have negative radii?

I don't see how that could be reasonably implemented... what does IE actually do
here?

Comment 3

[Isaac Hwak Han]

> what does IE actually do here?

I think IE treats negative CSS lengths as their positive counterpart by simply
dropping minus (probably only where applying negative is not relevant in
context). This misfeature is also seen in other places such as 'font-size: -20px'.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't think we want to do that.

Comment 5

[Jeff Walden [:Waldo]]

-->NEW, seems unique to me.

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsImageMap.cpp#685
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsImageMap.cpp#707

Could inserting the line:

radius=-radius;

in the if() code blocks referenced above instead of return; or return PR_FALSE;
do the trick?  Forgive me if I'm completely off here, but I think I've tracked
the code correctly.  Following LXR seems to suggest that that action is possible
for "nscoord" variables, as they're "PRInt32" which appears to be just a signed
32-bit number.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

at least in that first line, I'd do
     nscoord radius = PR_MAX(0, mCoords[2]);
instead...

but do we want to fix this? a negative radius is a pretty stupid thing to do,
I'd say the webmasters should fix their page.

Comment 7

[Jeff Walden [:Waldo]]

While opening the Javascript Console to check for errors in another site, I ran
across this warning for download.com:

------------------------------------------------------------------------------
Warning: The "coords" attribute of the <area shape="rect"> tag is not in the
"left,top,right,bottom" format.
Source File: http://www.download.com/
Line: 0
Source Code:
265,50,75,20
------------------------------------------------------------------------------

Recognizing the similarity between this bug and the coding mistake shown in the
warning, I realized it might make sense to *not* fix the initial bug but instead
fire off a similar warning to the Javascript Console, e.g.:

------------------------------------------------------------------------------
Warning: The "coords" attribute of the <area shape="circle"> tag has a negative
radius (-10).
Source File: http://www.oldtowncape.org/walking_tour/central.htm
Line: 18
Source Code:
298,38,-10
------------------------------------------------------------------------------

Thoughts?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that for rect shapes we actaully "do what I mean" in addition to giving the
warning.  See RectArea::ParseCoords.

If we decide to do this sign-flipping thing, it should only be done at parse time.

Note that unlike the case of rects (where it's really easy to screw up the
order), setting a negative radius requires malice, pretty much.  Which is why I
feel we shouldn't go out of our way to make it work.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #1 (circle & poly)

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch A rev. 1

Comment 11

[Mats Palmgren (inactive)]

Attachment: Screenshot of JS Console with patch A rev. 1

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm still wondering why we want to flip this sign, exactly...  Again, unlike
rect this one requires _effort_ to screw up.

The rest looks OK at first glance; I'll try to do a thorough review within the
next day or two.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky for the code.  I still don't think we should be correcting the
negative issue (though I'm ok with having an error message about it).

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch A rev. 2

Ok, I removed the error correction on the radius.
Consequently, I also made it an error rather than a warning.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Screenshot of JS Console with patch A rev. 2

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155448 [details] [diff] [review]
Patch A rev. 2

r+sr=bzbarsky

The error messages may look better if instead of using "aSpec" for the source
line one uses:

NS_LITERAL_STRING("coords=\"") + aSpec + NS_LITERAL_STRING("\"")

but either way is fine with me.

Comment 17

[Mats Palmgren (inactive)]

Yes, it looked much better.  I added that and checked in to trunk at
2004-08-08 07:05 PDT

The original bug report is WONTFIX, but there is an error message about it now...

-> WONTFIX