"Find in Preferences" placeholder is truncated in French after visual refresh

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: tchevalier, Assigned: evanxd)

Tracking

(Blocks 1 bug, {nightly-community})

Trunk
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-preference])

Attachments

(5 attachments, 1 obsolete attachment)

Posted image Screenshot, French
Since today’s Nightly, the placeholder is truncated again. I assume it’s a regression caused by the Preferences visual changes that landed yesterday.

A solution was initially found in bug 1385902, but filing a new bug since it’s a regression.
See Also: → 1385902
Assignee: nobody → evan
Status: NEW → ASSIGNED
Whiteboard: [photon-preference]
Version: 56 Branch → Trunk
Summary: "Find in Preferences" placeholder is truncated in French → "Find in Preferences" placeholder is truncated in French after visual refresh
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Target Milestone: --- → Firefox 57
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8895252 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8895255 - Flags: review?(mconley)
Assignee

Comment 4

2 years ago
Posted image bug-1388348.png
Hi Mike,

Could you help review the patch?
The attachment is the result of applying the patch.

Thank you.

Comment 5

2 years ago
mozreview-review
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review171748

Thanks Evan. Going to r- for now until I hear back about the `ch` unit idea.

::: browser/themes/shared/incontentprefs/preferences.inc.css:701
(Diff revision 1)
>    top: 0;
>    z-index: 1;
>  }
>  
>  #searchInput {
> -  width: 230px;
> +  width: 250px;

This feels pretty string-sensitive... we can take this, but I'm worried that there are other locales where this is still a problem.

Perhaps we should use the `ch` unit instead? https://developer.mozilla.org/en-US/docs/Web/CSS/length#Font-relative_lengths
Attachment #8895255 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review171748

> This feels pretty string-sensitive... we can take this, but I'm worried that there are other locales where this is still a problem.
> 
> Perhaps we should use the `ch` unit instead? https://developer.mozilla.org/en-US/docs/Web/CSS/length#Font-relative_lengths

Good idea! Let's do it.
Assignee

Comment 11

2 years ago
Hi Mike,

I update the width with ch unit. Could you please help review it again?

Thank you.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review172448

I'm close to an r+, though I'm curious about what the textbox change is about.

I'm going to needinfo flod to see if he foresees any problems with the ~30 character limit.

Until then, I'm going to keep this review flag set.

::: browser/themes/shared/incontentprefs/preferences.inc.css:31
(Diff revision 5)
>  #mainPrefPane deck,
>  #mainPrefPane description {
>    font-size: 1.36rem;
>  }
>  
> -input,
> +textbox,

Why is this needed?

::: browser/themes/shared/incontentprefs/preferences.inc.css:696
(Diff revision 5)
>    top: 0;
>    z-index: 1;
>  }
>  
>  #searchInput {
> -  width: 230px;
> +  width: 30.6ch;

It might be worth adding a comment to this string:

http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/locales/en-US/chrome/browser/preferences/preferences.properties#259-261

Saying that localizers have 30 characters to work with for the placeholder.
Actually, flod is traveling, so I'll try Pike.

Hey Pike, do you foresee any difficulty in having a 30 character width for the search input in about:preferences - specifically for the placeholder text? Or is that too short for some locales?
Flags: needinfo?(l10n)
Generally speaking, a max-width is just waiting for a locale to break it. Can't we just use min-width and scale up if needed? That would generally give you the layout you intend without breaking the functionality.

I'm also not sure if 'em' is what you want. I'd loop in jfkthame to find languages with fonts where that differs much, and to check if that's actually within the realm of our intended design.

As for the length of the string, `rm` seems the longest on https://transvision.mozfr.org/string/?entity=browser/chrome/browser/preferences/preferences.properties:searchInput.labelWin&repo=central and https://transvision.mozfr.org/string/?entity=browser/chrome/browser/preferences/preferences.properties:searchInput.labelUnix&repo=central.

As for the localization note, I think a note that encourages localizers to find shorter alternatives if it's growing to 30 characters or glyphs would help. I'm not sure this is the UI where we have to put up a hard constraint.
Flags: needinfo?(l10n)
Assignee

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review172448

> Why is this needed?

Do this because once we change the font size of textbox in the css rule and then the `#searchInput` textbox will update it's width automatically.

> It might be worth adding a comment to this string:
> 
> http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/browser/locales/en-US/chrome/browser/preferences/preferences.properties#259-261
> 
> Saying that localizers have 30 characters to work with for the placeholder.

Sure, let's do it.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8895255 - Flags: review?(mconley) → review?(timdream)
Assignee

Comment 17

2 years ago
Hi Tim,

Since Mike in on PTO, could you help review the patch?

Thank you.
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review172668

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:260
(Diff revision 6)
>  
>  removeContainerOkButton=Remove this Container
>  removeContainerButton2=Don’t remove this Container
>  
>  # Search Input
> +# Localizers have 30 characters to work with for the placeholder.

`# LOCALIZATION NOTE: Please keep the placeholder string shorter than around 30 characters to avoid truncation`
Attachment #8895255 - Flags: review?(timdream) → review+
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review172680

Giving this an r- from an l10n pov. I think this should use min-width instead of width. Also, the visual impact on fonts with off-ch width doesn't seem to be tested. Manually tested, I don't think there's an automatic test that would make sense.
Attachment #8895255 - Flags: review-
(In reply to Axel Hecht [:Pike] from comment #20)
> Giving this an r- from an l10n pov. I think this should use min-width
> instead of width. Also, the visual impact on fonts with off-ch width doesn't
> seem to be tested. Manually tested, I don't think there's an automatic test
> that would make sense.

I assume you mean we should stick to `px` instead of `ch`.

I have a question about `min-width` though: I don't think placeholder text expand the textbox, so using `min-width` should have no effect compared to `width`? Am I missing something that could cause <xul:textbox> to expand with placeholder text and input text?
Flags: needinfo?(l10n)
TBH, I'm not sure about px vs ch. What would happen to folks that set their UI font size bigger for a11y purposes? Maybe that's a case to be made for ch? I don't see the font-size pref having an impact on about:preferences, though I think it does on about:home. Not sure about:newtab. Do we have UX guidelines for that? Makes me wonder if we have telemetry on non-default settings for system fonts to see what people use as their OS font settings in the wild.

Re being able to use min-width, I don't know either, my devtools-on-nightly attempts don't seem to have much of an effect.

If we can't expand auto-magically, should the width be localizable? Within the same caveats for the font-size a11y features.

We should definitely test the current UX with Romansh at least, to be sure that the problem's not just shifted from one to the other.
Flags: needinfo?(l10n)
Comment hidden (mozreview-request)
Assignee

Comment 24

2 years ago
Posted image search-input-fr.png
Assignee

Comment 25

2 years ago
Posted image search-input-rm.png
Assignee

Comment 26

2 years ago
Hi Axel,

> We should definitely test the current UX with Romansh at least, to be sure
> that the problem's not just shifted from one to the other.

Discussed with Tim in person, we would like to update the patch with using px unit , setting a fixed width, and adding a localization note to fix this issue.

After updating the patch, I've tested with Romansh and French. The results are good. See them in attachment 8896832 [details] (for Romansh) and attachment 8896831 [details] (for French).
Assignee

Comment 27

2 years ago
Hi Axel,

What do you think of Comment 26? If you feel OK, let's r+ the patch and land it.

Thank you.
Flags: needinfo?(l10n)

Comment 28

2 years ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '4150' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: got lock after 222 seconds
remote: waiting for lock on repository /repo/hg/mozilla/try held by process '20022' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: abort: repository /repo/hg/mozilla/try: timed out waiting for lock held by 'hgssh4.dmz.scl3.mozilla.com/effffffc:20022'
abort: stream ended unexpectedly (got 0 bytes, expected 4)
I'm pretty sure that there are ways to set up your OS that is going to break this.

I strongly disagree that we claim this to be a localizer problem. Asking localizers to add inferior translations because we can't figure out how to set the width on a placeholder doesn't sound right.

That doesn't mean that I can propose a working technical solution either.

My guess is that we'll just land something that works now, and play it by ear. Once we hear from people that have cropped strings, we'll see why they're cropped in practice, and land (and possibly uplift) lengths that work better.
Flags: needinfo?(l10n)
Assignee

Comment 30

2 years ago
> My guess is that we'll just land something that works now, and play it by
> ear. Once we hear from people that have cropped strings, we'll see why
> they're cropped in practice, and land (and possibly uplift) lengths that
> work better.

Sure, let's land something that works now and hear feedback.
Attachment #8895255 - Flags: review?(rchien)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8895255 [details]
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.

https://reviewboard.mozilla.org/r/166420/#review173924
Attachment #8895255 - Flags: review?(rchien) → review+

Comment 32

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ce23254ee8c
Set width of search input as 250px to match Photon visual spec. r=rickychien,timdream

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ce23254ee8c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Comment 34

2 years ago
Hi,

This issue is verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12.
But on Ubuntu 16.04 the bug is still reproducible. So should I mark this bug as verified and log a new bug for Ubuntu?
Flags: needinfo?(evan)
Assignee

Comment 35

2 years ago
Yes, you're right. We still could reproduce this on Ubuntu. Let's file a new issue for it. Thank you.

This is also about font size issue, we will update the font size on Bug 1392532.
Flags: needinfo?(evan)
Assignee

Updated

2 years ago
Blocks: 1388414
Assignee

Updated

2 years ago
No longer blocks: 1388414
See Also: → 1392601
I filed bug 1392601 for Ubuntu.
As per comment 34, I will mark this bug as Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.