1399370 - Allow changing 'Spellcheck language' from status bar display

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #368915 +++

As a followup to bug 368915 where we introduced 'Spell checking language' display in status bar, let's make that more useful so that clicking on the status bar widget will pop up the language selection menu, allowing users to change the language conveniently, as seen in MS Word and my xul demo below.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #215)
> Created attachment 8648711 [details]
> Demo2.xul: statusbar language selector (like MS Word)

You need Remote XUL manager addon to see the demo in FF, then add https://bug368915.bmoattachments.org as a domain with remote XUL permission.

> (In reply to Thomas D. from comment #213)
> > Comment on attachment 8648525 [details]
> > (In reply to :aceman from comment #198)
> > > In MS Word, this is exactly how it works. You can click the current language
> > > in the status bar to switch to another one.
> > 
> > ...and switching the language from status bar in MS Word works like a
> > charme, very convenient because you don't have to search for it between all
> > the other options. So we could just try the same... :)
> 
> I really like that idea, allowing users to change the language from that
> statusbar widget also gives us a better excuse for hijacking the holy status
> bar at all...
> 
> We could also do this in a followup bug, but from coding the demo I'd think
> it might be simple enough to just do it while we are here.
> 
> Features & benefits (as seen in the XUL demo):
> * visually de-emphasise the permanent display of current language in status
> bar (ux-minimalism; ux-visual-hierarchy)

Maybe, or maybe not.

> * buttonize and tooltip on hover: "Choose language for spell check"
> (ux-discovery)

Word 2010 just has "Language" as a tooltip on the status bar widget. But for most other status bar widgets, they've got tooltips like this:
> Zoom level. Click to open the Zoom dialog box.
That's a bit bulky in the long run, and I don't like "Click to..." tooltips. We might add an action verb in the tooltip (as I suggested above), but maybe that's a bit yelling/slightly inappropriate for the 'status display' side of it. So given the hover effect for ux-discovery, I think we might just get away with what we currently have:
> Spell-checking language
But let's please add a hyphen, also in the menus for "Spell-Check As You Type".

Richard?

> * Language selection menu on left or right click, similar to MS Word
> (ux-efficiency; external ux-consistency)

For now, let's just show the language selector on left-click.
We might want other things in the right-click menu.

> * Re-using the existing language menu of the spellcheck dual button (no code
> duplication)

Comment 1

[Jorg K (CEST = GMT+2)]

Clearly you didn't check for duplicates since other people already had this idea.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #1)
> Clearly you didn't check for duplicates since other people already had this
> idea.

Clearly, that comment does not sound appreciative of my effort to get this done, as I'll probably implement this myself.
Clearly, it was only *one* person (not "other people") who had this idea just 2 months ago, and *after* we had "already" had that idea on Bug 368915, but we (me) just didn't get round to filing the follow-up.
Clearly, someone on bug 1383860 was asking the wrong questions ("Do we really need a third way?"), apparently oblivious of the obvious case of ux-efficiency as presented by the user and presenting itself in the large and most discoverable click target on status bar.
So clearly, as you stated, this bug is a better starting point to get this done.
Yes, I forgot to check for duplicates. Clearly. So what? No harm done. Clearly, tone matters.
That said, back to work.

Comment 4

[Thomas D. (:thomas8)]

For the status bar widget tooltip, I guess we don't need -ing form here either, so we could just have:
> Spell-check language
Short and simple, noun style.

Richard?

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #4)
> For the status bar widget tooltip, I guess we don't need -ing form here
> either, so we could just have:
> > Spell-check language
> Short and simple, noun style.

Yes, you can use it like this.

Comment 6

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Implement spellcheck language selector for language status display

Here we go!

Sweet and simple. I also noticed Jörg's really clever observer for the "lang" attribute, which updates the status bar immediately (but not the inline spell check, would that be possible?).

Due to various shortcomings and surprising behaviour in XUL, sharing the menupopup was a bit harder than expected, but with a four-line helper function every consumer can show the same popup correctly.

As a qualified language person, I cracked my head and did some research on
> 'spell check' vs. 'spell-check' vs. 'spellcheck'.
All of them are correct, used both as noun and verb.
In a vast en-US online corpus (1), these are the frequencies:
>'spell check' (36) vs. 'spell-check' (25) vs. 'spellcheck' (15)
This isn't surprising, because 'spell check' is the most generic stage in the development of a compound noun. Hyphens aren't very popular in English, but compound nouns can lose the space over time (2)(3), hence we now have 'policeman', not 'police man'.

So here's why I ultimately chose 'spellcheck':
- What we really want to say is: {Spellcheck}{as you type}, not {spell}{check as you type}. Imo dropping the space reduces logical ambiguity here, and hence arguably the cognitive burden.
- It's one word less for the already long phrase of "Spellcheck As You Type", and makes those strings a little bit shorter :)
- Generic is good, but I think in 2017 'spellcheck' can really be considered a strong and specific compound noun (like policeman). Imo 'spellcheck' is on a different level compared to 'background check', 'fitness check', and 'pulse check'. As a modern application, let's anticipate the future! ;)

(1) https://corpus.byu.edu/coca/
(2) http://www.ef.com/english-resources/english-grammar/compound-nouns/
(3) http://oxfordenglishglobal.com/learn-english-compound-nouns/

Comment 7

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #6)

> Sweet and simple. I also noticed Jörg's really clever observer for the
> "lang" attribute, which updates the status bar immediately (but not the
> inline spell check, would that be possible?).

It's only when changing language in the "Check spelling" dialogue that the inline spell check doesn't update immediately, which would be nice if it did.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #6)
> Created attachment 8908214 [details] [diff] [review]

[Caveat for reviewers]
Oh, to be applied on top of my patch in bug 759039, attachment 8907207 [details] [diff] [review].

Comment 9

[Richard Marti (:Paenglab)]

Comment on attachment 8908214 [details] [diff] [review]
Patch V.1: Implement spellcheck language selector for language status display

UI-r+ for the status bar popup. For the "spellcheck" I'll let other decide with not so poor English knowledge.

Comment 10

[Jorg K (CEST = GMT+2)]

http://app.aspell.net/lookup favours "spellcheck", me too.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8908214 [details] [diff] [review]
Patch V.1: Implement spellcheck language selector for language status display

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

Not a hard review ;-) Works nicely, and in fact, very functional. I guess I have to eat my words from bug 1383860 comment #1 ("Do we really need a third way?"). This is the most convenient switch of the three. Great job!

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #6)
> Sweet and simple. I also noticed Jörg's really clever observer for the
> "lang" attribute, which updates the status bar immediately (but not the
> inline spell check, would that be possible?).
The observer was necessary, since there are too many spots in the code changing the "lang" attribute. It would have been very clumsy and error-prone to let all those spots call the update function.

Now, the second part of the sentence "but not the inline spell check" greatly worries me. Is there something that doesn't work? Or are you referring to some refactoring? The inline spellcheck is updated when you change the language.

Note that we have a very tricky integration with Core::Editor and Core:Spellchecking here. Changing the language via the context menu is not under our control and there are tricky notifiers that core code *has already* changed the language and we need to update all our indicators, and hell, also recheck the subject when we next focus the field. This is very complicated stuff, and thank goodness, it all works and has been for a while, even with different languages in different windows, since I, doing multi-tasking all the time, at times write in two different languages at once. In the bad old days, you could get subject and body out of step and both out of step with the menu choice. Back then, there was no indicator.

r+ with one or two nits fixed and some explanation given.

Conclusion: Very nice, can't wait for the charset indicator to become a menu as well.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3653,5 @@
> + * aPosition  (optional) a single-word alignment value for the position attribute
> + *            of openPopup() method, defaults to "after_start" if omitted.
> + */
> +function showLanguagePopup(aAnchorID, aPosition) {
> +  aPosition = aPosition || "after_start";

You can do:
function showLanguagePopup(aAnchorID, aPosition = "after_start") {.
Not being a XUL expert, what is "after_start"?

@@ +3765,5 @@
>  
>    // No status display, if there is only one or no spelling dictionary available.
>    if (item == languageMenuList.lastChild) {
> +    spellCheckStatusPanel.collapsed = true;
> +    languageStatusButton.label = "-";

Why the "-"? The idea was not to show the element at all. OK, I disabled all dictionaries but on and I don't see a button or a "-".

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +309,5 @@
>  <!ENTITY customizeFromAddress.label "Customize From Address…">
>  <!ENTITY customizeFromAddress.accesskey "A">
>  
>  <!-- Status Bar -->
> +<!ENTITY languageStatusButton.tooltip "Spellcheck language">

Nice.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #11)
> Comment on attachment 8908214 [details] [diff] [review]
> Patch V.1: Implement spellcheck language selector for language status display
> 
> Review of attachment 8908214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not a hard review ;-) Works nicely, and in fact, very functional. I guess I
> have to eat my words from bug 1383860 comment #1 ("Do we really need a third
> way?"). This is the most convenient switch of the three. Great job!
> 
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #6)
> > Sweet and simple. I also noticed Jörg's really clever observer for the
> > "lang" attribute, which updates the status bar immediately (but not the
> > inline spell check, would that be possible?).
> The observer was necessary, since there are too many spots in the code
> changing the "lang" attribute. It would have been very clumsy and
> error-prone to let all those spots call the update function.
> 
> Now, the second part of the sentence "but not the inline spell check"
> greatly worries me. Is there something that doesn't work?

No worries, please see my clarifying comment 7. Would be a nice improvement for wysiwyg live/instant preview when changing language from inside spellcheck dialogue.

> Note that we have a very tricky integration with Core::Editor and
> Core:Spellchecking here. [...]

Yes, this is hairy code...

> In the bad old days, you could get subject and body out of step and
> both out of step with the menu choice. Back then, there was no indicator.

...and you did a great job in fixing those bugs many of which I had prepared for consumption and advocated for.

> Conclusion: Very nice, can't wait for the charset indicator to become a menu
> as well.

Any clues where the charset list gets generated?

> You can do:
> function showLanguagePopup(aAnchorID, aPosition = "after_start") {.

Nice, I must try to remember that default parameter trick.

> Not being a XUL expert, what is "after_start"?

A combined (one-word, simple) position attribute aligning the popup with the anchor.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/Positioning#Simple_Alignment_Values
Maybe if omitted in openPopup(), it would default to "after_start" anyway, but I preferred to be explicit so that consumers will know what they get. Even omitting anchorID will not break this (popup will show in upper-left corner), but it doesn't make much sense to show an un-anchored language menu.

> @@ +3765,5 @@
> > +    spellCheckStatusPanel.collapsed = true;
> > +    languageStatusButton.label = "-";
> Why the "-"? The idea was not to show the element at all. OK, I disabled all
> dictionaries but on and I don't see a button or a "-".

Bisschen um die Ecke gedacht...
As you said, the panel is collapsed, so the button isn't visible. But in the event something (e.g. addon) is checking the button label whilst it is collapsed, we shouldn't return an obsolete language label, so we want to clear the label here.
So something made me think that maybe having a non-empty label might assist to show that it's not an accident. Maybe not. It doesn't matter much.

We might want to polish this corner a bit:
- Given the familiarity (Word) and convenience of the status bar language display/switcher, I think it's fine to always display the language in the status bar even if there's only one dictionary. Even if we don't want to display single language in Status bar (why?), we could still update the language button label to the single language to match reality.
- Imo this language list (from spelling button and status bar) should have a trailing "Add dictionaries..." menuitem like the other language lists. Wherever you change the language, there's a chance that your required language isn't there, so why make it hard to add another one?
- Then, we could have "Language" as a label when there's no dictionary available (can that happen?) or no language selected (can that happen?), and show only "Add dictionaries..." in the menu when there's no dictionary available.
But I wouldn't mind postponing that to a followup bug.

Comment 13

[:aceman]

Comment on attachment 8908214 [details] [diff] [review]
Patch V.1: Implement spellcheck language selector for language status display

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

::: mail/locales/en-US/chrome/messenger/preferences/compose.dtd
@@ +30,5 @@
>  <!ENTITY defaultToParagraph.accesskey         "P">
>  
>  <!ENTITY spellCheck.label                     "Check spelling before sending">
>  <!ENTITY spellCheck.accesskey                 "C">
> +<!ENTITY spellCheckInline.label               "Enable spellcheck as you type">

OK, by not changing the string ID, we assume localizers have it correctly in their translations and this only to be fixed in en-US. Maybe en-GB needs a hint too.

Comment 14

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: Implement spellcheck language selector for language status display

So this might be ready for landing, notwithstanding some optional fine-tuning per my comment 12. Imo we could just land it even without its twin (Interactive Text encoding status display), after all, Beta 57 is not a beta of an actual TB release, so imo there's nothing wrong with having work in progress, and if we're getting feedback "please do the same for text encoding status", it just confirms our expectation that this is useful :)

Comment 15

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #13)
> Comment on attachment 8908214 [details] [diff] [review]
> Patch V.1: Implement spellcheck language selector for language status display
> 
> Review of attachment 8908214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/locales/en-US/chrome/messenger/preferences/compose.dtd
> @@ +30,5 @@
> >  <!ENTITY defaultToParagraph.accesskey         "P">
> >  
> >  <!ENTITY spellCheck.label                     "Check spelling before sending">
> >  <!ENTITY spellCheck.accesskey                 "C">
> > +<!ENTITY spellCheckInline.label               "Enable spellcheck as you type">
> 
> OK, by not changing the string ID, we assume localizers have it correctly in
> their translations and this only to be fixed in en-US. Maybe en-GB needs a
> hint too.

Yes, we're only changing this for en_* locales, so no need to alert other localizations.
How can we alert en-GB, or can't we just fix it for them directly in their locale repository?
Sorry, forgot where our locales live...
Closest I got was https://l10n.mozilla.org/teams/en-GB#tb, but I don't see the real code there.
As for alerting the team, Ian Neal (iann_bugzilla...)
https://wiki.mozilla.org/L10n:Teams:en-GB

en-ZA doesn't translate TB.

Comment 16

[:aceman]

The translations can be found at https://dxr.mozilla.org/l10n-central/source/ , you can search for the string ID and will see all the translations of it. We can't change the string in a localization directly with a patch against c-c.
I think CCing Ian here should be enough.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #12)
> No worries, please see my clarifying comment 7. Would be a nice improvement
> for wysiwyg live/instant preview when changing language from inside
> spellcheck dialogue.
I went through great pain (convincing Ehsan for Core::Spellchecking) that an editor gets only updated when it gains focus again. There are some tricks in TB where we update body and subject when it's under our control. Maybe it could be done if the "Spell Checking" panel triggered it. I never use that panel. You could file a bug and take a look what the code behind the language selector does on that panel.

> Any clues where the charset list gets generated?
Somewhere in M-C code I believe. You'd have to look how they get onto the other menus (Text Encoding on main and compose window).

> But in the
> event something (e.g. addon) is checking the button label whilst it is
> collapsed, we shouldn't return an obsolete language label, so we want to
> clear the label here.
Right, that's why I was setting it to "" which you have now restored.

> We might want to polish this corner a bit:
> - Given the familiarity (Word) and convenience of the status bar language
> display/switcher, I think it's fine to always display the language in the
> status bar even if there's only one dictionary.
I was decided in bug 368915 comment #197 (!!) to only show the language is there's more than one to now upset 95% of mono-lingual users.

> no dictionary available (can that happen?)
Yes, it happens in most/all non-en-US cases. As far as I know, only TB/FF en-US ships with a dictionary, all others don't. en-GB and de(-DE) ship with no dictionary, and the button actually looks quite defective in that case.

(In reply to :aceman from comment #13)
> OK, by not changing the string ID, we assume localizers have it correctly in
> their translations and this only to be fixed in en-US. Maybe en-GB needs a
> hint too.
I think no ID change is OK here, en-UK should have fixed it if they didn't like it.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #14)
> So this might be ready for landing, notwithstanding some optional
> fine-tuning per my comment 12.
What fine-tuning are you referring to? Fixing the body update when you use the "Spell Checking" panel. That's for another bug. Or having the language always visible. I'd say that's a "no" for now and only makes sense if you put "Add Dictionaries..." to the panel (bug 408209).

> Imo we could just land it even without its twin (Interactive Text encoding status display) ...
Yes, indeed, will land as soon as bug 759039 is ready.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #16)
> The translations can be found at
> https://dxr.mozilla.org/l10n-central/source/ , you can search for the string
> ID and will see all the translations of it.

https://dxr.mozilla.org/l10n-central/search?q=spellCheckInline.label&redirect=false
14 times "spell check" (including apparently untranslated strings, like "nr":
https://dxr.mozilla.org/l10n-central/source/nr/mail/chrome/messenger/preferences/compose.dtd#17

Maybe we should change the ID then?

Comment 19

[:aceman]

And then all the others will need to retranslate again.
I don't think we need to change the ID.
The untranslated languages hopefully will translate correctly whether they see 'spell check' or 'spellcheck'.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f109f12fd7b7
Allow changing 'Spellcheck language' from status bar display. Polish en-US 'spellcheck' strings. r=jorgk, ui-r=Paenglab

Comment 21

[Thomas D. (:thomas8)]

Awesome, thanks everyone!

Only 2 iterations, 4 days since this was filed, 2 months since first filed on the duplicate, and 2 years since we had that idea...

Comment 22

[Jorg K (CEST = GMT+2)]

And I'm going to start using it within 60 minutes when the build is done ;-)  BTW, I didn't have any ideas.

Comment 23

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #22)
> And I'm going to start using it within 60 minutes when the build is done ;-)

I bet Jörg won't be the only one to like this, given that it's a trick which has worked in MS Word for ages...
That said, we might well get user requests to allow marking some sections in one language and other sections in another...
So might want to file a bug for that if there isn't one already and start lobbying Editor to include this basic feature...

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #23)
> That said, we might well get user requests to allow marking some sections in
> one language and other sections in another...
> So might want to file a bug for that if there isn't one already and start
> lobbying Editor to include this basic feature...
Check for duplicates before filing bugs ;-) - Bug 69687. Good luck with lobbying.