Closed Bug 910532 Opened 6 years ago Closed 5 years ago

Canceling the context menu on full-screen video using Esc takes you out of the full-screen mode

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ehsan, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

STR:

1. Go to <http://video.vogue.com/watch/from-the-vogue-closet-pajama-dressing?c=browse&per_page=200&page=5>
2. Play the video and go full-screen.
3. Right click on the video and press Esc.

We should just close the context menu and not leave full-screen mode.
Depends on: 1121280
Blocks: 1121280
No longer depends on: 1121280
OS: Mac OS X → All
Hardware: x86 → All
Component: Video/Audio → DOM: Events
Assignee: nobody → quanxunzhen
Attachment #8550999 - Flags: review?(bugs)
Comment on attachment 8550998 [details] [diff] [review]
patch 1 - Add flag to indicate prevent default from chrome

Do we need this?

Isn't 
(mEvent->mFlags.mDefaultPrevented && !mEvent->mFlags.mDefaultPreventedByContent)
telling enough?

Or do you need to be able to detect cases when both chrome and content have
called preventDefault()?
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8550998 [details] [diff] [review]
> patch 1 - Add flag to indicate prevent default from chrome
> 
> Do we need this?
> 
> Isn't 
> (mEvent->mFlags.mDefaultPrevented &&
> !mEvent->mFlags.mDefaultPreventedByContent)
> telling enough?

It is not enough, because PresShell::HandleEventInternal could set mDefaultPrevented to true before dispatching the event to chrome. See https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8005
Attachment #8550998 - Flags: review?(bugs) → review+
Comment on attachment 8550998 [details] [diff] [review]
patch 1 - Add flag to indicate prevent default from chrome

Actually, while you're here could you fix
Event::InitEvent
to clear both 
mDefaultPreventedByContent and mDefaultPreventedByChrome.
Looks like we don't currently set them false when 
mDefaultPrevented is set to false.
(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8550998 [details] [diff] [review]
> patch 1 - Add flag to indicate prevent default from chrome
> 
> Actually, while you're here could you fix
> Event::InitEvent
> to clear both 
> mDefaultPreventedByContent and mDefaultPreventedByChrome.
> Looks like we don't currently set them false when 
> mDefaultPrevented is set to false.

Done locally.
Comment on attachment 8550999 [details] [diff] [review]
patch 2 - Prevent exiting fullscreen if escape is consumed by chrome

So this relies on, hmm, something...
that nsXULPopupManager ends up calling preventDefault() when KEY_DOWN is dispatched.

This needs some more comments. Like in the places
where we deal with aEvent->message == NS_KEY_UP and fullscreen/pointerlock, indicate that chrome event listeners can prevent the normal ESC behavior by calling prevent default on the keydown/press events.

But yeah, this setup should work, as far as I see. A test is needed for this kind of complicated stuff. See the existing fullscreen/pointerlock tests, and maybe you can just add test for this behavior to those tests.


Thanks for writing the patch.
Attachment #8550999 - Flags: review?(bugs) → review-
Attachment #8551539 - Flags: review?(bugs)
Attachment #8551538 - Flags: review?(bugs) → review+
Attachment #8551539 - Flags: review?(bugs) → review+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.