fix: Prevent ResizeManager from being clicked on safari, fix playerresize on firefox #5522
Conversation
brandonocasey
changed the title
|
Looks like pointer-events:none isn't good enough for this for some reason. |
|
Looks like this doesn't fix it in safari yet, right clicking in https://deploy-preview-5522--videojs-docs.netlify.com/test-example/responsive.html in safari, I still get the menu for the iframe rather than the video element. We may need to manually make sure that the element is positioned behind the video element. |
|
Oh I see what you mean, when the video is actually playing it gives a menu for the iframe, but before playback it gives a menu for the player. thats why I thought it was fixed. Looking further into this 👀 |
brandonocasey
force-pushed
the
fix/safari-contextmenu
branch
from
7107e62 to
388529f
Compare
|
Here is what I tried so far
After all that I opted to re-trigger the |
src/js/player.js
Outdated
| @@ -4069,15 +4069,15 @@ Player.prototype.options_ = { | |||
|
|
|||
| // Included control sets | |||
| children: [ | |||
| 'resizeManager', | |||
There was a problem hiding this comment.
maybe revert this change if we need to re-trigger the event?
There was a problem hiding this comment.
OK, so I was going to remove this change and put resizeManager back at the bottom of the children list, but it breaks the controls for Safari. When I removed the visibility: hidden; css for the resize manager it made it so that the iframe was also intercepting other clicks on the video element. So we have to keep resizeManager at the top for controls to work correctly.
There was a problem hiding this comment.
If we still wanted to move it, I would put it after mediaLoader.
src/js/resize-manager.js
Outdated
| * @param {Event} e the contextmenu event that fired | ||
| */ | ||
| handleContextmenu_(e) { | ||
| this.player_.trigger(e); |
There was a problem hiding this comment.
Should we preventDefault here? Thoughts @misteroneill?
| @@ -50,5 +50,5 @@ | |||
| width: 100%; | |||
| height: 100%; | |||
| border: none; | |||
| visibility: hidden; | |||
There was a problem hiding this comment.
Fixes firefox not firing playerresize
| @@ -50,5 +50,5 @@ | |||
| width: 100%; | |||
| height: 100%; | |||
| border: none; | |||
| visibility: hidden; | |||
| z-index: -1000; | |||
There was a problem hiding this comment.
Fixes safari and firefox iframe contextmenu hijacking
There was a problem hiding this comment.
(firefox was started to hijack contextmenu once we removed visibility: none)
…size on firefox (#5522) Move the ResizeManager behind the video element with negative z-index and remove visibility: hidden. This fixes issue where right click event wasn't triggered on the correct element and resize was not happening on firefox.
Currently right clicking on the player on safari does not fire a
contextmenuevent, but it does on every other browser. Right clicking on the player in safari fires acontextmenuevent on theResizeManageriframe instead. The only change that needed to happen for right click to fire on the player again, was to hideResizeManager.EDIT:
I also found that the
playerresizeevent does not work in Firefox. After removingvisibility: hidden;it fires as we would expect, I added that change to this pull request, as I think it makes sense.