1017086 - Don't expose onpointer* on global when dom.w3c_pointer_events.enabled is false

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Mike Taylor [:miketaylr]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Masatoshi Kimura [:emk]]

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

Comment 4

[Florian Bender]

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?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Maksim Lebedev]

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

Comment 7

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver1.diff

Comments and suggestions are welcome.

Comment 8

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver2.diff

+ Changes: Added two interfaces: pointerEvents and PointerCaptureEvents

Comments and suggestion are welcome.

https://tbpl.mozilla.org/?tree=Try&rev=0fe9cfa69497

Comment 9

[Masatoshi Kimura [:emk]]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Oleg Romashin (MS)]

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

Comment 12

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_test_ver1.diff

+ Changes: Test for mochitest system

Comments and suggestions are welcome.

Comment 13

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver3.diff

+ Changes: Moved interface from Document to Window

Comment 14

[Maksim Lebedev]

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

[Oleg Romashin (MS)]

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);
>+  }

Comment 16

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_test_ver2.diff

+ 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 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8437604 [details] [diff] [review]
allocate_pointer_events_interface_test_ver2.diff

No need to ask so many different feedbacks or reviews.

Comment 18

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver4.diff

+ Changes according with last version of source code

Comment 19

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_test_ver3.diff

+ Changes: tests were separated on two files

Comment 20

[Maksim Lebedev]

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[Maksim Lebedev]

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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 25

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver5.diff

+ Changes according with comments

Comment 26

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_test_ver4.diff

+ Changes: decrease code in test files

Comment 27

[Maksim Lebedev]

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

Comment 28

[Maksim Lebedev]

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

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

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 30

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 31

[Maksim Lebedev]

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

Comment 32

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_ver6.diff

- Changes: Removed .webidl part

Comment 33

[Maksim Lebedev]

Attachment: allocate_pointer_events_interface_test_ver5.diff

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

Comment 34

[Maksim Lebedev]

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

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=65d80aff29e3

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 37

[Maksim Lebedev]

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

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Ask approval for beta for the patch.

Comment 39

[Maksim Lebedev]

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

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 41

[Maksim Lebedev]

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 42

[Maksim Lebedev]

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]:

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 44

[Maksim Lebedev]

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

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

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

Comment 47

[Maksim Lebedev]

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

Comment 48

[Sylvestre Ledru [:Sylvestre]]

See comments #41 & #42 :)

Comment 49

[Maksim Lebedev]

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 50

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8453685 [details] [diff] [review]
allocate_pointer_events_interface_ver6.diff

We cannot accept uplift requests with IDL changes.

Comment 51

[Maksim Lebedev]

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?

Comment 52

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 53

[Mike Taylor [:miketaylr]]

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

Comment 54

[Maksim Lebedev]

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

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 56

[Maksim Lebedev]

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

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Fine.

Comment 58

[Maksim Lebedev]

Comment on attachment 8453685 [details] [diff] [review]
allocate_pointer_events_interface_ver6.diff

- Changes: Removed flag "patch" on this patch

Comment 59

[Maksim Lebedev]

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

Comment 60

[Maksim Lebedev]

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

Comment 61

[Florian Bender]

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

Comment 62

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 63

[Maksim Lebedev]

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

Comment 64

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=049c891eafb6

Comment 65

[Maksim Lebedev]

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

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ad27333f31

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70ad27333f31