ZoomIn/ZoomOut/ZoomReset should have alternative accel keys for localized builds

RESOLVED FIXED in Firefox 3

Status

()

defect
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(4 keywords)

Trunk
Firefox 3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

7.42 KB, patch
mconnor
: review+
beltzner
: ui-review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
1.47 KB, patch
beltzner
: review+
beltzner
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
4.10 KB, patch
mconnor
: review+
mconnor
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
7.69 KB, patch
Details | Diff | Splinter Review
1.71 KB, patch
masayuki
: review+
damons
: approval1.9+
Details | Diff | Splinter Review
Posted patch Patch v1.0 (obsolete) — Splinter Review
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.
Flags: blocking-firefox3?
Attachment #313064 - Flags: review?(mconnor)
Posted patch Patch v1.0.1Splinter Review
Oops, sorry for the spam.
Attachment #313064 - Attachment is obsolete: true
Attachment #313065 - Flags: review?(mconnor)
Attachment #313064 - Flags: review?(mconnor)
Attachment #313065 - Flags: ui-review?(beltzner)
Attachment #313065 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 313065 [details] [diff] [review]
Patch v1.0.1

I'm going to land the strings tonight.
Attachment #313065 - Flags: approval1.9?
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
Keywords: checkin-needed
Target Milestone: --- → Firefox 3

Comment 4

11 years ago
Nakano-san's patch missed source viewer part.
Now, this patch has only string changes because string-freeze is close.
Attachment #313554 - Flags: ui-review?(beltzner)
Attachment #313554 - Flags: review?(mconnor)
Comment on attachment 313554 [details] [diff] [review]
Patch for source viewer part, strings only

I'll land this now.
Attachment #313554 - Flags: approval1.9?
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 on attachment 313065 [details] [diff] [review]
Patch v1.0.1

r+a=me, sorry for the delay
Attachment #313065 - Flags: review?(mconnor)
Attachment #313065 - Flags: review+
Attachment #313065 - Flags: approval1.9?
Attachment #313065 - Flags: approval1.9+
Thank you everyone! I'll post the patch for xul part of viewsrc.
this is needed by attachment 313554 [details] [diff] [review] (that was already checked-in)
Attachment #313650 - Flags: ui-review?(beltzner)
Attachment #313650 - Flags: review?(mconnor)
Attachment #313650 - Flags: approval1.9?
Comment on attachment 313650 [details] [diff] [review]
Patch for viewsrc (xul part)

Sorry I missed this.
Attachment #313650 - Flags: ui-review?(beltzner)
Attachment #313650 - Flags: ui-review+
Attachment #313650 - Flags: review?(mconnor)
Attachment #313650 - Flags: review+
mconnor, can you please do the approval on this one?  I feel like a broken record asking every approval for tests.  :)
Whiteboard: XUL parts are not checked-in (wating a1.9)
Comment on attachment 313554 [details] [diff] [review]
Patch for source viewer part, strings only

(post hoc reviews and approvals)
Attachment #313554 - Flags: ui-review?(beltzner)
Attachment #313554 - Flags: ui-review+
Attachment #313554 - Flags: review?(mconnor)
Attachment #313554 - Flags: review+
Attachment #313554 - Flags: approval1.9?
Attachment #313554 - Flags: approval1.9+
Attachment #313650 - Flags: approval1.9? → approval1.9+
checked-in the xul part.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: blocking-firefox3?
Resolution: --- → FIXED
Whiteboard: XUL parts are not checked-in (wating a1.9)

Comment 14

11 years ago
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

11 years ago
(In reply to comment #14)
> try with IME mode.

ignore this, sorry.
backed-out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Patch for XBL key handler (obsolete) — Splinter Review
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).
Attachment #314269 - Flags: superreview?(neil)
Attachment #314269 - Flags: review?(neil)
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.
Attachment #314269 - Flags: review?(neil) → review?(enndeakin)

Comment 20

11 years ago
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.
Attachment #314269 - Flags: superreview?(neil)
Attachment #314269 - Flags: review?(enndeakin)
Attachment #314269 - Flags: review-
(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.
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.
Attachment #314269 - Attachment is obsolete: true
Attachment #314907 - Flags: review?(enndeakin)
enndeakin:

Would you review this?

Comment 24

11 years ago
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.
Attachment #314907 - Flags: review?(enndeakin) → review+
Attachment #314907 - Attachment is obsolete: true
Attachment #316021 - Flags: superreview?(neil)
Attachment #316021 - Flags: review+
(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
Status: REOPENED → ASSIGNED
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.
Attachment #316021 - Flags: superreview?(neil) → superreview+
Comment on attachment 316021 [details] [diff] [review]
Patch for XBL key handler v2.1

a1.9+=damons
Attachment #316021 - Flags: approval1.9? → approval1.9+
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
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].
You need to log in before you can comment on or make changes to this bug.