Closed
Bug 277296
Opened 20 years ago
Closed 16 years ago
The print dialogs have no accesskeys
Categories
(Core :: Printing: Output, defect)
Core
Printing: Output
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: Stefan.Borggraefe, Assigned: wladow)
References
(Blocks 1 open bug)
Details
(Keywords: access, late-l10n, verified1.9.1)
Attachments
(3 files, 7 obsolete files)
19.07 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
19.07 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
19.31 KB,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•20 years ago
|
||
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)
Comment 2•20 years ago
|
||
Unfortunately my schedule is totally full. Can you find a different review for
front-end accesskey bugs?
Updated•20 years ago
|
Attachment #170477 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 3•20 years ago
|
||
Comment on attachment 170477 [details] [diff] [review]
Patch
timeless, can you review this patch?
Attachment #170477 -
Flags: review?(timeless)
Reporter | ||
Comment 4•20 years ago
|
||
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)
Comment 5•19 years ago
|
||
Stefan, would you mind me attaching an updated patch?
Reporter | ||
Comment 6•19 years ago
|
||
(In reply to comment #5)
> Stefan, would you mind me attaching an updated patch?
No, of course not. Thank you! :-)
Comment 7•19 years ago
|
||
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•19 years ago
|
||
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 10•19 years ago
|
||
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 & images)">
>+<!ENTITY printBG.label "Print background (colors & 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•19 years ago
|
||
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•19 years ago
|
||
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 13•19 years ago
|
||
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 14•19 years ago
|
||
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+
Comment 15•19 years ago
|
||
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+
Comment 16•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
First stab at toolkit port, not sure I got it all.
Can't test myself since my tree is on fire currently.
Comment 19•19 years ago
|
||
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 20•19 years ago
|
||
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 21•19 years ago
|
||
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)
Comment 22•19 years ago
|
||
Attachment 207265 [details] [diff] still needs review. I'm not sure who I can ask now, maybe Deakin?
Comment 23•18 years ago
|
||
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+
Updated•18 years ago
|
Flags: in-testsuite-
Whiteboard: [checkin needed]
Comment 24•18 years ago
|
||
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•18 years ago
|
||
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?
Comment 26•18 years ago
|
||
This is a diff of the old and updated toolkit patches, to illustrate the changes.
Comment 27•18 years ago
|
||
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)
Updated•18 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin needed]
Updated•18 years ago
|
Assignee | ||
Comment 28•16 years ago
|
||
Kenneth, do you plan to make patch against hg? If not, I can take this.
Comment 29•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #268689 -
Flags: review?(mconnor)
Assignee | ||
Comment 30•16 years ago
|
||
- 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 31•16 years ago
|
||
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+
Assignee | ||
Comment 32•16 years ago
|
||
(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.
Assignee | ||
Comment 33•16 years ago
|
||
Attachment #350035 -
Flags: approval1.9.1?
Comment 34•16 years ago
|
||
> 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•16 years ago
|
||
(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•16 years ago
|
||
Comment on attachment 350035 [details] [diff] [review]
for checkin
a191=beltzner
Attachment #350035 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 37•16 years ago
|
||
Comment 38•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 39•16 years ago
|
||
Why did we decide to take the late-l10n hit here? It hardly seems worth the trouble it's causing for localizers...
Assignee | ||
Comment 40•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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
Comment 43•16 years ago
|
||
Verified.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•