920803 - [German] [doorhanger] Blocked plugin/plug-in, click-to-activate: Button text too long, width doesn't adapt/fit

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: screenshot

Firefox 24.0 on Mac OS X (CC-ed person Sören should know the version)

The button texts for the click-to-activate UI doesn't fit, the buttons don't adapt. On Windows XP, the spacing between text and button border is smaller than on the left, but doesn't overflow like on Mac OS X.

Comment 1

[Benjamin Smedberg]

The width of the entire doorhanger is currently limited to 28em in CSS: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/plugin-doorhanger.inc.css#5

This is because without a specified width, the main content of the doorhanger won't wrap and it becomes arbitrarily wide. Axel or Jaws, what's the correct way to deal with this so that the buttons fit but we still limit the total width to something reasonable?

Comment 2

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

We can allow the localizer to specify the width in this case.

Comment 3

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

I'll take this.

Comment 4

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

Attachment: Patch

I used 336 because it was the box object width in pixels when the width of the doorhanger was set to 28em.

I think we should be able to uplift this to Aurora because it is not a "user-facing" string, but I'm not sure what the technicalities around this are.

This patch will allow a localizer to increase (they shouldn't need to decrease) the width of the doorhanger to fit the locale's requirements.

Comment 5

[Francesco Lodolo [:flod]]

Please add a localization comment explaining what that value is used for and how to verify it.

Technically is a string, so it will break string freeze, be reported if missing and fall back to English (which is OK in this case). 

We should try to understand how many locales are affected to get a better idea of the situation (this is in release for Germany, right?).

Comment 6

[Dão Gottwald [:dao]]

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

We should continue to use em, not pixel values. See http://mxr.mozilla.org/mozilla-central/search?string=em%22&find=locales&filter=width for some precedent.

Comment 7

[Dão Gottwald [:dao]]

Also, "clickToPlayNotification" is (1) too unspecific (e.g. it doesn't mention plugins) and (2) not used in any other entity. Is there no common prefix for the strings used in this popup?

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Francesco Lodolo [:flod] from comment #5)
> We should try to understand how many locales are affected to get a better
> idea of the situation (this is in release for Germany, right?).
Yes, I have been told the screenshot is for Firefox 24.

Even with the entity to control the doorhanger width, this would have been missed because of the platform dependent styling.

Comment 9

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

(In reply to Archaeopteryx [:aryx] from comment #8)
> (In reply to Francesco Lodolo [:flod] from comment #5)
> > We should try to understand how many locales are affected to get a better
> > idea of the situation (this is in release for Germany, right?).
> Yes, I have been told the screenshot is for Firefox 24.
> 
> Even with the entity to control the doorhanger width, this would have been
> missed because of the platform dependent styling.

Should we do a separate Mac, Windows, and Linux platform width? It's possible, but is also likely overkill. I don't think our platform dependent styling here is that different between the three environments.

Comment 10

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

Attachment: Patch v1.1

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There is no need for separate widths. I will likely increase the width necessary on Windows by 10%. (Manual or automated testing if the button texts overflow would be better.)

Comment 12

[Francesco Lodolo [:flod]]

> We should try to understand how many locales are affected to get a better
> idea of the situation (this is in release for Germany, right?).

I did a quick check and I think more locales are affected. I'm pretty sure about bg and el, but others have long strings for those buttons, so it would be good to land it on Aurora (as I said, having en-US fallback in this case is good, talked with Pike on IRC and he agrees).

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 810678 [details] [diff] [review]
Patch v1.1

>-    <content align="start" class="click-to-play-plugins-notification-content">
>+    <content align="start" class="click-to-play-plugins-notification-content" style="width: &pluginNotification.width;;">

Please stop adding the now-unused click-to-play-plugins-notification-content class.

>+<!-- LOCALIZATION NOTE: (pluginNotification.width): This is used to determine the
>+     width of the plugin popup notification that can appear if a plugin has been
>+     blocked on a page. Should be wide enough to fit the pluginActivateNow.label
>+     and pluginActivateAlways.label strings above on a single line. -->
>+<!ENTITY pluginNotification.width "28em">

Should also note that this needs to be a CSS length value.

Comment 14

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

https://hg.mozilla.org/integration/fx-team/rev/1ea17ee6d1cd

Comment 15

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

Attachment: Patch for Aurora

<jaws> maybe 1 beta is enough for the few locales that this is affecting (with special outreach to them?)
<flod> I don't think we have time to get into beta4 (next monday), then there's the summit (no beta in the middle of the week)
<flod> but l10n-freeze is on Oct 21st, so we should have time to test it and update locales
<flod> Pike: thoughts? ^^
<•Pike> I'd take it
<flod> jaws: ^^ (I agree)
<jaws> cool, now to push on the review and get it uplifted quickly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign in fx24
User impact if declined: some locales will not be able to fit the click-to-play buttons in the popup notification
Testing completed (on m-c, etc.): locally, landed on fx-team
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: A new non-user-facing string was added to set the width of the popup notification. if the new string is not updated, it will fall back to the en-us locale value which was the same as the one used in fx24.

Comment 16

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

Attachment: Patch for Beta25

<jaws> maybe 1 beta is enough for the few locales that this is affecting (with special outreach to them?)
<flod> I don't think we have time to get into beta4 (next monday), then there's the summit (no beta in the middle of the week)
<flod> but l10n-freeze is on Oct 21st, so we should have time to test it and update locales
<flod> Pike: thoughts? ^^
<•Pike> I'd take it
<flod> jaws: ^^ (I agree)
<jaws> cool, now to push on the review and get it uplifted quickly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign in fx24
User impact if declined: some locales will not be able to fit the click-to-play buttons in the popup notification
Testing completed (on m-c, etc.): locally, landed on fx-team
Risk to taking this patch (and alternatives if risky): none expected
String or IDL/UUID changes made by this patch: A new non-user-facing string was added to set the width of the popup notification. if the new string is not updated, it will fall back to the en-us locale value which was the same as the one used in fx24.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ea17ee6d1cd

Comment 18

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

Thank you everyone for the quick turn-around-time!

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b885e13dfe7
https://hg.mozilla.org/releases/mozilla-beta/rev/0f4c69afd5b9

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

I just committed a second increase of the doorhanger width to German aurora and beta, the first increase wasn't sufficient.

http://hg.mozilla.org/releases/l10n/mozilla-aurora/de/rev/b1a68161b52d
http://hg.mozilla.org/releases/l10n/mozilla-beta/de/rev/f4679912815c

Comment 20

[:Gijs (he/him)]

(In reply to Archaeopteryx [:aryx] from comment #19)
> I just committed a second increase of the doorhanger width to German aurora
> and beta, the first increase wasn't sufficient.
> 
> http://hg.mozilla.org/releases/l10n/mozilla-aurora/de/rev/b1a68161b52d
> http://hg.mozilla.org/releases/l10n/mozilla-beta/de/rev/f4679912815c

Errr, did you mean to remove the undoCloseTabs.label in the same commit, without mentioning it in the commit message?

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to :Gijs Kruitbosch from comment #20)
> Errr, did you mean to remove the undoCloseTabs.label in the same commit,
> without mentioning it in the commit message?
Yes, I likely forgot to remove that obsolete string earlier and the l10n dashboard only shows missing strings on its landing page, so I saw that by surprise and thought "Let's also fix that".

Comment 22

[Mihai Morar, (:MihaiMorar)]

Tracy, I can confirm this is not reproducing on FF 25b9 using Windows 7x32 and Mac OS 10.8.4 but the problem is that I am not able to reproduce the initial issue on Nightly (2013-09-25) because there is no german nightly build (2013-09-25) on FTP or l10n.

Comment 23

[u279076]

Marking this [qa-] based on comment 22. Please reopen if you can still reproduce this bug in the latest Nightly.