110342 - Need a UI to control minimum font size preference

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P2)

Description

[James Green]

Summary says it all, need a UI to control the minimum font size preference. This
is probably one of the most useful features Opera has over us right now, and
it's important to get it in ASAP.

All it needs it a checkbox to turn on a pull-down selection of 4,6,8,10,12pt
values. Anything more should be considered an enhancement and implemented after
the initial functionalty.

Let's not spend months on this, *please*.

Comment 1

[Samir Gehani]

-> jbetak

Comment 2

[[e-mail] (away - not reading bugmail)]

cc'ing mpt for a UI proposal

Comment 3

[[e-mail] (away - not reading bugmail)]

Samir,

as previously discussed - I'm leaving Netscape, so this might be better off 
with someone else. I'm cc'ing Gerv and I already pinged mpt for a UI review of 
this request. Last thing I've heard, there was a new font prefs dialog in the 
making and perhaps this functionality is already included there.

Comment 4

[[e-mail] (away - not reading bugmail)]

OK, duping against bug 61883, the current UI proposal seems to have what James 
is asking for :-) Apologies for all the spam.


    Fonts ::::::::::::::::::::::::::::::::::::::::::::

    +-Fon_ts for: [default choices       :^]---------+
    | _Variable With Font: [Georgia      :^][  16:^] |
    |    Fi_xed With Font: [Courrier New :^][  13:^] |
    |                                                |
    |              _Serif: [Times New Rom:^][Auto:^] |
    |         Sa_ns-serif: [Trebuchet    :^][Auto:^] |
    |            _Cursive: [Zapf Chancery:^][Auto:^] |
    |            _Fantasy: [Desdemona    :^][Auto:^] |
    |          _Monospace: [Courier New  :^][Auto:^] |
    |                                                |
    | [ ] Minimum font si_ze:               [   8:^] |
    +------------------------------------------------+
    [/] Allow documents to use _other fonts


*** This bug has been marked as a duplicate of 61883 ***

Comment 5

[Akkana Peck]

Gerv says bug 68113 has fallen off his radar and we should un-dup this one so it
doesn't have to wait for all the issues in that long bug to be resolved.  Reopening.

Comment 6

[rbs]

Need new owner since jbetak's contract ran out (comment #3). The checkbox [ ]
that controls whether the pref is enabled or disabled might need new bool prefs
per language, or perphaps, to reduce the ever extending long list of prefs, the 
selected value can be set to negative to instruct the back-end to ignore, while 
the front-end will tweak its display to set the check mark in this case.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

taking

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 9

[rbs]

Comment on attachment 74649 [details] [diff] [review]
patch

+	 var minSizePrefVar = "font.min-size.variable." + language;
+	 var minSizePrefFixed = "font.min-size.fixed." + language;

There is only on min-size pref that governs everybody (fixed/monospace,
serif, sans-serif, fantasy, cursive), and it is:
+	 var minSizePref = "font.minimum-size." + language;

So you can further simplify @@ -300,6 +306,20 @@

Comment 10

[rbs]

Note that if the value is persisted and the pref is unchecked, the back-end will 
still continue to read it because the back-end doesn't know if it is unchecked 
or not. If you want to persist the value (which is nice), you can set it to 
negative to cause the back-end to ignore the pref. Then simply revert the sign 
at display time (see comment #6).

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Oh, great, I didn't know about that pref. (I now looked a bit through LXR...
font.minimum-size is used in nsPresContext, while .min-size is used in gfx/src/).

So I should simply ignore the font.min-size settings?

>Note that if the value is persisted and the pref is unchecked, the back-end will 
>still continue to read it because the back-end doesn't know if it is unchecked 
>or not. 

Huh? When switching panels, I write 0 into the language data, see this line and
following:
+        if (document.getElementById("minSizeActive").checked)
And if it's 0, it will be marked as checked later. Also, if it is 0 I remove the
prefs, so that the backend needs not know if it was checked.
But indeed, the last selected value will remain in the listbox. I didn't think
that fixing it was worth the effort.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch, using font.minimum-size.<language> now

Comment 13

[rbs]

>And if it's 0, it will be marked as checked later. Also, if it is 0 I remove
>the prefs, so that the backend needs not know if it was checked.

From my observations, I am anticipating some complaints if the value isn't
persisted... What about saving the old value (the negative of it, that is) and
extend what you are doing to allow the user to check/uncheck while keeping the
same min-size value. Instead of (watch the indentation, BTW):

+        if (document.getElementById("minSizeActive").checked)
+          languageData[currentLanguage].minSize = parseInt( minSize.value );
+        else
+                languageData[currentLanguage].minSize = 0;
       }

have:
if (document.getElementById("minSizeActive").checked) {
  minSizeVal = parseInt( minSize.value );
  if (minSizeVal < 0)
    minSizeVal = -minSizeVal;
  languageData[currentLanguage].minSize = minSizeVal;
}
else
  languageData[currentLanguage].minSize = 0;

instead of:
+ if (languageData[languageList.value].minSize) {

have:
if (languageData[languageList.value].minSize < 0) {

[you get the idea]

Comment 14

[André Dahlqvist]

I tried a build with the latest patch. Here are some reflections:

1) The minimum font size is correctly taken into account as soon as you hit OK
in the Preferences dialog. That is, even the current page is changed (if it has
smaller fonts as chosen). I assume that is by design? Unfortunately, closing
Preferences after changing this option takes quiet long time (probably twice as
long as if you don't change this setting).

2) If you have previously checked this option, and chosen a font size, and you
later go into Preferences and uncheck this dialog, then the current page does
NOT obey this setting withot a reload (like it is if you go the other way around).

3) As rbs already pointed out: it always jumps pack to size 6 if you uncheck
this setting and close Preferences.

Apart of these things it's fine. Thanks for doing this work Christian!

Comment 15

[rbs]

Note that if handling the negative value is troublesome, feel free to just have
another pref, e.g.,

. font.minimum-size.<language>.selection = user's choice
. if (is checked)
    font.minimum-size.<language> = font.minimum-size.<language>.selection
  else
    font.minimum-size.<language> = 0

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Is it OK if I persist the chosen value while the dialog is open, but use 6 again
when it is closed and opened again?

I'd like to avoid more prefs.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch with rbs's comments

better this way?

Comment 18

[rbs]

>Is it OK if I persist the chosen value while the dialog is open, but use 6
>again when it is closed and opened again?

That won't be good enough for the demanding mozilla's users. The alternatives
are to either use the negative trick or an additional pref. I am not a fan of
too much prefs either, but an additional pref appears as the simplest
implementation for you (and will save you from the trouble mentioned in point 3
in comment #14). And not all users visit the Fonts preference dialog, so...

Comment 19

[rbs]

re: "closing Preferences after changing this option takes quiet long time
(probably twice as long as if you don't change this setting)."

The overhead comes from the extra time to re-laid out the windows/tabs. But
since the minimum-size may seem to not have an apparent effect (i.e., documents
have no tiny text in the visible area), it would give the impression that
closing the pref dialog takes too long.

If there were a visual clue (e.g., a message on the status bar -- "applying
your preferences..."), that would have helped to dilute the problem.

Comment 20

[André Dahlqvist]

>The overhead comes from the extra time to re-laid out the windows/tabs. But
>since the minimum-size may seem to not have an apparent effect (i.e., documents
>have no tiny text in the visible area), it would give the impression that
>closing the pref dialog takes too long.

That's sort of what I thought, but I assumed it should only take longer if the
current page actually had to be changed. Should it really take equally long to
close Preferences on a page that doesn't have small fonts as on a page that has?
That's what I reacted over here, but it's nothing dramatic.

Comment 21

[rbs]

Since it is not known if the windows/tabs have embedded elements with tiny fonts
or no, no chance is taken, they are just relaid out.

Comment 22

[André Dahlqvist]

Ok. We can't wait until the next page visit for the change to take effect?

Comment 23

[rbs]

Possibly, but the other font prefs take effect immediately, and having the
minimum-size do something different might be ambiguous to users.

[A general visual cue for prefs changes to indicate that something is going on
seems the most attractive option to me.]

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

rbs, you're hard to please :)
I'll work on that this weekend, won't have time before that.

Comment 25

[André Dahlqvist]

Christian, I personally think your patch definately is good enough to get
checked in. I just pointed out the performance issue because I know that
Preferences performance is measured by the performance team, and they might
notice this change.

rbs, do I understand it correctly that you want the "visual cue" to be more
general and not only apply here? That's probably a good idea, but not something
that this patch has to handle IMHO.

Comment 26

[rbs]

general -- not meant for this bug. That's why I put as side comments in
brackets. But of course, if cbiesinger comes up with a patch for that, I won't
stop him :-) 

The thing that I think worthwhile for this bug is to persist the value as well.
My guess is that somebody will file a bug about that if it isn't done. So rather
than playing ping-pong with bugs that never end, it looks to me that it is worth
having it right now.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: remember selected font size between restarts

ok, here is the version with the change you wanted

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, and if the indentation is wrong in the patch, it's probably because of tabs,
I will convert them to spaces.

Comment 29

[Doron Rosenberg (IBM)]

please attach a screenshot

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Screenshot

This is a screenshot of how the font panel looks with my patch

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

mpt: the apparently missing accesskey is due to bug 68841

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: better patch

Comment 33

[André Dahlqvist]

Judging by the screenshot in attachment 75821 [details] it doesn't look like the text is
aligned correctly with the listbox. Small detail, but still...

Comment 34

[Gervase Markham [:gerv]]

Why not remove the checkbox and have a "None" option in the dropdown?

Gerv

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

That sounds like a good idea, I haven't thought of that myself.

mpt, your opinion?

Comment 36

[rbs]

> Why not remove the checkbox and have a "None" option in the dropdown?

Nice suggestion. Plus it will make the code simpler and solve the uncertainty of
choosing the default option to go with the checkbox... [i.e., to activate for
the first time, it will now take one click instead two].

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: new screenshot

this is a screenshot of the not-yet-attached patch

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: latest patch, with gerv's suggestion

Comment 39

[Doron Rosenberg (IBM)]

Comment on attachment 75854 [details] [diff] [review]
latest patch, with gerv's suggestion

r=doron

Comment 40

[jag (Peter Annema)]

+                minSize.value = minSizeVal;
+                minSizeSelect( minSizeVal );

When you set selectedItem, minSize's value will be updated too, and with the
adjusted value.

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, ok, I'll remove that code, then. But why is this code in the file, then:
                    variableSize.value = sizeVarVal;
                    variableSize.selectedItem =
variableSize.getElementsByAttribute( "value", sizeVarVal )[0];

                    fixedSize.value = sizeFixedVal;
                    fixedSize.selectedItem = fixedSize.getElementsByAttribute(
"value", sizeFixedVal )[0];

The .value setting could be removed, right?

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, testing showed that it _can_ be removed, so I did.

jag, could you super-review?

Comment 43

[jag (Peter Annema)]

Comment on attachment 75854 [details] [diff] [review]
latest patch, with gerv's suggestion

sr=jag

Comment 44

[Asa Dotzler [:asa]]

Comment on attachment 75854 [details] [diff] [review]
latest patch, with gerv's suggestion

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 46

[André Dahlqvist]

Is there any special reason why font sizes 11, 13 and 15 are not available on
the list? I think 13 is a nice size, but I still have to manually edit the
pref.js if I want that size now.

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

andré, I couldn't put every size there. the ones that are there were chosen by
mpt, the UI Expert.
Feel free to add a new size yourself, just add a line like this to pref-fonts.xul:
<menuitem value="13" label="13"/>
near the others

Comment 48

[André Dahlqvist]

One could argue that 11 and 13 are more useful values than 6 and 7, which are
currently on the list.

Comment 49

[Akkana Peck]

It's wonderful to see this pref finally exposed!  But: the pref isn't usable for
me because it doesn't let me pick my chosen size of 13; 12 is too small, but if
I pick 14, then I can't see my chosen monospace font any more.

All the other font dropdowns show all available sizes; it makes no sense for the
minimum font size dropdown to give fewer options than the other dropdowns, and
it especially makes no sense for the minimum font size dropdown not to include
sizes that are currently chosen for browser fonts, which just spells frustration
for the user.

Currently I see 21 size options in the proportional dropdown, 21 in the
monospace dropdown, and 9 in the minimum size dropdown.  Clearly we don't have
anything limiting us to 9 items since the other two are more than double that.

(Incidentally, is 16, the biggest size it offers, big enough for all low-vision
users?  I have no idea, but I hope someone knowledgeable about that is lurking
here.  Perhaps Aaron knows (cc'ing).

Reopening (sorry).

Comment 50

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch to add 11,13,15

(also marking attachment 75854 [details] [diff] [review] as obsolete since it has been checked in)

mpt suggested adding 11, 13 and 15 so I'm doing that in this patch.
also removing the minSizeSelect(size - 1) part because now every size between 6
and 16 is in the list.

Comment 51

[Stephen Walker]

Comment on attachment 76958 [details] [diff] [review]
patch to add 11,13,15

r=walk84

Comment 52

[jag (Peter Annema)]

Comment on attachment 76958 [details] [diff] [review]
patch to add 11,13,15

sr=jag

Comment 53

[Asa Dotzler [:asa]]

Comment on attachment 76958 [details] [diff] [review]
patch to add 11,13,15

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 54

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, I also added 18,20,22,24 on aaron's request (got r=aaronl for that) and
checked that in, assuming that sr and approval were still valid.
I did, of course, then not remove the minSizeSelect(size-1) line since the
change made it necessary again.

Comment 55

[Richard]

Very good! Except that my default font size is 28 !
I on a 15" CRT Monitor at 1024*768, 96dpi, large font size, WinMe.
And my eyes are young.
For older people like my mother who has, like most people her age, problems to
see  clear at close range, bigger font sizes should be available.
Just let all the font sizes available in the minimum font size box, and everyone
will be very happy!
Thanks for that great fix!

Comment 56

[Christian :Biesinger (don't email me, ping me on IRC)]

"all font sizes"? there's an infinite amount of possible font sizes!

Comment 57

[Johan Walles]

I think it would be great if the current version was checked in.  IMO, the
current version will help lots of people (like me for example) as is, and the
discussion of what font sizes should (not) go into the patch would be better off
in another (brand new) bug.

If you open such a bug, do mention it here so people can follow you over to it.

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

Johan: This bug is marked "RESOLVED FIXED". This means that it _is_ already
checked in (now offering all sizes from 6-16 + 18,20,22,24).

It's available in nightly builds starting with 20020402 (the earlier version
with not so much choices is in since 20020326)

Comment 59

[Hixie (not reading bugmail)]

letting users enter an arbitrary size would seem like the obvious solution to
the "infinite number of sizes" problem...

VERIFIED that users can't set any arbitrary size but that a limited selection of
sizes is available.