Last Comment Bug 731832 - Dropping HTMLImageElement.x broke this web page
: Dropping HTMLImageElement.x broke this web page
Status: RESOLVED FIXED
: dev-doc-complete, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks: 587021
  Show dependency treegraph
 
Reported: 2012-02-29 15:36 PST by Franklin S Werren
Modified: 2013-12-27 14:35 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
-
affected
-
fixed


Attachments
/buttons_1/xaramenu.js (8.07 KB, text/javascript)
2012-03-17 07:00 PDT, :Ms2ger
no flags Details
Aurora backout (3.23 KB, patch)
2012-04-24 11:23 PDT, :Ms2ger
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review
Trunk backout (3.49 KB, patch)
2012-04-24 12:18 PDT, :Ms2ger
bzbarsky: review+
Details | Diff | Review

Description Franklin S Werren 2012-02-29 15:36:21 PST
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] 2012-03-12 08:50:58 PDT
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] 2012-03-12 09:44:38 PDT
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 [:bz] (Out June 25-July 6) 2012-03-16 14:21:03 PDT
hg bisect says:

The first bad revision is:
changeset:   70271:f13a2020b5b9
user:        Ms2ger <ms2ger@gmail.com>
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 [:bz] (Out June 25-July 6) 2012-03-16 14:21:55 PDT
Ah, comment 0 says it does work in IE.  Again, the question is why.
Comment 5 :Ms2ger 2012-03-16 14:32:53 PDT
Because it's using .x/.y inside an if (!document.all && document.getElementById) block
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-16 16:12:57 PDT
Hmm.  What does it do on the other side of that block?
Comment 7 :Ms2ger 2012-03-17 07:00:11 PDT
Created attachment 606861 [details]
/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 [:bz] (Out June 25-July 6) 2012-03-17 07:26:03 PDT
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] 2012-03-17 11:13:54 PDT
(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 [:bz] (Out June 25-July 6) 2012-04-13 20:28:53 PDT
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] 2012-04-16 09:26:49 PDT
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 2012-04-24 11:23:17 PDT
Created attachment 617963 [details] [diff] [review]
Aurora backout
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-24 11:58:22 PDT
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 2012-04-24 12:18:35 PDT
Created attachment 617987 [details] [diff] [review]
Trunk backout
Comment 15 :Ms2ger 2012-04-24 12:27:36 PDT
As usual, this will break c-c.
Comment 16 :Ms2ger 2012-04-24 12:35:39 PDT
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 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-04-24 13:11:17 PDT
Comment on attachment 617987 [details] [diff] [review]
Trunk backout

r=me
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 12:28:56 PDT
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] 2012-04-25 12:33:39 PDT
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] 2012-04-30 10:44:42 PDT
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 24 Eric Shepherd [:sheppy] 2012-05-07 09:16:29 PDT
Docs updated:

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

Mentioned on Firefox 13 for developers.
Comment 25 kitchin 2012-05-07 10:21:20 PDT
Should be Firefox 14, right?
Comment 26 Eric Shepherd [:sheppy] 2012-05-07 10:33:55 PDT
Is it? I was told Aurora 13. I can fix it if that's wrong. Can anyone advise?
Comment 27 :Ms2ger 2012-05-07 10:36:57 PDT
Flags say "status-firefox13: 	affected "
Comment 28 kitchin 2012-05-07 12:35:07 PDT
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 [:bz] (Out June 25-July 6) 2012-05-07 12:38:35 PDT
Hmm.  Maybe this should ask for beta approval, then.  It certainly seems like this was approved for 13 initially.
Comment 30 kitchin 2012-05-09 07:19:44 PDT
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 [:bz] (Out June 25-July 6) 2012-05-09 08:33:08 PDT
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] 2012-05-09 09:21:41 PDT
Dev-doc-needed set so that we can update the doc once approval for beta is decided.
Comment 33 :Ms2ger 2012-05-09 10:18:07 PDT
(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 [:bz] (Out June 25-July 6) 2012-05-09 10:20:19 PDT
Hmm.  Then why did the approval comment talk about Firefox 13?  Anyway....
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-09 15:34:44 PDT
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] 2012-05-10 06:30:00 PDT
Docs revised to reflect this change was implemented in Firefox 14, not 13.

Note You need to log in before you can comment on or make changes to this bug.