Bug 462113 is adding a scrubber to the HTML5 <video> controls. It's basically a XUL <scale> overlayed on the video with anonymous XBL in content. The scrubber isn't working, trying to click or drag it never fires the onchange handler. Doing so files a "permission denied" error, for which I filed bug 470852. I commented out that security check, but the control was still broken. Here's what seems to be the problem now... toolkit/content/widgets/scale.xml uses a DOMAttrModified handler to watch for changes to the |curpos| attribute and then fire a |change| event. This code doesn't get the DOMAttrModified event when the <scale> is in the videocontrols. The <scale> is basically just a <slider>, which mostly seems to be working... Clicking the bar to change the position is invoking nsSliderFrame::HandleEvent() and eventually nsSliderFrame::SetCurrentPositionInternal(). It calls UpdateAttribute() which finally calls SetAttr() with nsGkAtoms::curpos. So far so good, and a plain standalone <scale> element works the same way up to this point. Things seem to go wrong in nsGenericElement::SetAttr(). The first sign of trouble is call to nsContentUtils::HasMutationListeners()... It returns FALSE for the video control, but TRUE for a standalone <scale>. This is the culprit: 3257 if (aNode->IsNodeOfType(nsINode::eCONTENT) && 3258 static_cast<nsIContent*>(aNode)->IsInNativeAnonymousSubtree()) { 3259 return PR_FALSE; 3260 } So, uhh, that seems to be what's happening, from what I can piece together.

Um, we don't want any binding to add mutation listeners to content pages. Having just one mutation listener in a page slows down pretty much all dom changes (dhtml). Anyway, do you have a minimal testcase for this bug?

And we really don't want to fire mutation events in native anonymous content.

I don't have a minimal testcase. It's reproducable by applying the patches from bug 462113 and bug 470852, loading http://www.double.co.nz/video_test/test5.html, and clicking the progress bar. And, I suppose, tweak the onchange in videocontrols.xml to alert/dump so you can see it. I just tried constructing a simple HTML file with a DIV and a binding like <content> <scale flex="1" onchange="alert('yay')"/> </content> ...but that seems to work. http://people.mozilla.com/~dolske/tmp/binding.html I guess the <video> anonymous content is built differently? Not sure how to make a testcase for that.

<video> has *native* anonymous content. http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp?mark=93-111#92 http://people.mozilla.com/~dolske/tmp/binding.html creates just normal anonymous content.

(In reply to comment #2) > And we really don't want to fire mutation events in native anonymous content. Is there a workaround that can be used here? Or is the only alternative to rewrite <scale> to not use DOMAttrModified? [I suppose I could try implementing a <scale> from scratch for the videocontrols, but that sounds bad too.] If the <slider> fired an event when curpos/minpos/maxmos was changed, the <scale> XBL could listed for that instead of using DOMAttrModified. I dunno if that has negative consequences, though...

Dispatching any extra events from native anonymous content sounds a bit scary. Perhaps slider could have some method to add "position listener" and scale could implement such listener.

(In reply to comment #6) > Perhaps slider could have some method to add "position listener" and scale > could implement such listener. Sounds good. In the long run, I'd like to combine this with cleaning up nsIScrollbarMediator and nsIScrollbarListener (the latter isn't used for anything) which serve a similar purpose.

This implementation calls a method to indicate that the value or range has changed. But it isn't fully safe as it calls into the binding script when curpos, minpos or maxpos is changed. It also fires the change event when curpos changes. Olli, what needs to happen here for this to be safely called?

Comment on attachment 355631 [details] [diff] [review] Basic implementation >+ if (aAttribute == nsGkAtoms::minpos) >+ sliderListener->RangeChanged(PR_FALSE, min); >+ else >+ sliderListener->RangeChanged(PR_TRUE, max); You should probably do this in a script runner. >+ if (sliderListener) >+ sliderListener->ValueChanged(oldPos, mCurPos); >+ } And this About the change event. Perhaps you could check if script can access 'this' element's properties without security exceptions and dispatch event only in that case. A bit hackish yes, but I don't think we use similar XBL bindings in native anonymous content elsewhere.

It looks as if videocontrols doesn't use much of the functionality of <scale>, so rather than relying on a change event could it perhaps use a <slider> directly that it could use with its own slider listener?

Indeed, that sounds better than checking if the script can access the element.

Using slider directly sounds like a better plan as the videocontrols aren't using the native theming and are only need the slider functionality. We can improve the scale/slider listener code later as we still want to remove the use of mutation events.

(In reply to comment #10) > It looks as if videocontrols doesn't use much of the functionality of <scale>, I don't quite understand this. It seems like I'd have to reimplement the thumb, the key bindings, minx/max handling, and the accessibility stuff... And that's most of <scale>. It's just cut'n'paste, so it's not hard, but then we'd have a fair chunk of duplicated code.

This is a better patch. Not sure about the change event or if the issue still applies here. Yeah, thinking about it more, using slider directly probably isn't as useful for your case.

Comment on attachment 356056 [details] [diff] [review] improved patch Actually I realize I didn't add the feature to disable the change event. Do we want that disabled always for videocontrols or just when script modifies it? I guess the former.

The videocontrols do want a change event. Sometimes. When the user changes the playback position by using the <scale>, the controls should be notified so the video can be seeked to the desired position. The tricky bit is that while a video is playing normally, the controls are updating the <scale> indicate the current playback position. The controls don't want a change event for that, because we don't want to trigger a seek back to that position.

(The change event stuff could probably be done in a separate bug, I think it's independent of this DOMAttrModified issue.)

I think we don't want the change event to be dispatched, because of possible security reasons, but also because web pages may not expect such event from non-form control.

Oh, hmm. Will that also be a problem with <button> and its command event? (added in bug 464371). In any case, as long as I have some reasonable way of getting notified when the <scale> is changed by the user, I don't care too much about the mechanism for doing so. [Would another option be to stick with the change event for now, given the pressing Beta 3 freeze, and switch to a listener before RC1? Though I dunno if that really helps, or if the security issue makes that a non-starter.]

(In reply to comment #13) > the key bindings Oh, so the <scale> inside the <videocontrols> gets keyboard focus?

Actually I realised we already do that with <input type="file"> too.

In this case I'm not so worried about security, but "future compatibility". If we start to expose some non-standard events on <video>, I'm pretty sure web content will use them. And that means we must support them forever. Focus/blur are IMO ok. And all the mouse events, because of the event retargeting.

(In reply to comment #22) > In this case I'm not so worried about security, but "future compatibility". > If we start to expose some non-standard events on <video>, I'm pretty sure web > content will use them. And that means we must support them forever. > Focus/blur are IMO ok. And all the mouse events, because of the event > retargeting. Yes, what we really need is a way for a binding to indicate that it is the root for dispatch of a particular event and not allow it to be seen outside of that element. But that's harder I assume. But anyway, I realize also that changing the default behaviour of the change event for <scale> for 1.9.1 would be an compatibility change that may be too late to take. Instead for 1.9.1, I'd propose that we use this patch, and the videocontrols should create a binding that extends the default scale and overrides just the valueChanged method so as not to fire the change event.

(In reply to comment #23) > Instead for 1.9.1, I'd propose that we use this patch, and the videocontrols > should create a binding that extends the default scale and overrides just the > valueChanged method so as not to fire the change event. I think I can also add some kind of indicator that the change was caused by user interaction as opposed to a script change.

(In reply to comment #23) > Yes, what we really need is a way for a binding to indicate that it is the root > for dispatch of a particular event and not allow it to be seen outside of that > element. But that's harder I assume. Shouldn't be very hard, but a bit late for 1.9.1.

Comment on attachment 356192 [details] [diff] [review] patch which adds a flag for checking user changes versus script changes > CurrentPositionChanged(frame->PresContext(), aImmediateRedraw); > } > } >+ mUserChanged = PR_FALSE; Why not just add an extra parameter to CurrentPositionChanged?

(In reply to comment #27) > (From update of attachment 356192 [details] [diff] [review]) > > CurrentPositionChanged(frame->PresContext(), aImmediateRedraw); > > } > > } > >+ mUserChanged = PR_FALSE; > Why not just add an extra parameter to CurrentPositionChanged? It isn't called directly. That line you listed here is only called for scrollbars which don't apply here.

Enn, could you provide a -P -U 8 patch. I'm not sure I like the name "userChanged" too much, though can't think anything better right now. How could we get rid of the "change" event? Could <scale> have _changeListener (something which also implements nsISliderListener), and if that is set, "change" isn't dispatched but valueChanged() call is just forwarded? Or do we need the "stop-anonymous-event-propagation" for XBL?

Comment on attachment 356192 [details] [diff] [review] patch which adds a flag for checking user changes versus script changes >+ // inform the parent <scale> if it exists that the value changed >+ nsIFrame* parent = GetParent(); >+ if (parent) { >+ nsCOMPtr<nsISliderListener> sliderListener = do_QueryInterface(parent->GetContent()); >+ if (sliderListener) { >+ nsContentUtils::AddScriptRunner( >+ new nsValueChangedRunnable(sliderListener, nsGkAtoms::curpos, mCurPos, mUserChanged)); >+ } >+ } So why not put this code block... > UpdateAttribute(scrollbar, aNewPos, PR_TRUE, aIsSmooth); here in the first place?

(In reply to comment #29) > How could we get rid of the "change" event? I'm not sure what you're asking here. We don't need to get rid of it. The videocontrols will override the valueChanged method in a derived binding which doesn't need to fire the change event and also asks as the needed notification.

(In reply to comment #31) > The > videocontrols will override the valueChanged method in a derived binding which > doesn't need to fire the change event and also asks as the needed notification. That was the answer to my question :) But still, could you please provide -p -U 8 patch

(In reply to comment #30) > > UpdateAttribute(scrollbar, aNewPos, PR_TRUE, aIsSmooth); > here in the first place? I think it would be more correct to make the notification when we know the attribute has actually changed. It could for example be changed externally for some reason.

Comment on attachment 356529 [details] [diff] [review] more context >+ mUserChanged = PR_TRUE; >+ > if (scrollbarFrame) { > // See if we have a mediator. Is it possible to have both a mediator and a slider listener?

Comment on attachment 356529 [details] [diff] [review] more context >+ NS_IMETHODIMP Run() >+ { >+ nsAutoString atom; A bit strange name for the variable. >+ >+ PRBool mUserChanged; Could you please document when this is true and when false. This misses nsIScrollbarListener.h and nsISliderListener.idl. But anyway, r=me

(In reply to comment #35) > (From update of attachment 356529 [details] [diff] [review]) > >+ mUserChanged = PR_TRUE; > >+ > > if (scrollbarFrame) { > > // See if we have a mediator. > Is it possible to have both a mediator and a slider listener? Possible, if someone made an extended scrollbar which implemented nsISliderListener and used it for a listbox or tree.