Font menu doesn't update when "Variable Width" text is selected.

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jorg K (GMT+2), Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Thunderbird 45.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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".
(Reporter)

Comment 1

2 years ago
Created attachment 8651640 [details] [diff] [review]
Proposed solution (v1).

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?
Assignee: nobody → mozilla
Attachment #8651640 - Flags: review?(neil)
(Reporter)

Comment 2

2 years ago
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.
Attachment #8651640 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 3

2 years ago
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.
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)

Comment 4

2 years ago
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
Flags: needinfo?(iann_bugzilla)
Attachment #8651640 - Flags: review?(iann_bugzilla) → feedback+
(Reporter)

Comment 5

2 years ago
(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 ;-)
(Assignee)

Comment 6

2 years ago
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");
Flags: needinfo?(neil)
(Reporter)

Comment 7

2 years ago
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.
Flags: needinfo?(neil)
(Assignee)

Comment 8

2 years ago
Created attachment 8662693 [details] [diff] [review]
Possible patch

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.
Flags: needinfo?(neil)
Attachment #8662693 - Flags: review?(mkmelin+mozilla)
(Reporter)

Updated

2 years ago
Attachment #8651640 - Attachment is obsolete: true
Attachment #8651640 - Flags: review?(neil)
(Reporter)

Comment 9

2 years ago
Thanks!

Comment 10

2 years ago
Comment on attachment 8662693 [details] [diff] [review]
Possible patch

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

Looks reasonable to me
Attachment #8662693 - Flags: review?(mkmelin+mozilla) → review+
(Reporter)

Comment 11

2 years ago
Can we check this in then? If I set "checkin-needed", Neil tells me that he has Level 3 rights ;-)
Flags: needinfo?(neil)

Comment 12

2 years ago
So why did Neil not r+ this? Was that a mis-click?

You need both reviews from mkmelin and Neil here.
Status: NEW → ASSIGNED
(Reporter)

Comment 13

2 years ago
It's Neil's patch, Magnus reviewed it. I have nothing to do with it any more.
Assignee: mozilla → neil

Comment 14

2 years ago
Ah OK, tne who will review the /editor parts instead of Neil?:)
(Reporter)

Comment 15

2 years ago
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.
(Reporter)

Comment 16

2 years ago
Created attachment 8671863 [details] [diff] [review]
Neil's "Possible patch" - rebased and tested

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.
Attachment #8662693 - Attachment is obsolete: true
Flags: needinfo?(neil)
Attachment #8671863 - Flags: review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
You need to log in before you can comment on or make changes to this bug.