277296 - The print dialogs have no accesskeys

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Stefan Borggraefe]

Comment 1

[Stefan Borggraefe]

Attachment: Patch

I also changed two labels:

"GrayScale" -> "Grayscale" (why the upper case s? This isn't a variable name.
;-))
"Download fonts to printer?" -> "Download fonts to printer" (The question mark
isn't needed. All (?) other checkboxes in Mozilla don't have one.)

Comment 2

[Aaron Leventhal]

Unfortunately my schedule is totally full. Can you find a different review for
front-end accesskey bugs?

Comment 3

[Stefan Borggraefe]

Comment on attachment 170477 [details] [diff] [review]
Patch

timeless, can you review this patch?

Comment 4

[Stefan Borggraefe]

Comment on attachment 170477 [details] [diff] [review]
Patch

Patch bitrotted because of the checkin for bug 277885.

Comment 5

[Vidar Haarr (not reading bugmail)]

Stefan, would you mind me attaching an updated patch?

Comment 6

[Stefan Borggraefe]

(In reply to comment #5)
> Stefan, would you mind me attaching an updated patch?

No, of course not. Thank you! :-)

Comment 7

[Vidar Haarr (not reading bugmail)]

Attachment: version 1

Updated to trunk and also:
 - line up cmdInput in printjoboptions.xul.
 - remove trailing ':' from Left/Center/Right headers in printPageOptions.dtd
 - Lowercased words in "Shrink To Fit Page Width" and "Print Background (colors & images)" in printPageOptions.dtd

Maybe you want that as seperate bugs/patches, but the changes were so small so I just included them here.

Comment 8

[Vidar Haarr (not reading bugmail)]

Comment on attachment 204130 [details] [diff] [review]
version 1

Err.. "You have entered a name that matches more than one user, so we have left the Requestee field blank." (I entered 'timeless' .. Man, you have like 40 e-mail addresses, how am I supposed to know which one you use? :P)

Comment 9

[timeless]

Comment on attachment 204130 [details] [diff] [review]
version 1

note to self:
deployed
should be:
employed

for the things &foo; where you're adding &foo.accesskey;, &foo; should really become &foo.label; but...

i'm not sure that we want the case changes you made to shrinkToFit.label/printBG.label.

i'm also unsure about your changes to hfLeft.label and friends

i wonder if aslaidoutRadio/frompage should/allpages should be recased (it looked like the style was interCapsByWord e.g. shrinkToFit).

i'm not fond of your choice of "T" for selectedframeRadio.accesskey

downloadFonts.label sounds like a proper change (assuming it's a checkbox)

Comment 10

[[e-mail]]

Comment on attachment 204130 [details] [diff] [review]
version 1

>-<!ENTITY shrinkToFit.label   "Shrink To Fit Page Width">
>+<!ENTITY shrinkToFit.label   "Shrink to fit page width">
>+<!ENTITY shrinkToFit.accesskey "f">
Should be Shrink to fit Page Width

>-<!ENTITY printBG.label       "Print Background (colors &amp; images)">
>+<!ENTITY printBG.label       "Print background (colors &amp; images)">
>+<!ENTITY printBG.accesskey   "n">
Should be at least Print Background, but the colours and images can be in lower case.

For elements whose attributes don't fit into 80 columns but do fit into two lines of 80 columns any future changes are unlikely to change the label and accesskey so I'd like them to be on the last line if possible i.e.
<foo id="foo" otherAttributes="here"
     label="&foo.label;" accesskey="&foo.accesskey;"/>

Comment 11

[[e-mail]]

Comment on attachment 204130 [details] [diff] [review]
version 1

Shrink to fit Page _W_idth perhaps?
Print background - I don't really like n, but d is hardly better.
The selected fra_m_e?

Comment 12

[Vidar Haarr (not reading bugmail)]

Attachment: version 2

1. Where I added &foo.accesskey; also changed &foo; to &foo.label;.
2. Fixed casing for shrinkToFit.label/printBG.label.
3. Recased all things I could find to interCapsByWord.
4. selectedframeRadio.accesskey is 'm'.
5. shrinkToFit.accesskey is 'W'.
6. printBG.accesskey stays 'n' because I think the underline is clearer on 'n' than 'd'.
7. Replaced all |label| and |accesskey| attributes to be on the last two or the last line, as appropriate.
 7.1. Did some unrelated changes to fit everything in 80 chars width.

(that's what I remember, at least)

I did the changes to hfLabel and friends because they refer to controls both above and below the labels, and having a ':' tells me that they refer to things that come next in the flow, so to speak.

I noticed one or more instances of |onfocus="this.select()"| -- I think this is really up to the OS if we should do or not. For GTK, for example, it's a system pref. I didn't remove it, but maybe I should've (or in a seperate bug.)

Comment 13

[[e-mail]]

Comment on attachment 205899 [details] [diff] [review]
version 2

I don't like wrapped <script> lines. sr=me with this fixed. If you get the chance though, please change onkeyup to oninput and ensure all event handlers are statements i.e. end in a ; or }

Comment 14

[timeless]

Comment on attachment 205899 [details] [diff] [review]
version 2

+                <label style="visibility: hidden;" value="&marginLeft.label;"/>

+                  <box style="border: 1px;
+                              border-style: dashed;
+                              border-color: gray;"
+                       flex="1"/>

i'm not particularly fond of inline style, i wonder if these should have ids to make it easier for other things to poke.

Comment 15

[Vidar Haarr (not reading bugmail)]

Attachment: version 3

Patch for checkin.
Please take a quick look at this and I'll need some help to check it in as well.

Thanks for the reviews.

Comment 16

[Vidar Haarr (not reading bugmail)]

Do we need the same changes (attachment 206992 [details] [diff] [review]) for toolkit?

Filed
 bug 321707 - Don't force this.select() in onfocus
 bug 321708 - Don't inline CSS styles in print dialogs

Comment 17

[[e-mail]]

(In reply to comment #16)
>Do we need the same changes for toolkit?
mconnor's the guy to ask, he'll let you know if he wants a separate bug or patch or if the files are sufficiently similar he might even port it for you.

Comment 18

[Vidar Haarr (not reading bugmail)]

Attachment: toolkit patch, version 1

First stab at toolkit port, not sure I got it all.
Can't test myself since my tree is on fire currently.

Comment 19

[Vidar Haarr (not reading bugmail)]

Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

Seems like I got all the parts.

Requesting review, although I'm not sure who I should ask.

Comment 20

[timeless]

Comment on attachment 206992 [details] [diff] [review]
version 3

mozilla/xpfe/global/resources/content/printPageSetup.xul 	1.15
mozilla/xpfe/global/resources/content/printdialog.js 	1.29
mozilla/xpfe/global/resources/content/printdialog.xul 	1.11
mozilla/xpfe/global/resources/content/unix/printjoboptions.xul 	1.12
mozilla/xpfe/global/resources/locale/en-US/unix/printjoboptions.dtd 	1.8
mozilla/xpfe/global/resources/locale/en-US/printPageSetup.dtd 	1.6
mozilla/xpfe/global/resources/locale/en-US/printdialog.dtd 	1.5

Comment 21

[Vidar Haarr (not reading bugmail)]

Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

Since Mano hasn't touched it, trying mconnor instead.

Comment 22

[Vidar Haarr (not reading bugmail)]

Attachment 207265 [details] [diff] still needs review. I'm not sure who I can ask now, maybe Deakin?

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

looks good, this isn't something that's usefully testable, afaict

Comment 24

[Nickolay_Ponomarev]

Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

doesn't apply cleanly, any chance someone could post an updated version?

Comment 25

[Kenneth Herron]

Attachment: Updated toolkit patch

I've tested this. All three print dialogs--page setup, print, and printer properties--seem to work including the access keys. But I'd like someone to look at this; I don't have much experience in this area.

Comment 26

[Kenneth Herron]

Attachment: Diff of old and new toolkit patches (not for checkin)

This is a diff of the old and updated toolkit patches, to illustrate the changes.

Comment 27

[Kenneth Herron]

Comment on attachment 268689 [details] [diff] [review]
Updated toolkit patch

mconnor, you reviewed the previous version of this patch. Could you take a look at this?

Comment 28

[Vlado Valastiak [:wladow]]

Kenneth, do you plan to make patch against hg? If not, I can take this.

Comment 29

[Kenneth Herron]

Vlado, I'm not working on mozilla these days. Please feel free to take this if you want.

Now that the unix port prints through gnome, I'm not sure that all of these dialogs are still used. You might want to review that.

Comment 30

[Vlado Valastiak [:wladow]]

Attachment: against hg, v1

- addressed nits mentioned above
- does not contain all the entity ids polishing and wrap long lines changes like previous patches, that's not what this bug is about

Comment 31

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

Comment on attachment 350020 [details] [diff] [review]
against hg, v1

>diff --git a/toolkit/components/printing/content/printjoboptions.xul b/toolkit/components/printing/content/printjoboptions.xul

>-          <label control="downloadFonts" value="&fontsGroup.label;"/>
>+          <label value="&fontsGroup.label;"/>

Why are you removing the control attribute?

>diff --git a/toolkit/locales/en-US/chrome/global/printPageSetup.dtd b/toolkit/locales/en-US/chrome/global/printPageSetup.dtd

>+<!ENTITY landscape.accesskey "L">
>+<!ENTITY marginLeft.accesskey "L">

>+<!ENTITY printBG.accesskey   "B">
>+<!ENTITY marginBottom.accesskey "B">

>diff --git a/toolkit/locales/en-US/chrome/global/printjoboptions.dtd b/toolkit/locales/en-US/chrome/global/printjoboptions.dtd

>+<!ENTITY resolutionInput.accesskey "R">
>+<!ENTITY rightInput.accesskey "R">

Do these not conflict?

r=me with these comments addressed.

Comment 32

[Vlado Valastiak [:wladow]]

(In reply to comment #31)
> (From update of attachment 350020 [details] [diff] [review])
> >diff --git a/toolkit/components/printing/content/printjoboptions.xul b/toolkit/components/printing/content/printjoboptions.xul
> 
> >-          <label control="downloadFonts" value="&fontsGroup.label;"/>
> >+          <label value="&fontsGroup.label;"/>
> 
> Why are you removing the control attribute?
>
Because the control is a checkbox and this label gets an accesskey defined for this checkbox, if the control is set. And that's not desired.  
 
> >diff --git a/toolkit/locales/en-US/chrome/global/printPageSetup.dtd b/toolkit/locales/en-US/chrome/global/printPageSetup.dtd
> 
> >+<!ENTITY landscape.accesskey "L">
> >+<!ENTITY marginLeft.accesskey "L">
> 
> >+<!ENTITY printBG.accesskey   "B">
> >+<!ENTITY marginBottom.accesskey "B">
> 
no, they are defined in separate tabs

> >diff --git a/toolkit/locales/en-US/chrome/global/printjoboptions.dtd b/toolkit/locales/en-US/chrome/global/printjoboptions.dtd
> 
> >+<!ENTITY resolutionInput.accesskey "R">
> >+<!ENTITY rightInput.accesskey "R">
> 
> Do these not conflict?
> 
yes, these do. thx for pointing this out

> r=me with these comments addressed.

Comment 33

[Vlado Valastiak [:wladow]]

Attachment: for checkin

Comment 34

[Neil Deakin]

> Because the control is a checkbox and this label gets an accesskey defined for
> this checkbox, if the control is set. And that's not desired.  

Why isn't is desired?

Comment 35

[[e-mail]]

(In reply to comment #34)
> > Because the control is a checkbox and this label gets an accesskey defined for
> > this checkbox, if the control is set. And that's not desired.  
> Why isn't is desired?
Because the checkbox will already display an access key itself.

Note that this dialog might have become unsupported when GTK1 builds died although I wouldn't want to be quoted on that.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350035 [details] [diff] [review]
for checkin

a191=beltzner

Comment 37

[Vlado Valastiak [:wladow]]

Attachment: hg export patch

Comment 38

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/1551ebc4dbaa

Comment 39

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

Why did we decide to take the late-l10n hit here? It hardly seems worth the trouble it's causing for localizers...

Comment 40

[Vlado Valastiak [:wladow]]

What do you mean? The TB3 Beta 1 opt-in process? We've asked localizers to skip this revision and only 4 localizers of 38 in total for b1 actually needed to do that when updating their localizations. Opt-in process is successfully done right now anyway.

Comment 41

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

Mark Banner was just mentioning on IRC that they had to back this patch out on their relbranch to avoid having to burden localizers, and Firefox is also relatively locked down as far as l10n goes. It just doesn't seem like taking this patch now rather than for 1.9.2 was worth the trouble it caused.

Comment 42

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

This landed pre-branching, so fixed1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1551ebc4dbaa
(was showing up in my queries of late-l10n bugs needing checkin)

Comment 43

[Charles Fenwick]

Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre