1114357 - Firefox does not understand XF86 ZoomIn and ZoomOut keys

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jehan]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141127111021

Steps to reproduce:

XF86 provides a list of keys. Some are well understood by Firefox (like the Forward and Backward keys). but some are not.
In particular if you have devices outputting XF86_ZoomIn/XF86_ZoomOut, I would expect these to zoom in Firefox.


Actual results:

XF86_ZoomIn and XF86_ZoomOut do not respectively zoom in and out in Firefox.


Expected results:

XF86_ZoomIn should be an alternative shortcut for ZoomIn (currently "+"), and XF86_ZoomOut an alternative shortcut for ZoomOut (currently "-").

Comment 1

[Jehan]

Attachment: Take into account the Zoom keysyms.

Well I thought that looked not too difficult, and I might as well propose a patch. Here it is, attached.
That's my first proposed patch, so I'm not sure of all the procedure yet. I hope I got it right. :-)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8540189 [details] [diff] [review]
Take into account the Zoom keysyms.

Hi Jehan, to request review you need to set the flag to "?" and enter the email address of a reviewer (Bugzilla will suggest some for you). I'll flag smaug for you now.

Comment 3

[Jehan]

Thanks Gavin.

Comment 4

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

Hmm, I don't see zoomin/out in DOM key code value list. I wonder why.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8540189 [details] [diff] [review]
Take into account the Zoom keysyms.

(In reply to Olli Pettay [:smaug] from comment #4)
> Hmm, I don't see zoomin/out in DOM key code value list. I wonder why.

https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-key.html#key-ZoomIn

I don't think that we should use content command event in this case because it causes additional runtime cost in e10s mode. Why don't you create a special default action handler for such keys in EventStateManager which is called by EventStateManager::PostHandleEvent()?

# I think that the others should also be so. But it's not scope of this bug.

Comment 6

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > Hmm, I don't see zoomin/out in DOM key code value list. I wonder why.
> 
> https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3Events-key.html#key-
> ZoomIn
Oh, https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html links to the wrong version of
"DOM Level 3 KeyboardEvent key Values" spec.

> 
> I don't think that we should use content command event in this case because
> it causes additional runtime cost in e10s mode. Why don't you create a
> special default action handler for such keys in EventStateManager which is
> called by EventStateManager::PostHandleEvent()?
Yeah, I agree.

Comment 7

[Jehan]

Attachment: New patch for zoom in/out.

> I don't think that we should use content command event in this case because it causes additional runtime cost in e10s mode.
> [...] # I think that the others should also be so. But it's not scope of this bug.

Ok. Was just trying to follow the current code logic, but since you consider even current state is not right, here is the new approach. :-)

> Why don't you create a special default action handler for such keys in EventStateManager which is called by EventStateManager::PostHandleEvent()?

Attached what I think is what you were telling about, if I correctly understood. I had to create DOM_VK_ZOOM_IN/OUT constants because there were none yet (note: there was a DOM_VK_ZOOM though I'm unsure if this is actually of any use right now) and the keycode was 0. Also I had no idea if the DOM_VK_* constants integer values had any relationship to a spec, but it looks like no, so I just gave it the next values after all the others.

Anyway this works well too, and I hope this one is the right.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8546837 [details] [diff] [review]
New patch for zoom in/out.

We shouldn't define new keyCode constants except when it fixes a compatibility issue with other browsers.

You should refer WidgetKeyboardEvent::mKeyNameIndex. The value should be KEY_NAME_INDEX_Zoom(In|Out).
http://mxr.mozilla.org/mozilla-central/source/widget/NativeKeyToDOMKeyName.h#413
http://mxr.mozilla.org/mozilla-central/source/widget/NativeKeyToDOMKeyName.h#418

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

And you should rewrite the switch statement with mKeyNameIndex and KEY_NAME_INDEX_(F6|Tab).

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

I wonder we should handle ZoomToggle too. However, I'm not sure what's the best (expected) behavior of the key. So, we should wait a bug report for that since the reporter must know what it should behave.

Comment 11

[Jehan]

Attachment: New patch for zoom in/out.

Ok new version. I followed the new directions, so this time, I think we got the one, right? :-)
Thanks!

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8548207 [details] [diff] [review]
New patch for zoom in/out.

>   case NS_KEY_PRESS:
>     if (nsEventStatus_eConsumeNoDefault != *aStatus) {
>       WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
>       //This is to prevent keyboard scrolling while alt modifier in use.
>       if (!keyEvent->IsAlt()) {
>-        switch(keyEvent->keyCode) {
>-          case NS_VK_TAB:
>-          case NS_VK_F6:
>+        switch(keyEvent->mKeyNameIndex) {
>+          case KEY_NAME_INDEX_ZoomIn:
>+          case KEY_NAME_INDEX_ZoomOut:
>+            ChangeFullZoom(keyEvent->mKeyNameIndex == KEY_NAME_INDEX_ZoomIn ? 1 : -1);

This line is over 80 characters. Please write this as:

ChangeFullZoom(
  keyEvent->mKeyNameIndex == KEY_NAME_INDEX_ZoomIn ? 1 : -1);

And after this, the key shouldn't be handled by OS. Therefore, we need to mark it as consumed. Insert this:
*aStatus = nsEventStatus_eConsumeNoDefault;

Otherwise, looks okay to me. Please attach new patch again because you're new hacker, I should confirm the final patch. Then, I'll give r+.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Or, like this code is okay:

case KEY_NAME_INDEX_ZoomIn:
  ChangeFullZoom(1);
  *aStatus = nsEventStatus_eConsumeNoDefault;
  break;
case KEY_NAME_INDEX_ZoomOut:
  ChangeFullZoom(-1);
  *aStatus = nsEventStatus_eConsumeNoDefault;
  break;

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

And I wonder. Do you need to ignore Alt+ZoomIn and Alt+ZoomOut? The switch statement is in |if (!keyEvent->IsAlt()) {|. If you don't want that, please remove the if statement, outdent the switch statement, and then, insert:

// This is to prevent keyboard scrolling while alt modifier in use.
if (keyEvent->IsAlt()) {
  break;
}

immediately after |case KEY_NAME_INDEX_F6:|.

Comment 15

[Jehan]

Attachment: bug1114357.patch

Line characters no more than 80 chars; consumed status added.

I also changed for accepting alt+Zoom(In|Out) as you proposed. Well not sure if this is very useful, but there is no other meaning for Alt+Zoom(In|Out) as of now anyway. So why not?

As of comment 13 though, I kept the original style since I don't like code repetition (both Zoom case basically have the same code, except for the 1/-1 value, so I prefer to factor the code). I hope that's ok with you.
Thanks.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8548225 [details] [diff] [review]
bug1114357.patch

Thank you for your work! r=masayuki with following changes:

>+                  uint32_t dir = keyEvent->IsShift() ?
>+                    (isDocMove ? static_cast<uint32_t>(nsIFocusManager::MOVEFOCUS_BACKWARDDOC) :
>+                     static_cast<uint32_t>(nsIFocusManager::MOVEFOCUS_BACKWARD)) :

this line should start from under "s" of static_cast in the previous line because it's in |?:|. So, don't align to its parent parenthesis.

>+                    (isDocMove ? static_cast<uint32_t>(nsIFocusManager::MOVEFOCUS_FORWARDDOC) :
>+                     static_cast<uint32_t>(nsIFocusManager::MOVEFOCUS_FORWARD));

Same here. I.e., use the original indent style before you outdent. Even if the line is over 80 characters, you can ignore that because it's not your fault.



Do you have a permission to land a patch into the tree? If you don't have it, I'll add "checkin-needed" keyword to this bug after I'll post the final patch to tryserver (test server). For that, please attach new patch here.  Thanks!

Comment 17

[Jehan]

Attachment: Reviewed r+ patch with indent fix only.

> Same here. I.e., use the original indent style before you outdent

Oups, sorry for the 2 indentation issues. To be honest, I reindented the whole paragraph with automatic indentation from my text editor (with indent rules of other projects I contribute to) and did not notice some of the indentations got messed up. My bad, I should have double checked.

> Do you have a permission to land a patch into the tree? 

No this is my first proposed patch to a Mozilla project. I guess I'll let you add the "checkin-needed" keyword then.

Attached the accepted updated patch with indent fixed. I'm not sure if I had to obsolete the previous patch though, since it is the one which got the review+ flag. Not sure what's the procedure here.
Thanks!

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8548266 [details] [diff] [review]
Reviewed r+ patch with indent fix only.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f4988cb278a

This causes bugstage because of warning as error.

You need to add default: case into the switch statement. Sorry for the miscatch.

Comment 19

[Jehan]

Attachment: Patch with added default case (empty).

No prob. Attached the new patch adding just an empty default case.

Actually I noticed that there was a warning without a default case, since it makes the switch non-exhaustive; but since that situation was existing prior my patch, I thought this was OK. I'll know for the next patch. :-)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

(In reply to Jehan from comment #19)
> but since that situation was existing prior
> my patch, I thought this was OK.

Really? keyCode values are just constants of uint32_t but key name index values are defined as typed enum. Therefore, compiler can detect if each switch tests all possible values only with the latter case. That's the cause of the bustage.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8548822 [details] [diff] [review]
Patch with added default case (empty).

I'll post this patch to tryserver soon, thanks!

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8548822 [details] [diff] [review]
Patch with added default case (empty).

Ugh, I'm very sorry.

This approach isn't available for current our automated tests (although, there is no problem for actual users). Our test API to synthesize KeyboardEvent cannot set WidgetKeyboardEvent::mKeyNameIndex. Therefore, synthesized Tab or F6 key event isn't handled by this change.

Could you recover the original switch statement and add |case 0:| to it. Then, could you add new switch statement for other keys (i.e., ZoomIn and ZoomOut) which don't have specific keyCode value? In this case, the indent level must be to deep due to our 80 characters per line rule. So, could you move the code to 

void
EventStateManager::PostHandleKeyboardEvent(WidgetKeyboardEvent* aKeyboardEvent,
                                           nsEventStatus& aStatus);

If you're tired due to my mistake, I'll take this bug. If so, let me know.

Comment 23

[Jehan]

Attachment: bug1114357.patch

> Really? keyCode values are just constants of uint32_t but key name index values are defined as typed enum.

Oh I forgot that we changed the switch. Yes sorry, I guess I mixed things up and the warning was probably not there before, then. :-)

> If you're tired due to my mistake, I'll take this bug. If so, let me know.

No I want to go through with it. And there is no problem. I've developed enough to know that mistakes are part of the process. ;-)

> Could you recover the original switch [..]

Ok all done. I also discovered my previous patch had a little broken indentation (my indent rules were indenting 4 spaces after an open brace while current code indents only 2 spaces). So this is fixed in the new patch too.
I made PostHandleKeyboardEvent() as a private function, and I was not sure if you wanted if to contain the whole code under the NS_KEY_PRESS case (in which case, one parameter at least would need to be added to the private function: dispatchedToContentProcess), or only the piece for keys with no keycode.
I went for the second version. Hope that's good.
Attached the patch.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8549586 [details] [diff] [review]
bug1114357.patch

>+void
>+EventStateManager::PostHandleKeyboardEvent(WidgetKeyboardEvent* aKeyboardEvent,
>+                                           nsEventStatus& aStatus)
>+{
>+  switch(aKeyboardEvent->mKeyNameIndex) {
>+    case KEY_NAME_INDEX_ZoomIn:
>+    case KEY_NAME_INDEX_ZoomOut:
>+      ChangeFullZoom(
>+        aKeyboardEvent->mKeyNameIndex == KEY_NAME_INDEX_ZoomIn ? 1 : -1);
>+      aStatus = nsEventStatus_eConsumeNoDefault;
>+      break;
>+    default:
>+      break;
>+  }
>+}

This looks good to me.

>@@ -3182,21 +3198,21 @@ EventStateManager::PostHandleEvent(nsPre
>   case NS_KEY_BEFORE_UP:
>   case NS_KEY_UP:
>   case NS_KEY_AFTER_UP:
>     break;
> 
>   case NS_KEY_PRESS:
>     if (nsEventStatus_eConsumeNoDefault != *aStatus) {
>       WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
>-      //This is to prevent keyboard scrolling while alt modifier in use.
>-      if (!keyEvent->IsAlt()) {
>-        switch(keyEvent->keyCode) {
>-          case NS_VK_TAB:
>-          case NS_VK_F6:
>+      switch(keyEvent->keyCode) {
>+        case NS_VK_TAB:
>+        case NS_VK_F6:
>+          //This is to prevent keyboard scrolling while alt modifier in use.

Please add " " after "//".

>+          if (!keyEvent->IsAlt()) {

This makes your patch simplest. However, I don't like this switch statement isn't included in PostHandleKeyboardEvent() because if other developers want to hack similarly and they find the method first, they may be confused not all keyboard events don't cause calling the method. Could you move this switch statement into the method?

Then, the method should be:

{
  if (aStatus == nsEventStatus_eConsumeNoDefault) {
    return;
  }

  // XXX Currently, our automated tests don't support mKeyNameIndex.
  //     Therefore, we still need to handle this with keyCode.
  switch (aKeyEvent->keyCode) {
    case NS_VK_TAB:
    case NS_VK_F6:
      // This is to prevent keyboard scrolling while alt modifier in use.
      if (aKeyEvent->IsAlt()) {
        return;
      }
      ...
      aStatus = nsEventStatus_eConsumeNoDefault;
      return;
    case 0:
      // Handle keys with no specific keycode value.
      break;
    default:
      return;
  }

  switch (aKeyEvent->mKeyNameIndex) {
    case KEY_NAME_INDEX_ZoomIn:
    case KEY_NAME_INDEX_ZoomOut:
      ...
      aStatus = nsEventStatus_eConsumeNoDefault;
      return;
    default:
      return;
  }
}
>             // Handling the tab event after it was sent to content is bad,
>             // because to the FocusManager the remote-browser looks like one
>             // element, so we would just move the focus to the next element
>             // in chrome, instead of handling it in content.
>             if (dispatchedToContentProcess)
>               break;
> 
>             EnsureDocument(mPresContext);
>+        case 0:
>+          /* Handle keys with no specific keycode value. */

Please use "//" for one line comment.

>+          PostHandleKeyboardEvent(keyEvent, *aStatus);
>+          break;

Comment 25

[Jehan]

Attachment: bug1114357.patch

Argh, this one, I really hope it's all right!

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8549784 [details] [diff] [review]
bug1114357.patch

>+      // This is to prevent keyboard scrolling while alt modifier in use.
>+      if (!aKeyboardEvent->IsAlt()) {

One of our coding rule is "early return style" since it reduces indentation and prevents easy mistake.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Check_for_errors_early_and_often

But it's okay for this.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

tryserver result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0b4119d298b

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c227c0e4d60

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c227c0e4d60