Closed
Bug 453968
Opened 16 years ago
Closed 7 years ago
Support IE extension Event.srcElement
Categories
(Core :: DOM: Events, enhancement, P2)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: henry.fai.hang.chan, Assigned: bzbarsky)
References
()
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)
388.00 KB,
text/csv
|
Details | |
362.58 KB,
image/png
|
Details | |
1017 bytes,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
Details | Diff | Splinter Review | |
4.41 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.)
Updated•16 years ago
|
Component: General → DOM: Events
Product: Firefox → Core
QA Contact: general → events
Comment 1•16 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•16 years ago
|
||
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?
Comment 3•13 years ago
|
||
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".
Comment 4•13 years ago
|
||
Note, IE9 supports DOM Events, so there shouldn't be need for this.
Comment 6•13 years ago
|
||
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.
Comment 7•13 years ago
|
||
(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?
Comment 8•13 years ago
|
||
(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).
Comment 9•13 years ago
|
||
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.)
Comment 10•13 years ago
|
||
John, have you tried passing some webkit UA string?
Comment 11•13 years ago
|
||
> 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.
Comment 13•13 years ago
|
||
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.
Comment 14•13 years ago
|
||
And we should propose to spec srcElement instead of imposing reverse-engineering on all implementers.
Comment 15•12 years ago
|
||
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
Updated•9 years ago
|
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [parity-chrome][parity-safari][parity-ie]
Version: unspecified → Trunk
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 17•9 years ago
|
||
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]
Updated•9 years ago
|
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]
Updated•9 years ago
|
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]
Comment hidden (obsolete) |
![]() |
||
Comment 19•9 years ago
|
||
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 → ---
Updated•8 years ago
|
platform-rel: ? → ---
Comment 20•8 years ago
|
||
My ASUS router is complaining about this too.
Comment 21•8 years ago
|
||
(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" :)
Comment 22•8 years ago
|
||
bumping because MDN claims this will never be supported by Firefox, and this thread was never resolved.
Flags: needinfo?(overholt)
Comment 23•8 years ago
|
||
> 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.
Comment 24•8 years ago
|
||
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
Comment 25•8 years ago
|
||
(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.
Comment 26•8 years ago
|
||
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
Updated•7 years ago
|
Flags: webcompat+
Updated•7 years ago
|
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]
![]() |
Assignee | |
Comment 27•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 28•7 years ago
|
||
Fix, if we decide to make this change
Attachment #8956989 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
Comment 29•7 years ago
|
||
(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)
Comment 30•7 years ago
|
||
(WIP web platform tests for srcElement are here: https://github.com/w3c/web-platform-tests/pull/4790/commits/593eb7b24c0bba3b2bbe66cae2e820082768cab6)
![]() |
Assignee | |
Comment 31•7 years ago
|
||
Alternate approach to enable in nightly only
Attachment #8957013 -
Flags: review?(bugs)
Comment 32•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8957013 -
Flags: review?(bugs)
Comment 33•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 34•7 years ago
|
||
OK. I'm going to spin off the nightly-only thing to bug 1444004 so we don't lose tracking info here.
Updated•7 years ago
|
Keywords: site-compat
Updated•7 years ago
|
User Story: (updated)
Updated•7 years ago
|
User Story: (updated)
Comment 35•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 36•7 years ago
|
||
I think we should try riding the trains in 61.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 37•7 years ago
|
||
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]
Comment 38•7 years ago
|
||
(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)
Comment 39•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 40•7 years ago
|
||
> 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)
![]() |
Assignee | |
Comment 41•7 years ago
|
||
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 | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8973297 -
Flags: review?(bugs) → review+
Comment 42•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1d365a6b74
Enable event.srcElement on all channels. r=smaug
Comment 43•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 44•7 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/support-for-event-prototype-srcelement-has-been-added/
Comment 45•7 years ago
|
||
Hi guys any idea on how I could be able to test this to be sure it is ok?
![]() |
Assignee | |
Comment 46•7 years ago
|
||
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.
Comment 47•7 years ago
|
||
https://developer.mozilla.org/en-US/Firefox/Releases/62#DOM
https://developer.mozilla.org/en-US/docs/Web/API/Event/srcElement
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•