I have a problem with Firefox Beta 53. It doesn't happen in Beta 52 and ESR 45. Sometimes when I scroll after clicking in video, it doesn't play, and the page doesn't scroll. Here's how to reproduce it: 1. Open https://www.w3schools.com/html/HTML5_video.asp 2. Play the video 3. Click near the beginning of timeline, scroll the page by mouse wheel Result: video temporarily stops. The page doesn't scroll. Expected: video should continue playing. The page should scroll.

I tried the STR on MacOS Firefox(53, 54, 55), but I could not reproduce the problem, the page scroll normally and video doesn't (temporarily) stop for me. Could you give information about OS? Thanks.

It's Windows 7, User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

I can reproduce the problem on Windows10 and Ubuntu16.04.

I can reproduce this in my Windows10 as well. Apart from this issue, it's interesting that if I disable e10s, the problem will be gone (perhaps it's another bug for input[type=range]). Since I have to disable e10s to debug on Windows, however, as mentioned, disabling e10s hide the problem from me. Instead, a full build is required to test my patch, and it would take long amounts of time to see the result. Plus, I don't have a workable Windows around, so I might push a patch for this later. I guess we need to check all default event handlers for input[type=range] in order not to trigger them unexpectedly, if I'm not misunderstanding, "wheel" should be the last to deal with. I've came up with several ways to fix the issue, but hopefully if Bug 379939 could be fixed, something similar to this bug should be solved once and for all. Thanks.

FYI. I can reproduce this even if I disable e10s

okay, to recap what we got so far: 1. affect all platform expect for MacOS 2. surely reproducible in non-e10s, but supposedly affects both 3. add a additional condition to STR: step3, cursor should not move outside timeline before mouse wheel. (at least I need this to reproduce the problem)

Just had a quick thought to this: adding `addListener(this.video, "wheel", e => e.stopPropagation(), true);` to suppress "wheel" event in videocontrols.xml Will test on Windows once I had a chance. Thanks.

> 3. add a additional condition to STR: step3, cursor should not move outside timeline before mouse wheel. (at least I need this to reproduce the problem) In my case, no need the additional condition. 3. Click the timeline area and just turn the mouse wheel in the timeline area. (You can move the mouse pointer out side video control before you turn mouse wheel)

(In reply to Alice0775 White from comment #8) > > 3. add a additional condition to STR: step3, cursor should not move outside timeline before mouse wheel. (at least I need this to reproduce the problem) > > In my case, no need the additional condition. > > 3. Click the timeline area and just turn the mouse wheel in the timeline > area. > (You can move the mouse pointer out side video control before you turn mouse > wheel) well, I guess it's not all about "wheel" if this case, I mean if it can reproduce with cursor outside <video> region....

These lines[0] can explain why "wheel" doesn't affects MacOS and also is reasonable for making this happened. I'll look more to this. [0] https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/dom/html/HTMLInputElement.cpp#4692-4726

(In reply to Alice0775 White from comment #8) > > 3. add a additional condition to STR: step3, cursor should not move outside timeline before mouse wheel. (at least I need this to reproduce the problem) > > In my case, no need the additional condition. > > 3. Click the timeline area and just turn the mouse wheel in the timeline > area. > (You can move the mouse pointer out side video control before you turn mouse > wheel) Hi Alice, can you do a screencast with an empty nightly profile? I can't reproduce what you're saying - scrolling with the mousewheel only affects the video if the cursor is over the video controls. As Ray said in comment #7, that might be reasonably straightforward to fix, but if there's something else going on then we need to understand what, exactly.

screencast: https://youtu.be/helcTnvDltc

(In reply to Alice0775 White from comment #12) > screencast: https://youtu.be/helcTnvDltc OK, so you're still actually using the mousewheel in the timeline area. That makes sense. Ray, can we just blur the range input immediately when it gets focused? I expect stopping propation as in comment #7 will still break scrolling.

Thank you Alice, (In reply to :Gijs from comment #13) > Ray, can we just blur the range input immediately when it gets focused? sure, that's my initial thought. Should we also transfer the focus to "video" after blurred? > I expect stopping propation as in comment #7 will still break scrolling. I need to confirm that again on Windows. I remember that yesterday I tested comment #7 with Browser Toolbox, scrolling seems fine.

>I remember that yesterday I tested comment #7 with Browser Toolbox, scrolling seems fine. temp0.addEventListener("wheel", e => e.stopPropagation(), {mozSystemGroup: true, capture: true});

(In reply to Ray Lin[:ralin] from comment #14) > Thank you Alice, > > (In reply to :Gijs from comment #13) > > Ray, can we just blur the range input immediately when it gets focused? > sure, that's my initial thought. Should we also transfer the focus to > "video" after blurred? I don't know. But given bug 1352879, would this even work? (In reply to Ray Lin[:ralin] from comment #15) > >I remember that yesterday I tested comment #7 with Browser Toolbox, scrolling seems fine. > > temp0.addEventListener("wheel", e => e.stopPropagation(), {mozSystemGroup: > true, capture: true}); This makes some amount of sense, I guess. Can you provide a patch?

Track 54+ as regression.

(In reply to :Gijs from comment #16) > I don't know. But given bug 1352879, would this even work? No, I guess we'll need DOM peer to look into bug 1352879. > This makes some amount of sense, I guess. Can you provide a patch? Sorry for the late, I've just uploaded a patch for this. It seems stopPropagation and preventDefault could not properly fix the problem, they somehow break either timeline or page scrolling. As you mentioned, blur before fall into default handler[0] might be more straight to this problem. So, in the patch, I simply check the original target of "wheel" event, and set focus to <video> before event is propagated to range input if needed. Could you help me review this patch? Thanks. [0] https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/dom/html/HTMLInputElement.cpp#4710

(In reply to Ray Lin[:ralin] from comment #19) > (In reply to :Gijs from comment #16) > > I don't know. But given bug 1352879, would this even work? > No, I guess we'll need DOM peer to look into bug 1352879. > > > This makes some amount of sense, I guess. Can you provide a patch? > Sorry for the late, I've just uploaded a patch for this. > > It seems stopPropagation and preventDefault could not properly fix the > problem, they somehow break either timeline or page scrolling. I don't think we should care about being able to scroll the video timeline - if we care about that, we should make the scroll operation scroll a meaningful amount. With that, does either of stopPropagation or preventDefault or stopImmediatePropagation help here? What does "break the timeline" mean in your original comment?

Comment on attachment 8856829 [details] Bug 1352686 - Prevent range value from being changed when wheel over it in anonymous content. https://reviewboard.mozilla.org/r/128752/#review131330 I'm not convinced this is the right approach, see my needinfo request on the bug. If we do need to do this, then please change this to just use a single if statement instead of a for() loop. ::: toolkit/content/widgets/videocontrols.xml:1867 (Diff revision 1) > + // Prevent input[type="range"]'s default handler in the case that mouse wheel over the > + // focused scrubber or volumn control. (bug 1352686) > + addListener(this.video, "wheel", e => { > + const originalTarget = e.originalTarget; > + > + for (const slider of [this.scrubber, this.volumeControl]) { Nit: extra spacing ::: toolkit/content/widgets/videocontrols.xml:1869 (Diff revision 1) > + addListener(this.video, "wheel", e => { > + const originalTarget = e.originalTarget; > + > + for (const slider of [this.scrubber, this.volumeControl]) { > + // Original target could be either input itself or an anonymouse div inside input. > + if (originalTarget === slider || originalTarget.parentNode === slider) { Nit: no triple equals please.

(In reply to :Gijs from comment #20) > I don't think we should care about being able to scroll the video timeline - I agree, scroll should not work for timeline. > if we care about that, we should make the scroll operation scroll a > meaningful amount. With that, does either of stopPropagation or > preventDefault or stopImmediatePropagation help here? What does "break the > timeline" mean in your original comment? Sorry, I meant break page scrolling only :P We need to stopPropagation() or preventDefault() before event reaching innermost range input[0], but if so, page can't scroll as no event will be be received or the event is already defaultPrevented then[1]. So, instead of using either, I'm afraid we might need a hacky workaround. Please correct me, if I'm misunderstanding that, thanks. document ... video ... range input | range input .. video ... document [0] [1]

OK, then let's use a focus-based solution. I do wonder if we should just blur() the range input (or focus the video as a whole or whatever) after every click/drag operation has finished (maybe onmouseup?). I'm guessing we have keyboard handling on the video as a whole so keyboard a11y won't be affected.

Oh, though one more thing: we should be careful not to regress bug 735251 when setting focus programmatically here (will need testing on OS X).

Doesn't sound like this blocks 53 release, wontfix. Gijs please let me know if you disagree.

(In reply to :Gijs from comment #24) > I do wonder if we should just blur() the range input (or focus the video as a whole or whatever) after every click/drag operation has finished (maybe onmouseup?) I don't really tend to land the focuse-based hack for "wheel" event like my patch does, onmouseup seems to be a more elegant way for this problem. I tried onmouseup approach today, and it does fixes the problem except it also regress bug 735251 :( I'd see my patch as a worst-case for this if we could not come up with a better approach. If you agree this doesn't block 53, I would like to spend more time dig into the feasibility of onmouseup approach. Thanks.

(In reply to Ray Lin[:ralin] from comment #26) > (In reply to :Gijs from comment #24) > > I do wonder if we should just blur() the range input (or focus the video as a whole or whatever) after every click/drag operation has finished (maybe onmouseup?) > I don't really tend to land the focuse-based hack for "wheel" event like my > patch does, onmouseup seems to be a more elegant way for this problem. I > tried onmouseup approach today, and it does fixes the problem except it also > regress bug 735251 :( Hm. Even when you use .blur() ? > I'd see my patch as a worst-case for this if we could not come up with a > better approach. If you agree this doesn't block 53, I would like to spend > more time dig into the feasibility of onmouseup approach. Thanks. Sure, I don't think this blocks 53.

Sorry for the late... I was working on form autofill in the last two weeks. Instead of focus on <video> which regress bug 735251, I guess focusing on <videocontrols> can bring the same effect. I've tried to do this before, but didn't realize `-moz-user-focus: ignore` was stopping me from focusing on <videocontrols> as it's a XUL element at that time. I guess this solution might be more elegant than others we've discussed, could you help me to review the patch? Thanks.

Comment on attachment 8856829 [details] Bug 1352686 - Prevent range value from being changed when wheel over it in anonymous content. https://reviewboard.mozilla.org/r/128752/#review140194 ::: toolkit/content/widgets/videocontrols.xml:1869 (Diff revision 2) > + addListener(this.video, "mousedown", e => { > + this.videocontrols.focus(); I think this will cause issues with accessibility for users that click with the mouse and then want to hit tab to move focus to a different control. On Windows, if I click on the scrubber, then hit Tab, then hit Space, the video player goes to fullscreen. I think this patch will cause that to no longer work.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30) > I think this will cause issues with accessibility for users that click with > the mouse and then want to hit tab to move focus to a different control. > > On Windows, if I click on the scrubber, then hit Tab, then hit Space, the > video player goes to fullscreen. I think this patch will cause that to no > longer work. Bug 1325594 comment 10 mentioned that we don't have keyboard shortcut for CC and fullscreen button, so we left them focusable. For the rest of controls, they've been non-focus for long time, I'm not so sure but it seems to me that this behavior is fine. Please correct me if I misunderstood your concerns, thanks :)

We should look at fixing this the right way, by maybe adding a property or attribute to input[type=range] that will ignore mousewheel events, since not all range sliders have a semantic use for mousewheel interaction. Stone, what do you think about this?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32) > We should look at fixing this the right way, by maybe adding a property or > attribute to input[type=range] that will ignore mousewheel events, since not > all range sliders have a semantic use for mousewheel interaction. Stone, > what do you think about this? Looks like we need a mechanism to stop the default behavior of the element in native anonymous content. Adding a chrome only attribute sounds ok to me.

(In reply to Ming-Chou Shih [:stone] from comment #33) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32) > > We should look at fixing this the right way, by maybe adding a property or > > attribute to input[type=range] that will ignore mousewheel events, since not > > all range sliders have a semantic use for mousewheel interaction. Stone, > > what do you think about this? > > Looks like we need a mechanism to stop the default behavior of the element > in native anonymous content. Adding a chrome only attribute sounds ok to me. Sounds a more straight fix to this bug :) Would the attribute apply to all elements or input only? And which interface is more appropriate to own this attribute? Thanks.

Comment on attachment 8856829 [details] Bug 1352686 - Prevent range value from being changed when wheel over it in anonymous content. Hi Stone, Instead of adding new attribute, perhaps, we can simply stop the default action if element IsInNativeAnonymousSubtree(). Could you give me feedback to this approach? Thanks.

Comment on attachment 8856829 [details] Bug 1352686 - Prevent range value from being changed when wheel over it in anonymous content. Do we really want to disable the wheel default behaviors of input range element in 'all' use cases in chrome document and in NAC? I'd prefer using an attribute to control the behavior if we don't have a firm answer to the former question so that we can get what we want and keep the flexibility.

Thank you :stone for the feedback and the discussion offline. ------ No update on it since I don't have cycle to dig into DOM implementation until I accomplish form autofill at some point in 57. However, I'll back to this once I got some spare time.

54 RC is released. Mark 54 won't fix.

Per comment 38, unassign myself.

Stone, does your opinion on comment 33 still holds, i.e. we want to implement a special HTML attribute just to change the mouse wheel behavior? If so copy-and-paste the patch in bug 1338961 should do the work I guess? Let me know if we still want to do that.

(Sorry -- didn't realize he is on leave)

... and I am told he is back already :P

Sorry for that I'm still busy with other bugs. Keep the ni flag opened to remind me to handle this.

Stone is no longer active AFAIK.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #41) > Stone, does your opinion on comment 33 still holds, i.e. we want to > implement a special HTML attribute just to change the mouse wheel behavior? > If so copy-and-paste the patch in bug 1338961 should do the work I guess? > Let me know if we still want to do that. Passing this to :jwatt who I think has worked on some of our input[type=...] html5 implementation stuff. See comment 33 / comment 34 for some context. :-)

From my testing it seems Chrome, Edge and Safari do not support changing the value of <input type=range> using the mouse wheel. It seems like we should align on that behavior then. If we really need range inputs to respond to the mouse wheel events for some part of our chrome then we could keep the behavior, guarded behind a chrome-only attribute (as opposed to having an attribute to disable the existing behavior).