472090 - DOMAttrModified doesn't fire, breaks XUL <scale> in native anonymous XBL.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Justin Dolske [:Dolske]]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

<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.

Comment 5

[Justin Dolske [:Dolske]]

(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...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Neil Deakin]

(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.

Comment 8

[Neil Deakin]

Attachment: Basic implementation

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 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[[e-mail]]

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?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Neil Deakin]

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.

Comment 13

[Justin Dolske [:Dolske]]

(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.

Comment 14

[Neil Deakin]

Attachment: improved patch

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 15

[Neil Deakin]

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.

Comment 16

[Justin Dolske [:Dolske]]

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.

Comment 17

[Justin Dolske [:Dolske]]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Justin Dolske [:Dolske]]

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.]

Comment 20

[[e-mail]]

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

Comment 21

[[e-mail]]

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

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[Neil Deakin]

(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.

Comment 24

[Neil Deakin]

(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.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(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 26

[Neil Deakin]

Attachment: patch which adds a flag for checking user changes versus script changes

Comment 27

[[e-mail]]

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?

Comment 28

[Neil Deakin]

(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.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

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 30

[[e-mail]]

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?

Comment 31

[Neil Deakin]

(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.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 33

[Neil Deakin]

(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 34

[Neil Deakin]

Attachment: more context

Comment 35

[[e-mail]]

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 36

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 37

[Neil Deakin]

(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.