Closed Bug 453968 Opened 11 years ago Closed Last year

Support IE extension Event.srcElement

Categories

(Core :: DOM: Events, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: henry.fai.hang.chan, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(6 keywords, Whiteboard: [plaform-rel-Edge][platform-rel-Microsoft][webcompat:p1])

User Story

Business driver: Mobile Webcompat (multiple site issues reported)

Note: Selena would like a security review here. Compat concern is that is could be used for browser sniffing.

Attachments

(5 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080903034741 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080903034741 Minefield/3.1b1pre

Since firefox already has an added undetectable document.all method, and has had detectable clientWidth clientHeight offsetWidth offsetHeight scrollWidth scrollHeight offsetLeft offsetRight scrollLeft scrollRight (just to name a few properties), why not add more undetectable Proprietary methods/properties?

The escape, find, atob, btab are other methods not found in w3c either.

Reproducible: Always




event.srcElement should just map directly to event.target (I find no problems and no difference in my scripts in IE and Fx if I just swap them over.)
Component: General → DOM: Events
Product: Firefox → Core
QA Contact: general → events
IE's event model is so different comparing to DOM Events that I don't see
real reason to support srcElement. (And IIRC, IE will support DOM Events at some point)
We implemented document.all (which was a major pain, btw) because it was a big compat win.  I don't see that doing srcElement will be nearly as big a win, but perhaps you have data to the contrary?
Not sure if this helps, but on a whim I today went through my dataset of 1m HTML/JS/CSS urls from the top 25,000 websites on the web, all collected recently using the latest Fennec UA. They were crawled by visiting the home page of each domain and each page in the same domain linked from the home page (ie one level deep).

1,580 of the domains had some Javascript that include the string "event.srcElement".
Note, IE9 supports DOM Events, so there shouldn't be need for this.
Note: Chrome, Safari, and Opera all support srcElement now.
Blocks: 771949
Please find attached a list of 4910 HTML and Javascript URLs from 900 of the top 18,000 sites on the web that make reference to .srcElement without a nearby reference to .target, implying that the latter is not present. While I can't say for certain that the evidence for each site is 100% conclusive, I can say that I've hand-checked a decent random sample of them.
(In reply to John Jensen from comment #6)
> I can say that I've hand-checked a decent random sample of them.
Meaning what? You've checked that the site doesn't work with FF?
(In reply to Olli Pettay [:smaug] from comment #7)

> Meaning what? You've checked that the site doesn't work with FF?

That 
 a) there isn't a DOM reference in same portion of JS code to both .srcElement and .target, and
 b) in most cases, that the related functionality doesn't work in Firefox. (In some cases I wasn't able to because my JS knowledge is not good enough to figure out what it was trying to do).

I should add that all data was collected using a very recent Fennec UA (v12 or 13).
Sounds like some very odd mobile-only problem.
It would be sad if we had to add more IE-ism, when IE has itself moved to DOM Events.
There is no specification for srcElement, so we should probably just look at webkit source code
and copy their behavior and hope the best.
But, I'm still against adding srcElement.

(Btw, the original comment mentions few properties and all those are specified in some spec.)
John, have you tried passing some webkit UA string?
> John, have you tried passing some webkit UA string?

I have a very similar datasets, collected with either the iPhone or stock Android UAs. I can rerun the effort, although my experience with a number of similar issues regarding CSS and UAs tells me the results will not be that different.
I'd really rather not support another undetectable property. We should definitely look at WebKit and look at what their implementation does. If it simply maps .srcElement to .target then it doesn't seem like a big deal to support.

It's not very surprising that a few "IE-isms" have taken foot hold in the mobile space given how prevalent webkit is there, and how many "IE-isms" are also "webkit-isms". I.e. given how many features webkit has copied from IE.
If we add srcElement, maybe we also need to support window.event. I don't think those sites consider the possibility that window.event is absent.
And we should propose to spec srcElement instead of imposing reverse-engineering on all implementers.
srcElement does return the same thing as target in WebKit:

http://mxr.mozilla.org/chromium/source/src/third_party/WebKit/Source/WebCore/dom/Event.h#114

I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=20713
Summary: Supporting of IE only property srcelement of event but as undetectable property → Support IE extension Event.srcElement
Chrome usage shows this at 9.8%: https://www.chromestatus.com/metrics/feature/timeline/popularity/343 (but I suspect they're hitting code like `target = event.srcElement || event.target`.

Some related srcElement bugs (some fixed by evangelism):
https://github.com/webcompat/web-bugs/issues/705
https://github.com/webcompat/web-bugs/issues/1090
https://bugzilla.mozilla.org/show_bug.cgi?id=1117220#c1
https://bugzilla.mozilla.org/show_bug.cgi?id=1107378#c6 mentions m.taobao.com have busted icons due to srcElement

ni? myself to dig thru our bugs and see how much pain is caused on the window.event side of things (my gut feeling is it's much more than srcElement).
Flags: needinfo?(miket)
Flags: needinfo?(miket)
See Also: → 218415
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [parity-chrome][parity-safari][parity-ie]
Version: unspecified → Trunk
Microsoft's "Global CSS Property Usage" dashboard is broken on Firefox because the page uses Event#srcElement.

STR:
1) https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/
2) Enter some text in the "Filter properties" field, such as "font".

Expected Behavior:
The list of CSS properties should only show the "font" names. This works correctly in Chrome, Safari, and (unsurprisingly) IE11 and Edge.

Actual Behavior:
Nothing happens on the page. The following JavaScript error is logged to the console:

TypeError: a.srcElement is undefined. usage.4e36d87.js:15:48

which points to the following code in https://az813057.vo.msecnd.net/scripts/usage.4e36d87.js:

// As the user types (or clicks the Clear button), perform filtering
i.addEventListener("input",function(a){k(),l(),m(a.srcElement.value)}),n(),window.addEventListener("popstate",n)}}(),/* eslint "strict": [2, "function"] */
Whiteboard: [parity-chrome][parity-safari][parity-ie] → [parity-chrome][parity-safari][parity-ie][parity-edge]
platform-rel: --- → ?
Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge] → [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Chrome][platform-rel-Microsoft]
Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Chrome][platform-rel-Microsoft] → [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Edge][platform-rel-Microsoft]
Sorry for closing quickly I thought it was in Tech Evangelism. :)
The issue reported in Comment #17 is fixed.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
platform-rel: ? → ---
Attached image srcElement-asus.png
My ASUS router is complaining about this too.
(In reply to Mike Taylor [:miketaylr] from comment #20)
> Created attachment 8834986 [details]
> srcElement-asus.png
> 
> My ASUS router is complaining about this too.

I like that your search term was "asus router ip" :)
bumping because MDN claims this will never be supported by Firefox, and this thread was never resolved.
Flags: needinfo?(overholt)
> MDN claims this will never be supported by Firefox

MDN can be wrong. :)

This is waiting on some spec work, which is in progress. At that point it'll be up to DOM to prioritize.
P2 means "in a month or three we'll re-evaluate if we can do this very soon (P1) or if it just gets into the bigger prioritization bucket (P3)"
Flags: needinfo?(overholt)
Priority: -- → P2
(In reply to alex.m.sheehan from comment #22)
> bumping because MDN claims this will never be supported by Firefox, and this
> thread was never resolved.

Can you point me to the page where we say "never"?  Things can change, so that's not very useful language to use; I'm happy to change it.
Sorry, the combination of the "Non-standard" warning, and the fact that this thread is so old, and FF has no support for it led me to the scathing accusation :)

https://developer.mozilla.org/en-US/docs/Web/API/Event/srcElement
Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Edge][platform-rel-Microsoft] → [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Edge][platform-rel-Microsoft][webcompat:p1]
Mike, do you see a problem with implementing this _without_ implementing window.event?  That is, do sites depend on window.event existing if srcElement does?  (I know they depend on the other way around, of course.)
Flags: needinfo?(miket)
Fix, if we decide to make this change
Attachment #8956989 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> Mike, do you see a problem with implementing this _without_ implementing
> window.event?  That is, do sites depend on window.event existing if
> srcElement does?  (I know they depend on the other way around, of course.)

Nope, I'm not aware of any sites like this.
Flags: needinfo?(miket)
Alternate approach to enable in nightly only
Attachment #8957013 - Flags: review?(bugs)
Comment on attachment 8956989 [details] [diff] [review]
Implement Event.prototype.srcElement as an alias for .target

Ok, based on Mike's comment, let's try this.
Attachment #8956989 - Flags: review?(bugs) → review+
Attachment #8957013 - Flags: review?(bugs)
I think landing the Nightly-only version would be good, at least for release or two (or wait on landing the non-release-only patch until after the next merge so we don't explode things in Beta sooner than we'd like).
Flags: needinfo?(bzbarsky)
OK.  I'm going to spin off the nightly-only thing to bug 1444004 so we don't lose tracking info here.
Assignee: bzbarsky → nobody
Status: ASSIGNED → NEW
Depends on: 1444004
Flags: needinfo?(bzbarsky)
See Also: → 1452569
So far, after about two months in Nightly, no known bustage (which is promising). Boris, do you think we should let this ride the trains in 61 or 62, or do you think we should for window.event?
Flags: needinfo?(bzbarsky)
I think we should try riding the trains in 61.
Flags: needinfo?(bzbarsky)
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge][plaform-rel-Edge][platform-rel-Microsoft][webcompat:p1] → [plaform-rel-Edge][platform-rel-Microsoft][webcompat:p1]
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #36)
> I think we should try riding the trains in 61.

Do we need window.event to let it ride (bug 218415)? Are we concerned about increasing the fingerprinting risk here? What about IE code paths (does comment 35 assuage our fears?)?
Flags: needinfo?(bzbarsky)
(In reply to Andrew Overholt [:overholt] from comment #38)
> Are we concerned about increasing the fingerprinting risk here?

Anne told me elsewhere this isn't a concern.
> Do we need window.event to let it ride (bug 218415)?

We _think_ we don't.

> What about IE code paths (does comment 35 assuage our fears?)?

Right.  Given the data we have so far, this is looking safe enough.

Given the timing at this point, I don't think we should aim for 61.  I will aim for 62.
Flags: needinfo?(bzbarsky)
I could keep using BinaryName to make the IDL look more like the spec, but this
is a bit more efficient...
Attachment #8973297 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8973297 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1d365a6b74
Enable event.srcElement on all channels.  r=smaug
https://hg.mozilla.org/mozilla-central/rev/9d1d365a6b74
Status: ASSIGNED → RESOLVED
Closed: 3 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Hi guys any idea on how I could be able to test this to be sure it is ok?
Grab a non-nightly build with this fix (which don't exist yet, of course) and then check whether "srcElement in Event.prototype" tests true.

But also, there are automated tests for this, so I don't know that it needs manual testing.
You need to log in before you can comment on or make changes to this bug.