Closed Bug 731832 Opened 11 years ago Closed 11 years ago

Dropping HTMLImageElement.x broke this web page

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox11 --- wontfix
firefox12 - wontfix
firefox13 - affected
firefox14 - fixed

People

(Reporter: admin, Assigned: Ms2ger)

References

Details

(Keywords: dev-doc-complete, regression)

Attachments

(3 files)

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....
Works for me with Firefox 6.0
Doesn't work starting with 7.0

So must have regressed in between.
OS: Linux → All
Hardware: x86_64 → All
Version: 10 Branch → Trunk
Last good nightly: 2011-05-28
First bad nightly: 2011-05-29

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f25c65e9335&tochange=5d7b5f2ea603
Component: Untriaged → Layout
Product: Firefox → Core
QA Contact: untriaged → layout
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Blocks: 587021
Keywords: regression
Ah, comment 0 says it does work in IE.  Again, the question is why.
Component: Layout → DOM: Core & HTML
QA Contact: layout → general
Because it's using .x/.y inside an if (!document.all && document.getElementById) block
Hmm.  What does it do on the other side of that block?
Attached file /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;
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.
(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.
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?
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.
Assignee: nobody → Ms2ger
Attached patch Aurora backoutSplinter Review
Attachment #617963 - Flags: review?(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
Attachment #617963 - Flags: review?(bzbarsky) → review+
Attached patch Trunk backoutSplinter Review
Attachment #617987 - Flags: review?(bzbarsky)
As usual, this will break c-c.
Keywords: dev-doc-needed
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
Attachment #617963 - Flags: approval-mozilla-aurora?
Comment on attachment 617987 [details] [diff] [review]
Trunk backout

r=me
Attachment #617987 - Flags: review?(bzbarsky) → review+
Comment on attachment 617963 [details] [diff] [review]
Aurora backout

[Triage comment]
Please go ahead and do the backout on Aurora (Firefox 13).
Attachment #617963 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [c-n attachment 617963 for aurora]
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?
Attachment #617963 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Keywords: checkin-needed
Whiteboard: [c-n attachment 617963 for aurora]
Summary: Issues loading a specific web page → Dropping HTMLImageElement.x broke this web page
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.
Attachment #617963 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/30d0f3aa2836
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Should be Firefox 14, right?
Is it? I was told Aurora 13. I can fix it if that's wrong. Can anyone advise?
Flags say "status-firefox13: 	affected "
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.
Hmm.  Maybe this should ask for beta approval, then.  It certainly seems like this was approved for 13 initially.
Keyword should be dev-doc-needed or the doc changed:
https://developer.mozilla.org/en/DOM/HTMLImageElement#Other_properties_%28Gecko%29
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.
Attachment #617963 - Flags: approval-mozilla-beta?
Dev-doc-needed set so that we can update the doc once approval for beta is decided.
(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.
Hmm.  Then why did the approval comment talk about Firefox 13?  Anyway....
Comment on attachment 617963 [details] [diff] [review]
Aurora backout

No dupes suggest low user effect, we'll have to let this ride the trains.
Attachment #617963 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Docs revised to reflect this change was implemented in Firefox 14, not 13.
You need to log in before you can comment on or make changes to this bug.