The font menu (Format > Font) doesn't update when "Variable Width" text is selected. For the record: Previous font menu/font indicator bugs were: Bug 1139524, bug 1148330 and bug 1140105. This bug was detected while working on bug 1148790, see bug 1148790 comment #42. The cause is (editor/ui/composer/content/editor.js): if (!anyHas.value) EditorGetTextProperty("font", "face", "", firstHas, anyHas, allHas); since "" is phased out and "variable width" needs to be queried now as "sans-serif" or "serif".

This reworks the font menu. What do we do to recognise stuff like <font face="Comic Sans MS,sans-serif" size="3" color="#7030A0"> (real world example)? Do you want another bug for that? I initially had the idea to strip ",serif", ",sans-serif" and ",monospace", but sadly there are more: (http://www.w3schools.com/cssref/pr_font_font-family.asp). There are also "cursive" and "fantasy". There is also the case where there are two "real" fonts mentioned, like: <font face="Times New Roman,Georgia,Serif"> One option would be to split the string and process the fonts individually. Sadly we have <font face="Helvetica, Arial, sans-serif"> as a standard, and there we don't want to process Helvetica and Arial separately. What's your opinion on this issue?

Comment on attachment 8651640 [details] [diff] [review] Proposed solution (v1). I'm not sure about Neil's availability. Just for the record, the idea used here comes from Neil's attachment 8591403 [details] [diff] [review] from another bug.

Sorry to be impatient. Could one of you guys review this before the next branch date (21st of Sept.), so it can go into version 43.

Comment on attachment 8651640 [details] [diff] [review] Proposed solution (v1). Some general comments: * It seems the style for this file is to have { and } on their own separate lines * I would say that "state" is not a good name for the variable, perhaps "face" or "type" * Is it good practise to keep re-declaring the variable menuItem in the for loop? The patch itself seems to do the right thing, but Neil is probably the best person to review it. f+ for the moment

(In reply to Ian Neal from comment #4) > * It seems the style for this file is to have { and } on their own separate > lines Agreed. > * I would say that "state" is not a good name for the variable, perhaps > "face" or "type" Copied from Neil's own patch - attachment 8591403 [details] [diff] [review] > * Is it good practise to keep re-declaring the variable menuItem in the for > loop? That's existing code. Neil didn't change it in his attachment 8591403 [details] [diff] [review]. Is this really an issue? JS should cope with that, shouldn't it? I'd blame it on the original author ;-)

Comment on attachment 8651640 [details] [diff] [review] Proposed solution (v1). So, I guess this is a little like onFontFaceChange, except it applies when the menu opens rather than when the font face changes (duh!), and it has to do things slightly differently because it's a radio menu rather than a menulist. I can't remember the history of the name="1"/name="2" distinction (CVS is no more, sadly) but as we're not making any distinction any more then I think we can just remove all of the names from those menuitems, which means that we can stop at the first checked menuitem. >+ if (mixed.value) { >+ menuItem.removeAttribute("checked"); >+ } else { >+ var faceType = menuItem.getAttribute("value").toLowerCase() >+ .replace(/, /g, ","); >+ if (faceType == state) { >+ menuItem.setAttribute("checked", "true"); >+ } else { >+ menuItem.removeAttribute("checked"); >+ } > } >- >- // in case none match, make sure we've cleared the checkmark >- menuItem.removeAttribute("checked"); This is a little ugly; ideally it would be something like this: if (!mixed.value) { var faceType = menuItem.getAttribute("value").toLowerCase() .replace(/, /g, ","); if (faceType == state) { menuItem.setAttribute("checked", "true"); break; } } // in case none match, make sure we've cleared the checkmark menuItem.removeAttribute("checked");

I'm in Spain with my travel laptop for three weeks. Would you like to take over and make the changes you like, after all, this is based on your original idea.

Brief summary in case you're not up-to-date: EditorGetTextProperty used to be able to detect that no font had been chosen, or the font had been reset back to variable width. This no longer works, probably due to web compatibility. The alternative call is getFontFaceState which will tell us if there's exactly one font in use and if so what it is, however it itself suffers from some flaws which make it harder to use. The first flaw is that it sometimes returns "serif" or "sans-serif" for text in the default font, or "monospace" for text inside a "tt" block. The second flaw is that it returns the font in an internal canonicalised form, rather than the actually specified font. Also since the first two menuitems are no longer being treated separately I simply removed all of the name attributes as they are no longer necessary.

Thanks!

Comment on attachment 8662693 [details] [diff] [review] Possible patch Review of attachment 8662693 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me

Can we check this in then? If I set "checkin-needed", Neil tells me that he has Level 3 rights ;-)

So why did Neil not r+ this? Was that a mis-click? You need both reviews from mkmelin and Neil here.

It's Neil's patch, Magnus reviewed it. I have nothing to do with it any more.

Ah OK, tne who will review the /editor parts instead of Neil?:)

Neil's patch (mostly in /editor) was reviewed by Magnus, and as far as I can see it, is ready for checkin. If Neil wants more reviews, he'd have to ask for them.

Carrying over Magnus' r+ I've refreshed this patch and tested it. Can we please land this now before it rots again. We've had four people look at this now (in order of appearance): Myself, Ian (f+), Neil (fixed the bits he didn't like in my patch), Magnus (r+), Myself. This is well and truly ready for check-in.

https://hg.mozilla.org/comm-central/rev/a948473f4ed9fd6096202a2f5f7cbae9e5018f89 Bug 1197687 - Reworking the font menu. r=mkmelin,jorgk a=aleth CLOSED TREE