944947 - Label truncation is not fully supported in Australis menu widgets

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Théo Chevalier]

Attachment: Strings not truncated in the UI (French build)

This is badly impacting localized builds.

As you can see on the screenshot of the French build, "Modules complémentaires" (translation of "Add-ons") is much longer than the en-US string and make the widget wider, making it impossible to have a third widget in the row.

Second case, if you drag'n'drop the Bookmark toolbar into the menu, the label isn't truncated.

The weird part is that truncation is working well for "Téléchargements" (Downloads) -> we can see "Télécharge…"

Comment 1

[Francesco Lodolo [:flod]]

(In reply to Théo Chevalier [:tchevalier] from comment #0)
> As you can see on the screenshot of the French build, "Modules
> complémentaires" (translation of "Add-ons") is much longer than the en-US
> string and make the widget wider, making it impossible to have a third
> widget in the row.

I think the problem here is managing a single long word ("complémentaires"), more than a long label.


> Second case, if you drag'n'drop the Bookmark toolbar into the menu, the
> label isn't truncated.
If you move it in the middle (not last row) does it work? As you can see I have a 3 lines widget in my menu
https://bug942454.bugzilla.mozilla.org/attachment.cgi?id=8337272

 
> The weird part is that truncation is working well for "Téléchargements"
> (Downloads) -> we can see "Télécharge…"
I suspect that's a defect related to bug 942454

Comment 2

[Théo Chevalier]

(In reply to Francesco Lodolo [:flod] from comment #1)
> (In reply to Théo Chevalier [:tchevalier] from comment #0)
> > As you can see on the screenshot of the French build, "Modules
> > complémentaires" (translation of "Add-ons") is much longer than the en-US
> > string and make the widget wider, making it impossible to have a third
> > widget in the row.
> 
> I think the problem here is managing a single long word ("complémentaires"),
> more than a long label.

Indeed.

> 
> 
> > Second case, if you drag'n'drop the Bookmark toolbar into the menu, the
> > label isn't truncated.
> If you move it in the middle (not last row) does it work? As you can see I
> have a 3 lines widget in my menu
> https://bug942454.bugzilla.mozilla.org/attachment.cgi?id=8337272

Hmm, not so much, see the second screenshot, the background is too short.

> 
>  
> > The weird part is that truncation is working well for "Téléchargements"
> > (Downloads) -> we can see "Télécharge…"
> I suspect that's a defect related to bug 942454

I see :)

Comment 3

[Théo Chevalier]

Attachment: Bookmark toolbar no in the last row

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Théo Chevalier [:tchevalier] from comment #3)
> Created attachment 8340757 [details]
> Bookmark toolbar no in the last row

Interesting. On OS X the background is fine for my longest widget ("nuova finestra anonima"), on Linux that widget is on 2 lines so I can't really compare.

Comment 5

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

What would you expect us to do with long words which don't fit? Breaking the panel layout (as we do now) breaks a shitload of stuff, so we can't keep the current situation. But I suspect changing the panel width for everyone isn't great, either. (Stephen?)

We could break words with the appropriate CSS, but I doubt that the result would be pretty, either.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #5)
> But I suspect changing the panel width for everyone isn't great, either. (Stephen?)

What about adding the panel width as a localizable property? We're already doing it for other parts of UI
http://hg.mozilla.org/mozilla-central/file/2581b84e0ca1/browser/locales/en-US/chrome/browser/browser.dtd#l710

Comment 7

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

(In reply to Francesco Lodolo [:flod] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > But I suspect changing the panel width for everyone isn't great, either. (Stephen?)
> 
> What about adding the panel width as a localizable property? We're already
> doing it for other parts of UI
> http://hg.mozilla.org/mozilla-central/file/2581b84e0ca1/browser/locales/en-
> US/chrome/browser/browser.dtd#l710

That's hard-to-impossible because the button widths are 1/3 of that minus padding. That's in the CSS. The CSS doesn't know about l10n and can't, really. We'd have to runtime-generate those lengths and stick them into a runtime-generated stylesheet. Which would then also confuse 3rd-party theme and extension authors because they'd test one thing and find that in French/Russian/Japanese/Gaelic their code doesn't work as they expect.

The plugin warning dialog is simple and not customizable. The menu panel is not simple and *is* customizable. So it's not a great fit, really.

Comment 8

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

P1 to figure out how to solve this because pain.

Comment 9

[Théo Chevalier]

(In reply to :Gijs Kruitbosch from comment #5)
> What would you expect us to do with long words which don't fit? Breaking the
> panel layout (as we do now) breaks a shitload of stuff, so we can't keep the
> current situation. But I suspect changing the panel width for everyone isn't
> great, either. (Stephen?)
> 
> We could break words with the appropriate CSS, but I doubt that the result
> would be pretty, either.

If you can't give localizers more room, I'd say, just prevent the widget from expanding more than 1/3 of panel width and truncate labels with an ellipsis.

FYI, I fixed the string I was referring to in c#1 by shortening it the way we did for about:home widget. But no need to say this bug is still valid for other locales.

Comment 10

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

(In reply to Théo Chevalier [:tchevalier] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #5)
> > What would you expect us to do with long words which don't fit? Breaking the
> > panel layout (as we do now) breaks a shitload of stuff, so we can't keep the
> > current situation. But I suspect changing the panel width for everyone isn't
> > great, either. (Stephen?)
> > 
> > We could break words with the appropriate CSS, but I doubt that the result
> > would be pretty, either.
> 
> If you can't give localizers more room, I'd say, just prevent the widget
> from expanding more than 1/3 of panel width and truncate labels with an
> ellipsis.

Right. I don't /think/ there is a way to get ellipsis-cropping to work on wrapped multiline text. :-(
So it's either single line with ellipsis or multiline without. :-\

Comment 12

[Dão Gottwald [:dao]]

Since we've always done that in various places in the UI, and for the sake of simplicity, I think we should just go with single line with ellipsis. Keep in mind that this is just a hint. The tooltip should provide the full description. When used a few times, users should also recognize the icon.

Comment 13

[Théo Chevalier]

(In reply to Dão Gottwald [:dao] from comment #12)
> Since we've always done that in various places in the UI, and for the sake
> of simplicity, I think we should just go with single line with ellipsis.
> Keep in mind that this is just a hint. The tooltip should provide the full
> description. When used a few times, users should also recognize the icon.

Keep in mind some locales need *much more* room than en-US, even if it's a hint. One single line would be worst than the current state.
I don't want to come to "Nouv. fen." for "New tab" (for instance) and you will agree that "New" or "Tab" alone is not meaningful at all to the user. That's just bad l10n, we can't accept that.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Théo Chevalier [:tchevalier] from comment #13)
> I don't want to come to "Nouv. fen." for "New tab" (for instance) and you
> will agree that "New" or "Tab" alone is not meaningful at all to the user.

Yes, it's not necessarily sufficiently meaningful. (The ellipse is there to communicate this.) If it was the only hint, this would be a problem, but it exists in addition to the icon and the tooltip.

Comment 15

[Justin Dolske [:Dolske]]

What do other things do in this situation?

Mobile, in particular, has exactly this issue with primary UI:  Android, iOS, and FFOS all have a grid of labeled icons (apps), and making the screen wider isn't even possible. At least for my (en-US) devices, iOS seems to be limited to a single line of text, while Android allows 2 lines (and even uses it for a few default apps).

The OS X Finder and Windows Explorer face a similar issue, though a bit different since these labels (filenames) are ofter user-supplied, and don't a skilled localizer/designer in the loop to optimize them. As a random example, I commonly see screenshots with names like "Screen Shot 2013-12-11 at 2.13.05 PM.png" displayed as "Screen Shot\n2013…5 PM".

Might be worth exploring if CSS can add support for this kind of thing (multiline, with per-line overflow), but I don't know how difficult that would be, or if it really helps here. There are actually two (related) problems here -- (1) ensuring we have good, non-overflowing for the common case (our default icons, default config) and (2) ensuring arbitrary labels don't break the UI even if that means cropping in unhelpful ways.

We might be able to tweak some sizes (layout or text) a little bit to help with #1, but I suspect that localizers will still need to be creative to try and make most of the limited space available.

Comment 16

[Justin Dolske [:Dolske]]

MattN reminds me that hyphenation is supported in CSS (and has some degree of localization?), and might help here too?

Comment 17

[Théo Chevalier]

(In reply to Justin Dolske [:Dolske] from comment #16)
> MattN reminds me that hyphenation is supported in CSS (and has some degree
> of localization?), and might help here too?

It looks like it does not support all locales, does it? https://developer.mozilla.org/en-US/docs/Web/CSS/hyphens
If it works for all locales, then yes it would do the trick.

(In reply to Justin Dolske [:Dolske] from comment #15)
> What do other things do in this situation?
> 
> Mobile, in particular, has exactly this issue with primary UI:  Android,
> iOS, and FFOS all have a grid of labeled icons (apps), and making the screen
> wider isn't even possible. At least for my (en-US) devices, iOS seems to be
> limited to a single line of text, while Android allows 2 lines (and even
> uses it for a few default apps).

I don't think we can use the same things as on mobile. We should rather compare with other Desktop browsers.
In Desktop products, we never shorten strings using dots (like "Product localization" shortened into "Prod. localiz."), while we do it on mobile where we have no choice (AFAICT, at least for French). Having it in only one part of the product would be weird — and AFAIK, no other browser is doing that.

> 
> The OS X Finder and Windows Explorer face a similar issue, though a bit
> different since these labels (filenames) are ofter user-supplied, and don't
> a skilled localizer/designer in the loop to optimize them. As a random
> example, I commonly see screenshots with names like "Screen Shot 2013-12-11
> at 2.13.05 PM.png" displayed as "Screen Shot\n2013…5 PM".

Quoting Axel https://groups.google.com/d/msg/mozilla.dev.l10n/n4PTcatmhhA/VCpjpgnt43IJ

> 
> Might be worth exploring if CSS can add support for this kind of thing
> (multiline, with per-line overflow), but I don't know how difficult that
> would be, or if it really helps here. There are actually two (related)
> problems here -- (1) ensuring we have good, non-overflowing for the common
> case (our default icons, default config) and (2) ensuring arbitrary labels
> don't break the UI even if that means cropping in unhelpful ways.
> 
> We might be able to tweak some sizes (layout or text) a little bit to help
> with #1, but I suspect that localizers will still need to be creative to try
> and make most of the limited space available.

NI'ed Axel since he may have a solution taking l10n constraints into account. (That's quite an important/visible part of the product)

Comment 18

[Axel Hecht [:Pike]]

Following up here:

Problem statement: Constrained space exposed to both FX-hosted data as well as add-on-hosted data. That means that the strings are outside of our control, but it also means that we're dealing with multi-language data.

I don't know if a good solution for this problem. We've played with a variety of options on http://htmlpad.org/spacing/ for single line strings, multi-line strings offer more variety still.

I don't think that the l10n-width is a good choice as mentioned previously, as we're dealing with entries provided by add-ons, too. (I think it'd be technically fine with css math and variables, though. But hard to get right then. Anywho, doesn't matter.)

http://mxr.mozilla.org/mozilla-central/source/intl/locales/ is the locales we support for hyphenation. If we don't have anything better, that would help. Better than not doing it, for sure.

Any variety of cropping faces problems. For some languages, the most significant word is the first, for some it's the last, for some it might even be the middle?

http://dev.w3.org/csswg/css-ui/#text-overflow indicates that ellipsis text-overflow should work in each line of a multi-line text.

It'd be great if there was a way to do horizontal "crop-middle" in a multi-line string.

Also, XUL supports crop-middle, but I don't know if that works for multi-line strings.

Comment 19

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

(In reply to Axel Hecht [:Pike] from comment #18)
> Also, XUL supports crop-middle, but I don't know if that works for
> multi-line strings.

As best I can tell no variety of crop works on multiline <label>s in XUL.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to Théo Chevalier [:tchevalier] from comment #17)

> > Mobile, in particular, has exactly this issue with primary UI:  Android,
> > iOS, and FFOS all have a grid of labeled icons (apps), and making the screen
> > wider isn't even possible. At least for my (en-US) devices, iOS seems to be
> > limited to a single line of text, while Android allows 2 lines (and even
> > uses it for a few default apps).
> 
> I don't think we can use the same things as on mobile. We should rather
> compare with other Desktop browsers.

The point is that mobile faces basically the same problem we have here -- labeled icons on a grid, with space/design constraints. (E.G., consider the menu panel as a mobile screen.) It would be nice if mobile yielded a clever solution, but instead it suggests that there just isn't a solution. And given that these mobile UIs are widely used, it's something people can, and do, live with.

Also, one additional datapoint -- I noticed that iOS7 puts a blue dot next to the label when an app is updated, and often causes the label to overflow with an ellipsis. So even en-us commonly runs into this.

Comment 21

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

So Mike (Conley) and I will try and un-...messed-up the labels and keep them on 2 lines here, probably fading out the 3rd and hyphenating, and working to make sure things don't mess up how many items fit in a row and the hover state works and... all that.

If we want to ship in 29 we have two weeks. If we don't manage, we may need to resort to either using a single line or... well, other sad choices.

Comment 22

[Neil Deakin]

(In reply to :Gijs Kruitbosch from comment #19)
> As best I can tell no variety of crop works on multiline <label>s in XUL.

Multiline labels are css block elements (same as html:p), not xul label elements, so you don't get crop on them. I would have thought text-overflow: ellipsis would have worked but sadly, that seems to be intentionally designed to only handle single line text.

I don't think you'll be able to get such a feature if you only have 2 weeks, despite being very useful. Both Mac and Windows crop their desktop icon labels for example with an ellipsis at two lines, so this isn't without precedent.

Comment 23

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

Attachment: label truncation is not fully supported in Australis menu widgets,

Alright, this seems to not regress anything obvious that I can see. What do you think?

Comment 24

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

Attachment: label truncation is not fully supported in Australis menu widgets,

Forgot to change the binding CSS.

Comment 25

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

Attachment: automatically hyphenate labels for items in Australis' menupanel,

Hyphenation support is in this bug.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8361649 [details] [diff] [review]
label truncation is not fully supported in Australis menu widgets,

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

r=me

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +162,5 @@
>  toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"] > iframe {
>    margin: 4px auto;
>  }
>  
> +toolbaritem[cui-areatype="menu-panel"][sdkstylewidget="true"]:not(.panel-wide-item) > .toolbarbutton-multiline-text {

Don't do this until we change the SDK to use the new class.

::: toolkit/content/widgets/toolbarbutton.xml
@@ -72,5 @@
>        <xul:image class="toolbarbutton-icon" xbl:inherits="src=image"/>
>      </content>
>    </binding>
> -
> -  <binding id="toolbarbutton-wrapping-label"

Delete the reference to this binding in xul.css

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8361682 [details] [diff] [review]
automatically hyphenate labels for items in Australis' menupanel,

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

r=me with the two fixes.

::: browser/components/customizableui/content/panelUI.js
@@ +421,5 @@
> + */
> +function getLocale() {
> +  const PREF_MATCH_OS_LOCALE            = "intl.locale.matchOS";
> +  const PREF_SELECTED_LOCALE            = "general.useragent.locale";
> +  if (Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE, false))

I don't think intl.locale.matchOS is ever true on desktop so leave this out for perf. reasons.

@@ +429,5 @@
> +    locale = Services.prefs.getComplexValue(PREF_SELECTED_LOCALE, Ci.nsIPrefLocalizedString);
> +  } catch (ex) { }
> +  if (locale)
> +    return locale;
> +  return Services.prefs.getCharPref(PREF_SELECTED_LOCALE, "en-US");

getCharPref doesn't have a second argument (unfortunately) so this isn't right. Take a copy of getLocale from another piece of code that doesn't use it's own pref helpers.

Comment 28

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/f9e54107a580
remote:   https://hg.mozilla.org/integration/fx-team/rev/b1494e03f231

Comment 29

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

Attachment: update bookmarks toolbar button styling some more for Australis,

This fixes a bunch of panel issues with the bookmarks toolbar placeholder, and cleans up some other issues related to customization.

Comment 30

[Dão Gottwald [:dao]]

Why did this not get reviewed by a toolkit peer? Can you back it out?

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8361649 [details] [diff] [review]
label truncation is not fully supported in Australis menu widgets,

>--- a/toolkit/themes/windows/global/toolbarbutton.css
>+++ b/toolkit/themes/windows/global/toolbarbutton.css

>+.toolbarbutton-multiline-text {
>+  display: none;
>+}
>+
>+toolbarbutton[wrap="true"] > .toolbarbutton-text {
>+  display: none;
>+}
>+
>+toolbarbutton[wrap="true"] > .toolbarbutton-multiline-text {
>+  display: -moz-box;
>+}

This belongs in xul.css and should be written as:

toolbarbutton:not([wrap="true"]) > .toolbarbutton-multiline-text,
toolbarbutton[wrap="true"] > .toolbarbutton-text {
  display: none;
}

I'd also like Neil to review this.

Comment 32

[Dão Gottwald [:dao]]

Speaking of xul.css, you seem to have forgotten to remove this rule: http://hg.mozilla.org/mozilla-central/annotate/b3c08e6fa790/toolkit/content/xul.css#l160

Comment 33

[Dão Gottwald [:dao]]

backed out:
https://hg.mozilla.org/integration/fx-team/rev/9634c792b10a
https://hg.mozilla.org/integration/fx-team/rev/88d36bf7903a

Comment 34

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

(In reply to Dão Gottwald [:dao] from comment #30)
> Why did this not get reviewed by a toolkit peer? Can you back it out?

I was on a plane when you asked, and not the kind with wifi. This got reviewed by MattN after extensive discussion between Jared, Matt and I when we noticed you'd left. We need to get this problem solved and sorted well before aurora uplift, in order for localizations to not have their feet pulled from under them, hence the hurry. As I understand it, Matt was a perfectly reasonable choice for a reviewer according to Firefox and Toolkit review rules.

(In reply to Dão Gottwald [:dao] from comment #32)
> Speaking of xul.css, you seem to have forgotten to remove this rule:
> http://hg.mozilla.org/mozilla-central/annotate/b3c08e6fa790/toolkit/content/
> xul.css#l160

I did not forget. Why didn't you check the commits that I linked to in comment #28, or your backout before you ran it? The changesets weren't merged to m-c, which is why that link still has the 'offending' code.

(In reply to Dão Gottwald [:dao] from comment #31)
> Comment on attachment 8361649 [details] [diff] [review]
> label truncation is not fully supported in Australis menu widgets,
> 
> >--- a/toolkit/themes/windows/global/toolbarbutton.css
> >+++ b/toolkit/themes/windows/global/toolbarbutton.css
> 
> >+.toolbarbutton-multiline-text {
> >+  display: none;
> >+}
> >+
> >+toolbarbutton[wrap="true"] > .toolbarbutton-text {
> >+  display: none;
> >+}
> >+
> >+toolbarbutton[wrap="true"] > .toolbarbutton-multiline-text {
> >+  display: -moz-box;
> >+}
> 
> This belongs in xul.css and should be written as:
> 
> toolbarbutton:not([wrap="true"]) > .toolbarbutton-multiline-text,
> toolbarbutton[wrap="true"] > .toolbarbutton-text {
>   display: none;
> }
> 
> I'd also like Neil to review this.

That's fine, I'll redo the patch moving the CSS into xul.css and request review from him, but I disagree with this change alone justifying a backout.

Comment 35

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

Attachment: label truncation is not fully supported in Australis menu widgets,

Comment 36

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

Comment on attachment 8361997 [details] [diff] [review]
update bookmarks toolbar button styling some more for Australis,

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

Thanks for cleaning this up.

::: browser/base/content/browser-places.js
@@ +912,5 @@
> +        placeholder.setAttribute("wrap", "true");
> +        placeholder.classList.add("toolbarbutton-1");
> +      } else {
> +        placeholder.removeAttribute("wrap");
> +        placeholder.classList.remove("toolbarbutton-1");

This can use:
placeholder.classList.toggle("toolbarbutton-1", shouldWrapNow);
instead of the add/remove dance. (if only we had a toggleAttribute function... ;) )

::: browser/base/content/browser.js
@@ +3279,1 @@
>  function BrowserCustomizeToolbar() {

Keeping this is good for add-on compat and won't hurt, whereas removing it is possibly bad for add-on compat. Or we could put a debug-log statement here to say that the function is deprecated and will be removed after Fx~31?

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +44,5 @@
>      -moz-image-region: rect(0, 96px, 32px, 64px);
>    }
>  }
>  
> +.panelUI-grid .toolbarbutton-1 {

Can this use a child selector?

Comment 37

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

(In reply to Jared Wein [:jaws] from comment #36)
> ::: browser/themes/osx/customizableui/panelUIOverlay.css
> @@ +44,5 @@
> >      -moz-image-region: rect(0, 96px, 32px, 64px);
> >    }
> >  }
> >  
> > +.panelUI-grid .toolbarbutton-1 {
> 
> Can this use a child selector?

The use of the descendant selector was deliberate, add-on authors have been yelling about us using child selectors so much that it breaks add-on styling, see also bug 940307. I added classes to the panels for this reason - I don't want the rules to be too specific.

Comment 38

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

(In reply to :Gijs Kruitbosch from comment #37)
> The use of the descendant selector was deliberate, add-on authors have been
> yelling about us using child selectors so much that it breaks add-on
> styling, see also bug 940307. I added classes to the panels for this reason
> - I don't want the rules to be too specific.

Ok, sounds good.

Comment 39

[Neil Deakin]

Comment on attachment 8362472 [details] [diff] [review]
label truncation is not fully supported in Australis menu widgets,

The only concern is perhaps the extra overhead of having an extra label in every toolbarbutton, but fortunately toolbarbuttons are sparingly used in most cases, so I think this is ok here.

Comment 40

[Neil Deakin]

For documentation, this patch adds support for <toolbarbutton label="Text" wrap="true"/> to allow the label to wrap to multiple lines.

Comment 41

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/993ee55e02cb
remote:   https://hg.mozilla.org/integration/fx-team/rev/5f84d2973b05
remote:   https://hg.mozilla.org/integration/fx-team/rev/228214210aa5

Comment 42

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

Following discussion on IRC, we'll take the remainder of the styling issues with 3-line-or-longer-label buttons to bug 897496.

Comment 43

[Théo Chevalier]

Attachment: Linux regression

Hey, thanks for your work on this bug with this tight schedule!

However, not sure if those patches are addressing that, but FYI Australis has badly regressed with today's Nightly on Linux (Windows is okay).

Comment 44

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

(In reply to Théo Chevalier [:tchevalier] from comment #43)
> Created attachment 8363861 [details]
> Linux regression
> 
> Hey, thanks for your work on this bug with this tight schedule!
> 
> However, not sure if those patches are addressing that, but FYI Australis
> has badly regressed with today's Nightly on Linux (Windows is okay).

It should help, yes. I expect our font and width changes will have made a slight difference to how stuff is laid out, and that's how today's Nightly is different from the day before. Tomorrow's should be better now that this bug has landed. If not, please file a new bug.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/993ee55e02cb
https://hg.mozilla.org/mozilla-central/rev/5f84d2973b05
https://hg.mozilla.org/mozilla-central/rev/228214210aa5

Comment 46

[Philip Chee]

Somewhere around here:
https://hg.mozilla.org/mozilla-central/rev/993ee55e02cb#l6.14
There should be a line that says:
toolbar[mode="icons"] .toolbarbutton-multiline-text,

e.g.
toolbar[mode="icons"] .toolbarbutton-text,
toolbar[mode="icons"] .toolbarbutton-multiline-text,
toolbar[mode="text"] .toolbarbutton-icon {
  display: none;
}

Comment 47

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

(In reply to Philip Chee from comment #46)
> Somewhere around here:
> https://hg.mozilla.org/mozilla-central/rev/993ee55e02cb#l6.14
> There should be a line that says:
> toolbar[mode="icons"] .toolbarbutton-multiline-text,
> 
> e.g.
> toolbar[mode="icons"] .toolbarbutton-text,
> toolbar[mode="icons"] .toolbarbutton-multiline-text,
> toolbar[mode="text"] .toolbarbutton-icon {
>   display: none;
> }

Maybe. It shouldn't make a difference because we never use wrap="true" on toolbars, so it will be display: none because of the rules in xul.css.

Comment 48

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

Attachment: followup: hide Australis' multiline text in toolbars with iconmode=icons,

We should probably cover this base, even if it shouldn't matter...

Comment 49

[Philip Chee]

(In reply to :Gijs Kruitbosch from comment #47)

> Maybe. It shouldn't make a difference because we never use wrap="true" on
> toolbars, so it will be display: none because of the rules in xul.css.
Don't know about that. But I spotted this regression when I built SeaMonkey trunk today. I have several toolbar buttons from addons (e.g. Linky, and Themes Menu) on a toolbar in icons-only mode. You might want to use these to test your fixes.

Comment 50

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

(In reply to Philip Chee from comment #49)
> (In reply to :Gijs Kruitbosch from comment #47)
> 
> > Maybe. It shouldn't make a difference because we never use wrap="true" on
> > toolbars, so it will be display: none because of the rules in xul.css.
> Don't know about that. But I spotted this regression when I built SeaMonkey
> trunk today. I have several toolbar buttons from addons (e.g. Linky, and
> Themes Menu) on a toolbar in icons-only mode. You might want to use these to
> test your fixes.

That still doesn't make sense, by pure rules of logic. Do those buttons have a wrap="true" attribute? Can you investigate why the label wasn't hidden by the rule I pointed at (maybe the add-ons themselves styled something)? Does this go away if you -purgecaches ?

Comment 51

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

So there are two outstanding issues here. One is that some bindings don't match the child selector. Another is that this regressed tpaint and ts_paint on Windows because the #label-control binding is pretty inefficient when it starts trying to display access keys, and the display: none by itself doesn't prevent the binding from being constructed, because there's a non-display: none parent in the shadow content tree (the toolbarbutton itself).

Given that we're going to back out the little keyboard support that the panel has, I'm going to put up a patch that just removes the binding support in a second. That fixes the tpaint/ts_paint regression for me locally.

For the child selector that display: none's the label, I'll put up a separate patch in a second. Many thanks to Philip for investigating

Comment 52

[Neil Deakin]

(In reply to :Gijs Kruitbosch from comment #51)
> Another is that this regressed tpaint and ts_paint
> on Windows because the #label-control binding is pretty inefficient when it
> starts trying to display access keys

I filed bug 963105 on improving this.

Comment 53

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

Attachment: Australis - fix binding for wrapped toolbarbuttons not to bother with an accesskey,

I've left the rule about the mode='icons' because we should probably ensure that even things with wrap='true' don't show a label at that point.

Comment 54

[Dão Gottwald [:dao]]

Comment on attachment 8364366 [details] [diff] [review]
Australis - fix binding for wrapped toolbarbuttons not to bother with an accesskey,

>+toolbarbutton:not([wrap="true"]) > hbox > vbox > .toolbarbutton-multiline-text,
> toolbarbutton:not([wrap="true"]) > .toolbarbutton-multiline-text,
>+toolbarbutton[wrap="true"] > hbox > vbox > .toolbarbutton-text,
> toolbarbutton[wrap="true"] > .toolbarbutton-text {
>   display: none;
> }

Seems like toolbarbutton-text and toolbarbutton-multiline-text should inherit the wrap attribute.

Comment 55

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

(In reply to Dão Gottwald [:dao] from comment #54)
> Comment on attachment 8364366 [details] [diff] [review]
> Australis - fix binding for wrapped toolbarbuttons not to bother with an
> accesskey,
> 
> >+toolbarbutton:not([wrap="true"]) > hbox > vbox > .toolbarbutton-multiline-text,
> > toolbarbutton:not([wrap="true"]) > .toolbarbutton-multiline-text,
> >+toolbarbutton[wrap="true"] > hbox > vbox > .toolbarbutton-text,
> > toolbarbutton[wrap="true"] > .toolbarbutton-text {
> >   display: none;
> > }
> 
> Seems like toolbarbutton-text and toolbarbutton-multiline-text should
> inherit the wrap attribute.

Does that require adding xbl:inherits attributes on the hbox and vbox as well? That seems very ugly.

Similarly, the following selector:

.toolbarbutton-multiline-text:not([wrap="true"]),
.toolbarbutton-text[wrap="true"] {
  display: none;
}

Is a lot harder to follow than the current one, IMO.

Comment 56

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #55)
> Does that require adding xbl:inherits attributes on the hbox and vbox as
> well?

No.

Comment 57

[Philip Chee]

(In reply to :Gijs Kruitbosch from comment #55)
> (In reply to Dão Gottwald [:dao] from comment #54)
>> Comment on attachment 8364366 [details] [diff] [review]

>> Seems like toolbarbutton-text and toolbarbutton-multiline-text should
>> inherit the wrap attribute.
Agreed.

> Similarly, the following selector:
> 
> .toolbarbutton-multiline-text:not([wrap="true"]),
> .toolbarbutton-text[wrap="true"] {
>   display: none;
> }
> 
> Is a lot harder to follow than the current one, IMO.
It's not that much harder. Plus it's more efficient as the CSS engine doesn't have to crawl up the DOM looking for ancestors.

Comment 58

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

Attachment: Australis - fix binding for wrapped toolbarbuttons not to bother with an accesskey,

Comment 59

[Neil Deakin]

Does 963105 mitigate the performance issue and remove the need for this? The patch there should eliminate all of the extra work done when no accesskeys are present.

Comment 60

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

(In reply to Neil Deakin from comment #59)
> Does 963105 mitigate the performance issue and remove the need for this? The
> patch there should eliminate all of the extra work done when no accesskeys
> are present.

Quite possibly, but the single-line labels also don't support access keys, as far as I can tell (they don't get this binding, as far as I can tell?) and we've removed keyboard accessibility support from the menu in bug 946395, so it seems to me that it would make sense to take this patch whether or not bug 963105 mitigates the performance issues.

There are also other hunks in this patch that are still necessary to ensure proper styling for menu-vertical buttons, and for type="wrap" buttons in toolbars generally.

Comment 61

[Neil Deakin]

(In reply to :Gijs Kruitbosch from comment #60)
> (In reply to Neil Deakin from comment #59)
> > Does 963105 mitigate the performance issue and remove the need for this? The
> > patch there should eliminate all of the extra work done when no accesskeys
> > are present.
> 
> Quite possibly, but the single-line labels also don't support access keys,
> as far as I can tell (they don't get this binding, as far as I can tell?)
> and we've removed keyboard accessibility support from the menu in bug
> 946395, so it seems to me that it would make sense to take this patch
> whether or not bug 963105 mitigates the performance issues.

The binding is only used so that multiline labels can be drawn in such a way to have an underline, since they don't get this automatically. The pressing of the accesskey will work whether you use this binding or not.

Single-line labels support both drawing the underline and the pressing of the accesskey without using a special binding (this is instead done in C++ code)

From what I can tell, this patch makes accesskeys work and draw inconsistently.

Note that this patch changes all toolbarbuttons for any application not just those that firefox uses on its main toolbar, so bug 946395 isn't relevant here.

Comment 62

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

Attachment: Australis - fix binding for wrapped toolbarbuttons not to bother with an accesskey,

Comment 63

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/d61e7d073cda

The perf issue should be handled by bug 963105.

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d61e7d073cda

Comment 65

[u428464]

I don't understand why but the "bookmarks toolbar items" text still appears out of the hover effect in the menu panel.

Comment 66

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

(In reply to Guillaume C. [:ge3k0s] from comment #65)
> I don't understand why but the "bookmarks toolbar items" text still appears
> out of the hover effect in the menu panel.

Because there's no overflow: hidden set. See bug 897496.

Comment 67

[u428464]

(In reply to :Gijs Kruitbosch from comment #66)
> (In reply to Guillaume C. [:ge3k0s] from comment #65)
> > I don't understand why but the "bookmarks toolbar items" text still appears
> > out of the hover effect in the menu panel.
> 
> Because there's no overflow: hidden set. See bug 897496.

It's a two rows items but the text is pushed down. I don't see this resolved on latest fx-team build.

Comment 68

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

(In reply to Guillaume C. [:ge3k0s] from comment #67)
> (In reply to :Gijs Kruitbosch from comment #66)
> > (In reply to Guillaume C. [:ge3k0s] from comment #65)
> > > I don't understand why but the "bookmarks toolbar items" text still appears
> > > out of the hover effect in the menu panel.
> > 
> > Because there's no overflow: hidden set. See bug 897496.
> 
> It's a two rows items but the text is pushed down. I don't see this resolved
> on latest fx-team build.

It's on 3 lines for me. Bug 897496 hasn't landed yet. Please file a separate bug and make it depend on bug 897496.

Comment 69

[Jonathan Kew [:jfkthame]]

I'm not sure it's a good idea to use auto-hyphenation for the labels; in most cases, wrapping whole words will look much better. (See also recent thread on dev-l10n, "Australis - Editing issues with the Customizable UI".)

Using -moz-hyphens:manual and encouraging localizers to insert explicit soft hyphen (­) characters in the rare cases where hyphenating a word is appropriate for a particular localization might be better.

Comment 70

[Dão Gottwald [:dao]]

(In reply to Jonathan Kew (:jfkthame) from comment #69)
> Using -moz-hyphens:manual and encouraging localizers to insert explicit soft
> hyphen (­) characters in the rare cases where hyphenating a word is
> appropriate for a particular localization might be better.

That might work for strings we ship but is probably too much to ask from add-on authors.

Comment 71

[Jonathan Kew [:jfkthame]]

(In reply to Dão Gottwald [:dao] from comment #70)
> (In reply to Jonathan Kew (:jfkthame) from comment #69)
> > Using -moz-hyphens:manual and encouraging localizers to insert explicit soft
> > hyphen (­) characters in the rare cases where hyphenating a word is
> > appropriate for a particular localization might be better.
> 
> That might work for strings we ship but is probably too much to ask from
> add-on authors.

Sure, I don't expect it would be done commonly. But for item labels, I think -not- hyphenating is a better default; it should only be done if an author or localizer specifically opts in (by using ­) for a particular label that looks bad by default. The trouble with enabling hyphens:auto is that it will often do things like

    item with hyphen-
       ated label

when it would look much better as

       item with
    hyphenated label

Comment 72

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

(In reply to Jonathan Kew (:jfkthame) from comment #71)
>     item with hyphen-
>        ated label
> 
> when it would look much better as
> 
>        item with
>     hyphenated label

I'm sure this is a dumb question, but why can't the algorithm be taught not to do that?

Comment 73

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs Kruitbosch from comment #72)
> (In reply to Jonathan Kew (:jfkthame) from comment #71)
> >     item with hyphen-
> >        ated label
> > 
> > when it would look much better as
> > 
> >        item with
> >     hyphenated label
> 
> I'm sure this is a dumb question, but why can't the algorithm be taught not
> to do that?

Currently, we choose line-breaks on a line-by-line basis: fill one line until no more text fits (finding the last available word boundary or hyphenation position to break at); then fill the next line, etc. So the hyphenation on the first line happens before we know whether all remaining text will fit on the next line or not.

To achieve the better result here, we'd need a "globally-optimal paragraph breaking" algorithm akin to TeX's, where different types of breaks are considered more or less preferable, and we find the optimal sequence of breaks for the paragraph (this would be bug 630181; it's really hard in the general case, given all the complexities that CSS can throw at it, and would have a performance cost that we might be reluctant to pay for all text reflows).

(A possible simpler option - but with much more limited capability, and still carrying a perf cost - would be some way to attempt to reflow the element without hyphenation, and then if and only if this leads to overflow, enable the hyphens feature and try again.)

Comment 74

[Jonathan Kew [:jfkthame]]

Currently, hyphens:auto is a rather blunt instrument, and should generally be used only on blocks of body text (especially in narrow columns) where avoiding an extremely ragged margin is the main consideration. For icon labels that may overflow onto two or three lines, I think wrapping without hyphenation is far more important than maintaining line lengths as close to maximum as possible.

So I still think it would be better *not* to use hyphens:auto here. The choice to hyphenate a word in order to improve the "fit" of a specific label text, in preference to allowing overflow/truncation, can only be made by human review of individual cases; blindly telling it to always fill lines from the top, hyphenating as needed to do so, will lead to too many ugly and unnecessary breaks.

Comment 75

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

(In reply to Jonathan Kew (:jfkthame) from comment #74)
> Currently, hyphens:auto is a rather blunt instrument, and should generally
> be used only on blocks of body text (especially in narrow columns) where
> avoiding an extremely ragged margin is the main consideration. For icon
> labels that may overflow onto two or three lines, I think wrapping without
> hyphenation is far more important than maintaining line lengths as close to
> maximum as possible.
> 
> So I still think it would be better *not* to use hyphens:auto here. The
> choice to hyphenate a word in order to improve the "fit" of a specific label
> text, in preference to allowing overflow/truncation, can only be made by
> human review of individual cases; blindly telling it to always fill lines
> from the top, hyphenating as needed to do so, will lead to too many ugly and
> unnecessary breaks.

So, first off, it's too late to be changing this for 29, as it will require l10n changes and we're now in beta. What's more, there are languages where even a single word simply *won't fit* without hyphenation, which means text just gets cut off and/or the panel layout gets messed up by the wide text. So not hyphenating at all isn't really an option.

Shy hyphenation would help if it wasn't for the fact that it's extremely hard to get it right: depending on font size and family, different amounts of text will fit. We're using ems for sizing the panel, but that still leaves some variance. We can't require localizers to test all platforms and associated fonts (nevermind the fact that some locales of the OS will ship different default fonts than others), and if they insert as many hyphenation points as possible in order to deal with various font sizes, the result will be no different from hyphens: auto.

On top of that, while some of these strings are in dtd files, many are in .properties files. I suspect our l10n tools are poorly equipped for dealing with invisible characters for hyphenation, and trying to use them would be a recipe for disaster/confusion.

The best solution here would probably be some way of having multiline crop, and giving the locale control over where things get cropped. However, as far as I know there's no standard for that, and XUL is in maintenance mode.

Ultimately, I think for me not doing anything ends up with breaking panel layout (which is *really* ugly, and breaks other things), shy hyphenation puts too much work (and potential pitfalls) in the hands of localizers (and ends up devolving into hyphens: auto anyway), and hyphens auto "sometimes looks ugly".

Between all of these, the last solution actually seems least problematic.

Anyway, if you think we should continue discussing this, I don't think this bug is the best place. Could you post to a newsgroup/mailing list that you think is better? Firefox-dev should at least be CC'd, I suppose, and probably the relevant l10n list, too.