Request for window.event object added to DOM to ease cross browser scripting

VERIFIED FIXED in Firefox 64

Status

()

P2
enhancement
VERIFIED FIXED
15 years ago
15 days ago

People

(Reporter: Eric.Gonia, Assigned: hsivonen)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, site-compat})

Trunk
mozilla63
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
webcompat +

Firefox Tracking Flags

(firefox63 disabled, firefox64 fixed)

Details

(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 attachment)

(Reporter)

Description

15 years ago
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.
> <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
(Reporter)

Comment 2

15 years ago
Can not locate a duplicate bug by searching for window.event or IE event . But 
specifying DoClick(event, 'blue') works great.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WONTFIX

Comment 3

14 years ago
*** Bug 272820 has been marked as a duplicate of this bug. ***

Comment 4

14 years ago
*** Bug 290904 has been marked as a duplicate of this bug. ***

Comment 5

14 years ago
*** Bug 292193 has been marked as a duplicate of this bug. ***

Updated

9 years ago
Duplicate of this bug: 419043

Updated

5 years ago
Whiteboard: DUPEME

Updated

2 years ago
Blocks: 501873

Updated

2 years ago
Whiteboard: [webcompat]

Comment 7

2 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.
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.
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

2 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
(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

2 years ago
Flags: webcompat?
Flags: webcompat? → webcompat+

Updated

10 months ago
Blocks: 1424634
Whiteboard: [webcompat] → [webcompat:p1]

Updated

7 months ago
Duplicate of this bug: 1439757
(Assignee)

Comment 14

7 months 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 months 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 months 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 months 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 months ago
Flags: needinfo?(bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

7 months ago
Does the current patch check for shadow DOMness too early?
Flags: needinfo?(bugs)
(Assignee)

Updated

7 months ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Comment 22

7 months 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 months 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

6 months ago
See Also: → bug 1452569
(Assignee)

Updated

5 months ago
Blocks: 1457671
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

5 months 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

5 months 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)

Comment 27

4 months ago
FYI: all the relevant standard and test PRs have now been merged.

Updated

4 months ago
Duplicate of this bug: 1469159

Updated

4 months ago
Priority: -- → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 30

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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)
(Assignee)

Comment 35

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

4 months ago
With the attached patch, the WPT test case passes.
Flags: needinfo?(annevk)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 46

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months ago
Adding NI, because I still don't know how to re-request review on ReviewBoard.
Flags: needinfo?(bugs)

Comment 58

4 months ago
I got a request again.
Flags: needinfo?(bugs)

Comment 59

4 months ago
(And one can always ask review in bugzilla. Reviews showing up in bugzilla are anyhow the only ones I see)

Comment 60

4 months 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

4 months 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

4 months 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 hidden (mozreview-request)
(Assignee)

Comment 69

4 months 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

4 months 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

4 months 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)

Comment 76

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e32f9974bd5
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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

Comment 78

3 months ago
fyi. Regression because of window.event support
https://webcompat.com/issues/17935#issuecomment-409045236

Comment 79

3 months 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?

Updated

3 months ago
Blocks: 1479964

Comment 80

3 months 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
Duplicate of this bug: 1487887

Comment 82

2 months 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

2 months ago
Sorry, I meant another website, I meant http://www.rocketleagueesports.com/ , and has over 500.000 visits per month.
Keywords: dev-doc-needed
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

a month ago
Can we please uplift it to FF 62.0.1 soon for example? (for the reason I stated above)
Flags: needinfo?(eshepherd)
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)
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

a month 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)
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)

Updated

28 days ago
Depends on: 1493098

Comment 90

28 days ago
How do you know that it's depending on that credit card number bug?
Flags: needinfo?(pascalc)

Updated

28 days ago
Keywords: site-compat

Updated

24 days ago
Flags: needinfo?(pascalc)
Update: too risky to ship to release until Bug 1479964 is fixed, unfortunately. :(
status-firefox63: fixed → disabled
status-firefox64: --- → fixed
Blocks: 1496288
You need to log in before you can comment on or make changes to this bug.