Bug 60278 - Removing the full screen element via parent.innerHTML="" does not result in a webkitfullscreenchange event.
Summary: Removing the full screen element via parent.innerHTML="" does not result in a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-05 10:18 PDT by Jer Noble
Modified: 2011-06-03 14:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2011-05-05 10:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (7.15 KB, patch)
2011-05-05 14:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (9.15 KB, patch)
2011-05-06 00:58 PDT, Jer Noble
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-05-05 10:18:35 PDT
Removing the full screen element via parent.innerHTML="" does not result in a webkitfullscreenchange event.
Comment 1 Jer Noble 2011-05-05 10:36:52 PDT
Created attachment 92432 [details]
Patch
Comment 2 Jer Noble 2011-05-05 10:39:01 PDT
<rdar://problem/9390280>
Comment 3 Darin Adler 2011-05-05 10:43:05 PDT
Comment on attachment 92432 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3378
> +#if ENABLE(FULLSCREEN_API)
> +    // If the current full screen element or any of its ancestors is removed, set the current
> +    // full screen element to the document root, and fire a fullscreenchange event to inform 
> +    // clients of the DOM.

It’s really inelegant to have to put full-screen-specific code right into the basic DOM manipulation. I also expect that calling the contains function every time a node is removed will have a measurable performance impact.

Is there a better way to do this?

Maybe m_fullScreenElement should be 0 rather than the document element, when there is no special element. Then there would be a quick null check.

Or maybe you should add a check that m_fullScreenElement is != documentElement() before you check anything else for less performance impact.
Comment 4 Jer Noble 2011-05-05 11:39:37 PDT
(In reply to comment #3)
> (From update of attachment 92432 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92432&action=review
> 
> > Source/WebCore/dom/Document.cpp:3378
> > +#if ENABLE(FULLSCREEN_API)
> > +    // If the current full screen element or any of its ancestors is removed, set the current
> > +    // full screen element to the document root, and fire a fullscreenchange event to inform 
> > +    // clients of the DOM.
> 
> It’s really inelegant to have to put full-screen-specific code right into the basic DOM manipulation. I also expect that calling the contains function every time a node is removed will have a measurable performance impact.

In the most frequent case, where m_fullScreenElement is 0; contains bails out immediately.  So for that case, the performance impact is negligable.  For the case where we're in full screen, the performace hit is walking up the tree from the full screen element to the removed one, once.

> Is there a better way to do this?

Signs point to yes!  I believe this code can be moved into Node::willRemove(), which can do a simple "am I the full screen element?" pointer equality check.

> Maybe m_fullScreenElement should be 0 rather than the document element, when there is no special element. Then there would be a quick null check.

That's possible, though it might introduce other problems.  And it probably won't be necessary if I move things into the function listed above.

> Or maybe you should add a check that m_fullScreenElement is != documentElement() before you check anything else for less performance impact.

Lets see if it's necessary after moving things around a bit.

I'll get a new patch up soon.  Thanks!
Comment 5 Jer Noble 2011-05-05 14:59:05 PDT
Created attachment 92479 [details]
Patch
Comment 6 Alexey Proskuryakov 2011-05-05 22:29:22 PDT
Comment on attachment 92479 [details]
Patch

 void Node::willRemove()
 {
+#if ENABLE(FULLSCREEN_API)
+    if (document()->webkitCurrentFullScreenElement() == this)
+        document()->fullScreenElementWillBeRemoved();
+#endif
 }

Can an arbitrary node be full screen? Even an attribute node?

Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?

Also, fullscreen looks very much like a renderer behavior, not a DOM one.
Comment 7 Jer Noble 2011-05-05 22:58:42 PDT
(In reply to comment #6)
> (From update of attachment 92479 [details])
>  void Node::willRemove()
>  {
> +#if ENABLE(FULLSCREEN_API)
> +    if (document()->webkitCurrentFullScreenElement() == this)
> +        document()->fullScreenElementWillBeRemoved();
> +#endif
>  }
> 
> Can an arbitrary node be full screen? Even an attribute node?

Any element.  I doubt anyone would try taking an attribute node full screen, but if they tried, we'd dutifully show the full screen UI.

> Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?

We could, in theory, add an ivar to Element called m_isFullScreen (or something), and check that instead of querying the document.  But would that be advantageous at all?

> Also, fullscreen looks very much like a renderer behavior, not a DOM one.

What do you mean?
Comment 8 Jer Noble 2011-05-05 23:12:18 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?
> 
> We could, in theory, add an ivar to Element called m_isFullScreen (or something), and check that instead of querying the document.  But would that be advantageous at all?


It looks like focused nodes are removed in ContainerNode::removeChild() and ContainerNode::removeChildren().  ContainerNode uncritically calls document()->removeFocusedNodeOfSubtree(...) with a pointer to the child being removed.  What we are doing here is functionally identical, however, it could be refactored to be technically identical.
Comment 9 Jer Noble 2011-05-06 00:58:33 PDT
Created attachment 92557 [details]
Patch
Comment 10 Maciej Stachowiak 2011-05-09 18:50:05 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 92479 [details] [details])
> >  void Node::willRemove()
> >  {
> > +#if ENABLE(FULLSCREEN_API)
> > +    if (document()->webkitCurrentFullScreenElement() == this)
> > +        document()->fullScreenElementWillBeRemoved();
> > +#endif
> >  }
> > 
> > Can an arbitrary node be full screen? Even an attribute node?
> 
> Any element.  I doubt anyone would try taking an attribute node full screen, but if they tried, we'd dutifully show the full screen UI.

I don't think so - the fullscreen API only exists in Element.idl. You could probably limit this to Element::willRemove()
Comment 11 Maciej Stachowiak 2011-05-10 13:11:01 PDT
Comment on attachment 92557 [details]
Patch

Since this exactly matches what we do for focus, I see no reason to object. If there is a better way to do this, we can refactor both these code paths in the future.
Comment 12 Jer Noble 2011-05-10 15:39:01 PDT
Committed r86185: <http://trac.webkit.org/changeset/86185>
Comment 13 Ademar Reis 2011-06-03 14:08:27 PDT
Revision r86185 cherry-picked into qtwebkit-2.2 with commit 2cacab2 <http://gitorious.org/webkit/qtwebkit/commit/2cacab2>