Closed Bug 1110555 Opened 5 years ago Closed 5 years ago

LWT bitmap is incorrectly clamped to the right after starting the device in portrait mode and rotating to landscape

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox38 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

STR:
1) Add a LWT on new tablet while in portrait mode (note, I used LWT switcher to do this)
2) Rotate the device

Expected: The theme updates
Actual: The theme dumps garbage in the added space

I think this is because we don't refresh the LWT on configuration change [1].

I'm not sure if this affects other devices.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=e2759f8d7232#1378
Summary: Graphical glitches when rotating LWT → Graphical glitches when rotating with LWT enabled
As discussed in our new-tablet-v1 triage meeting, this is (likely) too dangerous to get into 36: looking at 37, or, worst case, v2.
I noticed this also occurs on phone, just in a different and seemingly less intrusive way.

I think mBitmap.getWidth() [1] is incorrect - it is different when the device is started in portrait and landscape modes and also "left" is negative when the device is rotated from portrait to landscape.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/lwt/LightweightTheme.java#321
maxwidth is:
  * 1824 (portrait)
  * 1920 (landscape)

Which is calculated from [1]:

  int maxWidth = Math.max(dm.widthPixels, dm.heightPixels);

I'm assuming dm.heightPixels does not include the software system buttons and/or the notification bar.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/lwt/LightweightTheme.java#208
Tested on ICS, KitKat, and GB.

There is still some weirdness in LWT such as the height of certain elements
being incorrect and some incorrect states after rotation, but at least the
edges of the bitmap are not getting clamped to the right as before, causing
what appeared to be a graphical glitch.

However, I think this patch is all the time we should put into LWT for 36.
Attachment #8552797 - Flags: review?(mhaigh)
Summary: Graphical glitches when rotating with LWT enabled → LWT bitmap is incorrectly clamped to the right after starting the device in portrait mode and rotating to landscape
Note: there may be a better way to initialize the LWT such that we don't need the device height but this is the quick, easy win I feel confident uplifting.
Comment on attachment 8552797 [details] [diff] [review]
Use real device dimensions when calculating LWT bitmap sizes

f? rnewman on HardwareUtils - is not caching my values an acceptable approach?
Attachment #8552797 - Flags: feedback?(rnewman)
Comment on attachment 8552797 [details] [diff] [review]
Use real device dimensions when calculating LWT bitmap sizes

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

::: mobile/android/base/lwt/LightweightTheme.java
@@ +206,5 @@
>  
>          // Get the max display dimension so we can crop or expand the theme.
> +        final Point displayDimensions = new Point();
> +        HardwareUtils.getDeviceDimensions(mApplication, displayDimensions);
> +        final int maxWidth = Math.max(displayDimensions.x, displayDimensions.y);

(a) Why not just call your method getLargestDimension and have it return a single value?
(b) I think setLightweightTheme is the wrong place for this; if the maximum value is varying across orientations, then you're relying on the theme being set in the right orientation, no?

So perhaps the solution here is to figure out the situations in which our dimensions are wrong -- when we're in portrait and height is the largest value -- and pad accordingly.

::: mobile/android/base/util/HardwareUtils.java
@@ +18,2 @@
>  import android.view.ViewConfiguration;
> +import android.view.WindowManager;

Let's put the gfxey stuff in a separate class; DisplayUtils?

@@ +120,5 @@
> +    /**
> +     * Returns the best-guess physical device dimensions, including the system status bars. Note
> +     * that DisplayMetrics.height/widthPixels does not include the system bars.
> +     *
> +     * This method does not cache because the return value is dependent on device orientation.

But presumably there are only {2,4} device orientations?

@@ +132,5 @@
> +        final Display display = ((WindowManager) context.getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay();
> +        final int realWidth;
> +        final int realHeight;
> +
> +        if (Build.VERSION.SDK_INT >= 17) {

Use `Versions`.
Attachment #8552797 - Flags: feedback?(rnewman) → feedback-
(In reply to Richard Newman [:rnewman] from comment #7)
> (a) Why not just call your method getLargestDimension and have it return a
> single value?

Good idea.

> (b) I think setLightweightTheme is the wrong place for this; if the maximum
> value is varying across orientations, then you're relying on the theme being
> set in the right orientation, no?

Ideally, it shouldn't vary across orientations - it does because of inconsistent return values from Display.heightPixels - this bug is intended to fix that.

> So perhaps the solution here is to figure out the situations in which our
> dimensions are wrong -- when we're in portrait and height is the largest
> value -- and pad accordingly.

This bug adds a method to get the value we expect - no need for potentially ambiguous padding. There is probably a better way of writing the LWT code such that this is not an important value but I'm looking for the quick, safe win here so we can uplift.
Before we used DisplayMetrics.width/heightPixels, which does not include the
system bars.
Attachment #8553438 - Flags: review?(mhaigh)
Attachment #8553438 - Flags: feedback?(rnewman)
Comment on attachment 8553438 [details] [diff] [review]
Use real device dimensions when calculating LWT bitmap sizes

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

Good to go.

::: mobile/android/base/util/WindowUtils.java
@@ +9,5 @@
> +import android.util.DisplayMetrics;
> +import android.util.Log;
> +import android.view.Display;
> +import android.view.WindowManager;
> +import org.mozilla.gecko.AppConstants.Versions;

nit: Move this above the android imports

@@ +43,5 @@
> +        } else if (Versions.feature14Plus) {
> +            int tempWidth;
> +            int tempHeight;
> +            try {
> +                final Method mGetRawH = Display.class.getMethod("getRawHeight");

nit: Our convention is that new files do not use Hungarian notation - remove the `m`.

;)
Attachment #8553438 - Flags: review?(mhaigh) → review+
Attachment #8552797 - Flags: review?(mhaigh)
Attachment #8552797 - Attachment is obsolete: true
Comment on attachment 8553438 [details] [diff] [review]
Use real device dimensions when calculating LWT bitmap sizes

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

::: mobile/android/base/util/WindowUtils.java
@@ +37,5 @@
> +            final DisplayMetrics realMetrics = new DisplayMetrics();
> +            display.getRealMetrics(realMetrics);
> +
> +            realWidth = realMetrics.widthPixels;
> +            realHeight = realMetrics.heightPixels;

I think you can simplify each of these clauses to be:

  if (Versions....) {
    ...
    return Math.max(realMetrics.widthPixels, realMetrics.heightPixels);
  }

  if ( ... )

Then you can kill all of the 'else's and also realWidth and realHeight.
Attachment #8553438 - Flags: feedback?(rnewman) → feedback+
Attachment #8553438 - Attachment is obsolete: true
Comment on attachment 8553914 [details] [diff] [review]
Use real device dimensions when calculating LWT bitmap sizes

Approval Request Comment
[Feature/regressing bug #]:
  New tablet UI (bug 1014156)

[User impact if declined]:
  Users of Lightweight themes will see an unpolished experience when rotating the device where the location of the background is unpredictable and sometimes looks glitchy.

[Describe test coverage new/current, TreeHerder]:
  None

[Risks and why]: 
  Low - the change will only affect Lightweight themes. However, we do use a new method for determining the device size that should, but may not, work correctly on *all* devices (because Android). If it doesn't work correctly on some devices, the expected case is that these devices will have a similar experience to what is currently available. In the worst (unlikely) case, Lightweight themes will look completely terrible and basically be unusable.

[String/UUID change made/needed]: None
Attachment #8553914 - Flags: approval-mozilla-beta?
Attachment #8553914 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/88d56c9a2e73
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8553914 - Flags: approval-mozilla-beta?
Attachment #8553914 - Flags: approval-mozilla-beta+
Attachment #8553914 - Flags: approval-mozilla-aurora?
Attachment #8553914 - Flags: approval-mozilla-aurora+
Verified as fixed in builds:
Firefox for Android 38.0a1 (2015-01-26)
Firefox for Android 37.0a2 (2015-01-26)

Device: Asus Transformer Pad TF300T (Android 4.2.1)
Verified as fixed in Firefox for Android 36 Beta 4; 
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.