Closed
Bug 218415
Opened 21 years ago
Closed 7 years ago
Request for window.event object added to DOM to ease cross browser scripting
Categories
(Core :: DOM: Events, enhancement, P2)
Core
DOM: Events
Tracking
()
VERIFIED
FIXED
mozilla63
People
(Reporter: Eric.Gonia, Assigned: hsivonen)
References
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat:p1])
User Story
Business driver: Mobile Webcompat (multiple site issues reported) (and some long-tail desktop) Notes: watch out for other IE-isms if we have window.event (see http://bit.ly/2p10hp5 for some examples (click "Expand all"))
Attachments
(1 file)
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
I would like to make a feature request for a window.event object to be added
that would be populated like Internet Explorer does. It would contain the last
event that was fired.
Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Expected Results:
I would like to see a window.event object that would be available inside an
event handler.
The main problem is when I do something like this:
<SPAN id="someid" onclick="DoClick('blue')">
My event handler does not get an event object.
My understanding is I would have to register an event handler for each <SPAN>
tag I wanted to do this for. I have a php page that may need to register
literally hundreds of these. It would be mush simpler if an event object was
populated each time an event fired. I need information like event.pageX.
![]() |
||
Comment 1•21 years ago
|
||
> <SPAN id="someid" onclick="DoClick(event, 'blue')">
will work just fine in both Mozilla and IE.
This has been brought up before and the IE model has major issues for nested
event handler invocations (one handler doing something that triggers an event
and hence another handle). Please find the original bug on this and mark this
duplicate.
Whiteboard: DUPEME
Can not locate a duplicate bug by searching for window.event or IE event . But
specifying DoClick(event, 'blue') works great.
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
Comment 3•20 years ago
|
||
*** Bug 272820 has been marked as a duplicate of this bug. ***
Comment 4•20 years ago
|
||
*** Bug 290904 has been marked as a duplicate of this bug. ***
Comment 5•20 years ago
|
||
*** Bug 292193 has been marked as a duplicate of this bug. ***
Updated•9 years ago
|
![]() |
||
Updated•8 years ago
|
Whiteboard: [webcompat]
![]() |
||
Comment 7•8 years ago
|
||
On Chromium project, there is
* Consider removing window.event
https://bugs.chromium.org/p/chromium/issues/detail?id=223749
And this is the counter for the property
https://www.chromestatus.com/metrics/feature/timeline/popularity/69
Right now the usage for the year 2016 is stable to growing. For the previous 3 years it went from 6% to 2% but now stabilized. 2% is a lot.
Updated•8 years ago
|
Updated•8 years ago
|
Comment 8•8 years ago
|
||
Similar case in http://www.mobile01.com, they use
> <a style="cursor:pointer;" onclick="scroll(0,0);">回到頁首</a>
to scroll to top, but should use window.scroll(0,0) in Firefox.
Updated•8 years ago
|
Updated•8 years ago
|
Comment 9•8 years ago
|
||
I think we should finally add support for this -- it comes up enough and totally breaks a page when you run into it.
See also https://github.com/whatwg/dom/issues/334.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Comment 10•8 years ago
|
||
So per that standards issue we should add:
* Event's srcElement
* Event's returnValue
* Window's event
(Chrome successfully removed constants, so they are probably not problematic.)
https://github.com/w3c/web-platform-tests/pull/4305 has tests for returnValue. We still need tests for srcElement and event. And of course a change to the standard.
I'm happy to mentor the test/standard side if someone wants to take that on.
Assignee: events → nobody
Status: REOPENED → NEW
QA Contact: ian
Updated•8 years ago
|
Comment 11•8 years ago
|
||
(In reply to Anne (:annevk) from comment #10)
> So per that standards issue we should add:
>
> * Event's srcElement
> * Event's returnValue
> * Window's event
>
> (Chrome successfully removed constants, so they are probably not
> problematic.)
>
> https://github.com/w3c/web-platform-tests/pull/4305 has tests for
> returnValue. We still need tests for srcElement and event. And of course a
> change to the standard.
>
> I'm happy to mentor the test/standard side if someone wants to take that on.
Anne, I'll be the guinea pig, this keeps coming up and is blocking the MozillaOnline fork from using mozilla-central Fennec. Let me start with wpt tests and I'll ping you in the PR.
![]() |
||
Updated•8 years ago
|
Flags: webcompat?
Updated•8 years ago
|
Flags: webcompat? → webcompat+
Updated•8 years ago
|
![]() |
||
Updated•8 years ago
|
See Also: → https://webcompat.com/issues/6010
Updated•8 years ago
|
See Also: → https://webcompat.com/issues/8209
Updated•8 years ago
|
See Also: → https://webcompat.com/issues/8653
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/15056
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/15275
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/15381
Updated•7 years ago
|
Whiteboard: [webcompat] → [webcompat:p1]
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6836
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7549
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/7966
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/10359
Assignee | ||
Comment 14•7 years ago
|
||
Do I understand correctly that EventDispatcher::Dispatch at https://searchfox.org/mozilla-central/source/dom/events/EventDispatcher.cpp#659 is the method in our code that should set window.event and that it should do so by obtaining the window from the doc from the nsIPresShell from nsPresContext?
Flags: needinfo?(bugs)
Comment 15•7 years ago
|
||
The code shouldn't deal with PresShell nor PresContext. Just access window from document's defaultview, assuming that is how this is spec'ed.
Setting window.event in Dispatch would work ok in non-shadow-dom case - so it should be fine for now.
Shadow DOM requires to clear window.event whenever dealing with with nodes within Shadow DOM, so I'd assume ::HandleEventTargetChain to work better, since it does the event retargeting.
Another, possibly slower option is to set window.event in EventListenerManager::HandleEvent(Internal)
Flags: needinfo?(bugs)
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #15)
> The code shouldn't deal with PresShell nor PresContext. Just access window
> from document's defaultview, assuming that is how this is spec'ed.
Where should I get the document from? It looks like the inner window is available directly via the aTarget argument of EventDispatcher::Dispatch.
> Setting window.event in Dispatch would work ok in non-shadow-dom case - so
> it should be fine for now.
> Shadow DOM requires to clear window.event whenever dealing with with nodes
> within Shadow DOM, so I'd assume ::HandleEventTargetChain to work better,
> since it does the event retargeting.
> Another, possibly slower option is to set window.event in
> EventListenerManager::HandleEvent(Internal)
This code assumes the reader to have knowledge that I don't have. It seems that the nsIDOMEvent version of the event might not exist when EventDispatcher::Dispatch is called. I gather that nsIDOMEvent is the internal type of the object we want to expose as window.event. What's the point in code where it's guaranteed that nsIDOMEvent exists already but the page JS has not seen the event yet?
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> (In reply to Olli Pettay [:smaug] (only webcomponents and event handling
> reviews, please) from comment #15)
> > The code shouldn't deal with PresShell nor PresContext. Just access window
> > from document's defaultview, assuming that is how this is spec'ed.
>
> Where should I get the document from? It looks like the inner window is
> available directly via the aTarget argument of EventDispatcher::Dispatch.
In this case when window.event should be set, aTarget can be some node or window.
IsEventTargetChrome ends up QIing target either to node or window and returns then doc.
By refactoring that code a bit, one could easily get document's defaultView
>
> This code assumes the reader to have knowledge that I don't have. It seems
> that the nsIDOMEvent version of the event might not exist when
> EventDispatcher::Dispatch is called. I gather that nsIDOMEvent is the
> internal type of the object we want to expose as window.event. What's the
> point in code where it's guaranteed that nsIDOMEvent exists already but the
> page JS has not seen the event yet?
This is a good point, I'd rather not create DOM Event all the time.
If there is listener for the event, DOM event will be created (lazily).
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/events/EventListenerManager.cpp#1177
EventDispatcher should pass a flag to EventListenerManager's HandleEvent to tell whether to set window.event
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Does the current patch check for shadow DOMness too early?
Flags: needinfo?(bugs)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Comment 22•7 years ago
|
||
Note, if window.event is set to some random value before event dispatch, it should have that value also after dispatch. Does the patch guarantee that?
For shadow DOM handling, I think we want something else. If currentTarget was in shadow DOM before event dispatch, the listeners on it shouldn't see window.event pointing to event.
But I'm ok for not dealing with Shadow DOM yet.
What happens in other browsers if a dom node is moved to another document. Which window's event is set?
I don't see need for adding mInnerWindow to the visitor object when Pre/PostHandleEvents don't actually using that. HandleEventTargetChain and HandleEvent could just take window as param.
Flags: needinfo?(bugs)
Comment 23•7 years ago
|
||
> What happens in other browsers if a dom node is moved to another document. Which window's event is set?
As far as I can tell it's the Window of the document the element was inserted in: https://github.com/w3c/web-platform-tests/pull/10329.
Updated•7 years ago
|
User Story: (updated)
Updated•7 years ago
|
User Story: (updated)
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/14875
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/16819
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/14224
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/16903
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/13513
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6656
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/6186
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/10647
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/10742
Comment 24•7 years ago
|
||
Henri tells me his patch fixes the simple case. For the Shadow DOM case and the case of the listeners modifying the listener chain during event dispatch, he's waiting for spec work.
Comment 25•7 years ago
|
||
It seems like we did reach a conclusion in https://github.com/whatwg/dom/issues/334, although it wasn't entirely satisfactory. I'll try to align the DOM PR and tests with that conclusion and ping this when done.
Flags: needinfo?(annevk)
Comment 26•7 years ago
|
||
I updated https://github.com/whatwg/dom/pull/407 and the associated tests. If someone could review all (or part of) those that'd be much appreciated.
Flags: needinfo?(annevk)
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/16630
Comment 27•7 years ago
|
||
FYI: all the relevant standard and test PRs have now been merged.
Updated•7 years ago
|
Priority: -- → P2
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/17319
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
(In reply to Anne (:annevk) from comment #27)
> FYI: all the relevant standard and test PRs have now been merged.
The current patch fails "window.event is undefined if the target is in a shadow tree (event dispatched inside shadow tree)" from https://github.com/web-platform-tests/wpt/pull/4790 and passes the other tests from that PR.
Assignee | ||
Comment 31•7 years ago
|
||
How should "If target is a node and its root is a shadow root, then set item-in-shadow-tree to true." (https://dom.spec.whatwg.org/#concept-event-path-append) be checked and where? In the current patch, the "preTarget && preTarget->SubtreeRoot()->IsShadowRoot()" check seems good enough when the even is dispatched from outside a shadow tree to a target in a shadow tree but not good enough for dispatching to shadow tree from within a shadow tree.
Flags: needinfo?(bugs)
Comment 32•7 years ago
|
||
I don't understand the question. what does "the event is dispatched from outside a shadow tree to a target in a shadow tree" mean?
Does 'window.event is undefined if the target is in a shadow tree (event dispatched inside shadow tree)' fail because on parent node window.event is undefined?
Flags: needinfo?(bugs)
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #32)
> I don't understand the question.
I'm asking how the bit in https://dom.spec.whatwg.org/#concept-event-path-append that sets item-in-shadow-tree to true should map to Gecko's event dispatching code.
> what does "the event is dispatched from
> outside a shadow tree to a target in a shadow tree" mean?
It means that dispatchEvent is called on a node in the shadow tree. See
https://github.com/web-platform-tests/wpt/pull/4790/files#diff-ca51d95009792cf7fa01bb56e6929a24R45
> Does 'window.event is undefined if the target is in a shadow tree (event
> dispatched inside shadow tree)' fail because on parent node window.event is
> undefined?
Yes. The failure is:
assert_equals: expected (object) object "[object Event]" but got (undefined) undefined
@https://web-platform.test:8443/dom/events/event-global.html:58:5
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1535:20
Test.prototype.step_func_done/<@https://web-platform.test:8443/resources/testharness.js:1575:17
@https://web-platform.test:8443/dom/events/event-global.html:62:3
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1535:20
async_test@https://web-platform.test:8443/resources/testharness.js:562:13
@https://web-platform.test:8443/dom/events/event-global.html:45:1
Flags: needinfo?(bugs)
Comment 34•7 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #33)
> > what does "the event is dispatched from
> > outside a shadow tree to a target in a shadow tree" mean?
>
> It means that dispatchEvent is called on a node in the shadow tree. See
> https://github.com/web-platform-tests/wpt/pull/4790/files#diff-
> ca51d95009792cf7fa01bb56e6929a24R45
What does "outside" mean there?
So item-in-shadow-tree could be set in nsIContent::GetEventTargetParent.
If IsInShadowTree() returns true, the flag should be set.
The flag should be added to the aVisitor object, and then in
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/events/EventDispatcher.cpp#478 copied to the EventTargetChainItem so that it can be used during
event dispatch.
Flags: needinfo?(bugs)
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/17438
Assignee | ||
Comment 35•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #34)
> (In reply to Henri Sivonen (:hsivonen) from comment #33)
>
> > > what does "the event is dispatched from
> > > outside a shadow tree to a target in a shadow tree" mean?
> >
> > It means that dispatchEvent is called on a node in the shadow tree. See
> > https://github.com/web-platform-tests/wpt/pull/4790/files#diff-
> > ca51d95009792cf7fa01bb56e6929a24R45
> What does "outside" mean there?
That's a question for annevk.
> So item-in-shadow-tree could be set in nsIContent::GetEventTargetParent.
> If IsInShadowTree() returns true, the flag should be set.
> The flag should be added to the aVisitor object, and then in
> https://searchfox.org/mozilla-central/rev/
> 39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/events/EventDispatcher.cpp#478
> copied to the EventTargetChainItem so that it can be used during
> event dispatch.
Thanks.
Flags: needinfo?(annevk)
Assignee | ||
Comment 36•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #34)
> So item-in-shadow-tree could be set in nsIContent::GetEventTargetParent.
> If IsInShadowTree() returns true, the flag should be set.
> The flag should be added to the aVisitor object, and then in
> https://searchfox.org/mozilla-central/rev/
> 39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/events/EventDispatcher.cpp#478
> copied to the EventTargetChainItem so that it can be used during
> event dispatch.
Is there a cheap way to do the check without the visitor before the visitor is created? Failing that, is there a guarantee that nsIContent::GetEventTargetParent() runs before the DOM event is lazily created on the visitor?
Flags: needinfo?(bugs)
Comment 37•7 years ago
|
||
IsInShadowTree() is very cheap, and it needs to be done on all the (Element/Text) targets on the path, no?
What has DOM event creation to do with this? Event handling happens after GetEventTargetParent() has been called on all the targets so that event path is created.
Flags: needinfo?(bugs)
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #37)
> IsInShadowTree() is very cheap, and it needs to be done on all the
> (Element/Text) targets on the path, no?
I'm still not familiar enough with this area to be sure about that.
> What has DOM event creation to do with this?
Since the event is created lazily, it's difficult to do stuff with the event, specifically to set it as window.event, from elsewhere than the lazy creation point. Therefore, the information about whether window.event needs to be set needs to propagate to the point where the event is lazily created.
It's still a bit unclear to me what the place to restore the previous window.event is and where the previous one should be held.
(This would all be so much simpler without the shadow tree special casing.)
Assignee | ||
Comment 39•7 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #38)
> It's still a bit unclear to me what the place to restore the previous
> window.event is and where the previous one should be held.
I think I've now located the right place for this, but now it's unclear to me how "tuple" from "inner invoke" in the spec appears in EventListenerManager::HandleEventInternal.
Comment hidden (mozreview-request) |
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/17434
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
With the attached patch, the WPT test case passes.
Flags: needinfo?(annevk)
![]() |
||
Updated•7 years ago
|
See Also: → https://webcompat.com/issues/12436
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•7 years ago
|
||
...and https://searchfox.org/mozilla-central/source/testing/web-platform/tests/dom/events/event-global-extra.window.js showed up and most of the tests in it don't pass. :-( :-( :-(
How does GetInnerWindowForTarget() in EventListenerManager returning a non-null nsPIDOMWindowInner* differ from "listener callback’s associated Realm’s global object" "is a Window object"?
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #46)
> How does GetInnerWindowForTarget() in EventListenerManager returning a
> non-null nsPIDOMWindowInner* differ from "listener callback’s associated
> Realm’s global object" "is a Window object"?
The failures are:
Fail window.event for constructors from another global: EventTarget assert_equals: expected (undefined) undefined but got (object) object "[object Event]"
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:8:7
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1578:17
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:10:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
async_test@http://web-platform.test:8000/resources/testharness.js:565:13
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:4:3
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:3:1
Fail window.event for constructors from another global: XMLHttpRequest assert_equals: expected (undefined) undefined but got (object) object "[object Event]"
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:8:7
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1578:17
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:10:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
async_test@http://web-platform.test:8000/resources/testharness.js:565:13
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:4:3
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:3:1
Pass window.event and element from another document
Fail window.event and moving an element post-dispatch assert_equals: expected (object) object "[object Event]" but got (undefined) undefined
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:32:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:45:3
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
async_test@http://web-platform.test:8000/resources/testharness.js:565:13
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:25:1
Fail window.event should not be affected by nodes moving post-dispatch assert_equals: expected (object) object "[object Event]" but got (undefined) undefined
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:64:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:71:3
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
test@http://web-platform.test:8000/resources/testharness.js:548:9
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:48:1
Fail Listener from a different global assert_equals: expected (object) object "[object Event]" but got (undefined) undefined
frame.onload<@http://web-platform.test:8000/dom/events/event-global-extra.window.js:87:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1578:17
EventHandlerNonNull*@http://web-platform.test:8000/dom/events/event-global-extra.window.js:78:18
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
async_test@http://web-platform.test:8000/resources/testharness.js:565:13
@http://web-platform.test:8000/dom/events/event-global-extra.window.js:75:1
In general, GetInnerWindowForTarget() returns a different window compared to what the tests expect.
Assignee | ||
Comment 49•7 years ago
|
||
Ah, GetInnerWindowForTarget() returns the window for the target node, but the spec wants the window for the callback.
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•7 years ago
|
||
I gave up and requested review with two edge cases in https://searchfox.org/mozilla-central/source/testing/web-platform/tests/dom/events/event-global-extra.window.js still failing. This bug blocks an unusual number of Web compat issues, so let's get those fixed instead of chasing edge cases endlessly.
As the commit message says, when the target and the callback are from different realms and callback is an XPCOM callback, window.event is set on the wrong window. This is because AFAICT we don't support extracting the global from an XPCOM callback.
Not sure why "window.event should not be affected by nodes moving post-dispatch" is failing.
Comment 52•7 years ago
|
||
Isn't "window.event should not be affected by nodes moving post-dispatch" buggy.
Why assert_equals(window.event, e) checks there are right? Is that the part which is failing?
assert_equals(window.event, e) check should not-fail only on host level.
Comment 53•7 years ago
|
||
mozreview-review |
Comment on attachment 8964896 [details]
Bug 218415 - Add window.event. .
https://reviewboard.mozilla.org/r/233638/#review261244
Some small tweak needed, but looking good.
::: dom/base/nsPIDOMWindow.h:620
(Diff revision 11)
>
> + // Sets the event for window.event. Does NOT take ownership, so
> + // the caller is responsible for clearing the event before the
> + // event gets deallocated. Pass nullptr to set window.event to
> + // undefined. Returns the previous value.
> + virtual mozilla::dom::Event* SetEvent(mozilla::dom::Event* aEvent) = 0;
This needs to be non-virtual. We're calling this in super hot code path. So either make nsPIDOMWindowInner to have the mEvent member variable, or just implement this is nsGlobalWindowInner as non-virtual and make caller to use nsGlobalWindowInner::Cast before calling.
::: dom/events/EventListenerManager.cpp:1175
(Diff revision 11)
> +EventListenerManager::WindowFromListener(Listener* aListener,
> + bool aItemInShadowTree)
> +{
> + nsCOMPtr<nsPIDOMWindowInner> innerWindow;
> + if (!aItemInShadowTree) {
> + nsCOMPtr<nsIGlobalObject> global;
This doesn't need to be nsCOMPtr. nsIGlobalObject* is fine, and faster.
::: dom/events/EventListenerManager.cpp:1177
(Diff revision 11)
> +{
> + nsCOMPtr<nsPIDOMWindowInner> innerWindow;
> + if (!aItemInShadowTree) {
> + nsCOMPtr<nsIGlobalObject> global;
> + if (aListener->mListener.HasWebIDLCallback()) {
> + auto callback = aListener->mListener.GetWebIDLCallback();
I would prefer if this wasn't using auto
::: dom/events/EventListenerManager.cpp:1181
(Diff revision 11)
> + if (aListener->mListener.HasWebIDLCallback()) {
> + auto callback = aListener->mListener.GetWebIDLCallback();
> + if (callback) {
> + global = callback->IncumbentGlobalOrNull();
> + }
> + innerWindow = do_QueryInterface(global);
so if there was a helper in nsIGlobalObject, this could be just
global->GetAsInnerWindow() or some such, no need to QI
Attachment #8964896 -
Flags: review?(bugs) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 55•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8964896 [details]
Bug 218415 - Add window.event. .
https://reviewboard.mozilla.org/r/233638/#review261244
> This needs to be non-virtual. We're calling this in super hot code path. So either make nsPIDOMWindowInner to have the mEvent member variable, or just implement this is nsGlobalWindowInner as non-virtual and make caller to use nsGlobalWindowInner::Cast before calling.
Moved `mEvent` to `nsPIDOMWindowInner` to make the call non-virtual.
> This doesn't need to be nsCOMPtr. nsIGlobalObject* is fine, and faster.
Used plain pointer.
> I would prefer if this wasn't using auto
Wrote out the type instead of using `auto`.
> so if there was a helper in nsIGlobalObject, this could be just
> global->GetAsInnerWindow() or some such, no need to QI
Added a special QI avoidance helper.
Assignee | ||
Comment 56•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #52)
> Isn't "window.event should not be affected by nodes moving post-dispatch"
> buggy.
Possibly. In all the other cases, the tests were right and I misunderstood something, so I didn't look at the last case too carefully.
Assignee | ||
Comment 57•7 years ago
|
||
Adding NI, because I still don't know how to re-request review on ReviewBoard.
Flags: needinfo?(bugs)
Comment 59•7 years ago
|
||
(And one can always ask review in bugzilla. Reviews showing up in bugzilla are anyhow the only ones I see)
Comment 60•7 years ago
|
||
mozreview-review |
Comment on attachment 8964896 [details]
Bug 218415 - Add window.event. .
https://reviewboard.mozilla.org/r/233638/#review261582
Keep the window object alive during event handling, r+
::: dom/base/nsIGlobalObject.h:57
(Diff revision 12)
> +
> + bool mIsInnerWindow;
> +
> nsIGlobalObject()
> : mIsDying(false)
> + , mIsInnerWindow(false)
Oh, this way. Nice, non-virtual way to access inner window. I was thinking virtual, but this is better.
::: dom/events/EventListenerManager.cpp:1168
(Diff revision 12)
>
> +nsPIDOMWindowInner*
> +EventListenerManager::WindowFromListener(Listener* aListener,
> + bool aItemInShadowTree)
> +{
> + nsPIDOMWindowInner* innerWindow = nullptr;
I didn't ask this to be raw pointer, since the caller anyhow needs to keep the window object alive. So, nsCOMPtr here and return already_Addrefed
::: dom/events/EventListenerManager.cpp:1292
(Diff revision 12)
> listener = listenerHolder.ptr();
> hasRemovedListener = true;
> }
>
> nsresult rv = NS_OK;
> + nsPIDOMWindowInner* innerWindow =
So this unfortunately must be nsCOMPtr
::: dom/events/EventListenerManager.cpp:1332
(Diff revision 12)
> #endif
> {
> rv = HandleEventSubType(listener, *aDOMEvent, aCurrentTarget);
> }
> + if (innerWindow) {
> + Unused << innerWindow->SetEvent(oldWindowEvent);
because otherwise this might point to a deleted object
Attachment #8964896 -
Flags: review?(bugs) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 62•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8964896 [details]
Bug 218415 - Add window.event. .
https://reviewboard.mozilla.org/r/233638/#review261582
Thanks!
I also modified `testing/web-platform/meta/html/semantics/embedded-content/media-elements/track/track-element/track-remove-track.html.ini` still to remove an expected failure. Looks like `window.event` was used in a `window.event`-unrelated test.
> So this unfortunately must be nsCOMPtr
Changed back.
Comment 63•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s e4299f6ee46458f67b9669e402534116a6872b27 -d 346b316a0fe4: rebasing 471197:e4299f6ee464 "Bug 218415 - Add window.event. r=smaug." (tip)
merging dom/base/nsGlobalWindowInner.cpp
merging dom/base/nsGlobalWindowInner.h
merging dom/base/nsIGlobalObject.cpp
merging dom/base/nsIGlobalObject.h
merging dom/base/nsPIDOMWindow.h
merging dom/webidl/Window.webidl
warning: conflicts while merging dom/base/nsPIDOMWindow.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 65•7 years ago
|
||
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27257fdd6c67
Add window.event. r=smaug.
Comment 66•7 years ago
|
||
Backed out changeset 27257fdd6c67 (bug 218415) for xpcshell failures in js/xpconnect/tests/unit/test_nuke_sandbox_event_listeners.js
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=27257fdd6c6777ca72ac965501da3ef360846bb4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=186430127
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186430127&repo=autoland&lineNumber=1458
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b1585f3426c28f984e48179eed30d56297b0c95d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=xpcshell
Flags: needinfo?(hsivonen)
Comment hidden (mozreview-request) |
Comment 68•7 years ago
|
||
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/306ec43e7ab9
Add window.event. r=smaug.
Assignee | ||
Comment 69•7 years ago
|
||
(In reply to Eliza Balazs [:ebalazs_] from comment #66)
> Backed out changeset 27257fdd6c67 (bug 218415) for xpcshell failures in
> js/xpconnect/tests/unit/test_nuke_sandbox_event_listeners.js
A null check was missing. (Wasn't necessary when using QI but is necessary when not using QI.) The patch is now in the autoland queue with the null check added.
Sorry and thanks.
Flags: needinfo?(hsivonen)
Comment 70•7 years ago
|
||
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/495f61d7af01
Backed out changeset 306ec43e7ab9 for failures on track-remove-track-inband.html CLOSED TREE
Comment hidden (mozreview-request) |
Assignee | ||
Comment 72•7 years ago
|
||
Try run with track-remove-track-inband.html disabled (was marked as a failure before this patch anyway):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3f3aaee2f13b67af2394bc40528d7835fb6d6f1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 74•7 years ago
|
||
Actually with the right test disabled this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a21911561663ad7d51dba5a25305df0991fee03
Comment 75•7 years ago
|
||
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e32f9974bd5
Add window.event. r=smaug.
Comment 76•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 77•7 years ago
|
||
I've verified all the "see also" and dependent bugs (and re-opened for diagnosis when not fixed) -- the vast majority are taken care of by this bug. Thanks everyone!
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
![]() |
||
Comment 78•7 years ago
|
||
fyi. Regression because of window.event support
https://webcompat.com/issues/17935#issuecomment-409045236
Comment 79•7 years ago
|
||
I was hit by this fix for window.event with some company-internal software from the stone age. It uses the following (terrible) code to retrieve a key code for a keyboard event:
function GetValidKeyCode(event)
{
var keyCode;
if(window.event != null) //IE
{
keyCode = event.keyCode;
}
else //Netscape/Firefox/Opera
{
keyCode = event.which;
}
return keyCode;
}
With Firefox now supporting window.event, that first case is being hit, but `event.keyCode` is always 0. So this software is now broken on Firefox. Other browsers correctly populate `event.keyCode` here.
I’ve already asked for this code to be changed (since it certainly does not do what it says it does as all browsers will go through the first path now although the latter is certainly better) but is there anything we can do about `event.keyCode` to be correct? Is there a bug about that?
![]() |
||
Comment 80•7 years ago
|
||
Patrick, I created Bug 1479964 (following the discussion in the webcompat team on Tuesday, July 31, 2018)
https://wiki.mozilla.org/Compatibility/Meetings/2018-07-31
Comment 82•6 years ago
|
||
Can you please uplift this bug to Fx62 (62.0.3?) as it's appearing on www.rocketleague.com, which has over 4 million visitors per month (of which 1-2 million Firefox 62 alone probably), and on that website there is a huge bottom banner about privacy policy which can't be closed currently on FF 62.0.2. (in Chrome browser it can already)
Comment 83•6 years ago
|
||
Sorry, I meant another website, I meant http://www.rocketleagueesports.com/ , and has over 500.000 visits per month.
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 84•6 years ago
|
||
Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/Window
https://developer.mozilla.org/en-US/docs/Web/API/Window/event
BCD PR submitted: https://github.com/mdn/browser-compat-data/pull/2757
Added to Firefox 63 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Comment 85•6 years ago
|
||
Can we please uplift it to FF 62.0.1 soon for example? (for the reason I stated above)
Flags: needinfo?(eshepherd)
Comment 86•6 years ago
|
||
Hi Daniel, sorry no -- this is something that we feel should ride the trains given how big a change this is, and the potential for compat fallout. A full 63 beta cycle will help us understand the risks better. Thanks.
Flags: needinfo?(eshepherd)
Comment 87•6 years ago
|
||
You could also try to get in touch with the site developers -- their fix is to change the following:
$("#close-message").click(function(){
event.preventDefault();
$("#message").addClass("close-message");
});
to
$("#close-message").click(function(event){
event.preventDefault();
$("#message").addClass("close-message");
});
Comment 88•6 years ago
|
||
Alright, thanks for the reply. It's not possible to contact them. (when clicking "submit" in their contact form, a message "This request was blocked by our web application firewall" appears... omg)
Can you explain "ride the trains" and "compat fallout", please? I don't know what those mean.
Flags: needinfo?(miket)
Comment 89•6 years ago
|
||
Yeah, no problem!
"Ride the trains" is a term we use to describe the process of a change or feature that will land in Nightly, then ~6 weeks later, advance to Beta, then ~6 weeks later, advance to Release. https://en.wikipedia.org/wiki/Software_release_train explains the "train" theory a bit. Ideally if there are bugs we can fix them, or decide to not ship the feature during this time.
"compat fallout" describes the situation where we might implement something like window.event, and then later discover that some sites use that code to, for example, detect a user on IE then serve some code that only works in IE6. Or something like that. So we will have learned that it's not safe to ship. In reality, for a change like this, which fixes a ton of bugs, we'd need some really good evidence to want to do that (perhaps if it broke YouTube or something, we'd be more convinced).
Hope that helps!
Flags: needinfo?(miket)
Comment 90•6 years ago
|
||
How do you know that it's depending on that credit card number bug?
Flags: needinfo?(pascalc)
Updated•6 years ago
|
Keywords: site-compat
Updated•6 years ago
|
Flags: needinfo?(pascalc)
Comment 91•6 years ago
|
||
Update: too risky to ship to release until Bug 1479964 is fixed, unfortunately. :(
Updated•6 years ago
|
status-firefox64:
--- → fixed
Comment 92•6 years ago
|
||
Comment 94•6 years ago
|
||
Release Note Request
[Why is this notable]: Window.event and related keycode changes are shipping to ESR in 68
[Affects Firefox for Android]: Yes
[Suggested wording]:
[Links (documentation, blog post, etc)]: Will provide a SUMO link with workaround instructions.
relnote-firefox:
--- → ?
Comment 96•6 years ago
|
||
This is in the draft 68esr relnotes, for reference the sumo link is https://support.mozilla.org/kb/dom-events-changes-introduced-firefox-66
Flags: needinfo?(jcristau)
You need to log in
before you can comment on or make changes to this bug.
Description
•