Currently onpointerup, onpointerover, onpointerout, onpointermove, onpointerleave, onpointerenter, onpointerdown, onpointercancel are exposed on the global object, even with dom.w3c_pointer_events.enabled set to `false`. This breaks feature detection of the form: if ('onpointerup' in window) {...}, which breaks Google Play on mobile.

How... http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl?rev=417acde736e7#91

Oh, I see, http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIInlineEventHandlers.idl#73 :( We need to move those onfoo properties to a separate interface, similar to ontouch* properties. Maksim, could you take this? I think we need to fix this asap.

Are XPCOM properties still visible from the content even after bug 789261?

A fix for this should probably be uplifted to Beta even if that means that IDL UUIDS need to change … or is there a way to "unset" the property through e.g. the hotfix addon?

(for branches) This would require idl interface changes, and there is no way for a hotfix addon.

(In reply to Olli Pettay [:smaug] from comment #2) > Maksim, could you take this? I think we need to fix this asap. I will try to help.

Comments and suggestions are welcome.

+ Changes: Added two interfaces: pointerEvents and PointerCaptureEvents Comments and suggestion are welcome. https://tbpl.mozilla.org/?tree=Try&rev=0fe9cfa69497

Comment on attachment 8435872 [details] [diff] [review] allocate_pointer_events_interface_ver2.diff Review of attachment 8435872 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/core/nsIInlineEventHandlers.idl @@ -80,5 @@ > - [implicit_jscontext] attribute jsval onpointerleave; > - [implicit_jscontext] attribute jsval ongotpointercapture; > - [implicit_jscontext] attribute jsval onlostpointercapture; > - [implicit_jscontext] attribute jsval onpointercancel; > - You will have to bump the iid. Or leaving them to avoid bumping iid and making it a bit easier to land on branches? (I'm not really sure it is actually possible.)

We want this stuff especially on branches. On trunk things should be now fine since Window uses Webidl.

Comment on attachment 8435872 [details] [diff] [review] allocate_pointer_events_interface_ver2.diff >+bool >+PointerEvent::PrefEnabled() >+{ >+ bool flag = false; nit: no need to assign false here, if GetBool succeeded then flag will be initialized, otherwise you returning false >+ if (NS_SUCCEEDED(Preferences::GetBool("dom.w3c_pointer_events.enabled", &flag))) { >+ return flag; >+ } Try results are welcome

+ Changes: Test for mochitest system Comments and suggestions are welcome.

+ Changes: Moved interface from Document to Window

(In reply to Oleg Romashin (MS) from comment #11) > nit: no need to assign false here, if GetBool succeeded then flag will be > initialized, otherwise you returning false In this case we potentially have non-initialized value.

Comment on attachment 8436990 [details] [diff] [review] allocate_pointer_events_interface_test_ver1.diff Can you wrap partone and partwo functions content into one method with bool argument, and call CheckInterface(enabled: true) or CheckInterface(enabled: false) >+ function partone() { >+ check(window, 'onpointerover', false); >+ check(window, 'onpointerenter', false); >+ check(window, 'onpointermove', false); >+ check(window, 'onpointerdown', false); >+ check(window, 'onpointerup', false); >+ check(window, 'onpointerout', false); >+ check(window, 'onpointerleave', false); ..... >+ } >+ function parttwo() { >+ check(window, 'onpointerover', true); >+ check(window, 'onpointerenter', true); >+ check(testelem, 'onpointerleave', true); ...... >+ check(testelem, 'onpointercancel', true); >+ check(testelem, 'ongotpointercapture', true); >+ check(testelem, 'onlostpointercapture', true); >+ }

+ Changes: Decrease code of testing functions Without patch: https://tbpl.mozilla.org/?tree=Try&rev=d8d254aec91f With patch: https://tbpl.mozilla.org/?tree=Try&rev=ab62f1f3001a Looks like, we cannot manage interfaces according with preferences during work

Comment on attachment 8437604 [details] [diff] [review] allocate_pointer_events_interface_test_ver2.diff No need to ask so many different feedbacks or reviews.

+ Changes according with last version of source code

+ Changes: tests were separated on two files

Tests without patch: https://tbpl.mozilla.org/?tree=Try&rev=15496a75b525 Tests with patch: https://tbpl.mozilla.org/?tree=Try&rev=29294085cfc4

Comment on attachment 8449259 [details] [diff] [review] allocate_pointer_events_interface_test_ver3.diff Couldn't you just reuse some inner file, and set pref in the parent. Then expect different results. Having almost exactly the same code in 3 files isn't too nice. And it is always better to set the pref in the main document before even loading iframe. That guarantees the right behavior.

Comment on attachment 8449252 [details] [diff] [review] allocate_pointer_events_interface_ver4.diff >+PointerEvent::PrefEnabled() >+{ >+ bool flag = false; >+ if (NS_SUCCEEDED(Preferences::GetBool("dom.w3c_pointer_events.enabled", &flag))) { >+ return flag; >+ } >+ return false; You could just do return Preferences::GetBool("dom.w3c_pointer_events.enabled"); >diff --git a/dom/interfaces/events/nsIDOMPointerEvent.idl b/dom/interfaces/events/nsIDOMPointerEvent.idl Call this file nsIPointerEventReceiver >+++ b/dom/webidl/EventHandler.webidl >@@ -83,38 +83,16 @@ interface GlobalEventHandlers { > //(Not implemented)attribute EventHandler onsort; > attribute EventHandler onstalled; > attribute EventHandler onsubmit; > attribute EventHandler onsuspend; > attribute EventHandler ontimeupdate; > attribute EventHandler onvolumechange; > attribute EventHandler onwaiting; > >- // Pointer events handlers >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointercancel; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerdown; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerup; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointermove; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerout; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerover; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerenter; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onpointerleave; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler ongotpointercapture; >- [Pref="dom.w3c_pointer_events.enabled"] >- attribute EventHandler onlostpointercapture; >- Don't remove these. These are the once which add onfoo attributes to Elements >+[NoInterfaceObject] >+interface PointerEventHandlers { >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerover; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerenter; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointermove; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerdown; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointercancel; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerup; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerout; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onpointerleave; >+}; >+ >+[NoInterfaceObject] >+interface PointerCaptureEventHandlers { >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler ongotpointercapture; >+ [Pref="dom.w3c_pointer_events.enabled"] >+ attribute EventHandler onlostpointercapture; >+}; >+ No need for this stuff. This bug is just about window having onpointerfoo (on beta). Elements and Documents use already webidl, so Pref works fine there. Window just isn't using webidl binding in the release/beta.

(In reply to Olli Pettay [:smaug] from comment #22) > This bug is just about window having onpointerfoo (on beta). Elements and > Documents use already webidl, so Pref works fine there. > Window just isn't using webidl binding in the release/beta. Look like, without this changes, we will get this errors: 1541 INFO TEST-UNEXPECTED-FAIL | /tests/dom/events/test/test_bug1017086_enable.html | ongotpointercapture in window should be false - got true, expected false According with specification, window should not have this possibilities. 1545 INFO TEST-UNEXPECTED-FAIL | /tests/dom/events/test/test_bug1017086_enable.html | onpointermove in document should be false - got true, expected false Seems that document should not have this possibilities too. But I can wrong.

Did you test beta? We want the patch for beta only (and it is possible that it is too late for that too).

+ Changes according with comments

+ Changes: decrease code in test files

(In reply to Olli Pettay [:smaug] from comment #24) > Did you test beta? We want the patch for beta only (and it is possible that > it is too late for that too). No, I didn't. I use m-c always.

Tests without patch: https://tbpl.mozilla.org/?tree=Try&rev=24e0b971e441 Tests with patch: https://tbpl.mozilla.org/?tree=Try&rev=b0dffa94ed0e

Comment on attachment 8452324 [details] [diff] [review] allocate_pointer_events_interface_ver5.diff r+, but the .webidl changes shouldn't be there. That is about different bug. Could you please file a new bug and make the .webidl changes there, and upload a new patch here without those changes.

Comment on attachment 8452329 [details] [diff] [review] allocate_pointer_events_interface_test_ver4.diff And add the tests for "ongotpointercapture" "onlostpointercapture" in that follow bug, not in this bug. Rename turnPointerEvents to turnOffPointerEvents and turnOnPointerEvents in _disable and _enable test files.

Bug 1036817 - got(lost)pointercapture events should be triggered only at element

- Changes: Removed .webidl part

+ Changes: turnPointerEvents renamed to turnOnOffPointerEvents. - Changes: removed ongot(lost)pointercapture events part

(In reply to Olli Pettay [:smaug] from comment #30) > Rename turnPointerEvents to turnOffPointerEvents and turnOnPointerEvents in > _disable and _enable test files. Unfortunately, we should use one interface, because we have one common inner test file.

Comment on attachment 8453685 [details] [diff] [review] allocate_pointer_events_interface_ver6.diff So then ask approval where this is needed. It is possible, and even likely that this isn't approved for beta, and AFAIK, this isn't needed for Aurora nor trunk.

(In reply to Olli Pettay [:smaug] from comment #36) > It is possible, and even likely that this isn't approved for beta, and > AFAIK, this isn't needed for Aurora nor trunk. What should I do with this bug? Will we land it to m-c trunc?

Ask approval for beta for the patch.

(In reply to Olli Pettay [:smaug] from comment #38) > Ask approval for beta for the patch. How I can do it?

Click the details link next to a patch, and there you have approval‑mozilla‑beta.

Comment on attachment 8453685 [details] [diff] [review] allocate_pointer_events_interface_ver6.diff Approval Request Comment [Feature/regressing bug #]: 1017086 [User impact if declined]: [Describe test coverage new/current, TBPL]: [Risks and why]: [String/UUID change made/needed]:

Comment on attachment 8453688 [details] [diff] [review] allocate_pointer_events_interface_test_ver5.diff Approval Request Comment [Feature/regressing bug #]: 1017086 [User impact if declined]: [Describe test coverage new/current, TBPL]: [Risks and why]: [String/UUID change made/needed]:

We're about to release next version like tomorrow, so this is too late. But can we verify onpointer* properties are properly disabled on Aurora and Trunk when the pref is not set.

Should we check-in this bug to m-c?

As I've said, there isn't need to take this to m-c since m-c uses webidl bindings for Window. We could land the tests though, assuming they work on m-c.

Maksim, could you answer to the questions in the uplift requests? Thanks

(In reply to Sylvestre Ledru [:sylvestre] from comment #46) > Maksim, could you answer to the questions in the uplift requests? Thanks Where I can find this questions?

See comments #41 & #42 :)

Allocate pointer events interface to independent item and test for that. r=smaug [Feature/regressing bug #]: 1017086 [User impact if declined]: Some sites will be uncorrect detect support of pointer events of FF [Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=65d80aff29e3 [Risks and why]: Risk is small because pointer event is still turned off [String/UUID change made/needed]: Yes. On nsIInlineEventHandlers and nsIPointerEventReceiver

Comment on attachment 8453685 [details] [diff] [review] allocate_pointer_events_interface_ver6.diff We cannot accept uplift requests with IDL changes.

Should I put obsolete flag to allocate_pointer_events_interface_ver6.diff ? And only after that put checkin flag to bug (to land this to m-c)? What is my next actions for this bug?

Note: I wrote these questions before seeing Sylvestre mark as beta- but I think the answers are still useful in this case. (In reply to Maksim Lebedev from comment #49) > [Feature/regressing bug #]: 1017086 Bug 1017086 is this bug. In what bug was this issue introduced into Firefox? > [User impact if declined]: Some sites will be uncorrect detect support of > pointer events of FF Mike pointed out in the description that this breaks Google Play on mobile. Mike - Does this impact all of Google Play or only a subset? How well does Google Play work in Fennec and B2G? Do you know of other sites that this impacts? Maksim - Given that we shipped this in 31, how important is it that we fix this in 32? Can we live with this functionality being exposed for another cycle? > [Risks and why]: Risk is small because pointer event is still turned off > [String/UUID change made/needed]: Yes. On nsIInlineEventHandlers and > nsIPointerEventReceiver We don't normally take UUID changes on beta. I think we need a compelling case to take these changes. Is there no way to respect the pref without making UUID changes? Olli - In comment 36 you questioned whether this was needed on 32. In comment 45 you confirmed that this bug isn't needed on 33. Can you confirm that this change is relevant for 32? (In reply to Maksim Lebedev from comment #51) > Should I put obsolete flag to allocate_pointer_events_interface_ver6.diff ? > And only after that put checkin flag to bug (to land this to m-c)? > What is my next actions for this bug? Given comment 45, if we don't take this change on beta I think the next action is to resolve this bug as won't fix.

(In reply to Lawrence Mandel [:lmandel] from comment #52) > Mike - Does this impact all of Google Play or only a subset? How well does > Google Play work in Fennec and B2G? Do you know of other sites that this > impacts? When reported, it affected the menu not being able to open or close. But according to https://bugzilla.mozilla.org/show_bug.cgi?id=1015600#c9 and my tests just now, it does open &close now (it's just super janky/jumpy--seems unrelated to this bug).

(In reply to Lawrence Mandel [:lmandel] from comment #52) > Maksim - Given that we shipped this in 31, how important is it that we fix > this in 32? Can we live with this functionality being exposed for another > cycle? I don't know about how many sites use this detections, how many users will come in that sites. I know only about that FF provide incorrect info about his behavior to sites, which analizes this info about FF behavior. > We don't normally take UUID changes on beta. I think we need a compelling > case to take these changes. Is there no way to respect the pref without > making UUID changes? Theoretically, I can provide this patch without strings with UUID changes, but I don't know about responsibilities of UUID. > Given comment 45, if we don't take this change on beta I think the next > action is to resolve this bug as won't fix. I think the last word should be from Olli (or other person from Mozilla community). In any case this solution should not be mine.

(In reply to Lawrence Mandel [:lmandel] from comment #52) > Olli - In comment 36 you questioned whether this was needed on 32. In > comment 45 you confirmed that this bug isn't needed on 33. Can you confirm > that this change is relevant for 32? This was needed for 31. 32 should have webidl bindings for Window object and this shouldn't be needed anymore. > Given comment 45, if we don't take this change on beta I think the next > action is to resolve this bug as won't fix. won't fix is fine, and then file a different bug to just land the tests.

(In reply to Olli Pettay [:smaug] from comment #55) > won't fix is fine, and then file a different bug to just land the tests. Maybe we can land only patch with test in this bug? (Without new bug)

Fine.

Comment on attachment 8453685 [details] [diff] [review] allocate_pointer_events_interface_ver6.diff - Changes: Removed flag "patch" on this patch

If nobody have any objections, we will land only tests for this bug.

Can anybody put "checkin-needed" keyword to this bug?

Which patches should be ckecked in and on which branch (if not m-i/m-c)?

(In reply to Florian Bender from comment #61) > Which patches should be ckecked in and on which branch (if not m-i/m-c)? I think the answer is just the test patch - allocate_pointer_events_interface_test_ver5.diff Let's start with the tests on inbound/central and confirm that they're good. We can then look to uplift to aurora and beta if desired.

(In reply to Lawrence Mandel [:lmandel] from comment #62) > (In reply to Florian Bender from comment #61) > > Which patches should be ckecked in and on which branch (if not m-i/m-c)? > I think the answer is just the test patch - allocate_pointer_events_interface_test_ver5.diff Yes. I think we should land patch on trunc at least. > Let's start with the tests on inbound/central and confirm that they're good. > We can then look to uplift to aurora and beta if desired. In on of comments above I wrote try test already, but if you want I will try to retest this one more time.

Can anybody put "checkin-needed" keyword to this bug?