769604 - Get rid of Thunderbird use of increaseFontSize/decreaseFontSize commands (replace with absolute size and increase/decrease to the next available size).

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Bug 767684 reports a regression in increaseFontSize/decreaseFontSize commands, probably from bug 590640.  Ehsan and I agree that we'd be happy to get rid of these commands entirely.  IE/WebKit don't support them, and rich-text editors I looked at support no functionality of the sort -- they allow you to pick font size from a drop-down, with no increase/decrease buttons.  (E.g., Gmail has four options for size: Small, Normal, Large, Huge.)  These commands both work differently from every other supported formatting command, because they add a tag whose effect is relative to surrounding style.  So it would be nice if we could kill them.

However, Thunderbird apparently is still using them.  As far as I can tell from MXR, the only usage is mail/components/compose/content/editorOverlay.xul.  Could this be removed and replaced with a drop-down that allows specific selection of size?  Until this is done, bug 767684 is a fairly serious standing regression in Thunderbird 15 -- it looks like increase/decreaseFontSize are broken in some extremely common cases.  bwinton suggested mconley as a candidate to change the UI here, saying he was on vacation but would be back Monday.

Comment 1

[(no longer active)]

Note that if this cannot be fixed, we should back out bug 590640 preferably through the Aurora period.  And since that will make many Thunderbird unhappy, I suggest that we fix this as soon as possible.  :-)

Comment 2

[Ian Neal]

Seems like more than just TB is using them:
http://mxr.mozilla.org/comm-central/search?string=cmd_increaseFont&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
http://mxr.mozilla.org/comm-central/search?string=cmd_decreaseFont&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 3

[Philip Chee]

Also you might want to notify the Blue Griffon people.

Comment 4

[Joe Sabash [:JoeS1]]

Please note that bug 767684 remains unconfirmed, and does not even have a typical reproducible testcase.
In a typical TB context, increase/decrease font size seems to wfm in common cases.
Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=767684#c21

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Blake:

Here's my first shot at it. I'm a *little* concerned that for the default compose window size, the new font selector dropdown menu shoves the picture/smiley insertion toolbarbuttons out of visibility...

Thoughts?

-Mike

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 639170 [details] [diff] [review]
Patch v1

Neil:

hg log says that you should know your way around editor.js fairly well. How's my driving? Also, I imagine this change will also affect SeaMonkey - you SM folks might also want to consider getting rid of the increase/decrease buttons...

-Mike

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Forgot to remove some cruft.

Comment 8

[[e-mail]]

(In reply to Mike Conley from comment #6)
> you SM folks might also want to consider getting rid of the
> increase/decrease buttons...
Actually I found the regression. It's just a case of whether someone can be bothered to write a test for it, in which case my fix can land, otherwise the regression will get backed out.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to [e-mail] from comment #8)
> Actually I found the regression. It's just a case of whether someone can be
> bothered to write a test for it, in which case my fix can land, otherwise
> the regression will get backed out.

Regardless of the regression, I still want to get rid of the commands unless they're actually useful to people.  No non-Gecko WYSIWYG editor I know has such commands, and they add complexity.  Even if SeaMonkey really wants to keep them, I still want to remove them from execCommand() so they're not web-exposed, because they're not standard.  So I think it's in SeaMonkey's best interest to drop support too, unless there are strong use-cases.

Comment 10

[[e-mail]]

Comment on attachment 639174 [details] [diff] [review]
Patch v2

>+function ConvertFontClassToSize(aSize)
We should get a definitive answer as to whether font sizes use HTML or CSS values, and then fix the code properly.

>-    // Fixed size items start after menu separator
>-    var menuIndex = 3;
>+    var menuIndex = 0;
>     // Index of the medium (default) item
>-    var mediumIndex = 5;
>+    var mediumIndex = 2;
This is... not useful, sorry.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to [e-mail] from comment #10)
> Comment on attachment 639174 [details] [diff] [review]
> Patch v2
> 
> >+function ConvertFontClassToSize(aSize)
> We should get a definitive answer as to whether font sizes use HTML or CSS
> values, and then fix the code properly.
> 
> >-    // Fixed size items start after menu separator
> >-    var menuIndex = 3;
> >+    var menuIndex = 0;
> >     // Index of the medium (default) item
> >-    var mediumIndex = 5;
> >+    var mediumIndex = 2;
> This is... not useful, sorry.

Hm. Not sure what you mean by "not useful". This makes the font size selector in the Format menu work properly now that the increase/decrease menuitems are now gone.

Is there another approach you'd rather I take?

Comment 12

[(no longer active)]

Comment on attachment 639174 [details] [diff] [review]
Patch v2

Review of attachment 639174 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me in general, however I don't know this code very well, so please ask somebody else to look at it as well.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 639174 [details] [diff] [review]
Patch v2

I'm not really happy with the labels, since they seem kind of jargon-y.  What the heck is "xx-small" anyways?  In a related comment, I didn't see the new dropdown push the smileys out of the window, but perhaps finding better names will help allay your concerns here, too.  (We could also take the opportunity to reduce the number of choices here.  I'm thinking something like "tiny", "small", "normal", "large", "huge", maybe?)

The other thing that kinda bothers me is how much variation we're losing.  Check out the old (left) vs. new (right):
http://dl.dropbox.com/u/2301433/Screenshots/fs.png

Now sure, we don't _really_ need that large an "f", but I think, if there was a way to make the sizes a little more different, it would be nice.

So, f- for the ui, but I think that the problems I've raised are fixable.

Thanks,
Blake.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) [On vacation until July 6th!] from comment #13)
> I'm not really happy with the labels, since they seem kind of jargon-y. 
> What the heck is "xx-small" anyways?  In a related comment, I didn't see the
> new dropdown push the smileys out of the window, but perhaps finding better
> names will help allay your concerns here, too.  (We could also take the
> opportunity to reduce the number of choices here.  I'm thinking something
> like "tiny", "small", "normal", "large", "huge", maybe?)

FWIW, Gmail has only "Small", "Normal", "Large", "Huge".

Comment 15

[Jim Porter (:squib)]

Would it be possible to change the size list to a point size? I think that's what most people would expect, since that's what most rich-text editors use (office suites, CKEditor, etc).

If we do just change the labels though, remember to update the strings in Options -> Composition -> General -> HTML. :)

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The only way to allow point sizes would be to use CSS inline style, which as I understand it is not as reliably supported by mail clients as <font size>.  I've been told that mail clients on phones, etc., commonly only support a limited subset of HTML markup.  (Someone who knows more about richtext e-mail please correct me if I'm wrong!)  Furthermore, the underlying editor framework doesn't currently support font sizes of anything other than HTML's old 1-7, although we do want to support that for the sake of HTML rich editing.

So basically, I think it's not possible, but maybe someone who knows more can correct me.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Aryeh Gregor from comment #16)
> So basically, I think it's not possible, but maybe someone who knows more
> can correct me.

Ehsan and I took a look at the editor code centered around setting inline font sizes on Friday.

While it sounds / feels like it *should* be trivial to change to CSS inline style, it would actually take quite a bit of engineering effort in terms of editor re-tooling in order to make it happen. Probably more effort than it's worth.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I would like to make it happen, and spec a fontSizePt command, because for general-purpose rich-text editors (Word replacements) sizes 1-7 are not very useful.  Also because queryCommandValue() is very broken for fontSize if anything but sizes 1-7 are used.  There's a spec bug for it:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=14253

I think it would be pretty doable, although it might take a bunch of refactoring.

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Mike Conley (:mconley) from comment #17)
> (In reply to :Aryeh Gregor from comment #16)
> > So basically, I think it's not possible, but maybe someone who knows more
> > can correct me.
> 
> Ehsan and I took a look at the editor code centered around setting inline
> font sizes on Friday.
> 
> While it sounds / feels like it *should* be trivial to change to CSS inline
> style, it would actually take quite a bit of engineering effort in terms of
> editor re-tooling in order to make it happen. Probably more effort than it's
> worth.

When I implemented the CSS part of the editor, the big question was "what
do we do when the user hits the 'bigger' or 'smaller' button? and the font size
is not keyword-based?".
That said, no, this is not difficult to implement and does not require
to touch the existing code. I am currently on vacation and connected only about
once a week. I'll look at this when I come back if

  a. you're kind enough to specify the behaviour for "bigger" and "smaller" when
     the font size is absolute

  b. you've not implemented it already :-)

Aryeh, give me a draft spec and I'll do it when I'm back.

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't intend to spec increaseFontSize/decreaseFontSize.  They aren't found in any other WYSIWYG editors I saw, and IE/WebKit don't implement them.  I would like to drop support for them entirely.  So that would solve that problem.

I do intend to write a spec, but currently I'm occupied with other things (mainly bug 671152).

Comment 21

[Mark Banner (:standard8)]

As bug 767684 has been fixed, taking this off the tracking list. We can still implement the changes if we want to, but we don't need to make sure this gets fixed for a particular release.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: Patch v2a - unbitrotted version from April 2012

Here we have another abandoned bug/patch.

I un-bitrotted it and tried it out. It basically works (although xx-small doesn't seem to work).

What I want to know is whether we can finally go forward with this. I think the increase/decrease size is absolutely terrible and should be replaced with specifying the absolute size.

So here are questions:

Thomas:
What do you think? I don't want to be bogged down by yet another lengthy discussion about the user interface.

Richard:
What do you think?

Magnus:
What do you think?

Blake:
(hmm, not taking NI or f? - what do I do?)
In comment #13 you weren't so happy with this. What exactly do you want to change to make it acceptable?
Change the label to "tiny", "small", "normal", "large", "huge"?

Neil:
I know that you complained about some technical aspects in comment #10. Sadly you didn't explain your "not useful" any further. So please let me know any detail I should know. I've just picked up the patch like so many abandoned patches before and I'm learning by doing ;-)

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot showing patch v2a applied

Comment 24

[Jorg K (CEST = GMT+2)]

Current increase/decrease buttons produce this HTML:
    <tt>normal<br>
      <big>normal increased</big><br>
      <big><big>normal increased twice</big></big><br>
      <big><big><big>normal increased 3 times</big></big></big><br>
      <small>normal decreased</small><br>
      <small><small>normal decreased twice</small></small><br>
      <small><small><small>normal decreased 3 times</small></small></small><br>
    </tt>

With the patch, this gets produced:
    <tt><font size="-2">xx-small</font><br>  <-- looks like it should be -3 here,
                                                 or maybe that doesn't exist, see below.
      <font size="-2">x-small</font><br>
      <font size="-1">small</font><br>
      medium<br>
      <font size="+1">large</font><br>
      <font size="+2">x-large</font><br>
      <font size="+3">xx-large</font><br>
      <br>
    </tt>

This matches what is produced when the default composition font size is changed in
  Tools > Options, Composition, HTML, Size.
Actually, there we offer x-small, small, medium, large, x-large and xx-large (no xx-small), so to keep the user interface consistent, this is what should be used on the drop-down, although Blake didn't like it. Or for consistency sake we'd have to change the labels in the preferences.

Another argument to get rid of the terrible decrease/increase is that TB sends e-mail with <font size="xx">, depending on the preference, but when answering such an e-mail, it is impossible to change the received font size in any reasonable way, not even for the reply text. The result is a terrible mix like:
  <big><font size="-1"><big><font face="Arial">huhu</font></big></font></big>
when trying to increase the font size in the answer to an e-mail written in size="-1".

Using the patch, the result is simply: <font face="Arial">huhu</font><br> after setting the size to medium.

So I think the patch presented by Mike Conley in 2012 has a lot going for it, and its a pity it was abandoned five minutes before landing it.

Comments?

Comment 25

[Jorg K (CEST = GMT+2)]

According to this http://www.w3schools.com/tags/att_font_size.asp the range is from 1 to 7, medium being 3, or using +/- from -2 to +4. No wonder "xx-small" (-3) doesn't work.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jorg K (GMT+2) from comment #22)
> Blake:
> (hmm, not taking NI or f? - what do I do?)

As it turns out, I'm watching all the bugs in this component, so just mention my name, and I'll probably get around to it…  ;)

> In comment #13 you weren't so happy with this. What exactly do you want to
> change to make it acceptable?
> Change the label to "tiny", "small", "normal", "large", "huge"?

That sounds good.  I'ld also suggest using the strings from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#270-274 for consistency, and to give the translators an easier job.  ;)

Comment 27

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #22)
> Created attachment 8670961 [details] [diff] [review]
> Patch v2a - unbitrotted version from April 2012
> 
> Here we have another abandoned bug/patch.
> 
> I un-bitrotted it and tried it out. It basically works (although xx-small
> doesn't seem to work).
> 
> What I want to know is whether we can finally go forward with this. I think
> the increase/decrease size is absolutely terrible and should be replaced
> with specifying the absolute size.

It would be good to have some understandable reasons WHY you think the current UI is terrible.
Any proposed UI changes should be assessed in a rational manner weighing pros and cons.
"I don't like this" is not an UI argument unless supported by something tangible and intelligible for others.

> So here are questions:
> 
> Thomas:
> What do you think?

Like BWinton's comment 13, I'm not a big fan of this bug.
I don't see any reason to remove the increase/decrease commands.
They are found in MS Word, and we deliberately implemented the same keyboard shortcuts as in Word. Very handy if you don't care about the exact size but just want the font smaller or bigger than usual.

Like BWinton's comment 13, I don't see why we should randomly limit our users to font sizes between 4pt and 24point, while currently they can choose and adjust their font size freely, including bigger font sizes > 24pt.

We could try a point value combo selector like in word processors.

I don't know how helpful an absolute font size selector is for email messages, but there's probably nothing wrong with it. Still, no need to dump the relative selectors; they would just tweak the point value as they do in Word (or the predefined tiny-small-normal-large-huge set). even if we don't like keeping the realative selectors on the toolbar (why?), we could still keep them in customization palette for those users who have come to appreciate that use case. (And there seem to be no user complaints requesting removal, no votes, no duplicates, no supportive comments - so WHY are we doing this? Never change a running system...)

The increase/decrease *commands* from the *menu* should definitely stay, including the shortcuts, because they are simple, intuitive, and allow for more font size variety.

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #26)
> > In comment #13 you weren't so happy with this. What exactly do you want to
> > change to make it acceptable?
> > Change the label to "tiny", "small", "normal", "large", "huge"?
> That sounds good.  I'ld also suggest using the strings from ...
... which are: Tiny, Small, Medium, Large, Extra Large.

Actually, please read comment #24 (quote):
===
This matches what is produced when the default composition font size is changed in
  Tools > Options, Composition, HTML, Size.
Actually, there we offer x-small, small, medium, large, x-large and xx-large (no xx-small), so to keep the user interface consistent, this is what should be used on the drop-down, although Blake didn't like it. Or for consistency sake we'd have to change the labels in the preferences.
===

The preferences already use: x-small, small, medium, large, x-large and xx-large, so for consistency we need to use that or change it as well.

Comment 29

[Thomas D. (:thomas8)]

Comment on attachment 8670961 [details] [diff] [review]
Patch v2a - unbitrotted version from April 2012

f- per my comment 27.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #27)
> Like BWinton's comment 13, I'm not a big fan of this bug.
> I don't see any reason to remove the increase/decrease commands.
> They are found in MS Word, and we deliberately implemented the same keyboard
> shortcuts as in Word. Very handy if you don't care about the exact size but
> just want the font smaller or bigger than usual.

It sounds like they're going away from the platform, so I think we should adjust to that, assuming we don't want to try maintaining that code ourselves…

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #27)
> It would be good to have some understandable reasons WHY you think the
> current UI is terrible.
Here we go, another bug which promises a lengthy discussion.

I don't care so much about the UI. The current behaviour is terrible, I cannot get the program to do what I need to do. I cannot achieve a decent predicable size. The HTML produced is terrible, please read comment #24 (which you have ignored completely).

The decrease/increase in inconsistent, Mr. Consistency, with what the size preference does.

Enough reasons?

> They are found in MS Word, ...
I haven't used MS Word in years, I can't remember ever seeing it. It's not an option in OO Write.

> We could try a point value combo selector like in word processors.
Out current HTML is restricted to the old size="nn" or size="+/-nn" values.

> so WHY are we doing this?
It's something it has annoyed me for years, and when I found this bug, I put it onto my "to do" list. It simply annoys me that I can't write a decent HTML reply to any e-mail received where a size is specified, because the sender used TB and had a different preference.

If you want to maintain the existing decrease/increase controls and the mess they produce, fine, I don't have to use them. I haven't looked into it, but it should/may be doable.

Comment 32

[Magnus Melin [:mkmelin]]

What it does to the formatting sounds good to me. I'm slightly worried though that a dropdown will take up a lot of space. Ideally it would just be a size button showing the available sizes, similar to what gmail does. (And our insert smiley)

Likely many people mistakenly think of this as zoom so it would be good not to make it too predominant.

Comment 33

[Jorg K (CEST = GMT+2)]

Just to summarise:

Can I have a clear decision from the people who can make a decision on which option to take:

1) Implement the bug as stated in the description:
   Get rid of Thunderbird use of increaseFontSize/decreaseFontSize commands,
   replace with drop-down size selection.
   Please make sure you've read comment #0 and comment #24.
2) Add the drop-down size and leave the existing controls in place
   (causing a potentially even bigger mess).
3) Do none of the above and mark the bug as WONTFIX
   (and wait for the days of milk an honey when TB will support proper CSS).

There is no point having bugs lingering in the system for years or decades, just because no one can decide anything.

Kent, you are the chief of this crowd, so perhaps you want to comment as well.

Comment 34

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #32)
> What it does to the formatting sounds good to me.

Thanks. Once we have a firm decision on what we want to do, we can worry about the details.
I refuse to work on another 250+ comment bug ;-(

Yes, Gmail has a button with a pop-up. I'm cool with that. We can also look at other smart editors, or maybe not:
http://neilj.github.io/Squire/ - hmm, I don't see a size control.
http://ckeditor.com/demo - hmm, I don't see a size control here either.
http://www.tinymce.com/tryit/basic.php - still no size control, hmm.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #34)
> Yes, Gmail has a button with a pop-up. I'm cool with that. 

I take that back. Like the font *indicator* this is a size *indicator*. You click on the text and it shows you the size. So a button won't fly.

Comment 36

[Jorg K (CEST = GMT+2)]

Kent, looks like a mid-air collision discarded the NI I wanted to attach at comment #33.

Comment 37

[Jorg K (CEST = GMT+2)]

Here are some more facts. Import the attachment 8646778 [details] from bug 1193631.
    <tt>
      <small><small>Smaller</small></small><br>
      <small>Small</small><br>
      Normal<br>
      <big>Big</big><br>
      <big><big>Bigger</big></big><br>
      <br>
      <span style="font-size:9.0pt">9 point</span><br>
      <span style="font-size:12.0pt">12 point</span><br>
      <span style="font-size:15.0pt">15 point</span><br>
    </tt>
Clicking on those spans, the size indicator doesn't update, since the size is not recognised. That would be an argument for Magnus' button. We could make the button popup with an indicator that shows which item is selected, so if we can't detect it, we select nothing. That's better than an indicator that doesn't indicate anything. So I take back the "taking back" from comment #35 and I'm now again in favour of Magnus' suggestion.

<off-topic>
In the current implementation it is *impossible* to change the font size of the CSS sizes (see bug 1193631). So much for Thomas' "Never change a running system". It's not running as it currently stands, and it's highly confusing for users that the controls don't do anything. CSS sizes are produced by Outlook, so it would be good if we could deal with them in one way.

Sadly, the the suggested implementation doesn't fix this either. When making the 15pt item smaller/small, in the existing implementation we get:
  <small><span style="font-size:15.0pt">15 point</span></small>
With the patch it's:
  <font size="-1"><span style="font-size:15.0pt">15 point</span></font>
and both don't show any effect. Anyway, that's another bug. However, maybe absolute sizes are a step closer to fixing this. I don't see how <small> of a point size could be made to work. For the absolute sizes one would just strip the CSS.
</off-topic>

Comment 38

[Thomas D. (:thomas8)]

Jorg, if you are not interested in my opinion/contributions, don't ask me. I don't like the irony and the impressions you're trying to create about me.

Fact: From an UX perspective, implementing this bug in its current shape reduces user options for font size control significantly, including keyboard efficiency.

I never said the current technical *implementation* using <big>/<small> is ideal (clearly not). I only said the *functionality* is useful and should not be removed just because the current implementation is poor.
Current functionality (lost by this bug):
- free definition of font size between what appears on screen as 2pt and 500pt (estimate).
- handy increase/decrease font size commands with same keyboard shortcuts as in MS Word.

Terrible HTML code is a bug in implementation, which does not necessarily justify removing the functionality. Yes, there's always a pragmatic argument "oh, we don't have time and manpower to do this properly". I'm concerned about taking the simplistic route of just ripping out what we think we can't fix properly.

There's another problem in the background of this: Who is maintaining Mozilla's "Editor"? My impression is that nobody cares much, and even less about the needs of TB.

Having an absolute font control is good. Imo having that as an indicator control would also be good.
Zoom values come with percentages, and with a "Font Size" tooltip, I don't see dangers of mixing up with zoom. That's an argument for implementing a separate zoom control, if anything (which would be very nice in terms of accessibility for people with poor eyesight etc.; accessibility being a part of Mozilla's mission).
If we implement css-font-size in points, user can define freely (problem solved).
Nothing wrong with keeping increase/decrease functionality which should just change the point-size in predefined steps. Yes, it's more work. And better UX.

Anyway, I don't care much. Feel free to do what you want here. Just don't be surprised if users start filing bugs that their font-size controls no longer work as flexibly and conveniently as before.
I don't want to be made feel guilty in advance for any comment that I add, so this is probably my last comment on this matter.

And I think that statements like your comment 31 are unprofessional, because when you're not even using Word, the fact you haven't seen a certain command does not mean it's not there, and I don't like the undertone which seems to subtly suggest that what I said about the status quo in Word might be wrong.
> I haven't used MS Word in years, I can't remember ever seeing it
> [increase/decrease font size controls and keyboard shortcuts]

Fwiw, at least all the pre-ribbon versions of Word even have buttons for that in the toolbar customization palette, and the shortcut seems to work all the way up to current Word 2016:

http://www.officetooltips.com/word/tips/increase__decrease_and_change_font_size_without_mouse.html

Comment 39

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #38)
> Jorg, if you are not interested in my opinion/contributions, don't ask me. I
> don't like the irony and the impressions you're trying to create about me.
That was not the intention. I appreciate your opinion/contribution, and even in bug 368915 we reached a good solution after 250+ comments, also thanks to your insistence.

> Fact: From an UX perspective, implementing this bug in its current shape
> reduces user options for font size control significantly, including keyboard
> efficiency.
Agreed.

> Current functionality (lost by this bug):
> - free definition of font size between what appears on screen as 2pt and
> 500pt (estimate).
I don't think this is useful functionality. TB is not QuarkXPress or Scribus.
You don't get any reasonable reproducible size, you just get bigger, bigger, bigger or smaller, smaller, smaller.
Some links about the <big> tag:
http://www.w3schools.com/tags/tag_big.asp - The <big> tag is not supported in HTML5. Use CSS instead.
(In fairness: The same applies to <font size="nn").
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/big -
  This feature is obsolete. Although it may still work in some browsers,
  its use is discouraged since it *could be removed at any time*.

> Terrible HTML code is a bug in implementation, which does not necessarily
> justify removing the functionality. Yes, there's always a pragmatic argument
> "oh, we don't have time and manpower to do this properly". I'm concerned
> about taking the simplistic route of just ripping out what we think we can't
> fix properly.
Well, if the functionality is not reasonable, and it stands in the way of a simple, defined and working solution, I won't hesitate a minute to remove it. But read on for a compromise proposal presented below.

> There's another problem in the background of this: Who is maintaining
> Mozilla's "Editor"? My impression is that nobody cares much, and even less
> about the needs of TB.
Well, Ehsan and Aryeh are still working in the area, and lately I have landed a few bugs there. But no major changes are expected in the short to medium term. CSS is still somewhat bolted on, and it shows ;-(

> Having an absolute font control is good.
Agreed.

I think we have come to the point where we agree on the following:
1) The <big>/<small> implementation is not future-proof, gives unpredictable results and is
   inconsistent the preference values of x-small, small, medium, large, x-large, xx-large.
2) Absolute font control is good.
3) Controls to make it bigger or smaller are desirable
   (because we always had them, and because other software offers them).

Thank you so much for the link below:
http://www.officetooltips.com/word/tips/increase__decrease_and_change_font_size_without_mouse.html
I quote:
Ctrl+Shift+> Increases the font to the next larger point size available in the Font size list box.
Ctrl+Shift+< Decreases the font to the next smaller point size available in the Font size list box.

Aha, so this does NOT just make the size bigger or smaller, it actually shifts to a new *defined* (!!) size.

So how about this approach:
To the existing
  A(down-arrow) - make text smaller
  A(up-arrow) - make text bigger
we add and inconspicuous third button
  A (double-arrow/up-down)
that will pop up a menu from which to select the size.

We change the behaviour of the existing decrease/increase buttons to shift to the next available smaller/bigger size. Yes, you lose the wide range you mentioned, but at the same time we lose the <big>/<small> implementation and you gain consistency with the preference value.

(In the fullness of time, when CSS comes around, the old <font size="nn"> implementation can be rolled out and the CSS implementation can be rolled in.)

I have no scruples to hijack this bug and change the direction from
  Get rid of Thunderbird use of increaseFontSize/decreaseFontSize commands
  (replace with drop-down size selection)
to
  Get rid of Thunderbird use of increaseFontSize/decreaseFontSize commands
  (replace with absolute size and increase/decrease to the next available size).

Note that the increaseFontSize/decreaseFontSize commands are used internally and result in <big>/<small>.

Can we agree on this?
Thomas? Richard? Magnus? Neil? Kent? Aceman? Blake?

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: Mock-up of proposed solution.

Here is the inconspicuous third button.

Comment 41

[Richard Marti (:Paenglab)]

I'm fine with this bug. And as you, Jörg said, Mail isn't a DTP program which needs this big variety of font sizes. If someone really needs them he can define them with injecting the needed code in "Insert HTML...", not simple but this is a special case.

Also the proposal in comment 39 and attachment 8671299 [details] makes sense and the <big><small> mess would go. I would move the size button before the two inc./dec. buttons. And the icon could be two As, one smaller than the other, like GMail has with it's Ts. And adding a dropmarker makes it also clearer a popup will open.

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #41)
> Also the proposal in comment 39 and attachment 8671299 [details] makes sense
> and the <big><small> mess would go. I would move the size button before the
> two inc./dec. buttons. And the icon could be two As, one smaller than the
> other, like GMail has with it's Ts. And adding a dropmarker makes it also
> clearer a popup will open.

Perfect. Thanks for the comment. Once I have agreement from the other parties, I'll get right to it ;-)

Comment 43

[Jorg K (CEST = GMT+2)]

Richard: Labels?
x-small, small, medium, large, x-large, xx-large - as in the preferences, or
Tiny, Small, Medium, Large, Extra Large - as suggested by Blake. (Drop "xx-large"?)
If so, we need to change the preferences menu as well, agreed?

Comment 44

[Richard Marti (:Paenglab)]

Ah yes, I knew I forgot something. :)

We should take Blake's suggested ones with also changing all occurrences of the old names.

Comment 45

[Jorg K (CEST = GMT+2)]

OK, summarising:

Proposed solution:
- One new button preceding the two existing ones to set a fixed size.
  Icon: A small A and big A, like Gmail's "TT", with a drop marker indicating the drop-down.
  Sizes: Tiny, Small, Medium, Large, Extra Large.
  Drop-down menu, showing the sizes and indicating the current size were possible
  (like the spelling drop-down or the Format > Size menu, oh, there we need the new sizes too).
- Existing buttons remain, but slightly change their behaviour. They now
  set the next possible smaller or larger size in the range of five sizes.

Do we have agreement? Let me guess people's answer, no offence intended:
Richard: already agreed.
Magnus: likely to agree since we're doing his button and fixing the resulting HTML.
Blake: likely to agree since we're doing his labels and removing the deprecated decrease/increase.
Thomas? I hope we can convince you that losing the wide range of sizes possible with <big>/<small> is unavoidable. We keep the increase/decrease buttons you wanted, and add the absolute size you were in favour of.

Now last not least, Neil? Please give me your opinion. This will affect /editor, and therefore SM.

Comment 46

[:aceman]

Also try Ian if it affects Seamonkey compose window.

Comment 47

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #45)
> OK, summarising:
> 
> Proposed solution:
> - One new button preceding the two existing ones to set a fixed size.
>   Icon: A small A and big A, like Gmail's "TT", with a drop marker
> indicating the drop-down.
>   Sizes: Tiny, Small, Medium, Large, Extra Large.
>   Drop-down menu, showing the sizes and indicating the current size were
> possible
>   (like the spelling drop-down or the Format > Size menu, oh, there we need
> the new sizes too).
> - Existing buttons remain, but slightly change their behaviour. They now
>   set the next possible smaller or larger size in the range of five sizes.
> 
> Do we have agreement? Let me guess people's answer, no offence intended:
> Richard: already agreed.
> Magnus: likely to agree since we're doing his button and fixing the
> resulting HTML.
> Blake: likely to agree since we're doing his labels and removing the
> deprecated decrease/increase.
> Thomas? I hope we can convince you that losing the wide range of sizes
> possible with <big>/<small> is unavoidable. We keep the increase/decrease
> buttons you wanted, and add the absolute size you were in favour of.

Convinced. :) Thank you, Jorg, and well done!

Can we use the positive energy and the time saved by avoiding the next 200 comments and find out in another bug (existing/new?) what it takes to implement css-font-size in points? :P

Comment 48

[[e-mail]]

(In reply to Jorg K from comment #45)
>   Drop-down menu, showing the sizes and indicating the current size were possible
>   (like the spelling drop-down or the Format > Size menu, oh, there we need the new sizes too).
What's wrong with the sizes in the Format > Size menu? Why can't you just provide a dropdown equivalent to it?

Comment 49

[Jorg K (CEST = GMT+2)]

That's what I meant, but the UX folks like to change the labels from
x-small, small, medium, large, x-large (remove xx-large) to Tiny, Small, Medium, Large, Extra Large.

The point is to change what happens behind the decrease/increase buttons.

Comment 50

[:aceman]

I also think adding human readable labels is good, instead of the technical (CSS keywords) x-small, etc.

Comment 51

[Jorg K (CEST = GMT+2)]

Richard, would you be able to prepare these buttons for me.
Looks like they are still in the PNG files and there are three of them:
mail/themes/linux/mail/compose/format-buttons.png
mail/themes/osx/mail/compose/format-buttons.png
mail/themes/windows/mail/compose/format-buttons.png
Any plans to move to SVG?

Comment 52

[Richard Marti (:Paenglab)]

I have not much time this weekend but you could start with placeholder images which can be replaced later.

Comment 53

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #45)
Sounds ok, though are both the new button and the +- buttons needed? I don't have a strong opinion about it.

Comment 54

[Jorg K (CEST = GMT+2)]

The new button makes the functionality from the "Format > Size" menu easier available, and the +- buttons (which have keyboard shortcuts) are for "quick" action. Thomas wanted to maintain them.
The point is that we will change their behaviour.

Comment 55

[Richard Marti (:Paenglab)]

It makes sense to show all buttons for the mouse centric users. Outlook has also the size menulist (in px) and the +- buttons aside of it.

Comment 56

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #49)
> That's what I meant, but the UX folks like to change the labels from
> x-small, small, medium, large, x-large (remove xx-large) to Tiny, Small,
> Medium, Large, Extra Large.

Please do NOT remove the font-size currently listed in font-size selector as xx-large (+3).
Nobody requested this and we should not limit the choice of font sizes even further without need, using only 6 out of 7 possible font sizes.
Wording is simple. Btw other languages like German have already switched to natural wording, so we can borrow from them, with just one addition (*) to the currently proposed natural language labels:

x-small  -> Tiny        -2  ( 7.5 pt)
small    -> Small       -1  (10 pt)
medium   -> Medium       0  (12 pt)
large    -> Large       +1  (13.5 pt - weird, how did this come about?)
x-large  -> Very Large* +2  (18 pt)
xx-large -> Extra Large +3  (24 pt)

24pt is still just twice the normal size, which really isn't much, so we should keep that option. No need to regress even more.

Comment 57

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #56)
> Please do NOT remove the font-size currently listed in font-size selector as
> xx-large (+3).

Removing this option would also require undesirable migration code for those who have currently set xx-large as their default font size in options.

Comment 58

[Jorg K (CEST = GMT+2)]

I agree, however, I'd do this:

x-small  -> Tiny        -2  (7.5 pt)
small    -> Small       -1  (10 pt)
medium   -> Medium       0  (12 pt)
large    -> Large       +1  (13.5 pt - weird, how did this come about?)
x-large  -> Extra Large +2  (18 pt)
xx-large -> Huge        +3  (24 pt)

Where did you get the point sizes from? Are they defined somewhere or did you measure them?

Comment 59

[Richard Marti (:Paenglab)]

Attachment: Part 1: New icons

Nevertheless had some time to create the icons.

Comment 60

[Jorg K (CEST = GMT+2)]

Comment on attachment 8672215 [details] [diff] [review]
Part 1: New icons

Thanks, they look great. It looks like a lot of work producing them in five versions:
mail/themes/linux/mail/compose/format-buttons.png
mail/themes/osx/mail/compose/format-buttons.png
mail/themes/osx/mail/compose/[e-mail]
mail/themes/windows/mail/compose/format-buttons-XP.png
mail/themes/windows/mail/compose/format-buttons.png

I'm organising the patch into multiple parts, so I don't have to resubmit the icons with every version. Here goes the first part.

Comment 61

[Jorg K (CEST = GMT+2)]

Actually, the Linux version is not missing a "disabled" version?

Comment 62

[Richard Marti (:Paenglab)]

Attachment: Part 1: New icons

Ah yes, added.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with new icon for "absolute font" (Windows)

I was going to take the weekend off, but since Richard is working ...

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: WIP - XUL and CSS changes

Comment 65

[Richard Marti (:Paenglab)]

Attachment: Part 2: WIP - XUL and CSS changes

The only change I made is in the OS X CSS to let the buttons look correctly.

Why are you using a button type="menu-button" and not a type="menu"? The type="menu" isn't splitted and shows directly the popup. The type="menu-button" uses more space and would need at least on OS X some more styling to fit again.

Comment 66

[Richard Marti (:Paenglab)]

Attachment: Screenshot of toolbar with new button for "absolute font" (OS X)

This is how it would look on OS X but with button type="menu" (like the three buttons at the end).

Comment 67

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with button for "absolute font" (Windows) (corrected)

(In reply to Richard Marti (:Paenglab) from comment #65)
> Why are you using a button type="menu-button" and not a type="menu"? 
Plain ignorance, I'm afraid :-(
Thanks for pointing it out!

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: WIP - XUL and CSS changes (v2)

Including Richards changes:
1) Mac button locking
2) type="menu"

Richard, I looked at this for a while, but can't work out, what that does:

#AbsoluteFontSizeButton:-moz-locale-dir(rtl),
#IncreaseFontSizeButton:-moz-locale-dir(rtl),
#underlineButton:-moz-locale-dir(rtl),
#indentButton:-moz-locale-dir(rtl),
#boldButton:-moz-locale-dir(ltr),
#ulButton:-moz-locale-dir(ltr) {
  border-top-left-radius: 0;
  border-bottom-left-radius: 0;
}

#AbsoluteFontSizeButton:-moz-locale-dir(ltr),
#IncreaseFontSizeButton:-moz-locale-dir(ltr),
#underlineButton:-moz-locale-dir(ltr),
#indentButton:-moz-locale-dir(ltr),
#boldButton:-moz-locale-dir(rtl),
#ulButton:-moz-locale-dir(rtl) {
  border-top-right-radius: 0;
  border-bottom-right-radius: 0;
}

The documentation at
https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir%28ltr%29 and
https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir%28rtl%29
doesn't make it clear.

On Mac, we have three blocks of buttons
Block 1:
- Absolute size
- Decrease
- Increase

Block 2:
- Bold
- Italic
- Underline

Block 3:
- un-numbered list ul
- numbered list
- outdent
- indent.

I understand that the "outer" buttons need fixing, so their inner edge is not rounded.

Can you please explain how to read the CSS.

Comment 69

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of modified size menu.

Comment 70

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of modified preferences menu (Composition, HTML, Size)

Comment 71

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with new button for "absolute font" with dropdown (Windows)

Comment 72

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v3).

This implements the new toolbar button for absolute font size choice.
It also changes the labels everywhere, see enclosed screenshots.
x-small  -> tiny        -2
small    -> small       -1
medium   -> medium       0
large    -> large       +1
x-large  -> extra large +2
xx-large -> huge        +3
(xx-small never worked and was removed).

Feedback welcome.

Oh yes, can someone explain why we/I have the button defined twice:
editor/ui/composer/content/editorOverlay.xul
mail/components/compose/content/editorOverlay.xul

Next would be to change what happens behind the decrease/increase buttons.

Comment 73

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v3a).

(removed duplicated code, better this way).

Comment 74

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #58)
>> x-small  -> Tiny        -2  (7.5 pt)
>> xx-large -> Huge        +3  (24 pt)
> 
> Where did you get the point sizes from? Are they defined somewhere or did
> you measure them?

I copied text from mail composition into to MS Word and checked it out there.
However, for very big font sizes currently creatable with <big><big>..., this method seems to fail because iirc they were shrinked to not bigger than 36pt or so when pasting.

Comment 75

[Jorg K (CEST = GMT+2)]

Thanks, I see. I copied to OO Writer and got 7pt, 10pt, 12pt, 14pt, 18pt, 24pt.

Comment 76

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #68)
> 
> I understand that the "outer" buttons need fixing, so their inner edge is
> not rounded.
> 
> Can you please explain how to read the CSS.

Yes, that is what you understand. But it seems OS X doesn't use this styles because of -moz-appearance: toolbarbutton. OS X groups them itself with the needed stylings. I don't know when the stylings are used, but first we let them like they are.

Comment 77

[Richard Marti (:Paenglab)]

Comment on attachment 8672300 [details] [diff] [review]
Part 2: XUL, CSS and some JS changes for new toolbar button (v3a).

The patch doesn't apply cleanly on editor.js. Fixed manually and it looks good. I saw that the AbsoluteFontSizeButton is not disabled like the other buttons when the focus isn't in the editor field.

Comment 78

[:aceman]

Comment on attachment 8672237 [details] [diff] [review]
Part 1: New icons

Review of attachment 8672237 [details] [diff] [review]:
-----------------------------------------------------------------

The edges of the As in these icons are somehow more "rough" than the old neighbouring icons (like A^). It stands out to me quite a lot (on Linux). Can this be improved/smoothed?

Comment 79

[:aceman]

Comment on attachment 8672300 [details] [diff] [review]
Part 2: XUL, CSS and some JS changes for new toolbar button (v3a).

Review of attachment 8672300 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work.
I just find it strange that this uses <font size=> and the old increase/decreaseFontSize uses <big> and <small>. Does that mean that we can produce different non-aligning font sizes with each of the systems? I.e. you can never produce the same size as e.g.<big><big> (say it is 15pt) with the new absoluteFontSize widget (which only produces e.g. 12pt and 18pt)? In which comment was this mismatch discussed and agreed upon?

Also, if the goal is to produce absolute font sizes, why does it produce tags like <font size="-2">? What size is it relative to? The default of 3? Or the current surrounding text (no, it does not). Why not e.g. <font size="2">? The website in comment 25 does not seem to mention relative sizes.

This also produces to me unexpected results, e.g. this produces a huge font for the "All big" string, but the "l" is really tiny (not slightly smaller, like 2 levels down from the huge font):
<big><big><big><big><big>Al<font size="-2">l</font>big</big></big></big></big></big>

I also find it strange, that the code uses the css keywords x-small..xx-large but then produces html tags as <font size=>. That is probably why xx-small does not work. style='font-size: xx-small' would probably work once we decide to emit css in the composer.

Comment 80

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #77)
> The patch doesn't apply cleanly on editor.js.
That's because I have the fix to bug 1197687 in my stack. Perhaps you can grab that as well.
I was hoping that would land one day soon, but not there is bustage in the tree :-(

> Fixed manually and it looks
> good. I saw that the AbsoluteFontSizeButton is not disabled like the other
> buttons when the focus isn't in the editor field.
Oops, I noticed that at some stage and then got carried away by other stuff.
Thanks for the reminder!

(In reply to :aceman from comment #78)
> The edges of the As in these icons are somehow more "rough" than the old
> neighbouring icons (like A^). It stands out to me quite a lot (on Linux).
> Can this be improved/smoothed?
Maybe Richard can look into that.

(In reply to :aceman from comment #79)
> I just find it strange that this uses <font size=> and the old
> increase/decreaseFontSize uses <big> and <small>.
That's because Part 3 is not implemented yet. Please read comment #72 at the bottom:
  Next would be to change what happens behind the decrease/increase buttons.
That will be coming up. At them moment, please don't mix the new button with the decrease/increase buttons.
The patch is only to try out the *new* button in conjunction with "Format > Size".

> Also, if the goal is to produce absolute font sizes, why does it produce
> tags like <font size="-2">? What size is it relative to? The default of 3?
> Or the current surrounding text (no, it does not). Why not e.g. <font
> size="2">? The website in comment 25 does not seem to mention relative sizes.
I understand that the following are equivalent:
-2 == 1 - x-small - tiny
-1 == 2 - small
(0) == 3 - medium
+1 == 4 - large
+2 == 5 - x-large = extra large
+3 == 6 - xx-large = huge
The existing code uses the +/- values, so I stuck with that. Please look at
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#817
EditorSetFontSize(size)

> I also find it strange, that the code uses the css keywords
> x-small..xx-large but then produces html tags as <font size=>.
Agreed. Again, I didn't change any of that.

Comment 81

[:aceman]

(In reply to Jorg K (GMT+2) from comment #80)
> I understand that the following are equivalent:
> -2 == 1 - x-small - tiny
> -1 == 2 - small
> (0) == 3 - medium
> +1 == 4 - large
> +2 == 5 - x-large = extra large
> +3 == 6 - xx-large = huge
Why? Where is this documented and does it work in the relevant browsers/mail clients?
Also notice you are missing the value of 7, which is NOT available via your button/menu.

The docs I found say <font size> is not even supported in HTML5 so anything about how it will render is not guaranteed. The same for <big>. <small> seems to be supported but for a different use than just presentation of font size (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/small).

I assume probably need to use these old tags for compatibility with Outlook and similar.

> The existing code uses the +/- values, so I stuck with that. Please look at
> http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.
> js#817
> EditorSetFontSize(size)
> 
> > I also find it strange, that the code uses the css keywords
> > x-small..xx-large but then produces html tags as <font size=>.
> Agreed. Again, I didn't change any of that.

Yes, not your fault. I now see the comment at http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#828 mentions we currently change the css to <font size>, hopefully temporarily. In that case, please keep the xx-small xul code commented out as is (not remove it). We may support it in the future.

So yes, I am looking forward to the part 3.

Comment 82

[:aceman]

(In reply to Jorg K (GMT+2) from comment #72)
> Oh yes, can someone explain why we/I have the button defined twice:
> editor/ui/composer/content/editorOverlay.xul
> mail/components/compose/content/editorOverlay.xul
Maybe the editor/ version is used in Seamonkey mail composer or the HTML composer (for websites).
You will need to get the patch seen by SM people in all cases.

> Next would be to change what happens behind the decrease/increase buttons.
Yes, the old and new button should operate in the same font size system/steps so that you can achieve the same font sizes with both methods. Of course, if it is possible to go above size=7 with the increase button, there will be no equivalent in the absolute button.

Comment 83

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v3b).

Carrying over Aceman's and Richard's f+.
Fixed new buttons so it's enabled/disabled when it should be.

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v3c).

Carrying over Aceman's and Richard's f+.
Fixed whitespace (line length, indentation).

Comment 85

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #81)
> > I understand that the following are equivalent:
> > -2 == 1 - x-small - tiny
> > ...
> > +3 == 6 - xx-large = huge
> Why? Where is this documented and does it work in the relevant browsers/mail
> clients?
Look, it's always been that way.
All I'm doing here in Part 2 is implementing a handy button that works like "Format > Size". 

> Also notice you are missing the value of 7, which is NOT available via your
> button/menu.
Do I have to repeat myself? Yes, 7 == +4 was never available.
All I'm doing here in Part 2 is implementing a handy button that works like "Format > Size".

> Yes, not your fault.
None of it is my fault ... 
All I'm doing here in Part 2 is implementing a handy button that works like "Format > Size".

> I now see the comment at
> http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.
> js#828 mentions we currently change the css to <font size>, hopefully
> temporarily. In that case, please keep the xx-small xul code commented out
> as is (not remove it). We may support it in the future.
For now, I have removed what doesn't work.
If/when we make xx-small work, we will need to add labels for it, and then we can bring it back.
However, it is questionable whether xx-small is really desirable. x-small/tiny/7pt is really quite small already.

As for Part 3: Hmm, I wasn't going to work this weekend, but since Richard gave me the icons, I made a start, so he could see his work being used. Today is Sunday ;-)

Comment 86

[:aceman]

(In reply to Jorg K (GMT+2) from comment #85)
> (In reply to :aceman from comment #81)
> > > I understand that the following are equivalent:
> > > -2 == 1 - x-small - tiny
> > > ...
> > > +3 == 6 - xx-large = huge
> > Why? Where is this documented and does it work in the relevant browsers/mail
> > clients?
> Look, it's always been that way.
OK, they are equivalent but only in the current code, as long as it is not actually using the css keywords in the emitted HTML.

> > Also notice you are missing the value of 7, which is NOT available via your
> > button/menu.
> Do I have to repeat myself? Yes, 7 == +4 was never available.
It is available, via the increase size button. So we could improve the menu. Can you add size 7 to the menu (with a keyword of xxx-large and comment in the editor code it is not standard)?

> > I now see the comment at
> > http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.
> > js#828 mentions we currently change the css to <font size>, hopefully
> > temporarily. In that case, please keep the xx-small xul code commented out
> > as is (not remove it). We may support it in the future.
> For now, I have removed what doesn't work.
> If/when we make xx-small work, we will need to add labels for it, and then
> we can bring it back.
> However, it is questionable whether xx-small is really desirable.
> x-small/tiny/7pt is really quite small already.
Sometimes it is really good to see comments about unfinished features in the code. Otherwise nobody notices the size is actually missing. But I am OK with the removal if you file a bug about it :)
Yes, only if it makes sense to support it. I think currently the decrease font size button supports it (going down 3 steps) and even smaller.

Comment 87

[:aceman]

For comparison, Paypal does use a ton of CSS (including <style> element) in its emails, including text-size. So probably the support is not that bad in clients.

Comment 88

[Jorg K (CEST = GMT+2)]

Aceman: Can you please not try to increase the scope of this bug. This bug has two aims.

First:
Provide a handy button on the editing toolbar that allows a quick selection of the *existing* sizes.
Second:
Change the decrease/increase functionality to use font sizes that were always available and get rid of the <big>/<small> business which calls:
http://mxr.mozilla.org/mozilla-central/source/editor/composer/nsComposerController.cpp#126
http://mxr.mozilla.org/mozilla-central/source/editor/composer/nsComposerCommands.cpp#1216
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditorStyle.cpp#1415

This bug is NOT about:
- Providing size +4 (equiv. 7)
- Provinding CSS support, that is making xx-small work.
- Fixing M-C editor code.

I have to contradict: Size +4 is currently *not* available in Thunderbird, but surely you can get produce something bigger that size +3 by applying an increase. The result is:
<big><font size="+3"><big>Horrible mess</big></font></big>.

Please take another look at how the sizes are handled:
Using EditorSetTextProperty()
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#848
defined here:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editorUtilities.js#360
and using:
http://mxr.mozilla.org/comm-central/source/mozilla/editor/nsIHTMLEditor.idl#79
with code here:
http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditorStyle.cpp#111

<off-topic>
For more things that could be done in the area of sizes, please refer to bug 1193631, which is about the CSS aspect. Outlook produces <span style="font-size:12.0pt">12 point</span>, and I'm sure that this can be found generally in HTML e-mail (for example from PayPal). Sadly M-C's nsHTMLEditor::SetInlineProperty() doesn't accept CSS sizes being passed in and it also can't set/increase/decrease the size, if you pass it an element which has a CSS size. The result is <font size="+4"><span style="font-size:12.0pt">12 point</span></font> and that has no effect. In comment #39 I already said (quote): "no major changes are expected [in the M-C editor] in the short to medium term. CSS is still somewhat bolted on, and it shows ;-(". Let's make this clear: Gecko perfectly supports the rendering of CSS, but in the *editor* it is still neglected.
</off-topic>

I'm clearing the NI for Neil and Ian, they will be involved in the review.

Comment 89

[Richard Marti (:Paenglab)]

Attachment: Part 1: New icons.

This icons should now be better for Linux and XP.

Comment 90

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #88)
> Aceman: Can you please not try to increase the scope of this bug. This bug
> has two aims.

Look at the clarity with which Jorg is defining the exact territory of this bug! No room for manouvres! Don't even try! ;)
It may sound friendlier if you say: "I wouldn't like to increase the scope of this bug :| Imo, this bug is primarily about / should focus on two aims." Which might be more appreciative of Aceman's good and creative comments where he tries to playfully/gently push us to new heights by not giving up too early or imposing thought police.

> Second:
> Change the decrease/increase functionality to use font sizes that were
> always available and get rid of the <big>/<small> business which calls:

Yes, we do that as a preemptive measure to avoid breaking in the future.
As has been pointed out, we also lose a significant range of font sizes in the process as long as we don't/can't switch to CSS point sizes.

> This bug is NOT about:
> - Providing size +4 (equiv. 7)

Ah, let's not be too strict with simple things :) I appreciate you want to get done with this, but it's not only about developers, it's also and mainly about our users and their UX.

Providing size +4 (aka 7; 36pt) would be an excellent way of mitigating the significant loss of bigger font sizes caused by removing <big><small> feature. You know, it's hard to tell what users really want from their compositions, but nothing is stopping them from wanting to use big font sizes for whatever.
I can't believe this would be hard to do, even if we had to change it in editor code, as it works exactly the same as all the other font sizes. Exploiting the full range of our deprecated font size system is not asking too much. Please try to implement it. Thank you.

> - Providing CSS support, that is making xx-small work.

Sure. But I'd agree with Aceman that instead of traceless removal of a feature we'll perhaps want to bring in at a later time, comments in code are helpful:

> Sometimes it is really good to see comments about unfinished features in the code. Otherwise nobody
> notices the size is actually missing. But I am OK with the removal if you file a bug about it :)

> - Fixing M-C editor code.

Well, what are our options if editor devs don't get round to it...? :/

> <off-topic>
> For more things that could be done in the area of sizes, please refer to bug
> 1193631, which is about the CSS aspect... Sadly M-C's
> nsHTMLEditor::SetInlineProperty() doesn't accept CSS sizes being passed in
> and it also can't set/increase/decrease the size, if you pass it an element
> which has a CSS size. The result is <font size="+4"><span
> style="font-size:12.0pt">12 point</span></font> and that has no effect...
> CSS is still somewhat bolted on,
> and it shows ;-(". Let's make this clear: Gecko perfectly supports the
> rendering of CSS, but in the *editor* it is still neglected.
> </off-topic>

That sounds like you have a very good understanding of what would be necessary to fix editor code... ;)
About time to say that your coding work is very much appreciated. Thank you, Jorg. :)

Comment 91

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #90)
> > This bug is NOT about:
> > - Providing size +4 (equiv. 7)
> Providing size +4 (aka 7; 36pt) would be an excellent way of mitigating the
> significant loss of bigger font sizes caused by removing <big><small>
> feature.
size +4 works in Gecko, but was never implemented in TB.

This can easily be added, however:
- we have two small sizes that work (-2 and -1) so why four big sizes (+1 to +4).
- I'm running out of words, large, extra large, huge, what's next? Gigantic, humongous.
- Gmail does small, normal, large, huge. We already have two more.

The argument that kills the idea is that there is no xxx-large, which would be the equivalent, so, sorry: No, it would not be "future proof" and we'd have to withdraw it later.

Comment 92

[:aceman]

Jorg, we just do not understand 2 things here:
1. There are buttons for increase/decrease font. Now you add a new button that only provides a very limited set of sizes. Where is it decided what those sizes will be? Only the 6 levels supported by current editor code, even though 7 really exit?
2. When you reimplement the <big> and <small> in part 3, will you keep the range of sizes they can produce? Or do you plan to limit those down to the 6 levels that are now available in the new button? That is also what bwinton worries about in comment 13.

Comment 93

[:aceman]

Comment on attachment 8672352 [details] [diff] [review]
Part 1: New icons.

Review of attachment 8672352 [details] [diff] [review]:
-----------------------------------------------------------------

The big A is still different from the increase/decrease buttons, but if you need to keep the icon width, then maybe it can't get better.

Comment 94

[Richard Marti (:Paenglab)]

The problem is, the letters for Linux and XP are very bold and there are only 16px of space and I had to make the As smaller to fit both in this space.

I need to look if I could use the icons from the other Windows icon set.

Comment 95

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #92)
> 1. There are buttons for increase/decrease font. Now you add a new button
> that only provides a very limited set of sizes. Where is it decided what
> those sizes will be? Only the 6 levels supported by current editor code,
> even though 7 really exit?
Ok, I'm repeating it again:
All I'm doing here in Part 2 is implementing a handy button that works like "Format > Size".
What other words can I use to say this? We are not changing any functionality here. What you get in "Format > Size" you will get on the button: 6 levels.
"xx-small" is not on the menu, since it doesn't work, +4/7/xxx-large is not on the menu since it is not future-proof. Mike Conley's original proposal did exactly this (although he had xx-small included, which didn't work). I have nothing to add to this, so please, don't ask again.

> 2. When you reimplement the <big> and <small> in part 3, will you keep the
> range of sizes they can produce? Or do you plan to limit those down to the 6
> levels that are now available in the new button? That is also what bwinton
> worries about in comment 13.
I think I've said this a few times before, but I can repeat it again:
The new buttons will increase/decrease to the next available size (just like MS Word). There are a total of six sizes, the ones you currently get on the "Font > Size" menu.
Why are we doing that?
- Because the technology that provided more sizes is deprecated.
- Because the <big>/<small> stuff produces horrible HTML and confuses everyone
  when mixed with the other sizes. You said so yourself in comment #79.

Who has agreed to doing this so far:
Myself, Aryeh who raised the bug, Mike Conley who started it, Ehsan who is interested in removing deprecated stuff from the editor. Blake laments the loss (comment #14), but he has come to realise that it's necessary (comment #30). Magnus agreed in comment #32. Richard agreed in comment #41. Also, Thomas agreed in comment #47, quote: "Convinced". So let's not aim for 250+ comments in this bug ;-(

Comment 96

[:aceman]

(In reply to Jorg K (GMT+2) from comment #95)
> Who has agreed to doing this so far:
> Myself, Aryeh who raised the bug, Mike Conley who started it, Ehsan who is
> interested in removing deprecated stuff from the editor. Blake laments the
> loss (comment #14), but he has come to realise that it's necessary (comment
> #30). Magnus agreed in comment #32. Richard agreed in comment #41. Also,
> Thomas agreed in comment #47, quote: "Convinced". So let's not aim for 250+
> comments in this bug ;-(

Yes, I see now that you describe this in comment 45 and eveybody seems to agree with it. So yes, when the loss of sizes is agreed upon and consistent in all 3 buttons, I am fine with that. That is what I wanted to hear:) Just note that e.g. Outlook does allow 60pt font without problems. TB will also allow that once it can produce CSS.

Comment 97

[Jorg K (CEST = GMT+2)]

I'm glad that we finally all agree. ;-)

(In reply to :aceman from comment #96)
> Outlook does allow 60pt font without problems. TB will also
> allow that once it can produce CSS.
CSS issues should be discussed in bug 1193631.

<off-topic>
In the medium term I don't see any major changes in the M-C editor, so in the medium term I don't see any CSS support. In the long term, we're not really sure what TB will be based on. If you follow the discussion about the phasing out of XUL and the conversion of TB to a client based on JS/HTML (tb-planning topic "Future Planning: Thunderbird as a Web App", Sept. 2015), we might replace the editor technology one day. In a recent Tuesday meeting Kent already told me off for spending too much time on editor issues. So in view of all that, I will restrict myself to two things: 1) Fix the most blatant M-C editor bugs (and I'm already a good way along this path) and 2) Implement anything that is possible *now* that will make the life of users easier in the very short term. I believe that this bug falls into the this second category.

Also, please revisit comment #34 and the three "editors" described there. I just did again and this time I found the font size function for one of them:
http://neilj.github.io/Squire/ - Size control clicking on the "A": Small, Medium, Large.
http://ckeditor.com/demo - No size control, all done with styles.
http://www.tinymce.com/tryit/basic.php - No size control.
</off-topic>

Comment 98

[[e-mail]]

Comment on attachment 8672326 [details] [diff] [review]
Part 2: XUL, CSS and some JS changes for new toolbar button (v3c).

>-    // Fixed size items start after menu separator
>-    var menuIndex = 3;
>-    // Index of the medium (default) item
>-    var mediumIndex = 5;
>+    var menuIndex, mediumIndex;
>+    if (fullMenu)
>+    {
>+      // Fixed size items start after menu separator
>+      menuIndex = 3;
>+      // Index of the medium (default) item
>+      mediumIndex = 5;
>+    } else {
>+      // Shortened menu for the toolbar.
>+      menuIndex = 0;
>+      mediumIndex = 2;
>+    }
I would just use ?: e.g.
var menuIndex = fullMenu ? 3 : 0;
var mediumIndex = menuIndex + 2; // or fullMenu ? 5 : 2;

>diff --git a/mail/components/preferences/compose.xul b/mail/components/preferences/compose.xul
Also suite/mailnews/compose/prefs/pref-composing_messages.xul please.

Comment 99

[Jorg K (CEST = GMT+2)]

Attachment: Test e-mail with all possible cases.

For those who want to play with it ;-)

Comment 100

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v4).

In this update:
- Changed font size detection to also detect sizes 1 to 7.
  Previously those weren't detected and shown as "medium".
- Optimised the font size detection. It now doesn't query all sizes separately.
  (Hi Neil, thanks for the interest and the comment. I followed it.)
- Added XUL changes for SM as requested.

Please keep in mind: If you want to apply the patch, apply attachment 8671863 [details] [diff] [review] from bug 1197687 first.

Comment 101

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v4a).

(changed "" to null in one call, fixed white-space).

Comment 102

[Jorg K (CEST = GMT+2)]

Note that <small>, <big> and <font size="nn"> are all deprecated but will be supported for a long time to come. Really deprecated are the M-C editor increaseFontSize/decreaseFontSize commands and they may be retired long before the <small>/<big> support. Note the difference between rendering existing HTML and producing new HTML in a contenteditable element, which is what the editor does.

I'm changing the summary to what I said in comment #39.

Comment 103

[Jorg K (CEST = GMT+2)]

Houston, we've got a problem.

I've been thinking on how to implement the new increase/decrease functions.

I realised that presently you can select any mix of text sizes and click the increase/decrease button. What happens in the M-C editor is that the selection is wrapped in <small>/<big> tags and that is the end.

You can also select a mix of text sizes and assign a absolute new size. That works (as long as no CSS sizes are used), since we just tell the M-C editor to set the new size of the selection, which results in the selection being wrapped in <font size="new size">, and the editor is smart enough to remove the inner font size tags of the selection.

In the future this won't work.

For the new functionality to work, we first need to determine the font size, and then apply the next size up or down (or ignore the request, if we're already at the end of the range). The problem is in determining the font size: We ask the editor, and in case of a mixed selection it returns the first size encountered with the indication "mixed".

In other words, the increase/decrease can only work on a selection where all the text has the same size. You can't select, for example, the whole e-mail with different sizes and say: Bigger. (Well, you could, but it wouldn't happen.)

Please don't suggest that we analyse the selection in TB's own JS code or drill open the M-C editor to provide a function to change the all the sizes in the selection.

I'm sure that Mike Conley, who submitted the first patch, already knew about this problem. That's why he suggested to simply withdraw the increase/decrease options.

So once again, we have room for discussion. I see the following options:
0) Leave the increase/decrease options in place as they are, so all this bug does is
   add a handy button onto the toolbar and improve the size recognition (see comment #100).
   increase/decrease would continue to create the messy HTML, but people don't have to use them.
1) Withdraw the increase/decrease buttons. Users would have to assign a new absolute size.
   This was intended in the original patch.
2) Go ahead with the new increase/decrease function as described. It will only work on a
   non-mixed selection. I don't think that users will be very satisfied with this:
   Firstly instead of a wide range of sizes, they now only get six, and also, if they press the
   button on a mixed selection, nothing will happen.

So gentlemen, I need your opinion.

<unrelated>
Just about absolute sizes (1 .. 7) and relative ones (-2 .. +4, assuming base font size 3):
http://www.w3.org/TR/REC-html40/present/graphics.html#edef-BASEFONT
</unrelated>

Comment 104

[Magnus Melin [:mkmelin]]

#1 should be fine. I also don't think people really want to use mixed sizes normally. It's a common complaint with editors in general that people just want the text to look consistent and struggle to remove random formatting that comes from copy-paste or editor quirks.

Comment 105

[Axel Grude [:realRaven]]

(In reply to Magnus Melin from comment #104)
> #1 should be fine. I also don't think people really want to use mixed sizes
> normally. It's a common complaint with editors in general that people just
> want the text to look consistent and struggle to remove random formatting
> that comes from copy-paste or editor quirks.

I would have tended to #0, but #1 is fine if we do not propose to remove <span> or other tags within the selection. E.g. if I have a tag <code>someKeyword</code> and this assigns  a certain font size, this should remain unaffected. Any <font> tag within the selection could either have their font-size / size attributes removed, if the aim is as Magnus said to have uniform text size, or be left alone (no change in size). On problem with inline tags is that they often go go unnoticed as there is no "x-ray" mode that shows them.

Technically how is the sizing achieved? A span tag? Or font? And it would be fantastic if it could work on an existing tag (e.g. <p>) if the selection matches that, rather than adding more tags. So if a paragraph is selected, look at the effective size and then inline style to <p style="font-size:1.5em;">. Other question, should we work with pt, em, px, ex or another unit?

Comment 106

[Jorg K (CEST = GMT+2)]

Thank you for your interest and comment.
Option #0 does actually not fix the bug, since the bug is about removing the use of the M-C editor commands to increase/decrease the font size.

The new font size button does the very same thing already available on the "Format > Size" menu, so you can answer your questions yourself by trying it out right now (do the edit, save the draft and inspect the source).

Thunderbird code does very little, it passes the call to the M-C editor, in case of setting the font size we call EditorSetTextProperty()
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#848
which calls setInlineProperty()
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editorUtilities.js#366
which calls into the M-C editor:
http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditorStyle.cpp#111

Technically, a <font size="nn"> tag is used. As I said before, the M-C editor is still of the level of HTML 3 (since <font> was already deprecated in HTML 4 - http://www.w3.org/TR/html4/present/graphics.html#edef-FONT) and CSS is only bolted on.

Comment 107

[:aceman]

(In reply to Jorg K (GMT+2) from comment #103)
> You can also select a mix of text sizes and assign a absolute new size. That works (as long as no CSS sizes are >used), since we just tell the M-C editor to set the new size of the selection, which results in the selection being >wrapped in <font size="new size">, and the editor is smart enough to remove the inner font size tags of the selection.
> For the new functionality to work, we first need to determine the font size,
> and then apply the next size up or down (or ignore the request, if we're
> already at the end of the range). The problem is in determining the font
> size: We ask the editor, and in case of a mixed selection it returns the
> first size encountered with the indication "mixed".

Can this be done as option 3? Get the first size, increase/decrease it by 1, put the whole selection into the new element and clear all other <font size> or <big>/<small> tags inside the range. Would that be technically possible?

Comment 108

[Jorg K (CEST = GMT+2)]

Here a little survey:

Outlook/MSN/Hotmail web e-mail produces:
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0">Hi Jorg:</font>

Outlook, the program, generally uses CSS, it sends a <style> section at the beginning and also uses
<span style="FONT-SIZE: 10pt;">

Gmail sends this:
<b>bold<br></b> <i>italics<br></i> <u>underline<br></u>
<span style="color:rgb(255,0,0)">red</span><br>
<font size="1">small</font><br>
<font size="2">normal</font><br>
<font size="4">large</font><br>
<font size="6">huge</font>

Interesting to see that their "normal" is actually small.

Comment 109

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #107)
> Can this be done as option 3? Get the first size, increase/decrease it by 1,
> put the whole selection into the new element and clear all other <font size>
> or <big>/<small> tags inside the range. Would that be technically possible?

Yes.

When you assign a new absolute size using "Format > Size" (or with the new button), we first remove all <big>/<small> and existing sizes (I was wrong before, the M-C editor doesn't do that, we do):
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#822

After that, we can assign the incremented/decremented size. But is this useful? The user selects a mix of font sizes and says "bigger". What would happen is that everything would get the same absolute size which is the first size encountered incremented by one.

Comment 110

[Jorg K (CEST = GMT+2)]

In other words: The user selects all of the following:
  small
  huge
  large
  medium
an says "bigger". The result will be all in size "small"+1 = "medium". So some parts will go smaller, and one part doesn't change. How good is that? ;-)

Comment 111

[:aceman]

(In reply to Jorg K (GMT+2) from comment #109)
> After that, we can assign the incremented/decremented size. But is this
> useful? The user selects a mix of font sizes and says "bigger". What would
> happen is that everything would get the same absolute size which is the
> first size encountered incremented by one.

Well, we could also ask him what to do, but the only options seem to be to do what I said, or do nothing. From you said it seems you can't iterate the elements and e.g. get the max size of the range, or an average. It seems the loop over the range is hidden at http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditorStyle.cpp#1281, which we probably don't want to touch.
Maybe you could first get the values via something like nsHTMLEditor::GetInlineProperty before clearing them?

Comment 112

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #111)
> It seems the loop over the range is hidden at
> http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/
> nsHTMLEditorStyle.cpp#1281, which we probably don't want to touch.
That's interesting reading and shows the complexity.

> Maybe you could first get the values via something like
> nsHTMLEditor::GetInlineProperty before clearing them?
No, as I said, this returns the first value found in the selection with the usual indicators:
firstHas, someHas, allHas. If !allHas, it's mixed.

Comment 113

[Jorg K (CEST = GMT+2)]

I would like to progress with this bug, so sorry about the survey.

As detailed in comment #103, it is not possible (without extreme effort) to increase/decrease all sizes in the selection by one step. Therefore we have the following options (for details see comment #103):

0) Leave the increase/decrease options in place as they are, so all this bug does is
   add a handy button onto the toolbar and improve the size recognition (see comment #100).
1) Withdraw the increase/decrease buttons. Users would have to assign a new absolute size.
2) The new increase/decrease function will increase/decrease by one step in the
   case of a non-mixed selection. Do nothing for mixed selections or even disable the options
   in the drop-down menu.
3) As per 2) In case of a mixed selection, set everything to the increased/decreased *first* size
   found.

Pro's and con's:
0) Will not withdraw any functionality but will continue to produce messy and confusing HTML.
   However, no user is obligated to use the old function and with the new button right next to the old
   buttons, the user might explore the new button.
   Marketing strategists would call this: "The best of both worlds" ;-)
1) Withdraw of existing functionality (a function which is somewhat subobtimal).
2) Not really necessary, since the increase/decrease can be achieved by selecting the next size up/down
   from the menu.
3) Equally not necessary and will yield confusing results. See comment #110.

So I'm asking for opinions again:
Myself: I can live with #0 and #1 or #2. (I will never again use the increase/decrease.)
Magnus: Comment #104: Prefers #1.
Aceman: ?
Richard: ?
Thomas: (I know you don't like to withdraw existing functionality.)
Neil/Ian: Could the SM people please say something. They have the increase/decrease buttons on their menu.
They are already affected by the change of the size names (tiny, extra large and huge).

In case of #0, I will move the patches to a new bug called
"Provide font size selection button on toolbar, improve size detection for sizes 1 to 7".
since we're not really addressing this bug here. Note that the existing patches already cover option #0.

I think the retiring of the increase/decrease functions in the M-C editor is not a pressing issue, since the bug 767684 and bug 590640, which were driving this, got resolved. So option #0 could be supported for a few years to come.

Comment 114

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #113)
> I would like to progress with this bug, so sorry about the survey.
> 
> As detailed in comment #103, it is not possible (without extreme effort) to
> increase/decrease all sizes in the selection by one step. Therefore we have
> the following options (for details see comment #103):
> 
> 0) Leave the increase/decrease options in place as they are, so all this bug
> does is
>    add a handy button onto the toolbar and improve the size recognition (see
> comment #100).
> 1) Withdraw the increase/decrease buttons. Users would have to assign a new
> absolute size.
> 2) The new increase/decrease function will increase/decrease by one step in
> the
>    case of a non-mixed selection. Do nothing for mixed selections or even
> disable the options
>    in the drop-down menu.
> 3) As per 2) In case of a mixed selection, set everything to the
> increased/decreased *first* size
>    found.
> 
> Pro's and con's:
> 0) Will not withdraw any functionality but will continue to produce messy
> and confusing HTML.
>    However, no user is obligated to use the old function and with the new
> button right next to the old
>    buttons, the user might explore the new button.
>    Marketing strategists would call this: "The best of both worlds" ;-)
> 1) Withdraw of existing functionality (a function which is somewhat
> subobtimal).
> 2) Not really necessary, since the increase/decrease can be achieved by
> selecting the next size up/down
>    from the menu.
> 3) Equally not necessary and will yield confusing results. See comment #110.
> 
> So I'm asking for opinions again:
> Myself: I can live with #0 and #1 or #2. (I will never again use the
> increase/decrease.)
> Magnus: Comment #104: Prefers #1.
> Aceman: ?
> Richard: ?
> Thomas: (I know you don't like to withdraw existing functionality.)
> Neil/Ian: Could the SM people please say something. They have the
> increase/decrease buttons on their menu.
> They are already affected by the change of the size names (tiny, extra large
> and huge).
> 
> In case of #0, I will move the patches to a new bug called
> "Provide font size selection button on toolbar, improve size detection for
> sizes 1 to 7".

I would go with #0 - giving more choice and we can retire the increase / decrease later. Are you using <font> <span> or try to affect existing elements (e.g. <div><p>) if selected?

Comment 115

[Jorg K (CEST = GMT+2)]

(In reply to Axel Grude [:realRaven] from comment #114)
> I would go with #0 - giving more choice and we can retire the increase /
> decrease later.
Agreed.

> Are you using <font> <span> or try to affect existing
> elements (e.g. <div><p>) if selected?
Please try it out yourself, see comment #106, 2nd paragraph.

Comment 116

[:aceman]

(In reply to Jorg K (GMT+2) from comment #113)
> Pro's and con's:
> 0) Will not withdraw any functionality but will continue to produce messy
> and confusing HTML.
>    However, no user is obligated to use the old function and with the new
> button right next to the old
>    buttons, the user might explore the new button.
Yes, but users used to the old buttons will continue uisng them and they are not aware the functionality is deprecated. They will actually see the new button that has less options (steps) and hate it from day 1.

My preference would be 2) with disabling the buttons when on unsuitable range (multiple sizes). Then 3) 0) 1).

Comment 117

[Jorg K (CEST = GMT+2)]

Correction:
2) ... Do nothing for mixed selections or even disable the increase/decrease buttons.

Comment 118

[Richard Marti (:Paenglab)]

I'm for 2) with disabling the increase/decrease buttons. Only do nothing would confuse the user when he has no feedback. Then 3) 1) 0). I think 0) will only delay the deprecation and the outcry would come later. And better we begin now with cleaner code.

Comment 119

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #118)
> I'm for 2) with disabling the increase/decrease buttons.
Well, disabling the user interface for no *apparent* reason is also problematic.
Imagine a full stop "." at the end of the line in a different size causing the buttons to get disabled.

> I think 0) will only delay the deprecation and the outcry would come later.
Hard to say really. If we rebase TB on a different editor one day which can do CSS, we would get functionality of creating very big/small sizes, so there would never be a noticeable deprecation.

Anyway, your choice is noted.

Comment 120

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #113)
> So option #0 could be supported for a few years to come.

Hey Jorg, cooperative surveys are great.

I'll follow the marketing strategist and vote for
0) "add font size button, keep increase/decrease as is for now" - the best of both worlds
Otherwise in order of preference (most preferred first)
0) 2) 3) 1)

As you correctly point out, there's no time pressure to make it worse for an unknown number of users now by removing significant variability in font sizes. If required, we can still pick up on this at any time with a simple fix.

There are enough painpoints in the product already so I think we shouldn't add more without real need.
I suggest that in the meantime, we should try to get CSS font sizes running so we can provide a smooth transition experience later where big font sizes in points are possible again, so we'll keep all functionality but just change what's under the hood.

So I agree with Jorg (vs. Richard), that we're not just delaying the outcry but there's a chance to avoid it altogether.

Comment 121

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #116)
> Yes, but users used to the old buttons will continue uisng them and they are
> not aware the functionality is deprecated. They will actually see the new
> button that has less options (steps) and hate it from day 1.

I agree users will continue to use the old buttons. I suspect that they will also hate it when the old buttons no longer work as before because very small and many bigger font sizes > 24 pt are no longer possible. So I think your analysis is actually an argument for doing 0), add new absolute font size selector, but keep existing increase/decrease buttons as-is while we're working towrads a more flexible and sustainable solution with CSS font sizes.

> My preference would be 2) with disabling the buttons when on unsuitable
> range (multiple sizes). Then 3) 0) 1).

Comment 122

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #116)
> They will actually see the new
> button that has less options (steps) and hate it from day 1.
Since Thomas commented on it:
I think more savvy users will *love* the new button, since it gives them a quick size control in the toolbar without having to visit the format menu. I bet that many users have never used the "Format > Size" menu, so they will discover a way to make copied text look consistent (see comment #104).

Some users who know a little bit about HTML (the product as it stands confronts them with HTML options here and there), might actually ask themselves what the difference between the increase/decrease and the "Format > Size" function is and go discovering.

The new button which makes a good useful function more accessible and visible, is a good idea. What should happen to the existing increase/decrease functionality is the question. Changing it doesn't bring more functionality, since the new stepwise increase/decrease (option 2) can already be achieved by selecting the next size up/down (but then I already said that, sorry).

Comment 123

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #122)
> (In reply to :aceman from comment #116)
> > They will actually see the new
> > button that has less options (steps) and hate it from day 1.
> Since Thomas commented on it:
> I think more savvy users will *love* the new button, since it gives them a
> quick size control in the toolbar without having to visit the format menu. I
> bet that many users have never used the "Format > Size" menu, so they will
> discover a way to make copied text look consistent (see comment #104).

+1. In case I have been misunderstood, I think the new button is cool and worthwhile, and should definitely be added.

> The new button which makes a good useful function more accessible and
> visible, is a good idea. What should happen to the existing
> increase/decrease functionality is the question. Changing it doesn't bring
> more functionality, since the new stepwise increase/decrease (option 2) can
> already be achieved by selecting the next size up/down (but then I already
> said that, sorry).

That's not exactly true. Even if were to change the existing increase/decrease buttons to only use the very limited set of 6 fixed sizes, keeping them in addition to the absolute font size selector does offer functional benefits in terms of ux-efficiency. Compare:

* 1 click to change font size from increase/decrease
* Existing keyboard shortcuts for increase/decrease (Ctrl+> / Ctrl+<)

vs.

* 2 clicks to change font size from absolute font selector
* No keyboard shortcut for absolute font selector (yet)

Nobody ever complained about increase/decrease buttons, so we better keep them instead of annoying those who are using them, without much benefit on our side.

Comment 124

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with new button for "absolute font" with dropdown (Windows) - take 2

For backward compatibility it is probably better to show the "Smaller"/"Larger" items on the menu (just as they show on the "Format > Size" menu), so users can see when they have been used to modify text size.

Comment 125

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #122)
> Changing it doesn't bring
> more functionality, since the new stepwise increase/decrease (option 2) can
> already be achieved by selecting the next size up/down.
Looks like I've been misunderstood.

I said that *changing* them from the current behaviour (which allows many more sizes using the <small>/<big> tags) to doing the "step-wise thing" does not offer more functionality. The "step-wise thing" can be achieved already, but as Thomas said, with more clicks and no keyboard shortcut.

Option #0 offers more functionality than option #2. Functionality-wise options #1, #2 and #3 produce the same result, albeit with more clicks.

Comment 126

[Magnus Melin [:mkmelin]]

I don't think you should worry too much about "existing users" of this button. It's not like they're stripped of a valuable feature - it's just slightly adjusted, and it's a feature not used a lot anyway. We shouldn't keep things just for the sake of it, and that it produces awful markup is a good reason to remove it.

Comment 127

[Jorg K (CEST = GMT+2)]

Magnus, are you arguing for option #1 (removal) or option #2 (change to step-wise)?
Sounds like option #2 since you're saying "slightly adjusted".

Comment 128

[Magnus Melin [:mkmelin]]

Well, mostly just arguing against #0 (to keep producing messy code for questionable reasons).

Comment 129

[Jorg K (CEST = GMT+2)]

OK, looks like option #2 has got the most votes then. I just need to get the SM people to give their opinion before I start.

Comment 130

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #126)
> I don't think you should worry too much about "existing users" of this
> button. It's not like they're stripped of a valuable feature - it's just
> slightly adjusted, and it's a feature not used a lot anyway.

I wonder about Magnus secret source of knowledge about how valuable a feature is to users and how often it is used?

> We shouldn't keep things just for the sake of it, and that it produces awful markup is a
> good reason to remove it.

Well, offering the full range of font sizes instead of a tiny range of only 6 rather small sizes (max 24 pt because Jorg was doesn't implement size 7 which would be 36pt at least), is not just keeping things for the sake of it. I'm also for removing the awful markup, but there's no rush to do that now.
Why not wait so that we might get round to implementing CSS font sizing?

Comment 131

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #130)
> (In reply to Magnus Melin from comment #126)
> > I don't think you should worry too much about "existing users" of this
> > button. It's not like they're stripped of a valuable feature - it's just
> > slightly adjusted, and it's a feature not used a lot anyway.
> 
> I wonder about Magnus secret source of knowledge about how valuable a
> feature is to users and how often it is used?

It's just my guess of course.

> > We shouldn't keep things just for the sake of it, and that it produces awful markup is a
> > good reason to remove it.
> 
> Well, offering the full range of font sizes instead of a tiny range of only
> 6 rather small sizes (max 24 pt because Jorg was doesn't implement size 7
> which would be 36pt at least), is not just keeping things for the sake of
> it. 

One certainly could argue it's exactly that. What exactly would people use 36pt+ fonts for, in emails??

Comment 132

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with new button for "absolute font" with dropdown (Windows) - take 3

Since it looks like we're removing the <small>/<big> stuff by implementing option #2, how would we indicate that the user is answering an "old" e-mail that still contains those tags? My suggestion shown here is adding a disabled item to the end of the menu showing the tag (or some other text).

Comment 133

[Magnus Melin [:mkmelin]]

Why does it have to be indicated?

Comment 134

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of modified size menu - take 2

I'd also like to modify the existing "Format > Size" menu to indicate the usage of the <small>/<big> tags should the user come across them in some pre-existing e-mail or e-mail sent with a version of TB which still creates those tags (like Linux takes ages to update).

(In reply to Magnus Melin from comment #133)
> Why does it have to be indicated?
Even after phasing out the usage of those tags, there still will be some of them in existing e-mail or e-mail sent with a previous version of TB. There should be some sort of indicator to show the user what's going on, so they realise - if they're really interested - that the previous BIG font they could achieve and that they no longer can achieve used a different technique.

The last item on the menu is only put there when an "old" tag is detected, "normal" users who write new e-mail will not see this feature.

Without the indicator, pre-existing <big><big><big> text would just show as "medium" and the user would scratch their head. Don't you think?

Comment 135

[Magnus Melin [:mkmelin]]

Indeed there could be some slight room for confusion, but wouldn't it be better just to not indicate current size for that case?

Comment 136

[Jorg K (CEST = GMT+2)]

Once again, if you had written more than one sentence, I could perhaps understand what you're saying ;-)
"Current size"??

I considered adding a "(+)" or "(-)" to the detected size, so the menu would show:

  tiny
  small
* medium (+)
  large
  extra large
  huge

in case of a <big> modifier used on a "medium" size. But that doesn't fly on a mixed size selection, since no size is indicated as selected and I don't know where to place the "(+)". Hence my current proposal. Actually, with a mixed selection you can also produce a "<small> and <big>" at the same time :-(

We had a font indicator that actually didn't show the selected font, and we fixed that, so let's have a size indicator which is useful.

Comment 137

[Magnus Melin [:mkmelin]]

"Current size" indication == the menu radio button (the star in your menu in comment 136).
My proposal was not to show that radio button for big/small. Ideally selecting a size would then wipe the big/small tag in the process.

Comment 138

[Jorg K (CEST = GMT+2)]

Selecting a new size will wipe big/small and all pre-existing sizes, even in a mixed selection. That works now:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#822

The problem is: Where do I indicate the use of the old tags.

  tiny
  small
* medium (+)
  large
  extra large
  huge

won't work for mixed selections, therefore I suggested

  tiny
  small
* medium (+)
  large
  extra large
  huge
* <small> (disabled)

Can you please *spell out* your suggestion in no uncertain terms. Just add the <small> at the end without the radio button selected?? Also possible, but doesn't indicate a selection.

Comment 139

[Magnus Melin [:mkmelin]]

My suggestion is to not indicate a size if there's big/small or some mix. So you'd just show the list but NOT have any radio button next to medium (in your example).  You'd just see

  tiny
  small
  medium
  large
  extra large
  huge

... and be forced to suggest one.
If it looks wrong the user will select a better size.

Comment 140

[Jorg K (CEST = GMT+2)]

Well, my suggestion is to indicate the use of big/small to not confuse users (see comment #134 and comment #135). Mixed sizes are currently indicated by "no selection" (no radio button).

The use of big/small is currently indicated in the "Format > Size" menu via the 
  Smaller Ctrl+ <
  Larger Ctrl+ >
items. You get

  Smaller Ctrl+ <
* Larger Ctrl+ >
-----
  x-small
  small
  medium
  large
  x-large
* xx-large

if you augment an "xx-large".

Since this is existing functionality and helps clear confusion, I'd like to indicate it as shown in attachment 8674559 [details] and attachment 8674731 [details].

If you have a better idea, please let me know.

Comment 141

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of toolbar with new button for "absolute font" with dropdown (Windows) - take 4

Here another version: The "old" tags are indicated, for example, with
 HTML: <small>.

Comment 142

[Jorg K (CEST = GMT+2)]

Attachment: Part 2: XUL, CSS and some JS changes for new toolbar button (v5).

This is my current proposal for Part 2, the new button.
Part 3 will follow. Those who want to try, please apply attachment 8671863 [details] [diff] [review] from bug 1197687 first.

Comment 143

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: Changed behaviour of font size increase/decrease buttons (v1)

Here we have Part 3 which implements option #2: The increase/decrease buttons set the next size up or down. The buttons are not available for mixed selection and when the size is already at the end of the range.

Patch apply order:
Attachment 8671863 [details] [diff] from bug 1197687 (not my fault that nothing lands these days).
Then Part 1, 2 and 3 in that order.

Comment 144

[Jorg K (CEST = GMT+2)]

Oops, function getFontSizeIndex(anyHas), the "out" parameter anyHas is not needed, I'll remove in the next round.

Comment 145

[:aceman]

Comment on attachment 8675392 [details] [diff] [review]
Part 3: Changed behaviour of font size increase/decrease buttons (v1)

Review of attachment 8675392 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this seems to work for me.

::: editor/ui/composer/content/ComposerCommands.js
@@ +3112,5 @@
> +  var sizeWasFound = false;
> +
> +  var fontSize = EditorGetTextProperty("font", "size", null, firstHas, anyHas, allHas);
> +  // dump(fontSize + " " + anyHas.value + " " + allHas.value + "\n");
> +  var setIndex = -1;

Would you be allowed to convert to 'let' when touching code in editor? The same in the new functions below.

@@ +3148,5 @@
> +    case "+3":
> +    case "+4":
> +    case "6":
> +    case "7":
> +      // xx-large/

What is the trailing / ?

@@ +3153,5 @@
> +      setIndex = 5;
> +    }
> +  }
> +
> +  sizeWasFound = anyHas.value | (setIndex >= 0);

Should this be boolean || ?

::: mail/components/compose/content/editorOverlay.xul
@@ +331,2 @@
>                    type="radio"
>                    name="1"/>

It seems to me these 2 items do not need to be radio anymore. They are never kelpt selected (in contrast to old behaviour) when you reopen the menulist. They just change the size and that is properly indicated by ticking one of the items later in this menulist.

OK, correction. When a range with multiple sizes is selected, the "Smaller" menuitem is ticked, but disabled. That probably needs to be fixed. That is probably your question what to select on multi-value range. Either nothing or some special item "(mixed)". I would vote for the "mixed" item to get the user some indication why the items are disabled. Otherwise old users will wonder why the buttons/items are suddenly disabled while before they never were.

Comment 146

[Richard Marti (:Paenglab)]

Comment on attachment 8675030 [details] [diff] [review]
Part 2: XUL, CSS and some JS changes for new toolbar button (v5).

LGTM

Comment 147

[Richard Marti (:Paenglab)]

Comment on attachment 8675392 [details] [diff] [review]
Part 3: Changed behaviour of font size increase/decrease buttons (v1)

I think it's good how the buttons work now.

And yes, I think too on mixed selection there should be some feedback like a disabled "mixed" or "mixed sizes" menuitem to give feedback why no size is selected and the increase/decrease buttons are disabled.

Comment 148

[Jorg K (CEST = GMT+2)]

Attachment: Part 3: Changed behaviour of font size increase/decrease buttons (v1b)

Carrying forward Richard's ui-r+ and Aceman's f+.

Thank you for the quick reply, gentlemen.

As for the questions:
- I'll continue with "var" in that file unless instructed otherwise.
- Trailing slash should have been a full stop.
- || corrected.
- The size menu has two groups:
  1: the six sizes
  2: the additional info: HTML: <small> or HTML: <big> (or both)
     as per comment #141 (see attachment 8674996 [details]).
     The item is selected since this style is applied.
     The item is disabled since it serves as information only.
     Both choices are intentional.
     Please use an "old" e-mail (attachment 8672410 [details]) to try it.

As for the question what should happen on a mixed size selection.
The existing functionality doesn't show anything selected and I haven't changed that.
Also, Magnus asked for that in #139. I've considered a "(mixed)" indicator, but later discarded that idea. A mixed selection is clearly indicated by "no selection".

The only additional information I am providing is to show HTML: <small><big> as an additional indicator. Magnus doesn't like that either (comment #139), but I thinks it's necessary. "HTML" is international, so no translation there.

Comment 149

[Jorg K (CEST = GMT+2)]

Comment on attachment 8672352 [details] [diff] [review]
Part 1: New icons.

Who should approve the icons?

Comment 150

[:aceman]

(In reply to Jorg K (GMT+2) from comment #148)
>   2: the additional info: HTML: <small> or HTML: <big> (or both)
>      as per comment #141 (see attachment 8674996 [details]).
>      The item is selected since this style is applied.
>      The item is disabled since it serves as information only.
>      Both choices are intentional.
>      Please use an "old" e-mail (attachment 8672410 [details]) to try it.

I used a new composition to try out the buttons and I still got "smaller" radio selected in the menu. I do not see what information this conveys. No "smaller" stile is applied, just a mixed size is selected.

Comment 151

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #150)
> I used a new composition to try out the buttons and I still got "smaller"
> radio selected in the menu. I do not see what information this conveys. No
> "smaller" stile is applied, just a mixed size is selected.

Are you talking about the "Smaller" (or "Larger") item on the "Format > Size" menu?
They should never be selected. (They used to be selected to indicate HTML: <small> or <big>.)
If they are selected, that's a bug. Please submit a screenshot and the saved draft and tell me what was selected so I can reproduce it. (You can send me a private message.)

Comment 152

[:aceman]

(In reply to Jorg K (GMT+2) from comment #151)
> Are you talking about the "Smaller" (or "Larger") item on the "Format >
> Size" menu?
Yes, exactly.

> They should never be selected. (They used to be selected to indicate HTML:
> <small> or <big>.)
> If they are selected, that's a bug.
They are. The first item "Smaller" is selected for me and disabled. I also think it does not look correct.

> Please submit a screenshot and the saved
> draft and tell me what was selected so I can reproduce it. (You can send me
> a private message.)
I can't make screenshot right now, but here is what I did:
1. applied all 4 patches (3 here and 1 from the other bug).
2. composed new msg in HTML.
3. applied various font sizes using all the 3 buttons.
4. selected a range that contained multiple sizes.

Results:
1.The smaller/bigger toolbar buttons got disabled (correct)
2.In "Format > Size" menu the "Smaller" menuitem is selected and disabled (selecting it seems wrong)

Comment 153

[Jorg K (CEST = GMT+2)]

I'm seeing that too now. I'll fix. Thanks.

Comment 154

[Jorg K (CEST = GMT+2)]

Bug happened when you used "Smaller" or "Larger" from the menu, then selected a mix and visited the menu again. The previous choice, say "Smaller", was sadly not cleared and stayed selected. Fixed now.

Comment 155

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v6).

Carrying forward Richard's ui-r+ and Aceman's f+.

This is the unified patch for Part 2 (new button) and Part 3 (changed increase/decrease buttons).

This fixes the problem found by Aceman in comment #152.

Patch apply order:
Attachment 8671863 [details] [diff] from bug 1197687.
Then Part 1 and Part 2+3.

Comment 156

[[e-mail]]

Comment on attachment 8675442 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v6).

>@@ -256,18 +258,18 @@ function goUpdateCommandState(command)
>-      case "cmd_increaseFont":
>-      case "cmd_decreaseFont":
Why remove this? (Or if it's not needed, then the two added lines probably aren't needed either.)

>+// Helper function
>+
>+function getFontSizeIndex()
Belongs in editor.js.

>+  sizeWasFound = anyHas.value || (setIndex >= 0);
>+
...
>+  if (!sizeWasFound)
>+    setIndex = 2;
>+
>+  return setIndex;
This is far too ugly. Just return setIndex as soon as you know what it is. (So in the case of !anyHas.value, that's before you check allHas.value.)

>+      return;
Nit: blank line after return;

>+        children[0].setAttribute("checked", false);
>+        children[1].setAttribute("checked", false);
How do these get checked?

>+      children[lastItem].setAttribute("hidden", true);
Nit: use .hidden

>+    <toolbarbutton id="AbsoluteFontSizeButton"/>
Why is this in editor.xul?

>+        <menuitem id="fontSizeMenu_smallBigInfo"
>+                  type="radio" name="2" disabled="true" hidden="true"/>
Why is this type="radio" instead of "checkbox"?
I keep thinking this needs an opt-in of some sort.

Comment 157

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] from comment #156)
> >@@ -256,18 +258,18 @@ function goUpdateCommandState(command)
> >-      case "cmd_increaseFont":
> >-      case "cmd_decreaseFont":
> Why remove this? (Or if it's not needed, then the two added lines probably
> aren't needed either.)
Please help me out here. What's goUpdateCommandState used for?
We want to discontinue the use of cmd_increaseFont/cmd_decreaseFont, so I removed it and added the new commands.

> >+        children[0].setAttribute("checked", false);
> >+        children[1].setAttribute("checked", false);
> How do these get checked?
The user selects them from the menu. That checks them. That's a problem Aceman found in testing. Before I didn't have the "unchecking" here and it hung around when you next visited the menu with a "mixed" selection.

> >+    <toolbarbutton id="AbsoluteFontSizeButton"/>
> Why is this in editor.xul?
Due to ignorance. I guess, if I remove it, SM won't get that. It's also defined in
mail/components/compose/content/messengercompose.xul

> >+        <menuitem id="fontSizeMenu_smallBigInfo"
> >+                  type="radio" name="2" disabled="true" hidden="true"/>
> Why is this type="radio" instead of "checkbox"?
> I keep thinking this needs an opt-in of some sort.
This meant as an indicator. Previously you had a radio-button in front
  Smaller Ctrl+ <
* Larger Ctrl+ >
(see comment #140).

Now we don't produce these <small><big> tags any more, but we indicate their use in old e-mail, see attachment 8674996 [details].

Once settle these four issues, I'll submit an updated patch.

Comment 158

[Jorg K (CEST = GMT+2)]

I was referring to *my* own ignorance, of course.

I'm not even 100% sure why we define everything twice. In
editor/ui/composer/content/editorOverlay.xul and
mail/components/compose/content/editorOverlay.xul.

I guess, anything put into editor/ui will end up automatically in SM, so I'd need to know from you whether you want this change or whether it should be done in a way that doesn't affect SM.

Comment 159

[Jorg K (CEST = GMT+2)]

And please explain what you mean by "an opt-in of some sort". Or we could have a chat on IRC, if that suits you. Let me know when.

Comment 160

[[e-mail]]

(In reply to Jorg K (GMT+2) from comment #157)
> (In reply to comment #156)
> > >@@ -256,18 +258,18 @@ function goUpdateCommandState(command)
> > >-      case "cmd_increaseFont":
> > >-      case "cmd_decreaseFont":
> > Why remove this? (Or if it's not needed, then the two added lines probably
> > aren't needed either.)
> Please help me out here. What's goUpdateCommandState used for?
Some commands have a state (e.g. font colour) and goUpdateCommandState finds the current state and updates the UI appropriately. However I don't think any of those commands have a state so that block is technically useless, except in as much that if someone does accidentally try to update the state then it gets ignored rather than dumping a warning message to the terminal console.

> We want to discontinue the use of cmd_increaseFont/cmd_decreaseFont, so I
> removed it and added the new commands.
It's probably possible to emulate those commands in JavaScript, which would make those core editor hackers happier, but without completely removing the functionality.

> > >+        children[0].setAttribute("checked", false);
> > >+        children[1].setAttribute("checked", false);
> > How do these get checked?
> The user selects them from the menu. That checks them.
Ah of course. Well, I guess you could fiddle around with autocheck="false" instead, or you could just uncheck all the children.

> > >+    <toolbarbutton id="AbsoluteFontSizeButton"/>
> > Why is this in editor.xul?
> Due to ignorance. I guess, if I remove it, SM won't get that.
We'd need theming for it, so it would be unhelpful to have it without.

> > >+        <menuitem id="fontSizeMenu_smallBigInfo"
> > >+                  type="radio" name="2" disabled="true" hidden="true"/>
> > Why is this type="radio" instead of "checkbox"?
> This meant as an indicator.
Why doesn't a checkbox work as an indicator?

By "opt-in" I mean SeaMonkey Compose might not want an HTML: <big> indicator.

Comment 161

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7).

Carrying over Aceman's and Richard's f+.

In this patch I've changed the following based on Neil's input:
- getFontSizeIndex() moved to editor.js and "prettified" ;-)
- used autocheck="false" to not check the "Larger" and "Smaller" items on the menu
  (to avoid having to clear them later).

Some issues still remain since I haven't received clear instructions ;-)
1) goUpdateCommandState() still has the cases "cmd_increaseFontStep" and
   case "cmd_decreaseFontStep". I can of course remove them.
   Please indicate: Remove: Yes/No.
2) <toolbarbutton id="AbsoluteFontSizeButton"/> left in editor.xul.
   I'm not sure whether I understood the answer correctly:
   "it would be unhelpful to have it without" - sounds like "don't remove".
   Please indicate.
3) children[lastItem].setAttribute("hidden", true); should use .hidden.
   Only .hidden, or also .label and .checked? There are subtle differences.
   Please indicate.
4) HTML: <small><big> is still a radio button, simply because previously
   this was indicated as a radio button in front of the "Larger"/"Smaller" menu item.
   Richard liked it. I'm sure a check box would work as well.
   I will submit a screenshot with a checkbox in a minute (I hope).
   Do you have a preference for a check-box? May I ask why, since we never had one.
5) No opt-in for this yet. I can imagine the following:
   SM doesn't put this menuitem id="toobarmenu_fontSize_smallBigInfo" on the menu
   and we check its presence before setting it. Would that be a way?

Neil, how can we proceed with this?

Comment 162

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot of <small><big> indicator as checkbox instead of radio button.

This is how it would look like as a checkbox. I'm not sure why Neil asked for a checkbox. In any case, the indicator will only be shown when an "old" e-mail is replied to that contains such a <small> or <big> tag.

Comment 163

[Richard Marti (:Paenglab)]

Comment on attachment 8676922 [details]
Screenshot of <small><big> indicator as checkbox instead of radio button.

The "HTML: <big>" can be shown together with the other size items. Then it makes sense to use the checkmark as it doesn't exclude the setting of the other items and is a additional size indicator.

If only the "HTML: <big>" and no other size could be selected, then the radio would be the right one.

Comment 164

[Jorg K (CEST = GMT+2)]

OK, we go for checkboxes. I'll submit a new patch, if I can also address some more issues. I'll repeat the open issues:

1) goUpdateCommandState() still has the cases "cmd_increaseFontStep" and
   case "cmd_decreaseFontStep". I can of course remove them.
   Please indicate: Remove: Yes/No.
2) <toolbarbutton id="AbsoluteFontSizeButton"/> left in editor.xul.
   I'm not sure whether I understood the answer correctly:
   "it would be unhelpful to have it without" - sounds like "don't remove".
   Please indicate.
3) children[lastItem].setAttribute("hidden", true); should use .hidden.
   Only .hidden, or also .label and .checked? There are subtle differences.
   Please indicate.

5) As for the "opt-in" for SM: Maybe SM can just not put the checkbox on the menu.
   The code could check whether the presence of the checkbox. Would that work?

Comment 165

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7b).

Carrying forward Aceman's f+ and Richard's ui-r+.
In this patch I've changed the <big>/<small> indicator to checkboxes.

Magnus, while we're waiting for Neil's comments on the remaining issues (see previous comment), perhaps you can already take a look at this.

Patch apply order:
Attachment 8671863 [details] [diff] from bug 1197687.
Then Part 1 and Part 2+3.

Comment 166

[Magnus Melin [:mkmelin]]

Comment on attachment 8678548 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7b).

Review of attachment 8678548 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately this has bitrotted, so I didn't try running with it yet

::: editor/ui/composer/content/editor.js
@@ +983,5 @@
> +  // can imply "medium" and "CSS attribute present" which should not
> +  // imply "medium".
> +  if (!anyHas.value)
> +    return 2;
> +  

nit: spurious whitespace

@@ +992,5 @@
> +  switch (fontSize)
> +  {
> +  case "-3":
> +  case "-2":
> +  case "0":

nit: all of these should be indented 2 space more

@@ +1074,5 @@
> +      htmlInfo = "<small>";
> +    if (bigFound)
> +      htmlInfo += "<big>";
> +
> +    if (htmlInfo != "")

if (htmlInfo)

@@ +1083,2 @@
>      }
> +    else

nit: braces

Comment 167

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7c).

Carrying forward Aceman's f+ and Richard's ui-r+.

Fixed three nits (white-space, indentation, if).
In this source they have a different coding style and allow
if (foobar)
  doX();

The patch was NOT bitrotted.
Pleas apply Attachment 8671863 [details] [diff] from bug 1197687 first! (see comment #165).

Comment 168

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #167)
> In this source they have a different coding style and allow
> if (foobar)
>   doX();

Yes, but if one if-else have a braces they all should. (That's the style.)

Comment 169

[Jorg K (CEST = GMT+2)]

OK, I'll fix it.

So far I've adhered to the published Mozilla style (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style), but then got told (bug 1197687 comment #4):
  It seems the style for this file is to have { and } on their own separate lines.

So I'm really confused. Anyway, I'll add the braces and fix whatever else you find.
In the meantime you can run it, it will run without the brace ;-)

Comment 170

[Magnus Melin [:mkmelin]]

Comment on attachment 8680244 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7c).

Review of attachment 8680244 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't think the "<big>" menu is just confusing, but either way. r=mkmelin

::: editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
@@ +81,5 @@
>  <!ENTITY incrementFontSize.key2 "."> <!-- > is above this key on many keyboards -->
>  
>  <!ENTITY fontSizeMenu.label "Size">
>  <!ENTITY fontSizeMenu.accesskey "z">
> +<!ENTITY size-tinyCmd.label "tiny">

shouldn't we capitalize as Tiny, Medium etc.

Comment 171

[Magnus Melin [:mkmelin]]

Eh, that was supposed to be: I still think the "<big>" menu is just confusing

Comment 172

[Jorg K (CEST = GMT+2)]

Magnus, thanks for the quick review. Now I'm waiting for Neil. The issues to discuss with him are in comment #164. And thanks for reviewing the icons!

Issues from your review:
- Missing {} on one else.
- You don't like the checkbox indicator HTML: <big><small>. I think it's useful.
- Capitalization of Tiny, Small, Medium, etc.
  That's what Blake said in comment #26. To use these strings:
  https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#270-274

I'll ask Richard about the capitalization. Richard?

Comment 173

[Richard Marti (:Paenglab)]

All other menu items are capitalized, so it makes sense to do this here too. But to be consistent this should be done on every place this sizes are used.

Comment 174

[Jorg K (CEST = GMT+2)]

Will do. However, the sizes are currently lowercase.

Comment 175

[Axel Grude [:realRaven]]

Guys, while this bug is fixed, can we make the keyboard shortcuts less greedy? At the moment all CTRL+<, CTRL+SHIFT+<, CTRL+ATL+<  are triggering font size which is not good for my Addon ZombieKeys. I tried removing the shortcuts at runtime but it always left composer in an inconsistent state.

Comment 176

[Jorg K (CEST = GMT+2)]

(In reply to Axel Grude [:realRaven] from comment #175)
> Guys, while this bug is fixed, can we make the keyboard shortcuts less
> greedy?
Let's not mix two issues into one bug. Please raise another bug for this.

Comment 177

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v7d).

Carrying forward Magnus' r+ and Richard's ui-r+.
(How do I put r+ and r? at the same time?)

In this patch: Added the missing braces, capitalised the sizes: Tiny, Small, etc.

Please apply attachment 8671863 [details] [diff] [review] from bug 1197687 first.

Copying open issues (from comment #164):

1) goUpdateCommandState() still has the cases "cmd_increaseFontStep" and
   case "cmd_decreaseFontStep". I can of course remove them.
   Neil, please indicate: Remove: Yes/No.
2) <toolbarbutton id="AbsoluteFontSizeButton"/> left in editor.xul.
   I'm not sure whether I understood the answer correctly:
   "it would be unhelpful to have it without" - sounds like "don't remove".
   Neil, please indicate.
3) children[lastItem].setAttribute("hidden", true); should use .hidden.
   Only .hidden, or also .label and .checked? There are subtle differences.
   Neil, please indicate.

5) As for the "opt-in" for SM: Maybe SM can just not put the checkbox on
   the menu. The code could check whether the presence of the checkbox.
   Neil: Would that work?

Comment 178

[[e-mail]]

Sorry for taking so long to get around to this; birthdays, chess matches and orchestral concerts have conspired to reduce my availability over the last few weeks.

(In reply to Jorg K from comment #161)
> 1) goUpdateCommandState() still has the cases "cmd_increaseFontStep" and
>    case "cmd_decreaseFontStep". I can of course remove them.
I'd rather you restored cmd_[in/de]creaseFont.

> 2) <toolbarbutton id="AbsoluteFontSizeButton"/> left in editor.xul.
>    I'm not sure whether I understood the answer correctly:
>    "it would be unhelpful to have it without" - sounds like "don't remove".
No, it means the button would be unhelpful without theming, so please remove it.

> 3) children[lastItem].setAttribute("hidden", true); should use .hidden.
>    Only .hidden, or also .label and .checked? There are subtle differences.
Only hidden, please. (I can't remember whether .label and .checked are safe on the Mac.)

> 4) Do you have a preference for a check-box?
[Answered in comment #163]

> 5) No opt-in for this yet. I can imagine the following:
>    SM doesn't put this menuitem id="toobarmenu_fontSize_smallBigInfo" on the
>    menu and we check its presence before setting it. Would that be a way?
That would be fine. (Otherwise, I think it hiding it in suite/common/communicator.css might work.)

Comment 179

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8).

Addressed Neil;s issues:
1) Restored cmd_[in/de]creaseFont
2) Removed new button from editor/ui/editor.xul and
   removed "info" items from editor/ui/editorOverlay.xul.
   These now only get added for TB in
     mail/components/compose/content/editorOverlay.xul and
     mail/components/compose/content/messengercompose.xul
3) Used .hidden as indicated.
4) -
5) Opt-in option tests for presence of "smallBigInfo" menu items.
   Reshuffled initFontSizeMenu() quite a bit.

Comment 180

[Jorg K (CEST = GMT+2)]

Comment on attachment 8687376 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8).

I'm cancelling r? for Neil.

Magnus, can you please review this again. You gave r+ to version 7c of the patch in comment #170, now I'm at version 8 which addresses Neil's wishes from comment #178.

You could try an interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8680244&action=interdiff&newid=8687376&headers=1

Basically, since you've last looked at it I changed the following:
- Put case "cmd_increaseFont"/"cmd_decreaseFont" back, since Neil wanted it.
- Commented out some stuff that S/M might not want right now.
- Reshuffled initFontSizeMenu() quite a bit to cater for the last menu item
  being present or not.
- Capitalised the labels.

Comment 181

[[e-mail]]

Comment on attachment 8687376 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8).

r=me on the editor/suite parts.

>+      // In case of mixed, clear all items.
>+      var loopTo = hasSmallBigInfo ? lastItem - 1 : lastItem;
>+      for (var i = menuIndex; i <= loopTo; i++)
(Actually you can just loop to lastItem because you set the checked attribute on the smallBigInfo item anyway. Also I have a slight preference for menupopup.lastChild instead of children[lastItem], so you would have to use < children.length instead if you did that.)

Comment 182

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8b).

Carrying forward Magnus' r+ from comment #170.
Carrying forward Neil's r+ from comment #181.
Carrying forward Richard's ui-r+ from comment #146, comment #147 and comment #163.

Neil, thank you for the review. Your suggestions make initFontSizeMenu() more elegant, simpler to read and kill three variables (lastItem, loopTo and hasSmallBigInfo). So of course I implemented them.

Comment 183

[Magnus Melin [:mkmelin]]

Comment on attachment 8687376 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8).

Review of attachment 8687376 [details] [diff] [review]:
-----------------------------------------------------------------

Let's land this. Just a couple of minor nits. r=mkmelin

On a side note: you can put this in [hooks] in your .hgrc to notice extra whitespace on qrefresh:

post-qrefresh.whitespace = hg qdiff | (! egrep --color -C 2 -n '^\+.*\s$')

::: editor/ui/composer/content/editor.js
@@ +1049,5 @@
> +    else
> +    {
> +      // In case of mixed, clear all items.
> +      var loopTo = hasSmallBigInfo ? lastItem - 1 : lastItem;
> +      for (var i = menuIndex; i <= loopTo; i++)

please always use braces for loops

@@ +1052,5 @@
> +      var loopTo = hasSmallBigInfo ? lastItem - 1 : lastItem;
> +      for (var i = menuIndex; i <= loopTo; i++)
> +        children[i].setAttribute("checked", false);
> +    }
> +    

trailing space here

Comment 184

[Jorg K (CEST = GMT+2)]

Attachment: Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8c).

Oh, I had the luxury of two reviews concurrent reviews ;-)
To satisfy the second one, I added braces were requested.
At the same time I've s/contains/includes/ since the former is deprecated.

Comment 185

[Jorg K (CEST = GMT+2)]

This is ready to land. For approvals, please see comment #182 and comment #183.
This has two patches, "Part 1" and "Part 2+3".

Comment 186

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/40dcbc90e922064f67c3d6234c99a3cc8efc8bce
Bug 769604 - Provide new font size selection button, change internals of increase/decrease buttons. r=neil,mkmelin

Comment 187

[aleth [:aleth]]

I assume patch (v8) is obsolete.

Comment 188

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/0c301942fbe96d02b431d060974bb260fcb3dad2
Bug 769604 - New icons for the font size chooser. r=mkmelin

Comment 189

[Jorg K (CEST = GMT+2)]

Comment on attachment 8687376 [details] [diff] [review]
Part 2+3: Full solution: New toolbar button and changed increase/decrease buttons (v8).

> I assume patch (v8) is obsolete.
Yes.