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

RESOLVED FIXED in Firefox 3

Status

()

Firefox
Keyboard Navigation
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(4 keywords)

Trunk
Firefox 3
addon-compat, dev-doc-complete, intl, late-l10n
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

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.
Flags: blocking-firefox3?
Attachment #313064 - Flags: review?(mconnor)
Created attachment 313065 [details] [diff] [review]
Patch v1.0.1

Oops, sorry for the spam.
Attachment #313064 - Attachment is obsolete: true
Attachment #313065 - Flags: review?(mconnor)
Attachment #313064 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Keywords: late-l10n
Attachment #313065 - Flags: ui-review?(beltzner)
Attachment #313065 - Flags: ui-review?(beltzner) → ui-review+
Keywords: checkin-needed
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

10 years ago
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.
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.
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)
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.  :)
(Assignee)

Updated

10 years ago
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: 10 years ago
Flags: blocking-firefox3?
Resolution: --- → FIXED
Whiteboard: XUL parts are not checked-in (wating a1.9)

Comment 14

10 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

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

ignore this, sorry.
backed-out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 314261 [details] [diff] [review]
Patch (all xul part) (for checking-in)
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).
Attachment #314269 - Flags: superreview?(neil)
Attachment #314269 - Flags: review?(neil)

Comment 19

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

Would you review this?
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+
Created attachment 316021 [details] [diff] [review]
Patch for XBL key handler v2.1
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 27

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

Updated

10 years ago
Attachment #316021 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Keywords: dev-doc-needed, late-compat
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: 10 years ago10 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].
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.