Bug 60142 – Exiting full screen causes <video> element inline controls to become visible

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

For elements whose "controls" attribute is not set, entering full screen mode will force the controls to become visible.  The controls (erroneously) remain visible after exiting full screen mode.

[Jer Noble]

<rdar://problem/9380592>

[Jer Noble]

When full screen mode is exited, the current full screen element is set to 0, then the event is dispatched.  The proposed Mozilla API requires: "When a Document enters or leaves the full-screen state, the user agent must queue a task to dispatch this event. When the event is dispatched, if the document's current full-screen element is an element in the document, then the event target is that element, otherwise the event target is the document."  So we dispatch the event at the document element, and not at the previous full screen element.

HTMLMediaElement uses preDispatchMessage() to catch these events and set the control states accordingly.  And since the event after exiting is dispatched at the document element, it never goes through preDispatchMessage().

[Jer Noble]

Created attachment 93029 [details]
Patch

[Darin Adler]

Comment on attachment 93029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93029&action=review

It seems like if you quickly enter and then exit fullscreen you might get two fullscreenchange events, both after you are out of fullscreen. A little strange.

Also, it seems this patch doesn’t apply, so you are not getting any EWS testing.

> Source/WebCore/dom/Document.cpp:4921
> +    m_fullScreenChangeEventTargetQueue.append(m_fullScreenElement);
>      m_fullScreenElement = 0;

It should be more efficient to call m_fullScreenElement.release() here and then you don’t need to set m_fullScreenElement to 0 on the next line.

> Source/WebCore/dom/Document.cpp:4992
> +    m_fullScreenChangeEventTargetQueue.append(m_fullScreenElement);

It should be more efficient to call m_fullScreenElement.release() here.

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 93029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93029&action=review
> 
> It seems like if you quickly enter and then exit fullscreen you might get two fullscreenchange events, both after you are out of fullscreen. A little strange.

It is a little strange.  But I thought it was better to send one too many events than one too few.  These events don't carry any state, so they're essentially telling their handlers to clear their cached values and query for new data, so the danger of too many events should be minimal.

> Also, it seems this patch doesn’t apply, so you are not getting any EWS testing.

Okay, I'll rebase and re-upload.

> > Source/WebCore/dom/Document.cpp:4921
> > +    m_fullScreenChangeEventTargetQueue.append(m_fullScreenElement);
> >      m_fullScreenElement = 0;
> 
> It should be more efficient to call m_fullScreenElement.release() here and then you don’t need to set m_fullScreenElement to 0 on the next line.

Will do.

> > Source/WebCore/dom/Document.cpp:4992
> > +    m_fullScreenChangeEventTargetQueue.append(m_fullScreenElement);
> 
> It should be more efficient to call m_fullScreenElement.release() here.

Here too.

[Jer Noble]

Created attachment 93032 [details]
Patch

Uploading updated and rebased patch so that ews bots can chew on it.

[WebKit Review Bot]

Comment on attachment 93032 [details]
Patch

Attachment 93032 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8685160

New failing tests:
fullscreen/full-screen-remove.html
fullscreen/full-screen-remove-children.html
fullscreen/full-screen-remove-ancestor.html

[WebKit Review Bot]

Created attachment 93052 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jer Noble]

Created attachment 93087 [details]
Patch

Addressed the EWS LayoutTest failure.  Upon removal, the old patch attempted to dispatch an event at the removed element.  This failed, because events will not be cascaded to the element's document if the element has been removed from the DOM.  This patch directs an event at the document element in the case of full screen element removal.

[Jer Noble]

(In reply to comment #9)
> Created an attachment (id=93087) [details]
> Patch
> 
> Addressed the EWS LayoutTest failure.  Upon removal, the old patch attempted to dispatch an event at the removed element.  This failed, because events will not be cascaded to the element's document if the element has been removed from the DOM.  This patch directs an event at the document element in the case of full screen element removal.

After sleeping on it, I don't think I like this approach.  Instead, I think we should dispatch the event at the full screen element /before/ it is removed from the dom, so all the cascading happens as expected.

[Jer Noble]

Created attachment 93169 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 93169 [details]
Patch

Clearing flags on attachment: 93169

Committed r86488: <http://trac.webkit.org/changeset/86488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r86488 cherry-picked into qtwebkit-2.2 with commit c02b89c <http://gitorious.org/webkit/qtwebkit/commit/c02b89c>