Closed Bug 1314544 Opened 8 years ago Closed 8 years ago

[DateTimeInput] browser keyboard shortcut does not work when focus is on input box

Categories

(Core :: Layout: Form Controls, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

(Whiteboard: [datetime])

Attachments

(1 file, 4 obsolete files)

STR:
- Navigate to "data:text/html, <input type=time>
- click on input box to get focus
- press Cmd+R (on mac)

Expected Results: page is reloaded

Actual Results: nothing happens
Whiteboard: [datetime]
Attached patch WIP (obsolete) — Splinter Review
After reading bug 1274520, it seems that we should move our event listeners/handlers to system group, may do it in this bug.
Attached patch patch, v1. (obsolete) — Splinter Review
In this patch...
- use preventDefault() only on captured keypress events
- move xbl events listeners from default group to system group
- disallow cut/copy/paste
- disallow text selection from user (to avoid conflicts with editor, Chrome disallows selection as well)
Assignee: nobody → jjong
Comment on attachment 8808535 [details] [diff] [review]
patch, v1.

Please refer to comment 3 for details.
Attachment #8808535 - Flags: review?(mconley)
Comment on attachment 8808535 [details] [diff] [review]
patch, v1.

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

Hey Jessica,

This looks pretty good, but I'm worried about hardcoding some of these characters because I don't know if the cut/copy/paste characters are universal (we appear to localize them).

::: toolkit/content/widgets/datetimebox.xml
@@ +794,5 @@
>  
> +          // Ignore keys with modifiers.
> +          if (aEvent.ctrlKey || aEvent.altKey || aEvent.metaKey) {
> +            // Disallow copy, paste and cut.
> +            if (aEvent.key == "c" || aEvent.key == "v" || aEvent.key == "x") {

Are these keyboard shortcuts universal? I'm seeing evidence that we localize these keyboard shortcuts - for example:

http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/browser/locales/en-US/chrome/browser/browser.dtd#308

@@ +826,5 @@
> +              break;
> +            }
> +            default: {
> +              // printable characters
> +              if (aEvent.keyCode == 0) {

Apparently, keyCode is deprecated - see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code is recommended instead.

Also, it took me a few moments to figure out how this worked. Is this really the best way of determining if the character is printable, or is there a way that doesn't use deprecated APIs?
Attachment #8808535 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #5)
> Comment on attachment 8808535 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8808535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jessica,
> 
> This looks pretty good, but I'm worried about hardcoding some of these
> characters because I don't know if the cut/copy/paste characters are
> universal (we appear to localize them).

Thanks for point this out.

> 
> ::: toolkit/content/widgets/datetimebox.xml
> @@ +794,5 @@
> >  
> > +          // Ignore keys with modifiers.
> > +          if (aEvent.ctrlKey || aEvent.altKey || aEvent.metaKey) {
> > +            // Disallow copy, paste and cut.
> > +            if (aEvent.key == "c" || aEvent.key == "v" || aEvent.key == "x") {
> 
> Are these keyboard shortcuts universal? I'm seeing evidence that we localize
> these keyboard shortcuts - for example:
> 
> http://searchfox.org/mozilla-central/rev/
> 008fdd93290c83325de6629a1ae48dc2afaed868/browser/locales/en-US/chrome/
> browser/browser.dtd#308

You're right, we shouldn't hardcode this, and I think preventing the cut/paste/copy events will do. Using the shortcut keys still triggers those events, so we don't need this.

> 
> @@ +826,5 @@
> > +              break;
> > +            }
> > +            default: {
> > +              // printable characters
> > +              if (aEvent.keyCode == 0) {
> 
> Apparently, keyCode is deprecated - see
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
> 
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code is
> recommended instead.
> 
> Also, it took me a few moments to figure out how this worked. Is this really
> the best way of determining if the character is printable, or is there a way
> that doesn't use deprecated APIs?

Hmm.. I can think of two options:
1. Use KeyboardEvent.key and check for key.length == 1, since .key returns a char value if the key generates a printable character.
2. Use 'input' event, however input event does not tell you what key was pressed and it's *not* cancelable, so we need to keep track of the value before key insertion and do our trick and set it back to .value.

What do you think?
Flags: needinfo?(mconley)
(In reply to Jessica Jong [:jessica] from comment #6)
> > 
> > @@ +826,5 @@
> > > +              break;
> > > +            }
> > > +            default: {
> > > +              // printable characters
> > > +              if (aEvent.keyCode == 0) {
> > 
> > Apparently, keyCode is deprecated - see
> > https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code is
> > recommended instead.
> > 
> > Also, it took me a few moments to figure out how this worked. Is this really
> > the best way of determining if the character is printable, or is there a way
> > that doesn't use deprecated APIs?
> 
> Hmm.. I can think of two options:
> 1. Use KeyboardEvent.key and check for key.length == 1, since .key returns a
> char value if the key generates a printable character.
> 2. Use 'input' event, however input event does not tell you what key was
> pressed and it's *not* cancelable, so we need to keep track of the value
> before key insertion and do our trick and set it back to .value.
> 
> What do you think?

According to the documentation of KeyboardEvent.key (https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key):

"If more than one key is pressed and the combination includes a modifier that makes the resulting keystroke non printing, the returned value is the printable character. For example, if the combination were Control + a, the letter "a" is returned."

So the first proposal wouldn't catch that.

I did a quick hunt around Bugzilla looking for traces of a plan to get rid of keyCode, and I don't see one. I see that we introduced KeyboardEvent.code as an alternative, and there is still usage of keyCode in the tree. I'd be more worried if there were active plans on removing keyCode support, but because there aren't (and because we do want date/time to work properly), using keyCode is probably okay. I'm surprised there isn't a better API for determining if the key event is for a printable character.

So let's keep this part as is. Sorry about the noise!
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) (PTO - Nov 11 - Nov 14) from comment #7)
> 
> According to the documentation of KeyboardEvent.key
> (https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key):
> 
> "If more than one key is pressed and the combination includes a modifier
> that makes the resulting keystroke non printing, the returned value is the
> printable character. For example, if the combination were Control + a, the
> letter "a" is returned."
> 
> So the first proposal wouldn't catch that.

We filter out keys with modifiers at the beginning of the function, so I think we are good with using length == 1 to check if it's a printable character.

> 
> I did a quick hunt around Bugzilla looking for traces of a plan to get rid
> of keyCode, and I don't see one. I see that we introduced KeyboardEvent.code
> as an alternative, and there is still usage of keyCode in the tree. I'd be
> more worried if there were active plans on removing keyCode support, but
> because there aren't (and because we do want date/time to work properly),
> using keyCode is probably okay. I'm surprised there isn't a better API for
> determining if the key event is for a printable character.
> 
> So let's keep this part as is. Sorry about the noise!

Thanks Mike, I also notice that there are no traces of a plan to get rid of keyCode. I'll keep this part as is, please let me know if you'd like to change it based on the comment above.
Attached patch patch, v2. (obsolete) — Splinter Review
Hi Mike, besides from the changes mentioned in comment 3, I'm also clearing 'typeBuffer' where needed.
And since we are not allowing user to do text selections, we should process arrows key with modifiers, which are shortcuts for moving the cursor.

May I have your review again? Thanks.
Attachment #8807085 - Attachment is obsolete: true
Attachment #8808535 - Attachment is obsolete: true
Attachment #8810310 - Flags: review?(mconley)
Attached patch patch, v3. (obsolete) — Splinter Review
Sorry, here is a refined patch.
Attachment #8810310 - Attachment is obsolete: true
Attachment #8810310 - Flags: review?(mconley)
Attachment #8810314 - Flags: review?(mconley)
Comment on attachment 8810314 [details] [diff] [review]
patch, v3.

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

Hey Jessica - I have a minor suggestion. Feel free to push back if you think it's not worth it.

::: toolkit/content/widgets/datetimebox.xml
@@ +586,5 @@
> +
> +        this.addEventListener("keypress", onKeyPress, {
> +          capture: true,
> +          mozSystemGroup: true });
> +        this.addEventListener("click", onClick, { mozSystemGroup: true });

I'm actually surprised this works. I would have expected onClick to have resolved to window.onClick instead of this.onClick. Can you double-check that your event handlers are actually being fired?

It might be simpler to do something like:

const EVENTS = ["click", "focus", "blur", "paste", "copy", "cut"];
// ^-- maybe make this a read-only property on the binding

then in the constructor:

this.EVENTS.forEach((eventName) => {
  this.addEventListener(eventName, this, { mozSystemGroup: true });
});

and in the destructor:

this.EVENTS.forEach((eventName) => {
  this.removeEventListener(eventName, this, { mozSystemGroup: true });
});

and then instead of onClick, onFocus, etc, have a single handleEvent function, which should be called for each of these events. You can then use a switch statement there based on the event type. This would allow you to do a dropthrough for cut/copy/paste (since they effectively act the same). onClick, onKeyPress and onFocus are different enough that having them be split out into their own methods like you have them is good - but perhaps call those from the switch statement.

Does that sound do-able?
Attachment #8810314 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #11)
> Comment on attachment 8810314 [details] [diff] [review]
> patch, v3.
> 
> Review of attachment 8810314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jessica - I have a minor suggestion. Feel free to push back if you think
> it's not worth it.
> 
> ::: toolkit/content/widgets/datetimebox.xml
> @@ +586,5 @@
> > +
> > +        this.addEventListener("keypress", onKeyPress, {
> > +          capture: true,
> > +          mozSystemGroup: true });
> > +        this.addEventListener("click", onClick, { mozSystemGroup: true });
> 
> I'm actually surprised this works. I would have expected onClick to have
> resolved to window.onClick instead of this.onClick. Can you double-check
> that your event handlers are actually being fired?

Yes, it do does work. The 'currentTarget' is the xul element when getting the events. Could you explain more about your concern? Thanks.

> 
> It might be simpler to do something like:
> 
> const EVENTS = ["click", "focus", "blur", "paste", "copy", "cut"];
> // ^-- maybe make this a read-only property on the binding
> 
> then in the constructor:
> 
> this.EVENTS.forEach((eventName) => {
>   this.addEventListener(eventName, this, { mozSystemGroup: true });
> });
> 
> and in the destructor:
> 
> this.EVENTS.forEach((eventName) => {
>   this.removeEventListener(eventName, this, { mozSystemGroup: true });
> });
> 
> and then instead of onClick, onFocus, etc, have a single handleEvent
> function, which should be called for each of these events. You can then use
> a switch statement there based on the event type. This would allow you to do
> a dropthrough for cut/copy/paste (since they effectively act the same).
> onClick, onKeyPress and onFocus are different enough that having them be
> split out into their own methods like you have them is good - but perhaps
> call those from the switch statement.
> 
> Does that sound do-able?

Sure, sounds really good to me, will upload a new patch with this change.
Attached patch patch, v4.Splinter Review
addressed comment 11.
Attachment #8810314 - Attachment is obsolete: true
Attachment #8811612 - Flags: review?(mconley)
Comment on attachment 8811612 [details] [diff] [review]
patch, v4.

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

LGTM! Thanks!
Attachment #8811612 - Flags: review?(mconley) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f438bfb22339
Allow keyboard shortcut when focus is on date/time input box. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f438bfb22339
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: