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

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Blocks: 1 bug)

52 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [datetime])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 months ago
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
Blocks: 888320
Whiteboard: [datetime]
(Assignee)

Comment 1

10 months ago
Created attachment 8807085 [details] [diff] [review]
WIP
(Assignee)

Comment 2

10 months ago
After reading bug 1274520, it seems that we should move our event listeners/handlers to system group, may do it in this bug.
(Assignee)

Comment 3

10 months ago
Created attachment 8808535 [details] [diff] [review]
patch, v1.

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
(Assignee)

Comment 4

10 months ago
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-
(Assignee)

Comment 6

9 months ago
(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)
(Assignee)

Comment 8

9 months ago
(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.
(Assignee)

Comment 9

9 months ago
Created attachment 8810310 [details] [diff] [review]
patch, v2.

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)
(Assignee)

Comment 10

9 months ago
Created attachment 8810314 [details] [diff] [review]
patch, v3.

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-
(Assignee)

Comment 12

9 months ago
(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.
(Assignee)

Comment 13

9 months ago
Created attachment 8811612 [details] [diff] [review]
patch, v4.

addressed comment 11.
Attachment #8810314 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
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+
(Assignee)

Comment 15

9 months ago
Thank you Mike!

try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a336a91a3a0f6a7258edefe38ed5dec322c2ad5&group_state=expanded
Keywords: checkin-needed

Comment 16

9 months ago
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

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f438bfb22339
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 18

9 months ago
https://hg.mozilla.org/mozilla-central/rev/f438bfb22339
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.