The print dialogs have no accesskeys

VERIFIED FIXED in mozilla1.9.1b3

Status

()

defect
--
minor
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: Stefan.Borggraefe, Assigned: wladow)

Tracking

(Blocks 1 bug, {access, late-l10n, verified1.9.1})

Trunk
mozilla1.9.1b3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

 
Posted patch Patch (obsolete) — Splinter Review
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.)
Attachment #170477 - Flags: review?(aaronleventhal)
Unfortunately my schedule is totally full. Can you find a different review for
front-end accesskey bugs?
Attachment #170477 - Flags: review?(aaronleventhal)
Comment on attachment 170477 [details] [diff] [review]
Patch

timeless, can you review this patch?
Attachment #170477 - Flags: review?(timeless)
Comment on attachment 170477 [details] [diff] [review]
Patch

Patch bitrotted because of the checkin for bug 277885.
Attachment #170477 - Attachment is obsolete: true
Attachment #170477 - Flags: review?(timeless)
Stefan, would you mind me attaching an updated patch?
(In reply to comment #5)
> Stefan, would you mind me attaching an updated patch?

No, of course not. Thank you! :-)
Posted patch version 1 (obsolete) — Splinter Review
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.
Assignee: Stefan.Borggraefe → vhaarr+bmo
Status: NEW → ASSIGNED
Attachment #204130 - Flags: review?
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)
Attachment #204130 - Flags: review? → review?(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)
Attachment #204130 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #204130 - Flags: review?(timeless)
Attachment #204130 - Flags: review-
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 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?
Posted patch version 2 (obsolete) — Splinter Review
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.)
Attachment #204130 - Attachment is obsolete: true
Attachment #205899 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #205899 - Flags: review?(timeless)
Attachment #204130 - Flags: superreview?(neil.parkwaycc.co.uk)
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 }
Attachment #205899 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #205899 - Flags: review?(timeless) → review+
Posted patch version 3 (obsolete) — Splinter Review
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.
Attachment #205899 - Attachment is obsolete: true
Attachment #206992 - Flags: superreview+
Attachment #206992 - Flags: review+
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
(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.
Posted patch toolkit patch, version 1 (obsolete) — Splinter Review
First stab at toolkit port, not sure I got it all.
Can't test myself since my tree is on fire currently.
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.
Attachment #207265 - Flags: review?(bugs.mano)
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
Attachment #206992 - Attachment is obsolete: true
Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

Since Mano hasn't touched it, trying mconnor instead.
Attachment #207265 - Flags: review?(bugs.mano) → review?(mconnor)
Attachment 207265 [details] [diff] still needs review. I'm not sure who I can ask now, maybe Deakin?
Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

looks good, this isn't something that's usefully testable, afaict
Attachment #207265 - Flags: review?(mconnor) → review+
Flags: in-testsuite-
Whiteboard: [checkin needed]
Comment on attachment 207265 [details] [diff] [review]
toolkit patch, version 1

doesn't apply cleanly, any chance someone could post an updated version?
Posted patch Updated toolkit patch (obsolete) — Splinter Review
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.
Attachment #207265 - Attachment is obsolete: true
Attachment #268689 - Flags: review?
This is a diff of the old and updated toolkit patches, to illustrate the changes.
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?
Attachment #268689 - Flags: review? → review?(mconnor)
Keywords: checkin-needed
Whiteboard: [checkin needed]
Assignee: vhaarr+bmo → kherron+mozilla
Status: ASSIGNED → NEW
Keywords: checkin-needed
Kenneth, do you plan to make patch against hg? If not, I can take this.
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.
Attachment #268689 - Flags: review?(mconnor)
- 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
Assignee: kherron+mozilla → wladow
Attachment #268689 - Attachment is obsolete: true
Attachment #268690 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #350020 - Flags: review?(gavin.sharp)
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.
Attachment #350020 - Flags: review?(gavin.sharp) → review+
(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.
Posted patch for checkinSplinter Review
Attachment #350035 - Flags: approval1.9.1?
> 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?
(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 on attachment 350035 [details] [diff] [review]
for checkin

a191=beltzner
Attachment #350035 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/1551ebc4dbaa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Why did we decide to take the late-l10n hit here? It hardly seems worth the trouble it's causing for localizers...
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.
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.
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)
Keywords: fixed1.9.1
Verified.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.