Adjust ease-out curve while zooming out to try and reduce checkerboarding

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: kats)

Tracking

unspecified
Firefox 14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 beta+)

Details

(Whiteboard: [viewport])

Attachments

(3 attachments)

Reproducible on any page: zoom out quickly.

android-browser looks a lot better when zooming out.  It might be repainting faster than us, and possibly using other trickery, but there is one difference that I think I can see: it simply zooms out more slowly.  Firefox-android zooms out very quickly, which feels nice and responsive, but also makes it hard for the content thread to catch up.

We should try zooming out with an ease-out curve (and maybe some artificial lag) and compare the perceptual difference.

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 737577
Reopening to track the second suggestion in comment 0, which is to adjust the ease-out curve and try some artificial lag.
Status: RESOLVED → REOPENED
Component: Graphics → General
Product: Core → Fennec Native
QA Contact: thebes → general
Resolution: DUPLICATE → ---
Summary: Firefox-android has lots of checkerboarding on zoom-out → Adjust ease-out curve while zooming out to try and reduce checkerboarding
Whiteboard: [viewport]
For reference, the ease-out curve is in PanZoomController.java; we can adjust the numbers and/or add more values into the list to slow down the zooming. Based on the comment, the current values are taken from the CSS Transitions spec.

Updated

6 years ago
blocking-fennec1.0: --- → ?

Updated

6 years ago
Assignee: nobody → ibarlow
blocking-fennec1.0: ? → beta+
Sounds good, I'm all for easing transitions. 

JP is working to set other panning and zooming parameters as prefs that we can adjust and optimize. Is that something we could build into this as well?
Kats - Can you get the preferences in place?
Assignee: ibarlow → bugmail.mozilla
Priority: -- → P1
Depends on: 745501
Depends on: 745540
Created attachment 615183 [details] [diff] [review]
(1/3) Pref the zoom animation

This was turning out ugly no matter which way I did it, so I decided to go with the more modular approach of having separate sets of preferences in GeckoLayerClient and PanZoomController. It's slightly less efficient because it sends two messages instead of one but prevents excessive cross-package dependencies. Patch #2 moves the Axis preferences out of GeckoLayerClient as well.
Attachment #615183 - Flags: review?(chrislord.net)
Created attachment 615184 [details] [diff] [review]
(2/3) Move the axis pref-fetching into PanZoomController
Attachment #615184 - Flags: review?(chrislord.net)
Created attachment 615185 [details] [diff] [review]
(3/3) The patch cpeterson promised me months ago
Attachment #615185 - Flags: review?(cpeterson)
Comment on attachment 615183 [details] [diff] [review]
(1/3) Pref the zoom animation

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

Looks good to me.

::: mobile/android/base/ui/PanZoomController.java
@@ +233,5 @@
> +                    values[i] = Float.parseFloat(st.nextToken());
> +                }
> +                ZOOM_ANIMATION_FRAMES = values;
> +            }
> +        } catch (Exception e) {

I don't like the idea of just catching any old Exception, I guess there could only be NumberFormatException (or whatever it is) here?
Attachment #615183 - Flags: review?(chrislord.net) → review+
Comment on attachment 615184 [details] [diff] [review]
(2/3) Move the axis pref-fetching into PanZoomController

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

Reducing cross-package dependency is nice, though it's a shame the code size has increased.

::: mobile/android/base/ui/PanZoomController.java
@@ +217,1 @@
>                  for (int i = jsonPrefs.length() - 1; i >= 0; i--) {

It'd be good to factor out and share this for-loop prefs checking code at some point.

@@ +231,5 @@
> +                // check for null to make sure the batch of preferences we got notified
> +                // of are in the fact the ones we requested and not those requested by
> +                // other java code
> +                if (zoomAnimationFrames != null) {
> +                    setZoomAnimationFrames(zoomAnimationFrames);

Is there a possibility of zoomAnimationFrames being null even on a successful prefs get? (i.e. is that enough to determine if this was the correct prefs data?)
Attachment #615184 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #10)
> I don't like the idea of just catching any old Exception, I guess there
> could only be NumberFormatException (or whatever it is) here?

Ok, I'll change that to just catch the NumberFormatException then.

(In reply to Chris Lord [:cwiiis] from comment #11)
> It'd be good to factor out and share this for-loop prefs checking code at
> some point.

Ideally I'd also like to be able to listen for updates to prefs so we don't have to restart the app every time. To do that cleanly I'd like to introduce a new helper class that you can register with to get prefs and listen for updates and so on, but that's post-1.0 work.

> 
> Is there a possibility of zoomAnimationFrames being null even on a
> successful prefs get? (i.e. is that enough to determine if this was the
> correct prefs data?)

I don't think so. I did find it was coming out null because of bug 744501, and fixed that. I think there's no legitimate case where it should be null (i.e. it will only happen because of other bugs).
Comment on attachment 615185 [details] [diff] [review]
(3/3) The patch cpeterson promised me months ago

r+

Sorry, kats! I could have sworn I posted a patch for this promised fix, but I guess I dropped the ball.
Attachment #615185 - Flags: review?(cpeterson) → review+
You need to log in before you can comment on or make changes to this bug.