Firefox for Android Aurora 33.0a2 20140804, Android 4.1.2 on Nexus S Bug 900692 added a checkbox with the label 'Paint flashing' to the developer settings. Unfortunately, some localizations (e.g. German) are quite longer, e.g. "Hervorhebung neu gezeichneter Flächen". Firefox OS had to make the developer settings support two lines for the labels. Please also extend the available space here.

This is probably something as simple as changing some of the xml attributes of the "Paint flashing" TextView so it wraps instead of truncating and adding ellipses.

Hello, I started to work on this bug. 1- I have one question regarding the scope of the bug, should we implement a fix only for the “Paint flashing” checkbox or should we implement a fix for all the checkboxes in the settings menu? There are probably other localisations with the same issue? 2- On a device with a trakball or D-pad (tested on my Desire-Z for instance), it works without any issue. When the checkbox is selected using the D-pad, the text scrolls automatically to the left and you can read the end of the text. 3- I found that xml attributes cannot be used in this case. The xml element is “CheckBoxPreference” (http://developer.android.com/reference/android/preference/CheckBoxPreference.html) and there is no solution to change the style in the XML. But the documentation gives another way to implement the wrap/ellipse properties. In the method onBindView: “This is a good place to grab references to custom Views in the layout and set properties on them.”. I made a test, and that solution works (see attached apk). Let me know what you want to do. Regards, Dom

Hi Dominique, thanks for picking up this bug! With respect to your questions: 1. Let's do this just for checkboxes that we're seeing problems for - if it does end up being a much more widespread problem, then we can reconsider, but once this bug is fixed, using the solution for other problems will be easy. 2. Good to know - does your patch cause any problems with this kind of device? And we still do want to fix this for other devices because percentage-wise, devices with trackballs are not very common. 3. That's fine then - have you checked that we can't do what we need with setLayoutResource? That would be ideal, but we can use a custom CheckBox if necessary.

Comment on attachment 8473032 [details] [diff] [review] Proposed patch Review of attachment 8473032 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! I think this could be fine, if setLayoutResource doesn't work. ::: mobile/android/base/preferences/CustomCheckBoxPreference.java @@ +13,5 @@ > +/** > + * Represents a Checkbox element in a preference menu. > + * The title of the Checkbox can be larger than the view, > + * in this case it will be displayed in 2 or more lines. > + * The default behavior of the class CheckBoxPreference Nit: trailing whitespace @@ +35,5 @@ > + protected void onBindView(View view) { > + super.onBindView(view); > + TextView title = (TextView) view.findViewById(android.R.id.title); > + if (title != null) { > + title.setSingleLine(false); Nit: trailing ws ::: mobile/android/base/preferences/CustomListPreference.java @@ +89,5 @@ > public void setIsDefault(boolean isDefault) { > mIsDefault = isDefault; > if (isDefault) { > setOrder(0); > + setOrder(0); Duplicate?

Thanks for the code review, new patch added with your changes. About your remarks: 2 - It works also on a device with a D-Pad. The auto scroll of the title is not triggered because all the text is already visible. 3- I didn’t find a nice solution. The setLayoutResource implementation won’t be object oriented code, I need to test the preference keys in class GeckoPreferences for each checkbox concerned by the wrap: if (PREFS_DEVTOOLS_PAINT_FLASHING.equals(key)) { pref.setLayoutResource( R.layout.preference_custom_checkbox ); } and it will duplicate some xml files from the SDK just to set the wrap property of the title. The risk here is to get differences on devices between the checkboxes with the wrap option and the checkboxes without. The effort to add new checkboxes is also bigger, for each checkbox, we have to add a test in GeckoPreferences. When the SDK files will change, we will have to follow the changes in the duplicated files … not good for the code debt! It’s also probably for all those reasons that the change of the properties in the onBindView method is the only proposal of the Android documentation.

Final version of the patch after code review changes.

Comment on attachment 8474446 [details] [diff] [review] patch1048418AfterCodeReview.diff Review of attachment 8474446 [details] [diff] [review]: ----------------------------------------------------------------- Looks great when I tried it out! Just two very small nits, that are below. Also, not related to the code, but part of our patch-writing process, you should match the patch name to the bug title, and append me (liuche) as the reviewer. Bug 1048418: 'Paint flashing' localizations need more space for text, gets cropped/truncated. r=liuche Also, on future patches, you should set a r? flag to be the mentor so that person gets a notification to review your patch. You can also do f? for feedback if you're not sure about the approach. Try run: https://tbpl.mozilla.org/?tree=Try&rev=976e52e6e50b Thanks, Dominique! (If you're still looking for more bugs after this, feel free to check http://www.joshmatthews.net/bugsahoy/?mobile=1&unowned=1 or jump into #mobile and ask around!) ::: mobile/android/base/preferences/CustomCheckBoxPreference.java @@ +12,5 @@ > + > +/** > + * Represents a Checkbox element in a preference menu. > + * The title of the Checkbox can be larger than the view, > + * in this case it will be displayed in 2 or more lines. Nit: run-on sentence @@ +33,5 @@ > + > + @Override > + protected void onBindView(View view) { > + super.onBindView(view); > + TextView title = (TextView) view.findViewById(android.R.id.title); Nit: make this final.

Last patch (including the 3 last changes: patch title, run-on sentence, final keyword)

Comment on attachment 8475074 [details] [diff] [review] patch1048418.diff Review of attachment 8475074 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I'll flag this for checkin since there's a green try run.