...otherwise web content calling .stopPropagation() during capture phase prevents listeners to be called. If system group was used, one would need to explicitly check if preventDefault() was called and depending on whether to care about that do whatever the listener is doing.

The issue is videocontrols also just has content permission, so it probably cannot listen on system group. And making it able to access chrome may have security risk...

JS in XBL runs with its own privileges. I don't recall what all it can do. We could add some addSystemGroupListener to EventTarget with [Func="IsChromeOrXBL"] annontation if there is no other way to add such listener now.

(In reply to Olli Pettay [:smaug] from comment #2) > We could add some addSystemGroupListener to EventTarget with > [Func="IsChromeOrXBL"] annontation if there is no other way to add such > listener now. That sounds like a good idea.

(In reply to Olli Pettay [:smaug] from comment #0) > ...otherwise web content calling .stopPropagation() during capture phase > prevents listeners to be called. Why is this a problem? Web content controls whether it wants video controls (with the controls attribute). If it asks for controls and then breaks clicking on them, that's its own fault... if it replaces the behaviour with its own behaviour, this might actually make sense (see also e.g. bug 1181055).

(I'm kind of hoping the desired behaviour here is specced, but I'm not too optimistic that it really is...)

But how the videocontrols works is a implementation detail, which shouldn't be exposed to content badly. It should not be broken unexpectedly by the content. And we need that for fullscreenchange event anyway.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6) > But how the videocontrols works is a implementation detail, which shouldn't > be exposed to content badly. It should not be broken unexpectedly by the > content. Why not? Same thing's possible with e.g. submit buttons on forms, you can stop people typing in <input> fields using JS, etc. etc. Please articulate exactly what problem you're trying to solve here. Handwavy "don't get broken by content" arguments are not very convincing. What would content do that they expect not to break video controls that does break video controls, right now? And how would we fix that, but continue to honour the case where web developers add click event handlers to the <video> element and use it to toggle play/pause (which is pretty common - e.g. youtube does things like this!). > And we need that for fullscreenchange event anyway. We only need it for the fullscreenchange event because of the duplicate mozfullscreenchange/fullscreenchange malarkey. And that's fine, we can change how we deal with fullscreenchange. I'm just not convinced that all the other listeners need changing.

stopPropagation shouldn't affect to default handling. preventDefault() is the tool for that.

(In reply to Olli Pettay [:smaug] from comment #8) > stopPropagation shouldn't affect to default handling. preventDefault() is > the tool for that. In practice, people don't do that, though. See e.g. bug 1181055.

(In reply to :Gijs Kruitbosch from comment #9) > (In reply to Olli Pettay [:smaug] from comment #8) > > stopPropagation shouldn't affect to default handling. preventDefault() is > > the tool for that. > > In practice, people don't do that, though. See e.g. bug 1181055. Well, that fix itself makes sense but the description is... errr... The play button is supposed to act as what it looks like, so if it shows a play, it should execute play rather than "toggling the play state". This is unrelated to the topic here, I believe.

(In reply to Olli Pettay [:smaug] from comment #2) > JS in XBL runs with its own privileges. I don't recall what all it can do. > > We could add some addSystemGroupListener to EventTarget with > [Func="IsChromeOrXBL"] annontation if there is no other way to add such > listener now. It's basically a content principal (technically ExpandedPrincipal([contentPrincipal]), with the additional APIs that we expose via things like IsChromeOrXBL.

I guess I can do that directly via EventListenerOptions... So that we don't need additional methods.

Review commit: https://reviewboard.mozilla.org/r/54460/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54460/

Review commit: https://reviewboard.mozilla.org/r/54462/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54462/

I actually guess we want to listen video events in the system group as well, and do not allow them to be default prevented since we just use them to update the UI.

Review commit: https://reviewboard.mozilla.org/r/54488/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54488/

Review commit: https://reviewboard.mozilla.org/r/54490/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54490/

Comment on attachment 8755251 [details] MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug https://reviewboard.mozilla.org/r/54460/#review51162 This is quite nice approach indeed. ::: dom/events/EventListenerManager.cpp:1337 (Diff revision 1) > mTarget = nullptr; > RemoveAllListeners(); > } > > +static EventListenerFlags > +GetEventListenerFlagsFromOptions(const EventListenerOptions& aOptions) Odd that this takes just some flag from the options. Could you rename this to GetIsSystemGroupFromOptions and only take that flag

https://reviewboard.mozilla.org/r/54460/#review51162 > Odd that this takes just some flag from the options. > Could you rename this to GetIsSystemGroupFromOptions and only take that flag It takes all flags from EventListenerOptions, AddEventListenerOptions have some extra flags, though. AddEventListenerOptionsOrBoolean is not convertible to EventListenerOptionsOrBoolean, so including the aOptions.IsBoolean() branch in that function is not possible in terms of code reuse.

Oh, I see. Fine.

Comment on attachment 8755261 [details] MozReview Request: Bug 1274520 part 3 - Remove unused code in touch controls. r?gijs https://reviewboard.mozilla.org/r/54488/#review51174 If this code is dead anyway, it doesn't actually interfere with what we're doing, but it might mean the touch bindings have issues when the XBL binding gets rebound. Please instead file a followup to look at that.

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review51182 ::: toolkit/content/widgets/videocontrols.xml:1516 (Diff revision 1) > - let boundFunc = func.bind(self); > + let boundFunc = function(event) { > + if (!event.defaultPrevented) > + func.call(self, event); > + }; Nit: 'boundFunc' is now inaccurate. Name it something sensible. More generally, this changes behaviour in a way opposite to what was suggested in comment #0 in that, if our aim is to make it harder for content to 'interfere' with the video controls, we now 'obey' `defaultPrevented` on a lot more things than we used to. So for instance, you're breaking using the keyboard to interact with the video controls in this testcase: http://jsbin.com/suziketagu/1/edit?html,js,output While I understand that this is how web pages are "supposed" to be able to do this, I would fully expect a change like this to break web pages in the wild. And, for what it's worth, if you focus the play button (you can't see that happening, but it does...) in Chrome, on that testcase, and hit space on the keyboard, it still works, whereas your change is breaking similar behaviour when the <video> is focused in Firefox (you can't focus individual controls). Can we be more conservative?

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs https://reviewboard.mozilla.org/r/54490/#review51176 ::: toolkit/content/widgets/videocontrols.xml:1513 (Diff revision 1) > // Use the handleEvent() callback for all media events. > - // The "error" event listener must capture, so that it can trap error events > - // from the <source> children, which don't bubble. > - for (let event of this.videoEvents) > - this.video.addEventListener(event, this, (event == "error") ? true : false); > + // Actually only "error" event listener needs capture, so that it can trap > + // error events from <source> children, which don't bubble. > + for (let event of this.videoEvents) { > + this.video.addEventListener(event, this, { > + capture: true, Why change all the listeners to be capturing listeners?

https://reviewboard.mozilla.org/r/54490/#review51176 > Why change all the listeners to be capturing listeners? Because it doesn't actually matter whether they are capturing or not, and making all of them capture makes code simpler as we don't need to distinguish between error and other events.

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs https://reviewboard.mozilla.org/r/54490/#review51186 ::: toolkit/content/widgets/videocontrols.xml:1509 (Diff revision 1) > - // The "error" event listener must capture, so that it can trap error events > - // from the <source> children, which don't bubble. > + // Actually only "error" event listener needs capture, so that it can trap > + // error events from <source> children, which don't bubble. "Actually" is superfluous here, and '"error" event listener" needs an article (ie "the") in front of it. This doesn't explain why all the other ones are now `capture: true`. How about: Only the "error" event listener must capture, so that it can trap error events from <source> children, which don't bubble - but we use capture for all events in order to simplify the event listener addition/removal. TBH, I think just having the ternary for capture repeated twice would be simpler than having all the listeners capture and then having to explain that in a comment, but maybe that's just me. :-\

Review commit: https://reviewboard.mozilla.org/r/54720/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54720/

Comment on attachment 8755251 [details] MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/1-2/

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/1-2/

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/1-2/

Oops, mybase is accdentally pushed... but how can that happen...

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review51442

https://hg.mozilla.org/integration/fx-team/rev/4f15271e5488bb0ce3c6f64179702fc03dc78582 Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug https://hg.mozilla.org/integration/fx-team/rev/6c204be833d119fa6daae408f757e6519a531046 Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://hg.mozilla.org/integration/fx-team/rev/788365ddaf61746bc66479335ae72d3e566c38ec Bug 1274520 part 3 - Listen video events in system group. r=gijs

Backed out in https://hg.mozilla.org/integration/fx-team/rev/ea60f9e94a807a0be57df93c9716426b5651b21b for https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=788365ddaf61746bc66479335ae72d3e566c38ec&filter-failure_classification_id=2

The first patch adds a new option to EventTarget.addEventListener / removeEventListener, which needs document, though it is chrome and XBL only.

Comment on attachment 8755251 [details] MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/2-3/

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/2-3/

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/2-3/

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review52130 ::: toolkit/content/widgets/videocontrols.xml:1042 (Diff revision 3) > - > - // Read defaultPrevented and the playback state asynchronously, > - // since Web content may want to consume the "click" event > - // but will only receive it after us. If web content > + if (this.playButton.hasAttribute("paused")) { > + this.startPlay(); > + } else { > + this.video.pause(); This and the introduction of all the asynchronicity into the tests make me feel very uncomfortable. Some of the point of the tests is explicitly to deal with the case where web content does stuff synchronously as a result of events. Why are these changes necessary, and can you explain how we're still testing the case(s) described in the comment you're removing here? There should probably still be a comment as to why we're reading the play button's 'paused' attribute, rather than the video state.

(In reply to :Gijs Kruitbosch from comment #39) > > This and the introduction of all the asynchronicity into the tests make me > feel very uncomfortable. Some of the point of the tests is explicitly to > deal with the case where web content does stuff synchronously as a result of > events. All the asynchronicity introduced in the tests are for control UI check and user input simulation. Control UI state is not detectable from the content, so that should be fine. And user inputs are asynchronously by nature. I've realized that some of the tests are intended for testing stuff synchronously, so I carefully checked that neither DOM attribute check nor DOM method call is made asynchronous. If the content explicitly dispatches some of the events like user inputs, it could be broken. But those kinds of actions are always fragile, and I don't believe content does that to hack the browser-provided controls rather than just providing their own in general. > Why are these changes necessary, and can you explain how we're still testing > the case(s) described in the comment you're removing here? Those changes are necessary because listeners in the system group are invoked after all listeners from the content, which means at the moment a content listener is called, we haven't got the chance to update the UI. > There should > probably still be a comment as to why we're reading the play button's > 'paused' attribute, rather than the video state. Because the old way, reading the previous state and flipping it, would not work now, since the content would have changed the playing state before we getting there. So we have to do that based on the state of the button. And it also matches what I described in comment 10: the button is *not* supposed to "toggle the playing state", but to do what its appearance shows, which is controled by the attribute. Actually the old fix in bug 1181055 is always fragile. For example, if I have "<div><video>" and I set a capture listener on div for click and call video.play() there, things would be broken again. Unless you set a capture listener on the window root , it is not possible to fill the hole completely with "toggling the playing state". And without being able to add listener in the system group, you would always need asynchronicity to detect defaultPrevented flag.

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review52148 ::: toolkit/content/widgets/videocontrols.xml:1032 (Diff revision 3) > } > this.setFullscreenButtonState(); > }, > > clickToPlayClickHandler : function(e) { > - if (e.button != 0) > + if (e.button != 0 || e.defaultPrevented) I suspect we should bail for `e.defaultPrevented` *after* the error check, not before.

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs https://reviewboard.mozilla.org/r/54490/#review52150 r=me though it might be a good idea if :jaws had a look, too.

Comment on attachment 8755251 [details] MozReview Request: Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54460/diff/3-4/

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54462/diff/3-4/

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54490/diff/3-4/

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review53352 ::: toolkit/content/widgets/videocontrols.xml:1042 (Diff revision 4) > - > + if (e.defaultPrevented) > - // Read defaultPrevented and the playback state asynchronously, > - // since Web content may want to consume the "click" event > - // but will only receive it after us. If web content > - // doesn't use preventDefault but still toggles playback, > - // we will treat that act the same as a call to preventDefault() > - // so the web content-initiated toggle is not reverted. > - let previousState = this.video.paused; > - setTimeout(() => { > - if (e.defaultPrevented || > - this.video.paused != previousState) { > - return; > + return; > + if (this.playButton.hasAttribute("paused")) { > + this.startPlay(); > + } else { > + this.video.pause(); How does this not regress bug 1181055? Specifically, if content doesn't use preventDefault(), what's to stop us from pausing the video that content just requested to play?

Comment on attachment 8755262 [details] MozReview Request: Bug 1274520 part 3 - Listen video events in system group. r=gijs https://reviewboard.mozilla.org/r/54490/#review53358 rs=me

https://reviewboard.mozilla.org/r/54462/#review53352 > How does this not regress bug 1181055? Specifically, if content doesn't use preventDefault(), what's to stop us from pausing the video that content just requested to play? We do not pause the video at all now, because it is now execute play or pause according to the button state rather than simply toggling the playing state. The button state is updated by video play event, which is dispatched asynchronously after the click/command event, so when we are here, the state of the button hasn't been changed since the click. And the fact that this doesn't regress the test you added in bug 1181055 is an evidence that this mechanism works as expected.

Comment on attachment 8755252 [details] MozReview Request: Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs https://reviewboard.mozilla.org/r/54462/#review53416 Okay, thanks for the explanation.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a3ad34c694dce33e7c12fdb8e7ff681e93dae Bug 1274520 part 1 - Add mozSystemGroup to EventListenerOptions for chrome and XBL to add listener in the system group. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/615ccf4a8bdc8cc2af1c4a16c1f89f930b0427b0 Bug 1274520 part 2 - Listen control events in video controls on system group. r=gijs,jaws https://hg.mozilla.org/integration/mozilla-inbound/rev/2863702401b0927e2884c2b15476790265c1ff5c Bug 1274520 part 3 - Listen video events in system group. r=gijs,jaws

https://hg.mozilla.org/mozilla-central/rev/8e4a3ad34c69 https://hg.mozilla.org/mozilla-central/rev/615ccf4a8bdc https://hg.mozilla.org/mozilla-central/rev/2863702401b0

I'm trying to understand part 1 of this patch. If aOptions.mMozSystemGroup, we call nsContentUtils::GetCurrentJSContext, which is mainthread-only (and asserts thus). But EventListenerManager::AddEventListener is not mainthread-only. What happens if the mMozSystemGroup thing is passed on a worker thread? Anyway, it seems to me like the right thing here is to land the patches in bug 1141916, extend them slightly to support [Func="IsChromeOrXBL"] (well, not that one, that one is mainthread-only, but something similar that does the right thing on workers) on dictionary members, and not make this property web-visible at all. As checked in, content script will see us getting it....

(In reply to Boris Zbarsky [:bz] from comment #52) > I'm trying to understand part 1 of this patch. If aOptions.mMozSystemGroup, > we call nsContentUtils::GetCurrentJSContext, which is mainthread-only (and > asserts thus). But EventListenerManager::AddEventListener is not > mainthread-only. What happens if the mMozSystemGroup thing is passed on a > worker thread? Hmmm, probably it would assert with debug build, and I have no idea what would happen for release build :| > > Anyway, it seems to me like the right thing here is to land the patches in > bug 1141916, extend them slightly to support [Func="IsChromeOrXBL"] (well, > not that one, that one is mainthread-only, but something similar that does > the right thing on workers) on dictionary members, and not make this > property web-visible at all. As checked in, content script will see us > getting it.... Yeah, well, probably. I guess the right thing to do immediately here is probably to check IsMainThread explicitly before calling GetCurrentJSContext.

oh crap, I missed that. We should make the API work consistently, so keep the existing stuff for mainthread and then explicitly check if we're in a chrome worker and only in that case allow use of mMozSystemGroup

So XBL script cannot be in a worker thread currently?

XBL won't ever be run in workers, but it is also that IsChromeOrXBL method is main thread only. So need to explicitly check for chrome workers.

mozilla::dom::workers::IsCurrentThreadRunningChromeWorker()

Yeah, I guess you basically want something like nsContentUtils::ThreadsafeIsCallerChrome but the mainthread branch checks IsChromeOrXBL. And again, I would really prefer it if we did this right with conditional annotations on dictionary members.

Comment on attachment 8758937 [details] [diff] [review] followup patch - make the option thread-safe > static EventListenerFlags > GetEventListenerFlagsFromOptions(const EventListenerOptions& aOptions) Make this a method of EventListenerManager > { > EventListenerFlags flags; > flags.mCapture = aOptions.mCapture; > if (aOptions.mMozSystemGroup) { >- JSContext* cx = nsContentUtils::GetCurrentJSContext(); >- MOZ_ASSERT(cx, "Not being called from JS?"); >- flags.mInSystemGroup = IsChromeOrXBL(cx, nullptr); >+ if (NS_IsMainThread()) { ...then you can use mIsMainThreadELM here (which is faster). >+ flags.mInSystemGroup = nsContentUtils::IsCallerChrome() || >+ nsContentUtils::IsCallerContentXBL(); So this has different behavior than IsChromeOrXBL, both for IsCallerChrome part and for IsCallerContentXBL part. I think I'd prefer to keep the existing setup for main thread: get cx and pass to IsChromeOrXBL

(In reply to Boris Zbarsky [:bz] from comment #58) > And again, I would really prefer it if we did this right > with conditional annotations on dictionary members. Sure, but need not to block work on getting webidl support for that.

Making that function a method of EventListenerManager makes the line too long, and would need additional change to the header file, which isn't that necessary... So I decide to use a parameter for main thread check instead.

Comment on attachment 8759128 [details] [diff] [review] followup patch - make the option thread-safe ok ,fine. A bit silly to pass aIsMainThread this way, but fine :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8895ad68cf172ca74cecd822a2203b5d2e0a7 Bug 1274520 followup - Make EventListenerOptions.mozSystemGroup option thread-safe. r=smaug

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8895ad68c followup - Make EventListenerOptions.mozSystemGroup option thread-safe. r=smaug

> Sure, but need not to block work on getting webidl support for that. That's OK, but I just want to make sure the right solution actually happens. Ideally before we ship this to the world... If you want me to just do it, please let me know and I will. (As a separate note, GetCurrentJSContext is generally a code smell nowadays, because any uses of it are preventing us from removing the JSContext stack.)

Updated: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#Parameters and https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters Added a note in: https://developer.mozilla.org/en-US/Firefox/Releases/49#Interfaces

(In reply to Jean-Yves Perrier [:teoli] from comment #68) > Updated: > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/ > removeEventListener#Parameters > and > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/ > addEventListener#Parameters > > Added a note in: > https://developer.mozilla.org/en-US/Firefox/Releases/49#Interfaces The document isn't really correct. The option is mozSystemGroup, not mozsystemgroup. I'm not sure whether it is case-sensitive, though, I suppose it is better to just match what is written in the idl file.

Fixed capitalization, thx!