587910 - SVG SMIL: Implement begin/end accessKey

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

SMIL allows animations to be triggered with the keyboard with the begin/end accessKey syntax. Several tests in the SVG 1.1 test suite require support for this.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

Ok, here's accessKey timing which we're hoping to get in before next week's feature freeze.

Some particular areas I'd appreciate review feedback on are:

* I've decided to disallow characters in the range 0x0 to 0x1F (and 0x7F, i.e. del). The reasoning is that accesskey seems to be intended to match characters rather than keys (according to the parsing rules) and it also seems like we don't want backspace, esc etc. to trigger animations since they already do things in the browser.

One implication of this is that you can't use enter to trigger an animation. That means the last example in this tutorial won't work:
http://www.ibm.com/developerworks/library/x-svgint/#N1011F

However, I think that example wouldn't work anyway since I believe we normalise that newline to a space (0x20) before it gets to content anyway (or at least that's what's showing up in my debugger, I haven't really looked into it).

* I've made SMIL listen for keypress events at the document root element level. This is largely for convenience since elsewhere in the code we're expecting to register against an element (rather than say a document or window).

I think it's reasonable because that seems to match our event dispatching behaviour in that if you load up an SVG document and press a key the target of the keypress event is the root SVG element. That said, note that we're registering against the document root element rather than the outer SVG element so it still works for compound documents.

One situation where you might run into trouble however is if you create a synthesised keypress event and dispatch it to the document object or the window. In that case we won't catch it.

As for listening to events at the window level, I suspect this would be a security hazard as then external content referenced via <object>/<embed>/<img> could potentially listen in to events in the container document (i.e. set up separate animations on each of the keys and register event listeners on the corresponding animation begin events).

The drawback of course is that in a document with external SVG images, you need to click on the SVG to give it focus before it will receive key events which is not particularly intuitive for most users but seems to be what we do for Flash anyway.

* I haven't exposed the access keys through nsCoreUtils::GetAccessKeyFor(nsIContent*)
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsCoreUtils.cpp#203

This is because that interface doesn't seem to make sense for this scenario. I suppose the content you'd want to get the access key for would be the animation TARGET rather than the animation element itself and there's potentially a many-to-many mapping between accesskeys and animation targets so it's not clear what to return.

Also, SMIL accesskeys work differently to HTML accesskeys in that no modifier key is required. SMIL makes this explicit: http://www.w3.org/TR/SMIL/smil-timing.html#q29

I wonder if there's anywhere else we need to expose these access keys though or if we can just rely on the tools that need them scraping the content.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> * I've decided to disallow characters in the range 0x0 to 0x1F (and 0x7F, i.e.
> del). The reasoning is that accesskey seems to be intended to match characters
> rather than keys (according to the parsing rules) and it also seems like we
> don't want backspace, esc etc. to trigger animations since they already do
> things in the browser.

I actually think that's OK, provided events are handled at the right point in the chain. Web pages can already trap those keys and prevent them from activating browser UI.

> One implication of this is that you can't use enter to trigger an animation.
> That means the last example in this tutorial won't work:
> http://www.ibm.com/developerworks/library/x-svgint/#N1011F
> 
> However, I think that example wouldn't work anyway since I believe we
> normalise that newline to a space (0x20) before it gets to content anyway (or
> at least that's what's showing up in my debugger, I haven't really looked
> into it).

That sounds strange. Newlines should definitely not be converted to spaces in key events!

> * I've made SMIL listen for keypress events at the document root element
> level. This is largely for convenience since elsewhere in the code we're
> expecting to register against an element (rather than say a document or
> window).

You should be able to register on the window and get events that bubble to there. The spec should really say how these accesskey handlers are related to the rest of the DOM event system.

One question is whether we should fire accesskey animations on events that have been cancelled via preventDefault. Does the spec say? What do other browsers do? If not, then you should check getPreventDefault. Might need to do this for the other event listeners too...

> As for listening to events at the window level, I suspect this would be a
> security hazard as then external content referenced via <object>/<embed>/<img>
> could potentially listen in to events in the container document (i.e. set up
> separate animations on each of the keys and register event listeners on the
> corresponding animation begin events).

No, that won't happen. The <object>/<embed>/<img> content has its own DOM window object which will not receive events unless some content in the embedded document has focus. (Content in an SVG document inside an <img> will never have focus.) Basically there's one DOM window per document.

> The drawback of course is that in a document with external SVG images, you
> need to click on the SVG to give it focus before it will receive key events
> which is not particularly intuitive for most users but seems to be what we do
> for Flash anyway.

That is correct behaviour for <object>/<embed>.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 468588 [details] [diff] [review]
Patch v1a

>+nsSMILTimeValueSpec::CheckRepeatEventDetail(nsIDOMEvent *aEvent)
[snip]
>-  return detail == mParams.mRepeatIterationOrAccessKey;
>+  return detail > 0 && detail == mParams.mRepeatIterationOrAccessKey;

This chunk does half of what bug 588718's patch does -- might as well go all the way and add the PRUint32 cast, too, to fix the build warning.

(this effectively obsoletes bug 588718's patch -- that's fine -- it's probably simpler to just make the change here, rather than to have to request & wait for approval on that bug's patch, land it, and rebase your stuff)

Looks good aside from that! r=dholbert, though I'm interested to see the changes from roc's comments.

Comment 4

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

Comment on attachment 468588 [details] [diff] [review]
Patch v1a


>+PRBool
>+nsSMILTimeValueSpec::VerifyParams(
>+    const nsSMILTimeValueSpecParams& aParams) const
>+{
>+  if (aParams.mType != nsSMILTimeValueSpecParams::ACCESSKEY)
>+    return PR_TRUE;
>+
>+  // For accesskey specifications we disallow characters in the C0 control
>+  // character range (0x0 to 0x1F) including del (0x7F) since we don't really
>+  // want esc, backspace and co. firing animations.
>+  return aParams.mRepeatIterationOrAccessKey >= 0x20 &&
>+         aParams.mRepeatIterationOrAccessKey != 0x7F;
I don't really see the reason for this.

Should you somewhere check whether shift/ctrl/meta keys are pressed too and
not handle those events.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1b, r=dholbert

Thanks everyone for your feedback. Sorry, this will have to be really quick. I have to go but I'd like to get feedback as soon as possible so I'm just posting what I have so far.

I haven't yet implemented all of the suggestions (notably the preventDefault) behaviour nor run it through the tests but I'd appreciate your feedback on the points below.

(In reply to comment #2)
> (In reply to comment #1)
> > * I've decided to disallow characters in the range 0x0 to 0x1F (and 0x7F, i.e.
> > del). The reasoning is that accesskey seems to be intended to match characters
> > rather than keys (according to the parsing rules) and it also seems like we
> > don't want backspace, esc etc. to trigger animations since they already do
> > things in the browser.
> 
> I actually think that's OK, provided events are handled at the right point in
> the chain. Web pages can already trap those keys and prevent them from
> activating browser UI.

Ok, fair enough. Fixed.

> > One implication of this is that you can't use enter to trigger an animation.
> > That means the last example in this tutorial won't work:
> > http://www.ibm.com/developerworks/library/x-svgint/#N1011F
> > 
> > However, I think that example wouldn't work anyway since I believe we
> > normalise that newline to a space (0x20) before it gets to content anyway (or
> > at least that's what's showing up in my debugger, I haven't really looked
> > into it).
> 
> That sounds strange. Newlines should definitely not be converted to spaces in
> key events!

Sorry, I wasn't clear. I was referring to the parsing, not the event dispatching. In the example, there's a newline embedded in the attribute value, which is apparently "the standard HTML encoding for the carrage [sic] return (Enter) key." However, I think in our XML parsing this gets normalised to a space as per http://www.w3.org/TR/REC-xml/#AVNormalize. So I think the tutorial is at fault. Nevertheless I've added test cases to ensure we do support using Enter when suitably encoded.

> > * I've made SMIL listen for keypress events at the document root element
> > level. This is largely for convenience since elsewhere in the code we're
> > expecting to register against an element (rather than say a document or
> > window).
> 
> You should be able to register on the window and get events that bubble to
> there. The spec should really say how these accesskey handlers are related to
> the rest of the DOM event system.

Ok, I've made it register with the window instead although I'm not entirely confident it's what you had in mind.

Specifically, it seems like you need to get a window root object in order to get a listener manager. As a result, now if you have an SVG file included with an <object> element you no longer need to focus on the object element in order for it to catch keypress events. Is this correct?

> One question is whether we should fire accesskey animations on events that have
> been cancelled via preventDefault. Does the spec say? What do other browsers
> do? If not, then you should check getPreventDefault. Might need to do this for
> the other event listeners too...

I looked into this for event timing. The spec doesn't say but other browsers ignore calls to preventDefault. See: https://bugzilla.mozilla.org/show_bug.cgi?id=485157#c25

Therefore, for consistency I decided to ignore calls here too. But it does seem odd. I think we should honour preventDefault as this does feel like a default action.

I'll wait to hear from you before making this change though as I don't have time just now.

> > As for listening to events at the window level, I suspect this would be a
> > security hazard as then external content referenced via <object>/<embed>/<img>
> > could potentially listen in to events in the container document (i.e. set up
> > separate animations on each of the keys and register event listeners on the
> > corresponding animation begin events).
> 
> No, that won't happen. The <object>/<embed>/<img> content has its own DOM
> window object which will not receive events unless some content in the embedded
> document has focus. (Content in an SVG document inside an <img> will never have
> focus.) Basically there's one DOM window per document.

Ok, good, that makes more sense.

> > The drawback of course is that in a document with external SVG images, you
> > need to click on the SVG to give it focus before it will receive key events
> > which is not particularly intuitive for most users but seems to be what we do
> > for Flash anyway.
> 
> That is correct behaviour for <object>/<embed>.

Ok, as noted above, I think I've done something wrong then.

(In reply to comment #3)
> Comment on attachment 468588 [details] [diff] [review]
> Patch v1a
> 
> >+nsSMILTimeValueSpec::CheckRepeatEventDetail(nsIDOMEvent *aEvent)
> [snip]
> >-  return detail == mParams.mRepeatIterationOrAccessKey;
> >+  return detail > 0 && detail == mParams.mRepeatIterationOrAccessKey;
> 
> This chunk does half of what bug 588718's patch does -- might as well go all
> the way and add the PRUint32 cast, too, to fix the build warning.
Thanks, fixed.

(In reply to comment #4)
> >+PRBool
> >+nsSMILTimeValueSpec::VerifyParams(
> >+    const nsSMILTimeValueSpecParams& aParams) const
> >+{
> >+  if (aParams.mType != nsSMILTimeValueSpecParams::ACCESSKEY)
> >+    return PR_TRUE;
> >+
> >+  // For accesskey specifications we disallow characters in the C0 control
> >+  // character range (0x0 to 0x1F) including del (0x7F) since we don't really
> >+  // want esc, backspace and co. firing animations.
> >+  return aParams.mRepeatIterationOrAccessKey >= 0x20 &&
> >+         aParams.mRepeatIterationOrAccessKey != 0x7F;
> I don't really see the reason for this.

As noted, above, fixed.

> Should you somewhere check whether shift/ctrl/meta keys are pressed too and
> not handle those events.

Good point, yes we should.

I'm a little unsure what the behaviour should be but I think we should ignore the state of the shift key when matching on charCodes since we shouldn't care whether or not a user needs to press shift in order to produce a certain character on their keyboard.

For alt however, I note that on Windows if we want to type a character not available on the keyboard such as by typing Alt+0229 then the final keypress event has charCode 229 and alt=false. So we're ok to say that if alt is true we should ignore the event (so that, for example, we ignore alt+f). Not sure about meta?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Specifically, it seems like you need to get a window root object in order to
> get a listener manager. As a result, now if you have an SVG file included with
> an <object> element you no longer need to focus on the object element in order
> for it to catch keypress events. Is this correct?

No. You should be able to do the equivalent of nsGlobalWindow::AddEventListener to the nsGlobalWindow (DOM 'window') for the SVG document. That would not receive key events unless something in the SVG document is focused.

> > One question is whether we should fire accesskey animations on events that have
> > been cancelled via preventDefault. Does the spec say? What do other browsers
> > do? If not, then you should check getPreventDefault. Might need to do this for
> > the other event listeners too...
> 
> I looked into this for event timing. The spec doesn't say but other browsers
> ignore calls to preventDefault. See:
> https://bugzilla.mozilla.org/show_bug.cgi?id=485157#c25

Yeah, it certainly seems like it might be useful for scripts to be able to handle keypresses and prevent accesskey handlers from firing. I suggest you raise it as a spec issue with the SVG WG but be consistent with the other browsers for now (plus ignoring preventDefault is simpler). If the WG decides we should honor preventDefault, that's easy to add as a separate bug, and likely won't have compatibility issues (since preventDefault is rarely used, especially with accesskey SMIL animations...).

> > Should you somewhere check whether shift/ctrl/meta keys are pressed too and
> > not handle those events.
> 
> Good point, yes we should.
> 
> I'm a little unsure what the behaviour should be but I think we should ignore
> the state of the shift key when matching on charCodes since we shouldn't care
> whether or not a user needs to press shift in order to produce a certain
> character on their keyboard.

Definitely!

> For alt however, I note that on Windows if we want to type a character not
> available on the keyboard such as by typing Alt+0229 then the final keypress
> event has charCode 229 and alt=false. So we're ok to say that if alt is true
> we should ignore the event (so that, for example, we ignore alt+f). Not sure
> about meta?

I actually don't think we should do this. On my Mac keyboard, "Alt-letter" produces various Unicode characters, e.g. Alt-a gives å. It seems plausible that accesskey="å" should work if the user presses Alt-a.

I suggest we ignore "Alt" and "Shift" and bail out if any other modifiers are down.

Another thing to consider is whether we should follow the DOM accesskey logic in nsEventStateManager::HandleAccessKey. In particular it calls nsContentUtils::GetAccessKeyCandidates, to handle gnarly situations with where the character code produced by the user's keystroke is actually ambiguous! I suspect we shouldn't though. So never mind.

Comment 7

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

Comment on attachment 468921 [details] [diff] [review]
Patch v1b, r=dholbert

>-  nsIEventListenerManager* elm =
>+  nsCOMPtr<nsIEventListenerManager> elm =
>     GetEventListenerManager(aTarget, getter_AddRefs(sysGroup));
Why this change?


>-  nsIEventListenerManager* elm =
>+  nsCOMPtr<nsIEventListenerManager> elm =
>     GetEventListenerManager(aTarget, getter_AddRefs(sysGroup));
Here too.

> nsSMILTimeValueSpec::GetEventListenerManager(Element* aTarget,
>                                              nsIDOMEventGroup** aSystemGroup)
> {
>   NS_ABORT_IF_FALSE(aTarget, "null target; can't get EventListenerManager");
>   NS_ABORT_IF_FALSE(aSystemGroup && !*aSystemGroup,
>       "Bad out param for system group");
> 
>-  nsIEventListenerManager* elm = aTarget->GetListenerManager(PR_TRUE);
>+  nsPIDOMEventTarget*      piTarget;
>+  nsCOMPtr<nsPIWindowRoot> piWindowRoot;
You certainly don't want to use nsPIWindowRoot for anything.
I think you should add the listener to window. (QI to nsPIDOMEventTarget)

>+  // We need to addref the nsIEventListenerManager because otherwise, if we got
>+  // the listener manager from the window, there's no guarantee it will still be
>+  // valid after we drop the reference to the window root.
>+  NS_IF_ADDREF(elm);
This is wrong comment. If you're not changing anything in the DOM tree, EventListenerManagers will stay alive.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1c, r=dholbert

(In reply to comment #6)
> > Specifically, it seems like you need to get a window root object in order to
> > get a listener manager. As a result, now if you have an SVG file included with
> > an <object> element you no longer need to focus on the object element in order
> > for it to catch keypress events. Is this correct?
> 
> No. You should be able to do the equivalent of nsGlobalWindow::AddEventListener
> to the nsGlobalWindow (DOM 'window') for the SVG document. That would not
> receive key events unless something in the SVG document is focused.

I've made it use GetCurrentDoc()->GetWindow() and it seems to give us the behaviour we want. I got a bit lost navigating all the event interfaces (and MXR wasn't giving me what I thought it was) but smaug's comment helped.

> Yeah, it certainly seems like it might be useful for scripts to be able to
> handle keypresses and prevent accesskey handlers from firing. I suggest you
> raise it as a spec issue with the SVG WG but be consistent with the other
> browsers for now (plus ignoring preventDefault is simpler). If the WG decides
> we should honor preventDefault, that's easy to add as a separate bug, and
> likely won't have compatibility issues (since preventDefault is rarely used,
> especially with accesskey SMIL animations...).

Ok, I'll raise it later.

> > For alt however, I note that on Windows if we want to type a character not
> > available on the keyboard such as by typing Alt+0229 then the final keypress
> > event has charCode 229 and alt=false. So we're ok to say that if alt is true
> > we should ignore the event (so that, for example, we ignore alt+f). Not sure
> > about meta?
> 
> I actually don't think we should do this. On my Mac keyboard, "Alt-letter"
> produces various Unicode characters, e.g. Alt-a gives a. It seems plausible
> that accesskey="a" should work if the user presses Alt-a.
> 
> I suggest we ignore "Alt" and "Shift" and bail out if any other modifiers are
> down.
Fixed.

> >-  nsIEventListenerManager* elm =
> >+  nsCOMPtr<nsIEventListenerManager> elm =
> >     GetEventListenerManager(aTarget, getter_AddRefs(sysGroup));
> Why this change?

This is all gone away now. It was due to my confusion about the event interfaces, and because the lifetimes of some of these things isn't immediately obvious to an outside observer. Thanks, your comments cleared this up.

> You certainly don't want to use nsPIWindowRoot for anything.
> I think you should add the listener to window. (QI to nsPIDOMEventTarget)
Fixed.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  PRBool isAlt;
+  PRBool isShift;
+  keyEvent->GetAltKey(&isAlt);
+  keyEvent->GetShiftKey(&isShift);
+
+  keyEvent->GetKeyCode(&code);
+  return code == mParams.mRepeatIterationOrAccessKey && !isShift && !isAlt;

Do we really want to do this? Does the spec say to match keyCodes like this? How would an author use it?

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #9)
> +  PRBool isAlt;
> +  PRBool isShift;
> +  keyEvent->GetAltKey(&isAlt);
> +  keyEvent->GetShiftKey(&isShift);
> +
> +  keyEvent->GetKeyCode(&code);
> +  return code == mParams.mRepeatIterationOrAccessKey && !isShift && !isAlt;
> 
> Do we really want to do this? Does the spec say to match keyCodes like this?
> How would an author use it?

One example is the reftest in this patch: accesskey-entity-2.svg
It has the begin="accesskey(
)" to match on the enter key. When this key is pressed the charCode field will be 0 and the keyCode will be set to 0x0D. However, if the user presses shift+Enter etc. we ignore it. Likewise for delete, backspace etc.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #9)
> Does the spec say to match keyCodes like this?

Despite the fact that it is called accessKEY the parsing rules in the spec are expressed in terms of CHARACTERS. SMIL Animation wants a single character from ISO10646. SMIL 3.0 wants an XML 1.1 character. That was the reasoning behind my initial implementation that screened out control characters since they generally correspond to keys rather than characters (see comment 1). But I thought we decided to allow matching on keys too (comment 2).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that the keyCodes mostly do not map to Unicode characters. E.g. DOM_VK_NUMPAD1 is 0x61, so I think your current code would activate accesskey="a" if someone pressed DOM_VK_NUMPAD1. That seems wrong.

We might need to have a list of ASCII characters that can be activated for accesskey that don't generate charCodes. I think that list is just:
DOM_VK_BACK_SPACE (0x08)
DOM_VK_TAB (0x09)
DOM_VK_RETURN (0x0D) (maybe DOM_VK_ENTER should map to 0x0D as well?)
DOM_VK_ESCAPE (0x1B)

You might also want to map Ctrl-(a-z/A-Z) to ASCII 1-26 for old time's sake. This all needs to be in the spec though.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #8)
> > Yeah, it certainly seems like it might be useful for scripts to be able to
> > handle keypresses and prevent accesskey handlers from firing. I suggest you
> > raise it as a spec issue with the SVG WG but be consistent with the other
> > browsers for now (plus ignoring preventDefault is simpler). If the WG decides
> > we should honor preventDefault, that's easy to add as a separate bug, and
> > likely won't have compatibility issues (since preventDefault is rarely used,
> > especially with accesskey SMIL animations...).
> 
> Ok, I'll raise it later.

http://lists.w3.org/Archives/Public/www-smil/2010JulSep/0006.html
http://lists.w3.org/Archives/Public/www-svg/2010Aug/0086.html

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #12)
> The problem is that the keyCodes mostly do not map to Unicode characters. E.g.
> DOM_VK_NUMPAD1 is 0x61, so I think your current code would activate
> accesskey="a" if someone pressed DOM_VK_NUMPAD1. That seems wrong.
Agreed. I forgot about that. I guess that's why originally I ignored keyCodes outright.

> We might need to have a list of ASCII characters that can be activated for
> accesskey that don't generate charCodes. I think that list is just:
> DOM_VK_BACK_SPACE (0x08)
> DOM_VK_TAB (0x09)
> DOM_VK_RETURN (0x0D) (maybe DOM_VK_ENTER should map to 0x0D as well?)
> DOM_VK_ESCAPE (0x1B)
Ok, that seems reasonable.

However, I'm not sure about Escape and Backspace.
    <animate begin="accessKey(&#x1B;)" ...
Gives you an XML parsing error since it's not a valid character. Likewise for &#x08; (Backspace).
You can set these characters via javascript, i.e. setAttribute('begin', 'accessKey(\x08)') but it doesn't seem right that you can do something like that only via script.

Tab is also a bit funky since the event seems to get consumed until focus returns to content and then it works. i.e. press tab once and focus changes to the location bar and the animation doesn't start. Press it again and we go to the search box. Press it a third time and focus returns to content and the animation starts.

> You might also want to map Ctrl-(a-z/A-Z) to ASCII 1-26 for old time's sake.
I think I'd rather not. Plus doesn't that overlap? (e.g. with carriage-return etc.)

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1d, r=dholbert

Update with feedback from comment 12. Allow a limited range of ASCII characters that only produce keyCodes.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> However, I'm not sure about Escape and Backspace.
>     <animate begin="accessKey(&#x1B;)" ...
> Gives you an XML parsing error since it's not a valid character. Likewise for
> &#x08; (Backspace).
> You can set these characters via javascript, i.e. setAttribute('begin',
> 'accessKey(\x08)') but it doesn't seem right that you can do something like
> that only via script.

I bet you can do it in HTML though. Restrictions on XML syntax shouldn't necessarily determine what to do here.

> Tab is also a bit funky since the event seems to get consumed until focus
> returns to content and then it works. i.e. press tab once and focus changes to
> the location bar and the animation doesn't start. Press it again and we go to
> the search box. Press it a third time and focus returns to content and the
> animation starts.

:-) I guess we're switching focus before processing the keydown event. Fine, probably should just leave that out.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 469747 [details] [diff] [review]
Patch v1d, r=dholbert

This is OK, but I think we should support backspace and escape too, even if you can't write them in XML.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And I think you should take out TAB.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1e, r=dholbert, sr=roc

Removed tab.
Added backspace and escape.

Currently escape doesn't appear to work but that's because the default action of Esc is to pause animations (see bug 532484). So it does in fact work correctly but at the same time animations are paused.

Will wait for smaug's review before requesting landing approval.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 469836 [details] [diff] [review]
Patch v1e, r=dholbert, sr=roc

Requesting approval to land. This completes our begin/end timing support (since we're not doing wallclock) and so we'd like it to be included in FF4.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 469836 [details] [diff] [review]
Patch v1e, r=dholbert, sr=roc

Let's get this in!

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/a249d8624177