1589652 - Long sync engine choice translations cause issues about:preferences#sync and “Choose what to sync” dialog

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Théo Chevalier]

Attachment: Capture d’écran 2019-10-18 à 12.55.38.png

Even when the panel is wide enough, the label keeps wrapping

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Recent user-visible regression

Comment 2

[:Gijs (he/him)]

Mark, this is another one, I imagine the fix will be similar / the same as bug 1589651, but fyi...

Comment 3

[janet [:janet]]

Mark is working on this now.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: long-engine-names.png

This is what the patch I'm about to upload looks like with long engine names (although each part of the image is scaled differently) - obviously though I'm not going to include the changes to the strings or the width I used for testing.

• In about:preferences#sync, we don't really have the ability to widen the area used to display the names - so at some point we are still forced to wrap. However, not all the available space was used before we did wrap, and I've resolved that (ie, the labels now get much closer to the middle/end before wrapping). The patch also ensures that the icon and the label are not split, which is also what we saw before. I think even with the bug as reported, the translated string will fit without any wrapping, so this should be considered a "worst case".

• In the "choose what to sync" dialog we do have the ability to increase the width of the dialog - and this can be done in the .ftl file, so it can be different for each translation. In the screenshot attached, the .ftl specifies the width should be 60em instead of the 36em used by the EN/default .ftl file. Note that I lost my battle with CSS, so I couldn't arrange for it to also wrap if the specified width wasn't wide enough - so an overly long label will "bleed" into the second column if the width is too small - but the solution there is simply to adjust the width in that translation's .ftl file.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

FTR, part of the .ftl patch I used for testing included:

@@ -798,32 +798,32 @@ sync-change-options =

 sync-choose-what-to-sync-dialog =
     .title = Choose What To Sync
-    .style = width: 36em; min-height: 35em;
+    .style = width: 60em; min-height: 35em;
     .buttonlabelaccept = Save Changes

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1589652 - better handling of long sync engine names in the preferences UI. r?Gijs

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da926e5cf009
better handling of long sync engine names in the preferences UI. r=Gijs

Comment 9

[Dorel Luca [:dluca]]

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

Comment 10

[Pascal Chevrel:pascalc]

Mark, this bug is a P1 and the fix is CSS only, do you want to request an uplift to beta? Thanks

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 9103833 [details]
Bug 1589652 - better handling of long sync engine names in the preferences UI. r?Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Visually ugly UI for some non-English users
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): CSS only change, impact limited to sync preferences.
• String changes made/needed: None

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9103833 [details]
Bug 1589652 - better handling of long sync engine names in the preferences UI. r?Gijs

71 regression, visual fix for non-English users, CSS only and low risk, uplift approved for 71 beta 6, thanks.

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cecf35abeef6

Comment 14

[Alexandru Ionescu (needinfo me) [:alexandrui]]

For the record, this cause the below regression, but was fixed 4 days later.
== Change summary for alert #23716 (as of Tue, 05 Nov 2019 11:06:55 GMT) ==

Regressions:

6% JS linux64-shippable opt stylo tp6 194,898,781.08 -> 206,764,398.62
3% JS linux64-shippable opt stylo tp6 202,861,674.92 -> 208,933,206.16

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23716

Comment 15

[:Gijs (he/him)]

(In reply to Alexandru Ionescu :alexandrui from comment #14)

For the record, this cause the below regression, but was fixed 4 days later.
== Change summary for alert #23716 (as of Tue, 05 Nov 2019 11:06:55 GMT) ==

Regressions:

6% JS linux64-shippable opt stylo tp6 194,898,781.08 -> 206,764,398.62
3% JS linux64-shippable opt stylo tp6 202,861,674.92 -> 208,933,206.16

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23716

This genuinely doesn't make any sense - the only changes were to files not opened in this test. You're probably mis-attributing these from some other changeset.

Comment 16

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to :Gijs (he/him) from comment #15)

This genuinely doesn't make any sense - the only changes were to files not opened in this test. You're probably mis-attributing these from some other changeset.

Did some retriggers to confirm that.

Comment 17

[:Gijs (he/him)]

(In reply to Alexandru Ionescu :alexandrui from comment #16)

(In reply to :Gijs (he/him) from comment #15)

This genuinely doesn't make any sense - the only changes were to files not opened in this test. You're probably mis-attributing these from some other changeset.

Did some retriggers to confirm that.

Can we get a bug on file to figure out why the test values were influenced by this change? It doesn't make a lot of sense, and suggests the test is bogus.

Comment 18

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Done that.