Last Comment Bug 426501 - ZoomIn/ZoomOut/ZoomReset should have alternative accel keys for localized builds
: ZoomIn/ZoomOut/ZoomReset should have alternative accel keys for localized builds
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, intl, late-l10n
Product: Firefox
Classification: Client Software
Component: Keyboard Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 3
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-02 04:09 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2010-12-17 07:02 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (7.10 KB, patch)
2008-04-02 04:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v1.0.1 (7.42 KB, patch)
2008-04-02 04:11 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
mconnor: review+
mbeltzner: ui‑review+
mconnor: approval1.9+
Details | Diff | Splinter Review
Patch for source viewer part, strings only (1.47 KB, patch)
2008-04-04 00:42 PDT, Atsushi Sakai
mbeltzner: review+
mbeltzner: ui‑review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
Patch for viewsrc (xul part) (4.10 KB, patch)
2008-04-04 10:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
mconnor: review+
mconnor: ui‑review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
Patch (all xul part) (for checking-in) (7.69 KB, patch)
2008-04-07 22:01 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch for XBL key handler (1.31 KB, patch)
2008-04-07 22:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
enndeakin: review-
Details | Diff | Splinter Review
Patch for XBL key handler v2.0 (1.49 KB, patch)
2008-04-10 11:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
enndeakin: review+
Details | Diff | Splinter Review
Patch for XBL key handler v2.1 (1.71 KB, patch)
2008-04-16 09:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
masayuki: review+
neil: superreview+
dsicore: approval1.9+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-02 04:09:16 PDT
Created attachment 313064 [details] [diff] [review]
Patch v1.0

I and Asai-san discussed for the accel keys of zoom. They are now using '+', '-' and '0'. However, we cannot use them without shift key on many keyboard layouts. It's too unusable. Therefore, for en-US keyboards, '=' key is also specified for zoom in alternative accel key.

I think (and hope) that we should add other alternative keys like that for other keyboard layouts. And they can be used by localizers. Because we should not replace the original keys (+/-/0), they are in numpad too. And also in Japan, the en-US keyboard layout is used many environments, so, we cannot replace '=' too.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-02 04:11:56 PDT
Created attachment 313065 [details] [diff] [review]
Patch v1.0.1

Oops, sorry for the spam.
Comment 2 Reed Loden [:reed] (use needinfo?) 2008-04-03 23:29:10 PDT
Comment on attachment 313065 [details] [diff] [review]
Patch v1.0.1

I'm going to land the strings tonight.
Comment 3 Reed Loden [:reed] (use needinfo?) 2008-04-04 00:05:28 PDT
Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.106; previous revision: 1.105
done
Checking in toolkit/locales/en-US/chrome/mozapps/help/help.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/help/help.dtd,v  <--  help.dtd
new revision: 1.10; previous revision: 1.9
done
Comment 4 Atsushi Sakai 2008-04-04 00:42:53 PDT
Created attachment 313554 [details] [diff] [review]
Patch for source viewer part, strings only

Nakano-san's patch missed source viewer part.
Now, this patch has only string changes because string-freeze is close.
Comment 5 Reed Loden [:reed] (use needinfo?) 2008-04-04 02:25:05 PDT
Comment on attachment 313554 [details] [diff] [review]
Patch for source viewer part, strings only

I'll land this now.
Comment 6 Reed Loden [:reed] (use needinfo?) 2008-04-04 02:26:53 PDT
Checking in toolkit/locales/en-US/chrome/global/viewSource.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/viewSource.dtd,v  <--  viewSource.dtd
new revision: 1.7; previous revision: 1.6
done
Comment 7 Mike Connor [:mconnor] 2008-04-04 08:38:26 PDT
Comment on attachment 313065 [details] [diff] [review]
Patch v1.0.1

r+a=me, sorry for the delay
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-04 10:14:56 PDT
Thank you everyone! I'll post the patch for xul part of viewsrc.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-04 10:53:35 PDT
Created attachment 313650 [details] [diff] [review]
Patch for viewsrc (xul part)

this is needed by attachment 313554 [details] [diff] [review] (that was already checked-in)
Comment 10 Mike Connor [:mconnor] 2008-04-04 10:57:40 PDT
Comment on attachment 313650 [details] [diff] [review]
Patch for viewsrc (xul part)

Sorry I missed this.
Comment 11 Damon Sicore (:damons) 2008-04-04 13:05:22 PDT
mconnor, can you please do the approval on this one?  I feel like a broken record asking every approval for tests.  :)
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-07 13:22:30 PDT
Comment on attachment 313554 [details] [diff] [review]
Patch for source viewer part, strings only

(post hoc reviews and approvals)
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-07 17:02:10 PDT
checked-in the xul part.
Comment 14 pal-moz 2008-04-07 19:44:16 PDT
regression!!

20080407_1715_firefox-3.0pre.en-US.win32.zip

"Tab","hankaku/zenkaku","ctrl++/-/0","katakana/hiragana" etc. does not work, wrong behavior.

try with IME mode.
Comment 15 pal-moz 2008-04-07 19:56:45 PDT
(In reply to comment #14)
> try with IME mode.

ignore this, sorry.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-07 20:48:28 PDT
backed-out
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-07 22:01:23 PDT
Created attachment 314261 [details] [diff] [review]
Patch (all xul part) (for checking-in)
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-07 22:42:30 PDT
Created attachment 314269 [details] [diff] [review]
Patch for XBL key handler

We should ignore the *dummy* key element which is added by attachment 314261 [details] [diff] [review]. The key elements are only needed by localized build, but English build also has them (the key attribute is empty).
Comment 19 neil@parkwaycc.co.uk 2008-04-08 07:55:23 PDT
Comment on attachment 314269 [details] [diff] [review]
Patch for XBL key handler

Not 100% convinced on this so punting on one of the reviews for now.
Comment 20 Neil Deakin 2008-04-09 12:48:51 PDT
Comment on attachment 314269 [details] [diff] [review]
Patch for XBL key handler

The keycode and/or charcode attributes can also be used to define a key so checking for the key attribute isn't sufficient.

Using <key/> without specifying a key means listen for all keys. The first patch doesn't look correct either as it appears to create listeners which zoom the page whenever any key is pressed.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-10 02:23:13 PDT
(In reply to comment #20)
> (From update of attachment 314269 [details] [diff] [review])
> The keycode and/or charcode attributes can also be used to define a key so
> checking for the key attribute isn't sufficient.

When key attr or charcode attr is existing, but that has empty value, then, we ignore the key element, is it ok?

> Using <key/> without specifying a key means listen for all keys. The first
> patch doesn't look correct either as it appears to create listeners which zoom
> the page whenever any key is pressed.

Yeah, but I don't have other idea for this issue. The additional key elements are not needed by some locale.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-10 11:38:31 PDT
Created attachment 314907 [details] [diff] [review]
Patch for XBL key handler v2.0

This checkes both key attr and charcode attr. If each attr is existing but both values are empty, the key element is ignored. So, the key element doesn't have both attrs, that can handle the all keys.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-14 23:55:50 PDT
enndeakin:

Would you review this?
Comment 24 Neil Deakin 2008-04-16 02:52:14 PDT
Comment on attachment 314907 [details] [diff] [review]
Patch for XBL key handler v2.0

So is the idea that <key/> behaves the same as currently where all keys are listened to, and <key key=""/> listens to no key?

If so, then that seems like it should be ok, and we should document that behaviour for the <key> element.

Also, please check the 'keycode' attribute as well.
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-16 09:09:55 PDT
Created attachment 316021 [details] [diff] [review]
Patch for XBL key handler v2.1
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-16 09:11:08 PDT
(In reply to comment #24)
> (From update of attachment 314907 [details] [diff] [review])
> So is the idea that <key/> behaves the same as currently where all keys are
> listened to, and <key key=""/> listens to no key?

Yes.

> If so, then that seems like it should be ok, and we should document that
> behaviour for the <key> element.

O.K. Do you mean that the document is MDC?
http://developer.mozilla.org/en/docs/XUL:key
Comment 27 neil@parkwaycc.co.uk 2008-04-20 16:04:18 PDT
Comment on attachment 316021 [details] [diff] [review]
Patch for XBL key handler v2.1

Sorry for the delay; I got confused by the logic, but I see what it's doing now.
Comment 28 Damon Sicore (:damons) 2008-04-21 13:41:33 PDT
Comment on attachment 316021 [details] [diff] [review]
Patch for XBL key handler v2.1

a1.9+=damons
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-23 01:12:32 PDT
checked-in.
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-04-23 01:18:24 PDT
Wow, attachment 314269 [details] [diff] [review] was landed at bug 359638. Because the same file was also changed by me. Sorry for such dangerous mistake. And now, the file is updated by attachment 316021 [details] [diff] [review].
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-05-06 01:56:10 PDT
updated the dev-doc:
http://developer.mozilla.org/en/docs/index.php?title=XUL%3Akey&diff=71117&oldid=63358

Please modify the document by native speakers.

Note You need to log in before you can comment on or make changes to this bug.