1206916 - Control center cut off in localized build for sites with non default permissions

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Stefan Plewako [:stef]]

Attachment: screenshot

Comment 1

[Francesco Lodolo [:flod]]

This bug involves also to newer branches or just 41?

I've just got a similar report for Italian and Windows, but everything looks fine for me on OS X/Linux and Fx43.

I wonder if we can add this to the automated screenshots project.

Comment 2

[Axel Hecht [:Pike]]

I can't reproduce to get site permissions to show up in the control center, can you guys help with some STR?

Comment 3

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #2)
> I can't reproduce to get site permissions to show up in the control center,
> can you guys help with some STR?

Open about:permissions, search for a website and assign a special permission like allowing pop-ups.

Now the website's control center should show the permissions section.

Comment 4

[Francesco Lodolo [:flod]]

Attachment: screenshot.png

It's also worth noting that it breaks depending on the permission.

For Italian it breaks with "Enter Fullscreen", which has been removed after 41, and "Offline Data Storage".

Comment 5

[Stefan Plewako [:stef]]

Oh, I have cookies set to expire on closing browser and added exception for the screenshoted site.
I thought it was something new on 41 but now I don't know.
I can also reproduce it on 43.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43. Nomming for 41 tracking as well.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Comment 8

[Brian Grinstead [:bgrins]]

Paolo, I tested locally with some artificially long permission names and this seems to do the trick.  Can you check and see what you think?

Comment 9

[Francesco Lodolo [:flod]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Paolo, I tested locally with some artificially long permission names and
> this seems to do the trick.  Can you check and see what you think?

Can you attach a screenshot of the result? If crop means that we're cutting off text, at the risk of making it even less understandable, it doesn't sound like a great improvement.

Comment 10

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Paolo, I tested locally with some artificially long permission names and
> this seems to do the trick.  Can you check and see what you think?

Not great, but solves the most important usability issue with the security subview access button not being visible.

We should probably file a separate bug with a description of the truncated strings issue, since this is not the final state we want. We'll most probably end up handling that bug as part of the redesign of the permissions view in the main panel.

Comment 11

[:Paolo Amadini]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

https://reviewboard.mozilla.org/r/20255/#review18297

Comment 12

[:Paolo Amadini]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

https://reviewboard.mozilla.org/r/20255/#review18299

Comment 13

[Marco Mucci [:MarcoM]]

Hi Brian, should this bug be marked for QE verification?

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: screenshot-it.png

(In reply to Francesco Lodolo [:flod] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Paolo, I tested locally with some artificially long permission names and
> > this seems to do the trick.  Can you check and see what you think?
> 
> Can you attach a screenshot of the result? If crop means that we're cutting
> off text, at the risk of making it even less understandable, it doesn't
> sound like a great improvement.

Here's a screenshot of the Italian strings, including the tooltiptext on one that is cropped.

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Here's a screenshot of the Italian strings, including the tooltiptext on one
> that is cropped.

Thanks for the screenshot. I'll be honest: it's terrible :-\

At least for Italian, an almost usable dialog just became unreadable, the translation just looks made by an amateur. I also have no idea what happens with other locales using even longer translations.

If you need to truncate, I think it makes sense to put a limit width to the dropdown lists, and possibly widen the entire dialog. I don't think space is an issue here.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Francesco Lodolo [:flod] from comment #15)
> If you need to truncate, I think it makes sense to put a limit width to the
> dropdown lists, and possibly widen the entire dialog. I don't think space is
> an issue here.

From what I understand we don't have localizable widths for the entire dialog, so we wouldn't be able to uplift a change to that (unless if you are suggesting we universally increase the size).

We can try decreasing the capping the dropdown width also

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Francesco Lodolo [:flod] from comment #15)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > Here's a screenshot of the Italian strings, including the tooltiptext on one
> > that is cropped.
> 
> Thanks for the screenshot. I'll be honest: it's terrible :-\
> 
> At least for Italian, an almost usable dialog just became unreadable, the
> translation just looks made by an amateur. I also have no idea what happens
> with other locales using even longer translations.

I wouldn't consider the current behavior almost usable - in addition to looking broken, the user is unable to view to important security information about the page they are on.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: capped-dropdown-width.png

Would still need to figure out how to get the menu popup to become large enough to show the longer items when opened (the popup currently clips the items). This dropdown is going away as part of Bug 1188355 to something like: https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens/100404627.

Comment 19

[Francesco Lodolo [:flod]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> From what I understand we don't have localizable widths for the entire
> dialog, so we wouldn't be able to uplift a change to that (unless if you are
> suggesting we universally increase the size).

Increasing the width for all locales is what I'm suggesting. I'm not saying to double it, just to give l10n a little more breath.

(In reply to Brian Grinstead [:bgrins] from comment #17)
> I wouldn't consider the current behavior almost usable - in addition to
> looking broken, the user is unable to view to important security information
> about the page they are on.

Let's agree on disagreeing. The current one looks broken indeed, that's why this bug was filed, but it's still usable.

But how would you feel in having more than half of the labels truncated in English, to the point they're almost not understandable? Like "Allow c…".

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Francesco Lodolo [:flod] from comment #19)
> (In reply to Brian Grinstead [:bgrins] from comment #16)
> > From what I understand we don't have localizable widths for the entire
> > dialog, so we wouldn't be able to uplift a change to that (unless if you are
> > suggesting we universally increase the size).
> 
> Increasing the width for all locales is what I'm suggesting. I'm not saying
> to double it, just to give l10n a little more breath.

Ash, what do you think about increasing the width of the control center popup?

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: capped-dropdown-width-with-sizetopopup-none.png

With sizetopopup="none" on the dropdown so the popup resizes to fit the contents of the dropdown

Comment 22

[Brian Grinstead [:bgrins]]

Attachment: using-textcontent-with-wrapping.png

Using label.textContent to cause long labels to wrap.  Ash, :flod, what do you think?

Comment 23

[Francesco Lodolo [:flod]]

Comment on attachment 8666088 [details]
using-textcontent-with-wrapping.png

It looks fine to me, it would look even clearer with the label's top aligned with its dropdown list (instead of vertically centered), but I have no idea if that's possible and this is already a huge improvement.

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Comment 25

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> Created attachment 8666088 [details]
> using-textcontent-with-wrapping.png
> 
> Using label.textContent to cause long labels to wrap.  Ash, :flod, what do
> you think?

Sorry thought I replied - this looks a lot better. Only small thing would be add a little extra padding under the rows that have to wrap so that there is consistent spacing between each full row regardless of # of lines.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

While this is a valid issue, based on the fact that this is limited to a very few locales and that the fix is not ready, this is now a wontfix for 41 and will not be included in the proposed dot release (41.0.1).

Comment 27

[:Paolo Amadini]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Code looks good, my only concern is that if we don't let the dropdown expand, in some locales the options may become undistinguishable if they start with the same word.

Can you check in the l10n repositiory if there are any suspect cases? You may want to choose a dropdown width that at least allows the options to be distinguishable.

Comment 28

[Francesco Lodolo [:flod]]

Allow
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/sitePermissions.properties:allow&repo=aurora

Allow for session
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/sitePermissions.properties:allowForSession&repo=aurora

Block
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/sitePermissions.properties:block&repo=aurora

Comment 29

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c157c75de3

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #27)
> Comment on attachment 8665604 [details]
> MozReview Request: Bug 1206916 - Crop permissions labels if there isn't
> enough space in the Control Center;r=paolo
> 
> Code looks good, my only concern is that if we don't let the dropdown
> expand, in some locales the options may become undistinguishable if they
> start with the same word.

With this patch, we are allowing the dropdown to expand to the full width of the selected value (as in https://bug1206916.bmoattachments.org/attachment.cgi?id=8666088).  Then the sizetopopup="none" allows the popup itself to become as large as the largest item in the list.

My previous approach (try 2 of 3) was the one put a max-width on the dropdown: https://bug1206916.bmoattachments.org/attachment.cgi?id=8666077, but I moved past that because of the same concern you mention.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/218c7f64a425

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/218c7f64a425

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Brian, are you going to request uplift for this patch to aurora & beta? Thanks

But not tracking as we won't block the release for this bug.

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Approval Request Comment
[Feature/regressing bug #]: 885366 - as far as I can tell this would have been an issue ever since the permissions feature in identity popup feature was introduced in FF25, it doesn't seem like the labels were ever cropped.
[User impact if declined]: Visual glitch in the control center: https://bug1206916.bmoattachments.org/attachment.cgi?id=8664681.  This is worse since the introduction of the subview (FF 41) since it cuts off access to the security subview.
[Describe test coverage new/current, TreeHerder]: There's a simple test case to make sure that textContent is being set on the label, which allows long translated strings to wrap
[Risks and why]: A change like this could always cause some other XUL layout issues
[String/UUID change made/needed]:

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Approval Request Comment
[Feature/regressing bug #]: 885366 - as far as I can tell this would have been an issue ever since the permissions feature in identity popup feature was introduced in FF25, it doesn't seem like the labels were ever cropped.
[User impact if declined]: Visual glitch in the control center: https://bug1206916.bmoattachments.org/attachment.cgi?id=8664681.  This is worse since the introduction of the subview (FF 41) since it cuts off access to the security subview.
[Describe test coverage new/current, TreeHerder]: There's a simple test case to make sure that textContent is being set on the label, which allows long translated strings to wrap
[Risks and why]: A change like this could always cause some other XUL layout issues
[String/UUID change made/needed]:

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8665604 [details]
MozReview Request: Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo

Visual regression, taking it. Should be in 42 beta 5

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8465fe5c9598

Comment 38

[Carsten Book [:Tomcat]]

Hi, this cause problems when uplifting to beta like :
grafting 304719:218c7f64a425 "Bug 1206916 - Crop permissions labels if there isn't enough space in the Control Center;r=paolo"

merging browser/base/content/browser.js
merging browser/themes/shared/controlcenter/panel.inc.css
warning: conflicts during merge.
merging browser/themes/shared/controlcenter/panel.inc.css incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 39

[Petruta Horea [:phorea], Desktop Test Engineering]

Tried to reproduce under Win 7 64-bit using several locales and Fx versions (Fx 42 beta, Aurora 6-7th October) but everything was correctly aligned.

I could see the initial issue under Mac OS X 10.9.5 and checked it's fixed with Nightly 44.0a1 2015-10-06 with the following locales: it, pl, ar and fr.

I'll continue verifying with other OS's and come back with the results.

Comment 40

[Stefan Plewako [:stef]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #39)
> Tried to reproduce under Win 7 64-bit using several locales and Fx versions
> (Fx 42 beta, Aurora 6-7th October) but everything was correctly aligned.

I can still see the problem on 42.0b4 pl and OS X 10.11

> I could see the initial issue under Mac OS X 10.9.5 and checked it's fixed
> with Nightly 44.0a1 2015-10-06 with the following locales: it, pl, ar and fr.

Yes, seems fixed on 44.0a1 pl 2015-10-06

Comment 41

[Brian Grinstead [:bgrins]]

Try push on beta after a rebase: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd874211ecbb

Comment 42

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56367a87bddf

Comment 43

[Petruta Horea [:phorea], Desktop Test Engineering]

The initial issue doesn't reproduce with Win 10 64-bit Surface Pro 2 and Linux 14.04 32-bit.

I confirm the fix with Firefox 42 beta 5 (locales like pl. sk, es-ES) under Mac OS X 10.9.5.

Latest Aurora l10n 2015-10-08 doesn't have the fix.
I'm modifying the flag for 44.0a1 too based on above comments.

Comment 44

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed on 43.0a2 l10n 2015-10-09.