Closed Bug 920803 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect)

24 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox24 --- wontfix
firefox25 + fixed
firefox26 + fixed
firefox27 + fixed

People

(Reporter: aryx, Assigned: jaws)

Details

(Whiteboard: [qa-])

Attachments

(4 files, 1 obsolete file)

Attached image 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.
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?
Flags: needinfo?(l10n)
Flags: needinfo?(jaws)
Summary: [doorhanger] Blocked plugin/plug-in, click-to-activate: Button text too long, width doesn't adapt/fit → [German] [doorhanger] Blocked plugin/plug-in, click-to-activate: Button text too long, width doesn't adapt/fit
We can allow the localizer to specify the width in this case.
Flags: needinfo?(jaws)
I'll take this.
Status: NEW → ASSIGNED
OS: Mac OS X → All
QA Contact: jaws
Hardware: x86 → All
Flags: needinfo?(l10n)
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jaws
Attachment #810577 - Flags: review?(dao)
QA Contact: jaws
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 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.
Attachment #810577 - Flags: review?(dao) → review-
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?
(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.
(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.
Attached patch Patch v1.1Splinter Review
Attachment #810577 - Attachment is obsolete: true
Attachment #810678 - Flags: review?(dao)
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.)
> 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 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.
Attachment #810678 - Flags: review?(dao) → review+
Attached patch Patch for AuroraSplinter Review
<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.
Attachment #810736 - Flags: review+
Attachment #810736 - Flags: approval-mozilla-aurora?
Attached patch Patch for Beta25Splinter Review
<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.
Attachment #810745 - Flags: review+
Attachment #810745 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/1ea17ee6d1cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Attachment #810745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #810736 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
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
(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?
(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".
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.
QA Contact: mihai.morar
Marking this [qa-] based on comment 22. Please reopen if you can still reproduce this bug in the latest Nightly.
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.