715179 - UI for changing default font size for font inflation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Patryk Adamczyk [:patryk] UX]

The basic font size setting has been implemented but should be expanded to included specified interaction / features. See screenshot:
http://www.flickr.com/photos/patrykdesign/6427428933/in/photostream

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Matt - I thought you already had a bug for this

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Matt - I thought you already had a bug for this

Bug 703029 added a bare-bones UI with a standard menu list.  This bug is about replacing that basic list with the custom dialog from Patryk's mockup in comment 0.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: 1/2: new string for preview dialog

This just adds the new string from bug 703029 comment 7.  I hope to have the rest of the work done within a few days, but we could land this part early to make sure localization is able to start.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 588547 [details] [diff] [review]
1/2: new string for preview dialog

This is fine, but I don't know if it needs to be landed before the rest of the code. L10N has told us the pre-landing strings without a way to see the strings in context is not the useful.

Comment 5

[Axel Hecht [:Pike]]

Is this still 11 material?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Axel Hecht [:Pike] from comment #5)
> Is this still 11 material?

IMO we can't really say until we see what is landed on m-c and evaluate the risk in taking it for fx11.

Comment 7

[Axel Hecht [:Pike]]

Mark, Erin, are we clearer on this now?

Comment 8

[Erin Lancaster [:elan]]

Not for 1.0 so won't land on aurora in april 2012

Comment 9

[Ian Barlow (:ibarlow)]

Soft blocker nom?

Seems like a bit of a stretch to get in, based on what I'm reading here, but it would be a nice and thoughtful piece of UI to have beyond our current selector menu.

Comment 10

[Erin Lancaster [:elan]]

We really can't add any UI for beta or for release. We should be back on the train schedule after we ship 1.0 of the rewrite so people can get this 6 weeks later.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'm ready for the graphics to fill the text change buttons. Please attach them when they are ready.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01: Implemented previous mbrubeck string changes.

This change has been broken into many patches. Please wait for my comment when I am finished.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 02: Created FontSizePreference.java file.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 03: Fixed indentation and modified a few strings.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05: Fleshed out the UI of the box - new buttons and preview TextView are present.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06: Dialog buttons work – they increase and decrease the size of the preview text.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07: Preferences are saved to Gecko.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08: (Last) Dialog dynamically resizes itself per device.

This is the last patch. Note that while each patch should compile on its own, they are not completely functioning – a button could cause a NullPointerException and such.

Note that this changeset is not complete – the final images for the buttons are not finished yet so placeholder buttons with text are used instead. See the previously linked mockup to see an example of the images.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also note that this only works on top of the patch from bug 770345, where a change of orientation does not restart the preferences activity.

I believe GeckoPreferences will throw a NullPointerException if the change font size buttons are pressed after an orientation change if this is used without this patch (due to onCreate and onWindowFocusChanged).

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 640368 [details] [diff] [review]
02: Created FontSizePreference.java file.

You shouldn't need this - fold patch 4 into this one.

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 640369 [details] [diff] [review]
03: Fixed indentation and modified a few strings.

Review of attachment 640369 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values/arrays.xml
@@ +27,1 @@
>          <item>80</item>

Why did you change 0 to 40?

::: mobile/android/base/resources/xml/preferences.xml.in
@@ +35,5 @@
>          <ListPreference     android:key="font.size.inflation.minTwips"
>                              android:title="@string/pref_text_size"
>                              android:entries="@array/pref_font_size_entries"
>                              android:entryValues="@array/pref_font_size_values"
>                              android:persistent="false" />

This one should also be changed

Comment 23

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> This one should also be changed

Wrong line...I was referring to this change:

-        <item>0</item>
+        <item>40</item>

Comment 24

[Brian Nicholson (:bnicholson)]

Comment on attachment 640370 [details] [diff] [review]
04: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Review of attachment 640370 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, except it should be folded into patch 2.

::: mobile/android/base/resources/xml/preferences.xml.in
@@ -35,5 @@
> -        <ListPreference     android:key="font.size.inflation.minTwips"
> -                            android:title="@string/pref_text_size"
> -                            android:entries="@array/pref_font_size_entries"
> -                            android:entryValues="@array/pref_font_size_values"
> -                            android:persistent="false" />

I guess you can ignore the last comment about indentation if you're removing this

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 640371 [details] [diff] [review]
05: Fleshed out the UI of the box - new buttons and preview TextView are present.

Review of attachment 640371 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/FontSizePreference.java
@@ +43,5 @@
> +            mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
> +
> +            mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
> +            mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
> +        } catch (InflateException ex) {

No need for this try/catch

::: mobile/android/base/resources/layout/font_size_preference.xml
@@ +12,5 @@
> +              android:layout_width="match_parent"
> +              android:layout_height="wrap_content"
> +              android:layout_weight="1"
> +              android:background="#ffffffff"
> +              android:padding="2pt"

Use dip rather than pt.

@@ +27,5 @@
> +        <Button android:id="@+id/decrease_preview_font_button"
> +                android:layout_width="0dp"
> +                android:layout_height="match_parent"
> +                android:layout_weight="1"
> +                android:text="A"

"A" should be localized as in patch 1.

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 640371 [details] [diff] [review]
05: Fleshed out the UI of the box - new buttons and preview TextView are present.

Review of attachment 640371 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/layout/font_size_preference.xml
@@ +37,5 @@
> +                android:layout_height="match_parent"
> +                android:layout_weight="1"
> +                android:text="A"
> +                android:textSize="16sp"
> +                android:onClick="increasePreviewFontSize"/>

Also, I think it would be cleaner if you dropped the "increasePreviewFontSize" from here and GeckoPreferences. Just add the listeners in FontSizePreference so you don't have to forward them from GeckoPreferences.

Comment 27

[Matt Brubeck (:mbrubeck)]

(In reply to Brian Nicholson (:bnicholson) from comment #25)
> "A" should be localized as in patch 1.

I'm not sure we want to localize the "A" here, especially since it's just a placeholder until we have an image (which will also be a letter "A").

Comment 28

[Brian Nicholson (:bnicholson)]

Comment on attachment 640372 [details] [diff] [review]
06: Dialog buttons work – they increase and decrease the size of the preview text.

Review of attachment 640372 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall if you remove your changes to GeckoPreferences.

::: mobile/android/base/GeckoPreferences.java
@@ +44,5 @@
>      private PreferenceScreen mPreferenceScreen;
>      private static boolean sIsCharEncodingEnabled = false;
>      private static final String NON_PREF_PREFIX = "android.not_a_preference.";
> +    private static final String FONT_SIZE_PREF_KEY = "font.size.inflation.minTwips";
> +    private FontSizePreference mFontSizePreference;

You shouldn't need mFontSizePreference if you remove these callbacks from GeckoPreference as described above.

Comment 29

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #27)
> I'm not sure we want to localize the "A" here, especially since it's just a
> placeholder until we have an image (which will also be a letter "A").

I take this back; keeping this as a localizable string seems like a good idea.  Just be sure to include an L10n note in the string file to explain to translators how this string is used.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 640369 [details] [diff] [review]
> 03: Fixed indentation and modified a few strings.
> 
> Review of attachment 640369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/values/arrays.xml
> @@ +27,1 @@
> >          <item>80</item>
> 
> Why did you change 0 to 40?

While I'm not sure what happens in Gecko, in the Android TextView I use to create the dialog, giving a font size of 0 pt causes no change – the font size will remain the previous value.

The choice to 40 was fairly arbitrary (I just wanted a number divisible by 20 so that it converts well from twips), though the text is considerably small on both my tablet and my phone.

Comment 31

[Brian Nicholson (:bnicholson)]

Comment on attachment 640373 [details] [diff] [review]
07: Preferences are saved to Gecko.

Review of attachment 640373 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits fixed

::: mobile/android/base/FontSizePreference.java
@@ +38,5 @@
>      /** Index into the above arrays for the saved preference value (from Gecko). */
>      private int mSavedFontIndex = DEFAULT_FONT_INDEX;
>      /** Index into the above arrays for the currently displayed font size (the preview). */
>      private int mPreviewFontIndex = mSavedFontIndex;
> +

nit: remove blank line

@@ +89,5 @@
> +        if (prefChangeListener == null) {
> +            Log.e(LOGTAG, "PreferenceChangeListener is null. FontSizePreference will not be saved to Gecko.");
> +            return;
> +        }
> +        prefChangeListener.onPreferenceChange(this, (Object) twipVal);

You don't need this (Object) cast.

::: mobile/android/base/GeckoPreferences.java
@@ +171,5 @@
>              ((ListPreference)preference).setSummary(newEntry);
>          }
>          if (preference instanceof LinkPreference)
>              finish();
> +        if (preference instanceof FontSizePreference) {

Change these 'if's to be 'else if's.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 03a: Fixed indentation and modified a few strings.

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 640369 [details] [diff] [review]
> 03: Fixed indentation and modified a few strings.
> 
> Review of attachment 640369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/xml/preferences.xml.in
> @@ +35,5 @@
> >          <ListPreference     android:key="font.size.inflation.minTwips"
> >                              android:title="@string/pref_text_size"
> >                              android:entries="@array/pref_font_size_entries"
> >                              android:entryValues="@array/pref_font_size_values"
> >                              android:persistent="false" />
> 
> This one should also be changed

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08a: (Last) Dialog dynamically resizes itself per device.

Uploaded correct patch.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04a: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Folded patch 02 into patch 04. There should be no other changes. 04 was previously okayed after review.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05a: Fleshed out the UI of the box - new buttons and preview TextView are present.

(In reply to Brian Nicholson (:bnicholson) from comment #25)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/FontSizePreference.java
> @@ +43,5 @@
> > +            mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
> > +
> > +            mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
> > +            mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
> > +        } catch (InflateException ex) {
> 
> No need for this try/catch
Removed.

> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +12,5 @@
> > +              android:layout_width="match_parent"
> > +              android:layout_height="wrap_content"
> > +              android:layout_weight="1"
> > +              android:background="#ffffffff"
> > +              android:padding="2pt"
> 
> Use dip rather than pt.
Done.

> @@ +27,5 @@
> > +        <Button android:id="@+id/decrease_preview_font_button"
> > +                android:layout_width="0dp"
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> 
> "A" should be localized as in patch 1.
Done.

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +37,5 @@
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> > +                android:textSize="16sp"
> > +                android:onClick="increasePreviewFontSize"/>
> 
> Also, I think it would be cleaner if you dropped the
> "increasePreviewFontSize" from here and GeckoPreferences. Just add the
> listeners in FontSizePreference so you don't have to forward them from
> GeckoPreferences.
I will apply this in a new patch on top of everything – I think it would be the cleanest approach.

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06a: Dialog buttons work – they increase and decrease the size of the preview text.

Rebased patch 06. Moved r+ to this patch.

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07a: Preferences are saved to Gecko.

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Comment on attachment 640373 [details] [diff] [review]
> 07: Preferences are saved to Gecko.
> 
> Review of attachment 640373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits fixed
> 

Fixed. Moved r+.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08b: (Last) Dialog dynamically resizes itself per device.

Rebase. Previous review was "?".

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 09: Moved the font size callback functions from GeckoPreferences to FontSizePreferences.

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +37,5 @@
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> > +                android:textSize="16sp"
> > +                android:onClick="increasePreviewFontSize"/>
> 
> Also, I think it would be cleaner if you dropped the
> "increasePreviewFontSize" from here and GeckoPreferences. Just add the
> listeners in FontSizePreference so you don't have to forward them from
> GeckoPreferences.

Moved with this patch.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10: Special cased twip size of 0.

As per mbrubeck's comment on IRC, I special cased a twip size of 0 that represents no font inflation. This patch will render text at a size of 1pt for a twip size of 0.

However, I just realized that this preference doesn't explain that it's inflating font, but rather implies that it's setting a universal text size (since the title is "Text Size" and the summary text is "Tiny", "Small", etc.). While I think a user would end up at the same outcome if it were explained as inflation (if they're trying to make the text larger, they're going to select a size that looks good for them without caring that it's inflation rather than a universal text size), it's a little misleading. For example, the idea that text inflation is turned off when "Text Size" is set to "Tiny" doesn't really feel right. I think the stock browser has a more intuitive representation of this.

So I think this dialog is better than it was before and we should push it, but we may want to reconsider it in the future.

Also, I have an idea. We should reload the current tab if the text size setting is changed (otherwise a user might change the text size, go to the page and see no difference, change it again, still get no results, etc.). I'm not sure how to implement this (if it's even possible - though I assume so), but do you want me to take a look into it?

Comment 41

[Matt Brubeck (:mbrubeck)]

(In reply to Michael Comella (:mcomella) from comment #40)
> However, I just realized that this preference doesn't explain that it's
> inflating font, but rather implies that it's setting a universal text size
> (since the title is "Text Size" and the summary text is "Tiny", "Small",
> etc.)

I agree that a better name would be good, though I'm not sure of a name that is both more accurate and easily understandable.  Maybe "Text Zoom"?

We should also change the "Tiny" string to something "Original size" or "No zoom".

(Let's file a follow-up bug for these changes.)

> Also, I have an idea. We should reload the current tab if the text size
> setting is changed (otherwise a user might change the text size, go to the
> page and see no difference, change it again, still get no results, etc.).

Yes, and ideally we should re-lay-out the page without actually reloading it, to avoid losing any state.  I thought this already worked, though in my testing I see it doesn't.  (So either it stopped working, or I was just imagining things.)  Please file another follow-up bug for this.

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Matt Brubeck (:mbrubeck) from comment #41)
> I agree that a better name would be good, though I'm not sure of a name that
> is both more accurate and easily understandable.  Maybe "Text Zoom"?
> 
> We should also change the "Tiny" string to something "Original size" or "No
> zoom".
> 
> (Let's file a follow-up bug for these changes.)

Bug 752985.

> > Also, I have an idea. We should reload the current tab if the text size
> > setting is changed (otherwise a user might change the text size, go to the
> > page and see no difference, change it again, still get no results, etc.).
> 
> Yes, and ideally we should re-lay-out the page without actually reloading
> it, to avoid losing any state.  I thought this already worked, though in my
> testing I see it doesn't.  (So either it stopped working, or I was just
> imagining things.)  Please file another follow-up bug for this.

Bug 772541

Comment 43

[Brian Nicholson (:bnicholson)]

Comment on attachment 640456 [details] [diff] [review]
03a: Fixed indentation and modified a few strings.

Review of attachment 640456 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values/arrays.xml
@@ +22,5 @@
>          <item>@string/pref_font_size_large</item>
>          <item>@string/pref_font_size_xlarge</item>
>      </string-array>
>      <string-array name="pref_font_size_values">
> +        <item>40</item>

Since you're reverting this in patch 10, it'd be better to just not have this change at all.

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 03b: Fixed indentation and modified a few strings.

(In reply to Brian Nicholson (:bnicholson) from comment #43)
> Comment on attachment 640456 [details] [diff] [review]
> 03a: Fixed indentation and modified a few strings.
> 
> Review of attachment 640456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/values/arrays.xml
> @@ +22,5 @@
> >          <item>@string/pref_font_size_large</item>
> >          <item>@string/pref_font_size_xlarge</item>
> >      </string-array>
> >      <string-array name="pref_font_size_values">
> > +        <item>40</item>
> 
> Since you're reverting this in patch 10, it'd be better to just not have
> this change at all.

Removed. Moved r+ to new file. Will upload updated patch 10.

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10a: Special cased twip size of 0.

Rebased.

Comment 46

[Brian Nicholson (:bnicholson)]

Comment on attachment 640477 [details] [diff] [review]
05a: Fleshed out the UI of the box - new buttons and preview TextView are present.

I noticed you're using tabs here:

+    @Override
+    protected void onPrepareDialogBuilder(AlertDialog.Builder builder) {
+        View dialogView = null;
+
+        final LayoutInflater inflater =
+            (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
+	dialogView = inflater.inflate(R.layout.font_size_preference, null);
+	mDecreaseFontButton = (Button) dialogView.findViewById(R.id.decrease_preview_font_button);
+	mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
+
+	mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
+	mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
+        
+        builder.setView(dialogView);

Change this to use spaces (along with any other patches where you're using tabs).

Also, I think it would be cleaner if you folded patch 10 into this one so you can drop the font size callbacks in GeckoPreferences. You should generally try to avoid adding code that will simply be reverted in later patches.

Comment 47

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Also, I think it would be cleaner if you folded patch 10 into this one

Rather, patch 9

Comment 48

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Depends on bug 770345, otherwise the dialog may cause a NullPointerException on rotation.

Comment 49

[Brian Nicholson (:bnicholson)]

Comment on attachment 640485 [details] [diff] [review]
08b: (Last) Dialog dynamically resizes itself per device.

+                // Line height does not take into account spacing between lines. Since this info
+                // is not available until API 16, we add an arbitrary constant per line.

The method getLineSpacingExtra() was introduced in API 16, but it looks like an attribute has existed since API 1:
http://developer.android.com/reference/android/R.attr.html#lineSpacingExtra

+    private void setButtonState(int index) {
+        if (index == 0) {
+            mDecreaseFontButton.setEnabled(false);
+            mIncreaseFontButton.setEnabled(true);
+        } else if (index == mFontTwipValues.length - 1) {
+            mDecreaseFontButton.setEnabled(true);
+            mIncreaseFontButton.setEnabled(false);
+        } else {
+            mDecreaseFontButton.setEnabled(true);
+            mIncreaseFontButton.setEnabled(true);
+        }
+    }

Have these buttons enabled in XML and remove the setEnabled(true) calls.

+		if (pref instanceof FontSizePreference) {
+		    // Note, for safety, this instantiation is better put in onCreate. However, it is in
+		    // onWindowFocusChanged for speed purposes. If it is null, you may need to move it.
+		    mFontSizePreference = (FontSizePreference) pref;
+		}

+    private static final String NULL_FONT_SIZE_PREF_ERR_MSG = "mFontSizePreference is null. Buttons " +
+	"will have no effect.";

I don't think we want this change; if you're seeing a problem with onCreate()/onWindowFocusChanged() in your testing, we should just go ahead and make the fix.

decreasePreviewFontSize/increasePreviewFontSize/mFontSizePreference should be gone once patch 5 is updated.

Comment 50

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05b: Fleshed out the UI of the box - new buttons and preview TextView are present.

Removed tabs. More updates to follow.

Comment 51

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06b: Dialog buttons work – they increase and decrease the size of the preview text.

Tabs -> spaces. Moved r+.

Comment 52

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07b: Preferences are saved to Gecko.

Tabs -> spaces. Moved r+.

Comment 53

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08c: Dialog dynamically resizes itself per device.

Tabs -> spaces. Moved r-.

Comment 54

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 09a: Moved the font size callback functions from GeckoPreferences to FontSizePreferences.

Tabs -> spaces. Need to fold into patch 5 so turned off review.

Comment 55

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10b: Special cased twip size of 0.

Tabs -> spaces. Moved r+.

Comment 56

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Comment on attachment 640477 [details] [diff] [review]
> 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
>
> Change this to use spaces (along with any other patches where you're using
> tabs).

Done.

Comment 57

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #56)
> (In reply to Brian Nicholson (:bnicholson) from comment #46)
> > Comment on attachment 640477 [details] [diff] [review]
> > 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> > present.
> >
> > Change this to use spaces (along with any other patches where you're using
> > tabs).
> 
> Done.

Sorry, I may have done this incorrectly. I will update as soon as I fix it.

Comment 58

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04b: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Correct indentation. Moved r+.

Comment 59

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05c: Fleshed out the UI of the box - new buttons and preview TextView are present.

'a(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Comment on attachment 640477 [details] [diff] [review]
> 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Also, I think it would be cleaner if you folded patch 10 into this one so
> you can drop the font size callbacks in GeckoPreferences.

(In reply to Brian Nicholson (:bnicholson) from comment #47)
> (In reply to Brian Nicholson (:bnicholson) from comment #46)
> > Also, I think it would be cleaner if you folded patch 10 into this one
> 
> Rather, patch 9

Folded patch 9 into 6 as I felt it was more appropriate. I will re-requset a review on 6 as a result. Patches 7 and 8 will get rebasing patches.

Comment 60

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05d: Fleshed out the UI of the box - new buttons and preview TextView are present.

Uploaded the wrong file. I accidentally nuked my patch directory but I think I recovered properly so please double check this review.

Comment 61

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06c: Dialog buttons work – they increase and decrease the size of the preview text.

Folded 9 -> 6.

Comment 62

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07c: Preferences are saved to Gecko.

Rebase. Moving r+.

Comment 63

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08d: Dialog dynamically resizes itself per device.

(In reply to Brian Nicholson (:bnicholson) from comment #49)
> Comment on attachment 640485 [details] [diff] [review]
> 08b: (Last) Dialog dynamically resizes itself per device.
> 
> I don't think we want this change; if you're seeing a problem with
> onCreate()/onWindowFocusChanged() in your testing, we should just go ahead
> and make the fix.
> 
> decreasePreviewFontSize/increasePreviewFontSize/mFontSizePreference should
> be gone once patch 5 is updated.

Done and rebased.

Comment 64

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05e: Fleshed out the UI of the box - new buttons and preview TextView are present.

Rebasing error.

Comment 65

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05f: Fleshed out the UI of the box - new buttons and preview TextView are present.

My apologies. Wrong file upload.

Comment 66

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06d: Dialog buttons work – they increase and decrease the size of the preview text.

Rebase.

Comment 67

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07d: Preferences are saved to Gecko.

Rebase. Move r+.

Comment 68

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07e: Preferences are saved to Gecko.

Wrong file upload - forgot to qref. Moved r+.

Comment 69

[Brian Nicholson (:bnicholson)]

Comment on attachment 640747 [details] [diff] [review]
10b: Special cased twip size of 0.

private void updatePreviewFontSize(String twip) {
-        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, convertTwipStrToPT(twip));
+    float pt = convertTwipStrToPT(twip);
+    // Android will not render a font size of 0 pt but for Gecko, 0 twip turns off font inflation.
+    // Thus we special case 0 twip to display a renderable font size.
+    if (pt == 0) {
+        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, 1);
+    } else {
+        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, pt);
+    }

Make sure you fix this spacing

Comment 70

[Brian Nicholson (:bnicholson)]

Comment on attachment 640826 [details] [diff] [review]
06d: Dialog buttons work – they increase and decrease the size of the preview text.

+    private static final String FONT_SIZE_PREF_KEY = "font.size.inflation.minTwips";

I think you can remove this.

Comment 71

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06e: Dialog buttons work – they increase and decrease the size of the preview text.

(In reply to Brian Nicholson (:bnicholson) from comment #70)
> Comment on attachment 640826 [details] [diff] [review]
> 06d: Dialog buttons work – they increase and decrease the size of the
> preview text.
> 
> +    private static final String FONT_SIZE_PREF_KEY =
> "font.size.inflation.minTwips";
> 
> I think you can remove this.

Done. Note it will be re-added in patch 8. Moving r+.

Comment 72

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08e: Dialog dynamically resizes itself per device.

(In reply to Brian Nicholson (:bnicholson) from comment #49)
> Comment on attachment 640485 [details] [diff] [review]
> 08b: (Last) Dialog dynamically resizes itself per device.
> 
> +                // Line height does not take into account spacing between
> lines. Since this info
> +                // is not available until API 16, we add an arbitrary
> constant per line.
> 
> The method getLineSpacingExtra() was introduced in API 16, but it looks like
> an attribute has existed since API 1:
> http://developer.android.com/reference/android/R.attr.html#lineSpacingExtra

Took this into account in this patch, though not using that attribute (I was unable to get the value so I overrode it instead).

> 
> +    private void setButtonState(int index) {
> +        if (index == 0) {
> +            mDecreaseFontButton.setEnabled(false);
> +            mIncreaseFontButton.setEnabled(true);
> +        } else if (index == mFontTwipValues.length - 1) {
> +            mDecreaseFontButton.setEnabled(true);
> +            mIncreaseFontButton.setEnabled(false);
> +        } else {
> +            mDecreaseFontButton.setEnabled(true);
> +            mIncreaseFontButton.setEnabled(true);
> +        }
> +    }
> 
> Have these buttons enabled in XML and remove the setEnabled(true) calls.

onPrepareDialogBuilder() gets called each time so the buttons are initialized each time and the extra .setEnabled(true) calls are unnecessary (as is the extra xml).

There were a lot of fundamental changes so please look over this change carefully.

Comment 73

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10c: Special cased twip size of 0.

(In reply to Brian Nicholson (:bnicholson) from comment #69)
> Comment on attachment 640747 [details] [diff] [review]
> 10b: Special cased twip size of 0.
> 
> private void updatePreviewFontSize(String twip) {
> -        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT,
> convertTwipStrToPT(twip));
> +    float pt = convertTwipStrToPT(twip);
> +    // Android will not render a font size of 0 pt but for Gecko, 0 twip
> turns off font inflation.
> +    // Thus we special case 0 twip to display a renderable font size.
> +    if (pt == 0) {
> +        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, 1);
> +    } else {
> +        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, pt);
> +    }
> 
> Make sure you fix this spacing

Fixed and rebased. Moving r+.

Comment 74

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08f: Dialog dynamically resizes itself per device.

As discussed with bnicholson on IRC, I replaced the arbitrary line spacing constant with a line to remove the padding whose value we could determine – TextView.setIncludeFontPadding(...) (which is why we needed the constant in the first place).

Comment 75

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08g: Dialog dynamically resizes itself per device.

Adjusted the padding from the previous change.

Comment 76

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08h: Dialog dynamically resizes itself per device.

As per discussion with bnicholson in real life/IRC, moved the onGlobalLayoutListener so it set only once per dialog. While I feel the change made the code less linear, I feel it is more intuitive.

This change required a lot of renaming so you may need to be extra careful with the review.

Comment 77

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10d: Special cased twip size of 0.

Rebase. Move r+.

Comment 78

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04c: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Fixed Galaxy Note rotation bug. Only changed one line in resources/layout/font_size_preference.xml.

bnicholson: Removed r+ – while nothing but the above line should have changed, please check this is okay.

Comment 79

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08i: Dialog dynamically resizes itself per device.

Rebase from Galaxy Note fix.

Comment 80

[Brian Nicholson (:bnicholson)]

Comment on attachment 642755 [details] [diff] [review]
04c: Created a simple dialog with (non-functioning) Cancel and Set buttons.

+        View dialogView = null;
+        dialogView = inflater.inflate(R.layout.font_size_preference, null);

nit: combine these lines

Comment 81

[Brian Nicholson (:bnicholson)]

Comment on attachment 642757 [details] [diff] [review]
08i: Dialog dynamically resizes itself per device.

+        // Android does well with default width.

nit: remove or combine this comment with the "set dialog height" comment

Comment 82

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04d: Created a simple dialog with (non-functioning) Cancel and Set buttons.

(In reply to Brian Nicholson (:bnicholson) from comment #80)
> Comment on attachment 642755 [details] [diff] [review]
> 04c: Created a simple dialog with (non-functioning) Cancel and Set buttons.
> 
> +        View dialogView = null;
> +        dialogView = inflater.inflate(R.layout.font_size_preference, null);
> 
> nit: combine these lines

Done. Moved r+.

Comment 83

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05g: Fleshed out the UI of the box - new buttons and preview TextView are present.

Rebase. Moved r+.

Comment 84

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04e: Created a simple dialog with (non-functioning) Cancel and Set buttons.

I accidentally uploaded 05 into 04 and requested a new review on it. This is uploading the correct 04. Moved r+.

Comment 85

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05h: Fleshed out the UI of the box - new buttons and preview TextView are present.

Updated 05 (including bnicholson's comment). Moved r+.

Comment 86

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06f: Dialog buttons work – they increase and decrease the size of the preview text.

Rebase. Moved r+.

Comment 87

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07f: Preferences are saved to Gecko.

Rebase. Moved r+.

Comment 88

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08j: Dialog dynamically resizes itself per device.

(In reply to Brian Nicholson (:bnicholson) from comment #81)
> Comment on attachment 642757 [details] [diff] [review]
> 08i: Dialog dynamically resizes itself per device.
> 
> +        // Android does well with default width.
> 
> nit: remove or combine this comment with the "set dialog height" comment

Done. Also, rebase. Moved r+.

Comment 89

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01a: Implemented previous mbrubeck string changes.

Rebase to trunk. Moved r+.

Comment 90

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04f: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Rebase to trunk. Moved r+.

Comment 91

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04g: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Wrong file - forgot to qref.

Comment 92

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 642004 [details] [diff] [review]
10d: Special cased twip size of 0.

Changed patch names to make them easier to check in.

Note, I found a bug when moving to the smallest font size on Galaxy Nexus so I will need to fix that and request an additional review (it never ends!).

Comment 93

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05i: Fleshed out the UI of the box - new buttons and preview TextView are present.

Undid previous change to Galaxy Note fix.

Comment 94

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08k: Dialog dynamically resizes itself per device.

Now set textView width as well. Also changed some comments.

I am concerned because the dialog size does not seem to change in accordance with the textView size in ICS but does in Gingerbread and I can't seem to figure out why. bnicholson, do you know?

Comment 95

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I should mention, ICS does a fine job setting width by itself so the code works. I'd like to know why I can't change the TextView width in ICS though.

Comment 96

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08l: Dialog dynamically resizes itself per device.

Updated one more comment from the last patch.

I was unable to figure out why the dialog does not change size based on the content. In ICS, I found changing the size of the internal view to be larger or smaller than the dialog size will not actually change the dialog size (meaning the dialog is not wrapping the content). This is in opposition to Gingerbread, where the dialog wraps the content and we can set it to an arbitrary size based on the size of the internal views.

Comment 97

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08m: Dialog dynamically resizes itself per device.

As a final note, this does not affect the usability of the dialog. ICS will set the dialog size for us whereas Gingerbread (at least on a Galaxy Note) will use the dialog width code and set the width dynamically.

This patch adds this information to the comments in the code.

Comment 98

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08n: Dialog dynamically resizes itself per device.

Put the comments into the wrong patch. This is the correct patch.

Comment 99

[Brian Nicholson (:bnicholson)]

Comment on attachment 643230 [details] [diff] [review]
08n: Dialog dynamically resizes itself per device.

+        // onGlobalLayout(), and only then can changes can be made.

OCD nit: remove a can


+     * invalidates the view. Does not update the font indicies.

OCD nit: s/indicies/indices/

Comment 100

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08o: Dialog dynamically resizes itself per device.

(In reply to Brian Nicholson (:bnicholson) from comment #99)
> Comment on attachment 643230 [details] [diff] [review]
> 08n: Dialog dynamically resizes itself per device.
> 
> +        // onGlobalLayout(), and only then can changes can be made.
> 
> OCD nit: remove a can
> 
> 
> +     * invalidates the view. Does not update the font indicies.
> 
> OCD nit: s/indicies/indices/

Done and done. Moved r+.

Comment 101

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01b: Implemented previous mbrubeck string changes.

Comment 102

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 03c: Fixed indentation and modified a few strings.

Comment 103

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 04h: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Comment 104

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 05j: Fleshed out the UI of the box - new buttons and preview TextView are present.

Comment 105

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 06g: Dialog buttons work – they increase and decrease the size of the preview text.

Comment 106

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 07g: Preferences are saved to Gecko.

Comment 107

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 08p: Dialog dynamically resizes itself per device.

Comment 108

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 10e: Special cased twip size of 0.

Updated patch titles and rebased to trunk. Moved r+'s.

Comment 109

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To the checker-in:
There are 8 patches total which skip numbers (namely 2 and 9). The patch file names are the same as the numbers on Bugzilla (715179-##.patch). The patches should be applied in ascending order. The patch titles (inside the file) list which patch is which without skipping numbers (ex: "Bug 715179: (# of 8)").

Comment 110

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeed2f202ef7
https://hg.mozilla.org/integration/mozilla-inbound/rev/29d8cb8a0098
https://hg.mozilla.org/integration/mozilla-inbound/rev/719601a386ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/a627af2cd213
https://hg.mozilla.org/integration/mozilla-inbound/rev/186e0369cbe4
https://hg.mozilla.org/integration/mozilla-inbound/rev/37abfe4c6259
https://hg.mozilla.org/integration/mozilla-inbound/rev/7600ed1dea00
https://hg.mozilla.org/integration/mozilla-inbound/rev/41c81cb0672d

Comment 111

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aeed2f202ef7
https://hg.mozilla.org/mozilla-central/rev/29d8cb8a0098
https://hg.mozilla.org/mozilla-central/rev/719601a386ec
https://hg.mozilla.org/mozilla-central/rev/a627af2cd213
https://hg.mozilla.org/mozilla-central/rev/186e0369cbe4
https://hg.mozilla.org/mozilla-central/rev/37abfe4c6259
https://hg.mozilla.org/mozilla-central/rev/7600ed1dea00
https://hg.mozilla.org/mozilla-central/rev/41c81cb0672d

Comment 112

[Adrian Tamas (:AdrianT)]

The new UI has been added and the user can change the font size.

Verified on:
Aurora 17.0a2 2012-09-04 and Nightly 18.0a1 2012-09-04 
Samsung Galaxy R (Android 2.3.4)
Samsung Galaxy Tab 2 7.0 Android (4.0.4)