259883 - "accel" is Cmd, not Ctrl on Mac

Status: VERIFIED FIXED

(Firefox Graveyard :: Help Documentation, defect)

Description

[Hiro]

The Accl key is described by the Control key at Help of the Mac version Firefox.
This should be described by the Command key.

Mac OS X 10.3.5
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7.3) Gecko/20040916
Firefox/0.10

Comment 1

[Jeff Walden (gone starting June 8)]

Known, but we're not sure what'll happen with this.  If there's time, making an
entity for it that's built by platform might work, or something like that. 
Hopefully it'll get done by 1.0mac, but not sure now how to proceed.

Comment 2

[R.J. Keller]

Wouldn't the platform classes work? or are they broke. I remember doing <span
class="mac"> in a lot of places.

Comment 3

[Jeff Walden (gone starting June 8)]

The platform classes do work, but it'd be a lot of code bloat to do <kbd
class="mac">Cmd</kbd><kbd class="noMac">Ctrl</kbd> in every single place that
requires it.

Depending on how much time I have soon, this is something I really should do. 
The way to do it is as I mentioned in the Edit>Prefs bug for Help on Linux: use
a Help-specific entity to hold oft-repeated text and code (like the code
mentioned above).  It shouldn't be super-difficult, but it will be at least
somewhat time-consuming.

For now, I'll target this for 1.0 -- I expect to get it, but it might have to be
bumped to 1.0mac if my schedule gets even worse.

Comment 4

[Stefan [:stefanh]]

You might want to use "Option" instead of "Alt" and "Return" instead of "Enter"
as well...

Comment 5

[Jeff Walden (gone starting June 8)]

Attachment: Change key modifiers & Enter to entities

This patch converts all Ctrl, Shift, Alt, and Enter keys into entities.  It
also makes a few miscellaneous changes to get rid of some Mac-only or Mac- and
Linux-only information (like on how to right-click).

For the entitizing, I used a sed script with these lines in it to make changes:


s#<kbd>Ctrl<\/kbd>#\&accelKey;#g
s#<kbd>Alt<\/kbd>#\&altKey;#g
s#<kbd>Shift<\/kbd>#\&shiftKey;#g
s#<kbd>Enter<\/kbd>#\&enterKey;#g

I then went back and manually changed the instances of 'Ctrl', 'Alt', 'Enter',
and 'Shift' in source where they still existed.  Such cases happened when
shortcuts diverged between platforms even more than Cmd-Ctrl or Alt-Opt, though
such cases were rare (one of the few changes, for example, was Ctrl-Cmd-Home,
where Home maps to Alt+Home on non-Mac and Cmd+Home on Mac).

The next step here (tho it's not really a part of this bug but rather part of
general Help QA that follows from this bug) is to do a line-by-line comparison
of browser-sets.inc and the keyboard shortcuts to make sure everything's
correct.  This patch should mean that you can use &accelKey; whenever "accel"
is mentioned in modifiers, and similar for all the others.  There are some
instances, however, where deviations occur, as like the Home situation
mentioned above.  This can likely be discovered by simply checking all the
#ifdefs in the file, but we need to do it to make sure Help isn't inaccurate.

Comment 6

[Jeff Walden (gone starting June 8)]

One other:

There's also a 'control' modifier which isn't in this patch (entity or entity
switches).  It's used only a few places, and we should fix that in the general
shortcut QA work I mentioned in the last comment.

Comment 7

[Jeff Walden (gone starting June 8)]

Comment on attachment 161229 [details] [diff] [review]
Change key modifiers & Enter to entities

Let's put a halt on this for now...Firefox Help just disappeared from my Linux
branch build, and I need to see if the problem is related to this patch.

Comment 8

[Steffen Wilberg]

Comment on attachment 161229 [details] [diff] [review]
Change key modifiers & Enter to entities

The patch works fine for me, except for a XML parsing error in
>Index: mozilla/browser/components/help/locale/en-US/using_firebird.xhtml
>+  <li><span class="noMac">Right-click</a><span class="mac">Press &accelKey;,
Instead of </a>, this should be        </span>.

Also bump the dates. I assume IE and Opera use the Cmd key on Mac as well?

This is not part of this patch, but I just noticed that
<!ENTITY controlKey	    '<kbd class="noMac">Ctrl</kbd><kbd
			     class="mac">Cmd</kbd>' >
is wrong. Control is Ctrl on all platforms, according to
http://www.mozilla.org/access/keyboard/#codersummary

Comment 9

[Steffen Wilberg]

(In reply to comment #5)
> (one of the few changes, for example, was Ctrl-Cmd-Home,
> where Home maps to Alt+Home on non-Mac and Cmd+Home on Mac).
Huh? I can't find a Mac-specific Home shortcut in browser-sets.inc. Maybe you
mean the Back and Forward, which is Alt+Left/Right Arrow everywhere except Mac,
where it's Accel+Left/Right Arrow:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser-sets.inc&rev=1.16.2.1.2.21&mark=264-271#264

Another difference is History (Ctrl+Shift+H on Mac):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser-sets.inc&rev=1.16.2.1.2.21&mark=296-300#296

In bug 263146 comment 0, the first two bonsai links are of relevance here. But
let's not hold up this patch, we can fix that later.

Comment 10

[Jeff Walden (gone starting June 8)]

(In reply to comment #9)
> Huh? I can't find a Mac-specific Home shortcut in browser-sets.inc. Maybe you
> mean the Back and Forward, which is Alt+Left/Right Arrow everywhere except
> Mac, where it's Accel+Left/Right Arrow:

Perhaps I did.  That's what happens when I don't get enough sleep, I guess.

One other note to fix in updated patch: bug 256635.

Comment 11

[Jeff Walden (gone starting June 8)]

Attachment: Updated (dates, fix from last comment, etc.)

This fixes the stuff mentioned previously (except as listed below).

As for the shortcuts IE and Opera use, I'm not really sure.  I'd imagine most
are similar to the Ctrl-Cmd split Mozilla uses, but I'm not sure.  I hope bug
263228 will help resolve this.	For now I've reverted the changes from the
previous patch so that no entities are used, because we haven't verified that
the Ctrl-Cmd split exists in either IE *or* Opera.  This way, Help doesn't
accidentally get worse (as could very possibly happen, for all anyone here
knows).

The incorrect Mac shortcuts have been forked off into bug 263257, because I
don't have time to fix those right now.

Comment 12

[Jeff Walden (gone starting June 8)]

Comment on attachment 161335 [details] [diff] [review]
Updated (dates, fix from last comment, etc.)

Incidentally, I never figured out exactly why my build didn't display Help that
one time, because it doesn't look like it was the patch that did it.

Comment 13

[Steffen Wilberg]

Comment on attachment 161335 [details] [diff] [review]
Updated (dates, fix from last comment, etc.)

Opera and IE both use Cmd on Mac:
http://www.opera.com/features/keyboard/index.dml?platform=mac
  (doesn't list all the shortcuts, but shows that Opera adheres to Cmd)
http://www.premierpub.com/help/shortcuts.htm

So let's use the entities for Opera and IE as well, like in your first patch.
We shouldn't do such a massive change here twice.

Please also address the three keys I mentioned in comment 9 (back/forward,
history).

r=me with that. I wouldn't mind reviewing a final patch though.

Comment 14

[Jeff Walden (gone starting June 8)]

Entities used, split keys addressed (they do seem to be the only ones), patch
checked into branch and trunk.

Comment 15

[Steffen Wilberg]

Great work, Jeff!

I just noticed that this is wrong:
Index: mozilla/browser/components/help/locale/en-US/using_firebird.xhtml
+  <li>To go to your home page quickly, press <span
class="noMac">&altKey;</span><span
+    class="mac">&accelKey;</span>+<kbd>Home</kbd>.</li>

It's Alt on all platforms, see comment 9.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser-sets.inc&rev=1.16.2.1.2.21&mark=271#271

Comment 16

[Jeff Walden (gone starting June 8)]

(In reply to comment #15)

Indeed.  Fixed branch and trunk.

Comment 17

[sairuh (rarely reading bugmail)]

note that Alt on Mac should map to the Option key. does your patch do this?

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #17)
> note that Alt on Mac should map to the Option key. does your patch do this?

Yes.

Comment 19

[Steffen Wilberg]

I can't believe we changed Ctrl-click to &accelKey;+click here. Damnit!
See bug 267913.