Text cropped and overflowed in advanced pane in-content dialogs

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: stef, Assigned: joseph)

Tracking

({regression})

Trunk
Firefox 52
regression
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51+ fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8786024 [details]
Connection settings screenshot

Probably after bug 1036815 text gets cropped and overflowed in latest Polish (pl) nightly builds in advanced pane in-content dialogs.
(Reporter)

Comment 1

a year ago
Created attachment 8786025 [details]
Device Manager
[Tracking Requested - why for this release]: UI regression
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox51: --- → ?
Flags: needinfo?(jyeh)
Keywords: regression
Priority: -- → P1
Tracking 51+ for this UI regression.
tracking-firefox51: ? → +
(Assignee)

Comment 4

a year ago
Hi Matthew,

For the device manager, the min-width of that column is fixed.

https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/device_manager.xul#43

Enlarge min-width to 20em will prevent text getting cropped, but I am not sure if this is the correct way to fix.


And for the connection settings, I am not sure how to fix it. Have you been to this kind of issue before? or any bug that I can reference to?

Thanks!
Flags: needinfo?(jyeh) → needinfo?(MattN+bmo)
(In reply to Joseph Yeh [:joseph] from comment #4)
> Hi Matthew,
> 
> For the device manager, the min-width of that column is fixed.
> 
> https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/
> resources/content/device_manager.xul#43
> 
> Enlarge min-width to 20em will prevent text getting cropped, but I am not
> sure if this is the correct way to fix.

I'm not sure how much longer that text needs to be in Polish but the problem seems to be the larger font than before. The window (rather than the column) is already resizable per-locale so perhaps that's enough: https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/security/manager/locales/en-US/chrome/pippki/deviceManager.dtd#6-9

Stef, is devmgr.style enough for the device manager dialog?

> And for the connection settings, I am not sure how to fix it. Have you been
> to this kind of issue before? or any bug that I can reference to?

The window.width and window.macWidth localized widths is likely the solution but you removed them in https://hg.mozilla.org/mozilla-central/rev/20e45f63c2e6#l1.25 Gijs questioned the removal in bug 1036815 comment 30 btw and that's why I mentioned testing in other locales in bug 1036815 comment 45.

If you revert those changes it will allow each locale to specify a width. Since the fonts and such have changed by displaying the dialog in-content and due to l10n rules (where we need to bump the string ID whenever it's changed) we should switch to `window.width2` and `window.macWidth2`. Please test that this works by making the clipped checkbox string from the screenshot longer in your build and adjusting the window.*width2 value and checking that it would fix the problem
Flags: needinfo?(MattN+bmo) → needinfo?(splewako)
(Assignee)

Updated

a year ago
See Also: → bug 1049001
(Reporter)

Comment 6

a year ago
(In reply to Matthew N. [:MattN] from comment #5)
> (In reply to Joseph Yeh [:joseph] from comment #4)
> > For the device manager, the min-width of that column is fixed.
> > 
> > https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/
> > resources/content/device_manager.xul#43
> > 
> > Enlarge min-width to 20em will prevent text getting cropped, but I am not
> > sure if this is the correct way to fix.
> 
> I'm not sure how much longer that text needs to be in Polish but the problem
> seems to be the larger font than before. The window (rather than the column)
> is already resizable per-locale so perhaps that's enough:
> https://dxr.mozilla.org/mozilla-central/rev/
> 26e22af660e543ebb69930f082188b69ec756185/security/manager/locales/en-US/
> chrome/pippki/deviceManager.dtd#6-9
> 
> Stef, is devmgr.style enough for the device manager dialog?


Oh, forgot about it, changing the width to 62em seems to fix the issue.
Flags: needinfo?(splewako)
(Assignee)

Comment 7

a year ago
Here's my observation:

Connection dialog:

Mac: 44em
Win10: 40em
Ubuntu: 49em


Device manager dialog:

Mac: 62em
Win10: 50em
Ubuntu: 67em
(Assignee)

Updated

a year ago
Assignee: nobody → jyeh
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Hi Matthew,

According to comment 5, I put window.width and window.macWidth back with bumped string ID.

As described in comment 7, window.width2 is set to 49em and window.macWidth2 is set to 44em. And the width in devmgr.style is set to 67em which is the largest width needed in Ubuntu.

Thanks.
Thanks Joseph and Stefan.

(In reply to Stefan Plewako [:stef] from comment #6)
> (In reply to Matthew N. [:MattN] from comment #5)
> > (In reply to Joseph Yeh [:joseph] from comment #4)
> > > For the device manager, the min-width of that column is fixed.
> > > 
> > > https://dxr.mozilla.org/mozilla-central/source/security/manager/pki/
> > > resources/content/device_manager.xul#43
> > > 
> > > Enlarge min-width to 20em will prevent text getting cropped, but I am not
> > > sure if this is the correct way to fix.
> > 
> > I'm not sure how much longer that text needs to be in Polish but the problem
> > seems to be the larger font than before. The window (rather than the column)
> > is already resizable per-locale so perhaps that's enough:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 26e22af660e543ebb69930f082188b69ec756185/security/manager/locales/en-US/
> > chrome/pippki/deviceManager.dtd#6-9
> > 
> > Stef, is devmgr.style enough for the device manager dialog?
> 
> 
> Oh, forgot about it, changing the width to 62em seems to fix the issue.

Great, since the font-sizes changed by making this dialog in-content should we also change the ID of devmgr.style so localizers know to possibly change it? The downside is that they will maybe lose the old values they had though they'll probably need to be increased anyways.

(In reply to Joseph Yeh [:joseph] from comment #7)
> Here's my observation:

Are these the minimum values that you found reasonable or how did you come up with them?

Having these dialogs in mozscreenshots would make it easy for me to review…
Flags: needinfo?(jyeh)
Flags: needinfo?(francesco.lodolo)
Changing the ID would make sure people re-test, especially if the suggested values are higher. Also, falling back to en-US in the meantime is not as bad since it's not an English string showing up in the UI.

These are the current values in Aurora across locales (ignore the weird RTL representation)
https://transvision.mozfr.org/string/?entity=security/manager/chrome/pippki/deviceManager.dtd:devmgr.style&repo=aurora
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 12

a year ago
(In reply to Matthew N. [:MattN] from comment #10)
> (In reply to Joseph Yeh [:joseph] from comment #7)
> > Here's my observation:
> 
> Are these the minimum values that you found reasonable or how did you come
> up with them?
> 
> Having these dialogs in mozscreenshots would make it easy for me to review…

Yes, these are the minimum values that would fit the dialogs without text cropping in Polish.

I test it on my Mac and VM for Win10 and Ubuntu by adjusting the width manually until they fit in the dialog.
Flags: needinfo?(jyeh)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
I add two configurations named paneAdvanced-connectionDialog and paneAdvanced-deviceManagerDialog to the screenshot test.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=7e873393cc11d326338779e5a3ed2da031e30936&newProject=try&newRev=a1ebfd89b2ab12b296c41091a848882af7e07268&filter=paneAdvanced
Comment on attachment 8788334 [details]
Bug 1298872 - Fix text cropped and overflowed in advanced pane in-content dialogs;

https://reviewboard.mozilla.org/r/76856/#review77740

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:55
(Diff revision 2)
> +    "paneAdvanced-connectionDialog": {
> +      applyConfig: Task.async(function*() {
> +        let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");

Can you please rebase this on the latest version of the file which uses a different method to handle subdialogs.

::: security/manager/locales/en-US/chrome/pippki/deviceManager.dtd:9
(Diff revision 2)
>  
>  <!ENTITY devmgr.title                           "Device Manager">
>  <!-- LOCALIZATION NOTE (devmgr.style): This is CSS style for Device Manager
>       window size. Don't translate "width" nor "height". Adjust the numbers
>       to make window contents fit. -->
> -<!ENTITY devmgr.style                           "width: 52em; height: 32em;">
> +<!ENTITY devmgr.style                           "width: 67em; height: 32em;">

Could you also change this ID (here and in the file[s] that reference it) like was discussed in comment 11.
Attachment #8788334 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a year ago
Patch update with latest version.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=eaf5eb6f8fa0d8e7a09f3774c0da53c0dd6dadd7&newProject=try&newRev=c977aa83dec6c39b7ec7552edd27a7cc1c47896e&filter=prefsAdvanced
Comment on attachment 8788334 [details]
Bug 1298872 - Fix text cropped and overflowed in advanced pane in-content dialogs;

https://reviewboard.mozilla.org/r/76856/#review78074

Thanks Joseph, the screenshots looks good.
Attachment #8788334 - Flags: review?(MattN+bmo) → review+

Comment 19

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/91d86e6729a2
Fix text cropped and overflowed in advanced pane in-content dialogs; r=MattN

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91d86e6729a2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hi joseph,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?
Flags: needinfo?(jyeh)
(Assignee)

Comment 22

a year ago
Comment on attachment 8788334 [details]
Bug 1298872 - Fix text cropped and overflowed in advanced pane in-content dialogs;

Approval Request Comment
[Feature/regressing bug #]: bug 1036815
[User impact if declined]: Text gets cropped in Polish build.
[Describe test coverage new/current, TreeHerder]: Two screenshot tests added.
[Risks and why]: Low risk due to the changes are just enlarging the width of the dialog.
[String/UUID change made/needed]: String change made.
Flags: needinfo?(jyeh)
Attachment #8788334 - Flags: approval-mozilla-aurora?
Hi :flod, 
There is a string change in this patch, is this ok with you to change in aurora?
Flags: needinfo?(francesco.lodolo)
(In reply to Gerry Chang [:gchang] from comment #23)
> There is a string change in this patch, is this ok with you to change in
> aurora?

I wish this managed to land in m-c, but it's also true that falling back to en-US is OK in this case. Good to uplift for me.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8788334 [details]
Bug 1298872 - Fix text cropped and overflowed in advanced pane in-content dialogs;

This patch fixes a regression. Take it in 51 aurora.
Attachment #8788334 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 26

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dace749327dd
status-firefox51: affected → fixed
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.