Closed Bug 715469 Opened 13 years ago Closed 12 years ago

Images in video controls fire "load" events that somehow bubble out to the element

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
11 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12
Tracking Flags:
Tracking Status
firefox11 - affected
firefox12 --- fixed

People

(Reporter: roc, Assigned: cpearce)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Patch: with jwein's suggested change
4.90 KB, patch
Dolske
: review+
Details | Diff | Splinter Review 
This is bug 464371 all over again.

See bug 697955 comment #29, which I can reproduce.

The load event is actually fired on the anonymous <image> child of <button class="fullscreenButton">. I don't know how it ends up being delivered to the video element, since this event isn't supposed to bubble.
What would prevent load event to propagate from video controls?
Media element could have ::PreHandleEvent and there one could do something like:
if (aVisitor.mEvent->message == NS_LOAD && aVisitor.mOriginalTargetIsInAnon) {
  nsCOMPtr<nsINode> node = do_QueryInterface(aVisitor.mEvent->originalTarget);
  if (node && node->IsInNativeAnonymousSubtree()) {
    aVisitor.mCanHandle = false;
    return NS_OK;
  }
}
return nsGenericHTMLElement::PreHandleEvent(aVisitor);
See also: Bug 516811 - "load" events for poster image loads bubble out
(the poster image is also an anon child).
Depends on: 470628
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → 11 Branch
(In reply to Olli Pettay [:smaug] from comment #1)
> What would prevent load event to propagate from video controls?

Normally when a non-bubbling event fires at an element, non-capturing listeners on parents aren't notified. So why are they notified here? Something to do with XBL?

If comment #2 works, great.
The interesting part is that the event shouldn't propagate from anon content.
Capturing listeners shouldn't get the load event either.

(And non-bubbling events do fire at each bindingParent)
(In reply to Olli Pettay [:smaug] from comment #5)
> The interesting part is that the event shouldn't propagate from anon content.
> Capturing listeners shouldn't get the load event either.

So this is a DOM Events bug?

In this case, the setup is that nsVideoFrame creates anonymous child elements for the <video> element. One is an HTML <img>, the other is a XUL <videocontrols> which then has XBL bound to it. Apparently events are leaking from both of those.
Of course they are "leaking" if nothing prevents them from propagating.
Could someone familiar with video controls try what I suggested in comment 2?
I still don't understand why the events are being delivered to <video> element event listeners in the first place. Can you explain that in more detail?
Comment 5 parenthetical at the end explains it...
So the binding parent of anonymous content created by the frame is the frame's element? And we have to explicitly block any event that fires on the anonymous content from being also delivered to the parent? That would suck!
All the events propagate from anonymous content by default. That is why we get various mouse events
even when mouse is over a textarea (which contains anonymous div).
Then there are cases when we don't want to propagate certain events. And this is a such case.
The other buttons use background-image to set their graphic but the fullscreen button uses list-style-image for the graphic.

https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/media/videocontrols.css#41
Ah, interesting, I didn't know list-style-image could cause a load event.
I assume that means that we're creating some native anon <img> element to represent list-style-image
in layout.
I'm not convinced propagation should be the default, since that means every time you add a new event on an element, or a new element to an XBL binding, you run the risk of events not being blocked properly by every bit of anonymous content that might use the element or the binding (possibly indirectly). Instead, if you want an element to support an event and you want to implement that by having the event propagate from anonymous content, you should implement that explicitly.

If we can't do that, then maybe at least we can quash all non-bubbling events coming from the anonymous content created by nsVideoFrame, except maybe for some specific list (mouseenter/mouseleave?)
> So the binding parent of anonymous content created by the frame is the frame's element? 

Yes.
(In reply to Olli Pettay [:smaug] from comment #13)
> Ah, interesting, I didn't know list-style-image could cause a load event.
> I assume that means that we're creating some native anon <img> element to
> represent list-style-image
> in layout.

XUL buttons create anonymous XUL <image>s. I guess using CSS background on the button element means that XUL <image> is unused so doesn't fire a load event.

I guess we can do a quick fix for this bug by using CSS background instead of list-style-image, and for bug 516811 by using CSS background and background-size instead of <img>, but this is a nasty trap for developers :-(. We need a way to at least opt-in to events not propagating by default.
With non-native anon XBL, XBL pretty much expects to get all the events.

Native anon is a different thing, and there we could probably only explicitly allow some
events. We just happen to have reasonable few native anon widgets, and so far
there hasn't been need for such thing.

Also, things like <input type="image"> relies on load event to propagate.

(I don't understand what bubbling vs. non-bubbling has to do with this.)
Bubbling events tend to be the sort of event you want to propagate, that's all.
Assignee: nobody → chris
cpearce: I've started on the bug but haven't made much progress, so thanks for taking it. We'll need to split the fullscreenButton.png to two images since background-image doesn't allow us to clip the background image.
Patch with test. Looks like we can do the split using -moz-image-rect. :)

Looks like it's going greenish on Try:
https://tbpl.mozilla.org/?tree=Try&rev=982f8f7bd378
Attachment #588325 - Flags: review?(dolske)
Comment on attachment 588325 [details] [diff] [review]
Patch: use -moz-image-rect for fullscreen button

Review of attachment 588325 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, just the two CSS tweaks.

::: toolkit/themes/pinstripe/global/media/videocontrols.css
@@ +45,4 @@
>  }
>  
>  .fullscreenButton[fullscreened] {
> +  background: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 32, 16, 16) no-repeat center;

This can be changed to only set background-image here and in the other CSS file.
Attachment #588325 - Flags: feedback+
I forgot to add that I tested it out and it looks and works fine. I also ran the tests too. Thanks for adding a test to make sure this doesn't happen again :)
Status: NEW → ASSIGNED
(In reply to Jared Wein [:jwein and :jaws] from comment #21)
> Comment on attachment 588325 [details] [diff] [review]
> Patch: use -moz-image-rect for fullscreen button
> [...]
> >  .fullscreenButton[fullscreened] {
> > +  background: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 32, 16, 16) no-repeat center;
> 
> This can be changed to only set background-image here and in the other CSS
> file.

What did you have in mind? Everything I tried other than exactly what's in the patch didn't work correctly, at least on my Win7 machine. Can you test and post the exact CSS necessary?
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
(In reply to Chris Pearce (:cpearce) from comment #23)
> What did you have in mind? Everything I tried other than exactly what's in
> the patch didn't work correctly, at least on my Win7 machine. Can you test
> and post the exact CSS necessary?

I just tested it. Here is the CSS that I have for winstripe:

.fullscreenButton {
  background: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 16, 16, 0) no-repeat center;
  -moz-appearance: none;
  margin: 0;
  padding: 0;
  min-height: 28px;
  min-width: 28px;
  border: none;
}

.fullscreenButton[fullscreened] {
  background-image: -moz-image-rect(url("chrome://global/skin/media/fullscreenButton.png"), 0, 32, 16, 16);
}
Patch including jwein's suggested change.
Attachment #588325 - Attachment is obsolete: true
Attachment #588325 - Flags: review?(dolske)
Attachment #589090 - Flags: review?(dolske)
dolske: review ping?
Does this break our spec compliance?

It's getting late in the Aurora cycle of Firefox 11, but it would be nice if we could at least get this fixed for Firefox 12.
status-firefox11: --- → affected
tracking-firefox11: --- → ?
Attachment #589090 - Flags: review?(gavin.sharp)
Comment on attachment 589090 [details] [diff] [review]
Patch: with jwein's suggested change

Review of attachment 589090 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh, this bit us in the past (hence all the existing background-image usage). Double r+ for finally having a test that checks for this.
Attachment #589090 - Flags: review?(gavin.sharp)
Attachment #589090 - Flags: review?(dolske)
Attachment #589090 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0a6f3638cdab
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
It's not clear from this bug that the user effects are significant enough to track for release. Please nominate for Beta 11 approval if deemed appropriate.
tracking-firefox11: ? → -
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: