Closed Bug 1388348 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: theo, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, Whiteboard: [photon-preference])

Attachments

(5 files, 1 obsolete file)

Screenshot, French
35.42 KB, image/png
Details
Bug 1388348 - Set width of search input as 250px to match Photon visual spec.
59 bytes, text/x-review-board-request
rickychien
: review+
Details
bug-1388348.png
15.45 KB, image/png
Details
search-input-fr.png
13.15 KB, image/png
Details
search-input-rm.png
12.21 KB, image/png
Details
Attached 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
Attachment #8895252 - Attachment is obsolete: true
Attachment #8895255 - Flags: review?(mconley)
Attached image bug-1388348.png — 
Hi Mike,

Could you help review the patch?
The attachment is the result of applying 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/#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 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.
Hi Mike,

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

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/#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)
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.
Attachment #8895255 - Flags: review?(mconley) → review?(timdream)
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 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)
Attached image search-input-fr.png — 

    
    

    
  

      
      
      
      

        Attached image
          
        search-input-rm.png
        — 
      

    


  

    
    

    
  
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).

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
> 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 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+

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5ce23254ee8c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED

    
    

    
  
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)

    
    

    
  
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)

    
  
Blocks: 1388414

    
  
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

          status-firefox57:
          fixed → verified

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: