Closed Bug 218415 Opened 21 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
relnote-firefox --- 68+
firefox63 --- disabled
firefox64 --- fixed

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.
> <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
*** Bug 272820 has been marked as a duplicate of this bug. ***
*** Bug 290904 has been marked as a duplicate of this bug. ***
*** Bug 292193 has been marked as a duplicate of this bug. ***
Whiteboard: DUPEME
See Also: → 453968
Blocks: 1265258
Blocks: 501873
Blocks: 1185099
Blocks: 959137
Whiteboard: [webcompat]
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.
Blocks: 804710
Blocks: 905860
Blocks: 1243568
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 → ---
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.
Blocks: 244510
Blocks: 402476
Flags: webcompat?
Blocks: 1217881
Flags: webcompat? → webcompat+
Blocks: 1424634
Whiteboard: [webcompat] → [webcompat:p1]
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)
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)
(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)
(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
Flags: needinfo?(bugs)
Does the current patch check for shadow DOMness too early?
Flags: needinfo?(bugs)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
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)
> 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.
Blocks: 1440921
See Also: → 1452569
User Story: (updated)
User Story: (updated)
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.
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)
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)
FYI: all the relevant standard and test PRs have now been merged.
Priority: -- → P2
(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.
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)
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)
(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)
(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)
(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)
(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)
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)
(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.)
(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.
With the attached patch, the WPT test case passes.
Flags: needinfo?(annevk)
...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)
(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.
Ah, GetInnerWindowForTarget() returns the window for the target node, but the spec wants the window for the callback.
Flags: needinfo?(bugs)
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.
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 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 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.
(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.
Adding NI, because I still don't know how to re-request review on ReviewBoard.
Flags: needinfo?(bugs)
I got a request again.
Flags: needinfo?(bugs)
(And one can always ask review in bugzilla. Reviews showing up in bugzilla are anyhow the only ones I see)
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 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.
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)
(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)
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
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
https://hg.mozilla.org/mozilla-central/rev/5e32f9974bd5
Status: ASSIGNED → RESOLVED
Closed: 21 years ago6 years ago
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
fyi. Regression because of window.event support
https://webcompat.com/issues/17935#issuecomment-409045236
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?
Blocks: 1479964
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
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)
Sorry, I meant another website, I meant http://www.rocketleagueesports.com/ , and has over 500.000 visits per month.
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");
});
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)
Depends on: 1493098
How do you know that it's depending on that credit card number bug?
Flags: needinfo?(pascalc)
Keywords: site-compat
No longer depends on: 1493098
Flags: needinfo?(pascalc)
Update: too risky to ship to release until Bug 1479964 is fixed, unfortunately. :(

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: --- → ?

needinfo self for the 68 esr relnote

Flags: needinfo?(jcristau)

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)
See Also: → 1888094
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: