962884 - Offer opt-out from auto-hyphenation for labels in the menu panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement)

Description

[Marcus Husar]

Attachment: The panel menu before and after some adjustments (see bug 900428).

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140122030521

Steps to reproduce:

Just start the current nightly of Firefox and open the panel menu.

Platform: Fedora 20, x86_64
Product: Firefox nightly l10n-de (german)


Actual results:

Directly after the start of Firefox the panel menu displays a vertical scroll bar. Later the scroll bar disappears. The panel menu labels are too huge due to adjustments made in bug 900428.

See additional attachments.


Expected results:

There should be enough space to display the panel menu labels.

Comment 1

[Marcus Husar]

Attachment: The panel menu displaying a vertical scroll bar.

Comment 2

[Marcus Husar]

Attachment: The panel menu on mouse over without vertical scrollbar.

Comment 3

[:Gijs (out for now; he/him)]

This is basically impossible to fix. We're adjusting to operating system font sizes. The same font sizes are used for menus. We could opt for something smaller just because Ubuntu happens to ship with default font sizes of 14.5px or whatever it is, but then people on other distros will be sad because it translates to 10px on their system and it's too small.

Tomorrow's nightly will have multiline hyphenated labels, which will ease the pain somewhat, I would expect.

Comment 4

[Marcus Husar]

Attachment: The panel menu with hyphenation enabled.

I think hyphenation makes things worse. It breaks words unnecessarily. See attachment number 4 (hyphenation.png).

Before: Neues<line-break>Fenster
After: Neues Fen-<line-break>ster

An alternative approach would be to use text-overflow: ellipsis. The result is in my opinion much better. I’ll attach a screenshot with text-overflow enabled.

Comment 5

[Marcus Husar]

Attachment: The panel menu with „text-overflow: ellipsis“ enabled.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Marcus Husar from comment #5)
> Created attachment 8364357 [details]
> The panel menu with „text-overflow: ellipsis“ enabled.

For this we need to require single line labels, though, or the text-overflow doesn't work. That breaks other languages like French that have two long words ("Modules complémentaires" or something similar for 'Add-ons', for instance.

I don't think there's a perfect solution here, and I think the current one is the least problematic.

The one option I see is using manual hyphenation and requiring localizers to add 'shy' hyphenation marks into their labels. That is also sucky. :-(

Comment 7

[Marcus Husar]

> For this we need to require single line labels, though, or the text-overflow
> doesn't work.

It works perfectly with multiline labels. Please see the attached screenshots number 4 and 5 and reconsider.

You will see that hypenation may unnecessarily break labels which makes the panel menu at least ugly. It’s also bad for usability. One should never use hyphenation for menu labels.

> Neues Fen-
> ster
> Synchro-
> nisieren

Text-overflow ellipsis doesn’t provide these problems (see attachment number 5).

> Neues
> Fenster (multiline)
> Synchronisie...

> That breaks other languages like French that have two long
> words ("Modules complémentaires" or something similar for 'Add-ons', for
> instance.

No, it doesn’t (checked). It’s „Modules“ in the current nightly anyway.

If it woult be „Modules complémentaires“ at the moment (estimated):

hypenation (complémentaires is too long):
> Modules
> complémen-
> taires

ellipsis:
> Modules
> complément...

> I don't think there's a perfect solution here, and I think the current one
> is the least problematic.

I don’t think so. Shy hyphenation wouldn’t help either.

Here’s a userChrome.css to check things out:
.panelUI-grid .toolbarbutton-menubutton-button > .toolbarbutton-multiline-text,
.panelUI-grid .toolbarbutton-1 > .toolbarbutton-multiline-text {
  -moz-hyphens: none !important;
  overflow: hidden;
  text-overflow: ellipsis;
}

Comment 8

[:Gijs (out for now; he/him)]

Attachment: text-overflow ellipsis.png

(In reply to Marcus Husar from comment #7)
> > For this we need to require single line labels, though, or the text-overflow
> > doesn't work.
> 
> It works perfectly with multiline labels. Please see the attached
> screenshots number 4 and 5 and reconsider.

Screenshot number 5 doesn't have any text-ellipsis on multiline labels, only on the single-line "Synchronisieren".

So, for the XUL crop attribute to work, we need a single line label.

In order to use text-overflow: ellipsis, you need overflow: hidden, which doesn't work properly because the boxes are laid out using old-style flexbox. See bug 625700.

I also think it's debatable whether longer words being hyphenated or being text-ellipsis'd is "better". In the first case, at least you can read the entire thing. In the latter, you cannot.

> > That breaks other languages like French that have two long
> > words ("Modules complémentaires" or something similar for 'Add-ons', for
> > instance.
> 
> No, it doesn’t (checked). It’s „Modules“ in the current nightly anyway.

Yes, they changed it because it broke stuff. Did you read bug 944947? There was quite a lot of discussion about this...

> If it woult be „Modules complémentaires“ at the moment (estimated):
> 
> hypenation (complémentaires is too long):
> > Modules
> > complémen-
> > taires
> 
> ellipsis:
> > Modules
> > complément...

That's how it should look, but that's not what happens.


> Here’s a userChrome.css to check things out:
> .panelUI-grid .toolbarbutton-menubutton-button >
> .toolbarbutton-multiline-text,
> .panelUI-grid .toolbarbutton-1 > .toolbarbutton-multiline-text {
>   -moz-hyphens: none !important;
>   overflow: hidden;
>   text-overflow: ellipsis;
> }

I tried this (and used DOM Inspector to change the label of another button; it doesn't actually make a difference...), and it ends up looking like this attachment.

Comment 9

[:Gijs (out for now; he/him)]

Stephen, are we still OK with the direction here or do you think we need to revisit this, given the screenshots etc.?

(the scrollbar is tracked in e.g. bug 962855)

Comment 10

[Marcus Husar]

> In order to use text-overflow: ellipsis, you need overflow: hidden, which doesn't work properly because the boxes are > laid out using old-style flexbox. See bug 625700.
>
> ...
>
> That's how it should look, but that's not what happens.

My fault. I thought all of the labels where multiline labels and text-overflow ellipsis would work as expected.

Excuse me for beeing a bit harsh. I just wanted to find a better solution. Thanks for your hard work.

Comment 11

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #9)
> Stephen, are we still OK with the direction here or do you think we need to
> revisit this, given the screenshots etc.?
> 
> (the scrollbar is tracked in e.g. bug 962855)

Well… we don't want to break the labels, but we also don't want buttons that are the wrong size. Are those the only two options?

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Stephen Horlander [:shorlander] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Stephen, are we still OK with the direction here or do you think we need to
> > revisit this, given the screenshots etc.?
> > 
> > (the scrollbar is tracked in e.g. bug 962855)
> 
> Well… we don't want to break the labels, but we also don't want buttons that
> are the wrong size. Are those the only two options?

IIRC with Jared's work we could now also do something which would result in long words getting ellipses, but if you had something like:

Iamreallylong andiamalsoreallylong

as the label, it'd come out as

Iamreall...
andiama...

If instead you had:

I am a long label with lots of short words

It'd come out as

I am a long
label with
[lots of]

(where the last line is faded out)

(jsbin: http://jsbin.com/koviv/1/ )

which I think is also worse than the current solution because of how we (a) don't get ellipses on the last line if the words can be split up, and (b) we can get multiple ellipses if there are long unbreakable words.

The proper solution is some way to define "I want this label to be X or fewer lines and the last line should have an ellipsis if there is more text", but there is no way to do that in CSS (or XUL) right now, that I know of.

Comment 13

[:Gijs (out for now; he/him)]

P-'ing per discussion on IRC, because as it is there isn't much else/better we can do here.

Comment 14

[Matthew N. [:MattN]]

Hyphenating also leads to a (synchronous?) load of the hyphenation library upon first panel open as we are the first to use it :(

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Pascal Chevrel:pascalc from bug 987620 comment #3)
> I am going to dupe to bug 962884 because
> https://bugzilla.mozilla.org/show_bug.cgi?id=962884#c4 is exactly what I
> described. Also, I was about to suggest that we could use hyphens:manual and
> let the localizers indicate where to break words. it's certainly not the
> perfect solution but it would work for all the big locales (German, French,
> Spanish, Russian, Chinese...) that work directly on hg and are technical
> enough for that. it wouldn't work for the less technical localizers that
> only know translation, don't understand compiting and work via external
> translation platforms, so it's probably better to not hyphenate at all.

Keep in mind that in .properties files, you can't see shy hyphens. They're just zero-width unicode characters. I don't think this is a workable solution, even for technical people. As I noted in bug 987620, localizers would also need to add hyphens on all the plausible hyphenation points in order for things to not break when using various font sizes. :-(

> I have the feeling that there is no perfect solution and that no team
> (localizers, front end, gecko…) can fix it, I just hope that we can reach a
> good compromise that works for most if not all locales and, most
> importantly, that works for our user base. In that context, I don't think we
> can ship with crazy hyphenation on our  top locales like German, French and
> Italian, hence my duping to the 'no hyphenation' bug. 
> 
> Is it possible to have per locale css rules as we do on mozilla.org btw ?

Not simply, no. We could add a localizable string which gets put into an attribute which triggers which hyphenation mode we use (manual or auto), but it's quite late to be doing that (second week of beta...). This will give add-ons the worst of both worlds and also let them break the panel still.

Ultimately, we need to avoid a situation like in bug 987615, where the panel layout goes crazy because there are strings that are too long, even for add-ons or "non-technical" locales. Hyphenation: auto is the best way I know of to do that. Text-overflow doesn't work, crop doesn't work, manual hyphenation doesn't work. No text labels isn't acceptable UX considering that some of our icons just aren't obvious enough for users (and that's not something I see as fixable, either). I've not heard other solutions.

If that means "not great looking strings on some locales", then at the moment that seems to me like the least-terrible solution.

Keep in mind that there's a long tradition of tradeoffs for strings in these cases on mobile. I recently used my old candybar phone again and found that it actually just drops vowels from the middle of words in order to make things fit on the screen, even in the primary device language (English).

Comment 17

[Pascal Chevrel:pascalc]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to Pascal Chevrel:pascalc from bug 987620 comment #3)
> > I am going to dupe to bug 962884 because
> > https://bugzilla.mozilla.org/show_bug.cgi?id=962884#c4 is exactly what I
> > described. Also, I was about to suggest that we could use hyphens:manual and
> > let the localizers indicate where to break words. it's certainly not the
> > perfect solution but it would work for all the big locales (German, French,
> > Spanish, Russian, Chinese...) that work directly on hg and are technical
> > enough for that. it wouldn't work for the less technical localizers that
> > only know translation, don't understand compiting and work via external
> > translation platforms, so it's probably better to not hyphenate at all.
> 
> Keep in mind that in .properties files, you can't see shy hyphens. They're
> just zero-width unicode characters.

insert a unicode character \u00AD, AFAIK we already do that in other files to insert a trailing white space in strings

Comment 18

[:Gijs (out for now; he/him)]

Jared and I discussed this some more, and the current plan is to allow localizers to opt-out of auto hyphenation on a per-button basis by including shy hyphens in the label. That is, for any label that has shy hyphens, we'll instruct the panel to use manual hyphenation. To completely disable hyphenation and rely on word-wrap only, localizers could end or start the string with a shy hyphen (­ / \u00ad), as that'd trigger manual mode as well.

This gets around the add-on problem because we'll still use auto-hyphenation there, and also lets locales that think they can deal with this and/or need to adjust the labels of buttons in order to work better in the panel the ability to do that.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8396716 [details] [diff] [review]
Patch

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

This is the right direction, but we should add a handler to PanelUI for customization events, and call _adjustLabelsForAutoHyphens when the panel contents change while the user is in customize mode.

::: browser/components/customizableui/content/panelUI.js
@@ +390,5 @@
>      this.multiView.ignoreMutations = false;
>    },
>  
> +  _adjustLabelsForAutoHyphens: function() {
> +    for (let widget of this.contents.children) {

I think we should instead use the following collection:

this.contents.querySelectorAll("toolbarbutton-1")

@@ +395,5 @@
> +      let label = widget.getAttribute("label");
> +      if (!label) {
> +        continue;
> +      }
> +      if (label.contains("­") || label.contains("\u00ad")) {

In what case will it contain a literal ­ ? In my testing, attribute contents' XML entities get resolved to their representations when getAttributed, so I don't think the ­ case can happen.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> this.contents.querySelectorAll("toolbarbutton-1")

Err, ".toolbarbutton-1", d'oh.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 23

[:Gijs (out for now; he/him)]

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

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

f+ because no test, and the typo, and my musings on whether we should only care about this during customize mode...

::: browser/components/customizableui/content/panelUI.js
@@ +372,5 @@
>                        "chrome,modal=yes,resizable=yes",
>                        "browser");
>    },
>  
> +  onWidgetAdded: function(aWidgetId) {

Nit: check aArea for being equal to CustomizableUI.AREA_PANEL and only run _adjust... in that case?

I'm wondering if it's worth disabling this behaviour outside of customize mode (if you're adding an item to the panel through the context menu, the menu will be closed so onpopupshowing will deal with adjusting the hyphens...)

@@ +377,5 @@
> +    this._adjustLabelsForAutoHyphens(aWidgetId);
> +  },
> +
> +  onWidgetRemoved: function(aWidgetId) {
> +    let widget = doucment.getElementById(aWidgetId);

I'm... guessing you didn't test this? "doucment" sounds like it's unlikely to work...

An automated test for this would be swell.

Comment 24

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #23)
> I'm wondering if it's worth disabling this behaviour outside of customize
> mode

Yes please. Menu looks bad during normal usage as much as it does in customization mode, and the first has much more visibility.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Francesco Lodolo [:flod] from comment #24)
> (In reply to :Gijs Kruitbosch from comment #23)
> > I'm wondering if it's worth disabling this behaviour outside of customize
> > mode
> 
> Yes please. Menu looks bad during normal usage as much as it does in
> customization mode, and the first has much more visibility.

No, that's totally not what I meant. I meant that the specific bit of code that I was commenting on doesn't need to be run outside of customize mode.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Comment 27

[:Gijs (out for now; he/him)]

Comment on attachment 8397151 [details] [diff] [review]
Patch v3

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

f+ because event handlers are hard and you forgot to hg add the test file before qref'ing, it seems...

::: browser/components/customizableui/content/panelUI.js
@@ +377,5 @@
> +    if (aContainer != this.contents) {
> +      return;
> +    }
> +    if (this.panel.state == "open" ||
> +        document.documentElement.hasAttribute("customizing")) {

This should check !aWasRemoval.

@@ +386,5 @@
> +  onWidgetBeforeDOMChange: function(aNode, aNextNode, aContainer, aIsRemoval) {
> +    if (aContainer != this.contents) {
> +      return;
> +    }
> +    if (aNode && aIsRemoval) {

I don't think we can pass a null aNode, but this condition should check hasAttribute("auto-hyphens")

@@ +387,5 @@
> +    if (aContainer != this.contents) {
> +      return;
> +    }
> +    if (aNode && aIsRemoval) {
> +      aNode.removeAttribute("auto-hyphens");

Seems to me like we should do this *after* the node has left the building, err..., I mean container. So in the AfterDOMChange handler.

Conversely, actually, the bits in the after handler should be in this one - if you're inserting, stuff should happen *before* the insertion. Both of these so that we avoid causing an extra reflow.

@@ +420,5 @@
> +        continue;
> +      }
> +      if (label.contains("\u00ad")) {
> +        node.setAttribute("auto-hyphens", "off");
> +      } else {

Nit: } else if (node.hasAttribute("auto-hyphens")) {

::: browser/components/customizableui/test/browser.ini
@@ +64,5 @@
>  [browser_947987_removable_default.js]
>  [browser_948985_non_removable_defaultArea.js]
>  [browser_952963_areaType_getter_no_area.js]
>  [browser_956602_remove_special_widget.js]
> +[browser_962884_opt_in_disable_hyphens.js]

This would be a 100% better patch if it included this test file. :-(

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #27) 
> I don't think we can pass a null aNode, but this condition should check
> hasAttribute("auto-hyphens")
> 
> Nit: } else if (node.hasAttribute("auto-hyphens")) {

Why? Calling .removeAttribute("auto-hyphens") will no-op if the element doesn't have the attribute. Checking that the attribute exists seems unnecessary.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3.1

Sorry about the missing test file. It's added now.

Comment 30

[:Gijs (out for now; he/him)]

Comment on attachment 8397914 [details] [diff] [review]
Patch v3.1

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> (In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #27) 
> > I don't think we can pass a null aNode, but this condition should check
> > hasAttribute("auto-hyphens")
> > 
> > Nit: } else if (node.hasAttribute("auto-hyphens")) {
> 
> Why? Calling .removeAttribute("auto-hyphens") will no-op if the element
> doesn't have the attribute. Checking that the attribute exists seems
> unnecessary.

So there are documented cases where setAttribute("foo", "bar") on <div foo="bar"/> has nonzero costs. I don't know about removeAttribute. I guess we can leave it like this for now. I just get paranoid when I hear stories like that. :-(

::: browser/components/customizableui/test/browser_962884_opt_in_disable_hyphens.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +add_task(function() {
> +  const kLabelSuffix = "Character Encoding";

Nit: I think kNormalLabel would make more sense, but I don't really care too much.

@@ +12,5 @@
> +  CustomizableUI.addWidgetToArea("characterencoding-button", CustomizableUI.AREA_PANEL);
> +
> +  let shownPanelPromise = promisePanelShown(window);
> +  PanelUI.toggle();
> +  yield shownPanelPromise;

Nit: yield PanelUI.show() ? :-)

@@ +28,5 @@
> +  characterEncoding.setAttribute("label", kLabelSuffix);
> +
> +  shownPanelPromise = promisePanelShown(window);
> +  PanelUI.toggle();
> +  yield shownPanelPromise;

Ditto

@@ +60,5 @@
> +  CustomizableUI.addWidgetToArea("characterencoding-button", CustomizableUI.AREA_NAVBAR);
> +  ok(!characterEncoding.hasAttribute("auto-hyphens"),
> +     "Removing the widget from the panel should remove the auto-hyphens attribute");
> +
> +  characterEncoding.setAttribute("label", kLabelSuffix);

For resetting at the end, can you fetch the original label before you set it, and then reuse that here?

In practice that doesn't currently matter - but I've seen us do:

Character En-
coding

for this label, and so I actually think we might want to use the functionality you're adding in en-US, in which case the above would change the label, which means we might mess up later tests (which is somewhat theoretical, but considering all the random orange we have I'd rather not take risks)

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3.2

I removed the check for this._eventListenersAdded in PanelUI.uninit. When it was there, a window could be opened and then closed, and the CustomizableUI listener would be added but never removed if the window's menu panel wasn't opened.

Calling element.removeEventListener with an event+object that was never added is safe.

Comment 32

[:Gijs (out for now; he/him)]

Comment on attachment 8398046 [details] [diff] [review]
Patch v3.2

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

LGTM!

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/122bd8d437aa

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/122bd8d437aa

Comment 35

[Francesco Lodolo [:flod]]

Once tested on central, are we going to move this bug to other branches? I'm sure that would make a lot of localizers happy

Comment 36

[Francesco Lodolo [:flod]]

OK, I wanted to give it a try to be sure, since I have three strings with this problem (1 in customizableWidgets.properties, 2 in browser.dtd).

Unpacked browser/omni.jar, changed the strings, repacked, launched browser.

If a put ­ in a .dtd I get a Yellow Screen of Death for undefined entity.
If a put a \u00ad in the .properties file (or a .dtd), it's just displayed as text (part of the label).

Is this supposed to happen?

Comment 37

[Francesco Lodolo [:flod]]

Thanks to MattN for pointing me to a try build and sticking around.

\u00AD and \u00ad works fine in customizableWidgets.properties, while they didn't work with the previous build. I wonder if that's related to auto-hyphens being off (on = character displayed?)

.dtd files
­
­

These two works. ­ make things explode, so DO NOT USE.

Comment 38

[:Gijs (out for now; he/him)]

(In reply to Francesco Lodolo [:flod] from comment #37)
> Thanks to MattN for pointing me to a try build and sticking around.
> 
> \u00AD and \u00ad works fine in customizableWidgets.properties, while they
> didn't work with the previous build. I wonder if that's related to
> auto-hyphens being off (on = character displayed?)

I can't reproduce that:

http://jsbin.com/fivaj/1/edit

It's possible that XUL works differently, but that'd be very strange.

Comment 39

[:Gijs (out for now; he/him)]

Comment on attachment 8398046 [details] [diff] [review]
Patch v3.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 944947
User impact if declined: hyphens make fr/de/it and possibly other locales look quite bad.
Testing completed (on m-c, etc.): on m-c, locally, has automated test
Risk to taking this patch (and alternatives if risky): per the above, low. I'd like to land on Aurora today, and ideally on beta before monday's beta gets tagged (so on Sunday or early on Monday).
String or IDL/UUID changes made by this patch: none, but allows localizers to change their strings specifically for the purpose of making them display better in the panel.

Comment 40

[:Gijs (out for now; he/him)]

Francesco, can you announce this in the appropriate forum so localizers that aren't already aware can be made aware of this change? Thank you!

Comment 41

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #40)
> Francesco, can you announce this in the appropriate forum so localizers that
> aren't already aware can be made aware of this change? Thank you!

Done
https://groups.google.com/forum/#!topic/mozilla.dev.l10n/35JvQKfopYo

Comment 42

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/629760276aaa

Comment 43

[Mike de Boer [:mikedeboer]]

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/7a71dc1cde65

Comment 44

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems this is covered automatically by: https://hg.mozilla.org/releases/mozilla-beta/diff/7a71dc1cde65/browser/components/customizableui/test/browser_962884_opt_in_disable_hyphens.js.