731832 - Dropping HTMLImageElement.x broke this web page

Status: RESOLVED FIXED

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

Description

[Franklin S Werren]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120216115113

Steps to reproduce:

Loaded up www.crownhill.com and the java buttons will not load right. Also shows up in Sea Monkey. And this issue is not lock to a specific OS


Actual results:

Loaded up www.crownhill.com and the java buttons will not load right. Also shows up in Sea Monkey.


Expected results:

Should load up with the drop down menus not showing up in the left side. Works fine in Opera and Google Chrome as well as Internet Explorer in windows. I use Ubuntu myself. Complaint came in via customer and owner of www.crownhill.com. I am a bit fickle how to fix problem on my end....

Comment 1

[Virgil Dicu [:virgil] [QA]]

Works for me with Firefox 6.0
Doesn't work starting with 7.0

So must have regressed in between.

Comment 2

[Virgil Dicu [:virgil] [QA]]

Last good nightly: 2011-05-28
First bad nightly: 2011-05-29

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f25c65e9335&tochange=5d7b5f2ea603

Comment 3

[Boris Zbarsky [:bzbarsky]]

hg bisect says:

The first bad revision is:
changeset:   70271:f13a2020b5b9
user:        Ms2ger <[e-mail]>
date:        Sat May 28 09:43:43 2011 +0200
summary:     Bug 587021 - drop HTMLImageElement.x/.y; r=sicking sr=jst

That bug report said these are not supported in IE.  Does this site work in IE, and if so, why?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, comment 0 says it does work in IE.  Again, the question is why.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Because it's using .x/.y inside an if (!document.all && document.getElementById) block

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  What does it do on the other side of that block?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: /buttons_1/xaramenu.js

x = event.clientX - event.offsetX + document.body.scrollLeft - document.body.clientLeft;
    y = event.clientY - event.offsetY + document.body.scrollTop - document.body.clientTop - bd;
    dx = event.srcElement.offsetWidth;
    dy = event.srcElement.offsetHeight;

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so that code wouldn't work in Gecko even if the site changed the sniffing because we don't support offsetX, yes?

Quite honestly, I think we should consider backing out bug 587021 and then specifying .x and .y on images, since everyone but IE implements it.

Comment 9

[Asa Dotzler [:asa]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> OK, so that code wouldn't work in Gecko even if the site changed the
> sniffing because we don't support offsetX, yes?
> 
> Quite honestly, I think we should consider backing out bug 587021 and then
> specifying .x and .y on images, since everyone but IE implements it.

I'm in agreement on backing out bug 587021.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Nominating for tracking; I have no idea why I didn't do this back when I made comment 8.

Again, I think the spec here just needs to get changed and we should back out the removal of the properties...  Jonas, thoughts?

Comment 11

[Alex Keybl [:akeybl]]

I don't think this actually needs to track for a specific release since it's a regression from FF7. That being said, we'd definitely approve a backout for FF13 if nominated.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Aurora backout

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

That should be nsIntPoint not nsPoint, right?  Yes, I know it used to be wrong before, but while we're here....

r=me

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Trunk backout

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

As usual, this will break c-c.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: The menu on this particular site doesn't work for another 6 weeks
Testing completed (on m-c, etc.): Not yet
Risk to taking this patch (and alternatives if risky): small; adds back two attributes to an interface (+ uuid change)
String changes made by this patch: None

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Try pushes:
https://tbpl.mozilla.org/?tree=Try&rev=3195ccb1b0bc (aurora)
https://tbpl.mozilla.org/?tree=Try&rev=8de48e0a39c1 (m-c)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 617987 [details] [diff] [review]
Trunk backout

r=me

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/75c7378c87b6

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

[Triage comment]
Please go ahead and do the backout on Aurora (Firefox 13).

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

Hold off on landing this for a bit - I'd like to make doubly sure a UUID change won't cause any weird compatibility issues. Putting back to a?

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

[Triage Comment]
Spoke with Gavin last week. He let me know there was a very low chance that this would cause binary incompatibility issues. We'll be on the lookout.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30d0f3aa2836

Comment 24

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/DOM/HTMLImageElement#Other_properties_%28Gecko%29

Mentioned on Firefox 13 for developers.

Comment 25

[kitchin]

Should be Firefox 14, right?

Comment 26

[Eric Shepherd [:sheppy]]

Is it? I was told Aurora 13. I can fix it if that's wrong. Can anyone advise?

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Flags say "status-firefox13: 	affected "

Comment 28

[kitchin]

Here's the history:

2012-04-16: Comment 11 "we'd definitely approve a backout for FF13"
2012-04-24: Comment 12: Patch created for Aurora.
2012-04-25: Aurora bumped to FF14.
2012-05-05: Comment 23: Landed on Aurora.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Hmm.  Maybe this should ask for beta approval, then.  It certainly seems like this was approved for 13 initially.

Comment 30

[kitchin]

Keyword should be dev-doc-needed or the doc changed:
https://developer.mozilla.org/en/DOM/HTMLImageElement#Other_properties_%28Gecko%29

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

Asking for beta approval, per above.

Note that this was approved for aurora initially before aurora went to beta, but didn't land until _after_ aurora went to beta.

I can totally understand if beta approval is denied to limit risk...  Note that there is an interface change involved.

Comment 32

[Jean-Yves Perrier [:teoli]]

Dev-doc-needed set so that we can update the doc once approval for beta is decided.

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #31)
> Note that this was approved for aurora initially before aurora went to beta,
> but didn't land until _after_ aurora went to beta.

AFAICT, it was approved on 2012-04-25, and the merge was on 2012-04-24.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Hmm.  Then why did the approval comment talk about Firefox 13?  Anyway....

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 617963 [details] [diff] [review]
Aurora backout

No dupes suggest low user effect, we'll have to let this ride the trains.

Comment 36

[Eric Shepherd [:sheppy]]

Docs revised to reflect this change was implemented in Firefox 14, not 13.