The default bug view has changed. See this FAQ.

Status

Fennec Graveyard
Panning/Zooming
P2
enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Christian Sejersen, Assigned: Jaakko Kiviluoto)

Tracking

Trunk
fennec1.0b1
Bug Flags:
wanted-fennec1.0 +

Details

Attachments

(3 attachments, 9 obsolete attachments)

13.98 KB, patch
Details | Diff | Splinter Review
1.64 KB, patch
dougt
: review+
Details | Diff | Splinter Review
26.30 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
 

Updated

9 years ago
Assignee: nobody → gavin.sharp
Flags: wanted-fennec1.0+

Updated

9 years ago
Priority: -- → P2

Updated

9 years ago
Target Milestone: Fennec M6 → Fennec A1

Updated

9 years ago
Target Milestone: Fennec A1 → Fennec M7
Created attachment 334611 [details] [diff] [review]
WIP

This uses a second canvas and drawImage calls to animate zooming to elements. It works OK on the desktop but it's too slow on my n810.

Updated

9 years ago
Target Milestone: Fennec M7 → Fennec A1

Updated

9 years ago
Target Milestone: Fennec A1 → Fennec A2
I'll check what we can do with Xomap optimizations to make it faster...
I have apply this patch to the latest fennec with 450930 bugfix...
But I don't see any animation at all...

Updated

8 years ago
Flags: wanted-fennec1.0+ → blocking-fennec1.0+

Updated

8 years ago
Target Milestone: Fennec A2 → Fennec A3

Updated

8 years ago
Flags: blocking-fennec1.0+ → wanted-fennec1.0+
Assignee: gavin.sharp → nobody
OS: Linux (embedded) → All
Hardware: Other → All
Created attachment 400889 [details] [diff] [review]
New patch, same idea

Here's a rough patch for animated zooming, using the same trick Gavin used.  There are edge cases that don't work quite right and the performance is rather bad on N810.
Attachment #334611 - Attachment is obsolete: true
Component: General → Panning/Zooming
which fennec sources are you using? or what is the revision number? it is fail to apply for me..
from the patch it looks like http://hg.mozilla.org/mobile-browser/rev/5076b5dc9f32
(Assignee)

Comment 7

7 years ago
Created attachment 437516 [details] [diff] [review]
Updated patch proposal

This version updates AnimatedZoom class to provide an arbitrary zoom for pinch gesture purposes. It also uses larger screen snapshot area.

Currently the zoom starts with urlbar and sidebars hidden in order to avoid unspecified states after the zoom.
Attachment #437516 - Flags: review?(dougt)
(Assignee)

Comment 8

7 years ago
Created attachment 437517 [details] [diff] [review]
 Updated patch proposal

Patch updated to include missing AnimatedZoom.js
Attachment #437516 - Attachment is obsolete: true
Attachment #437517 - Flags: review?(dougt)
Attachment #437516 - Flags: review?(dougt)

Comment 9

7 years ago
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

brad or mfinkle might be the best person to review?
Attachment #437517 - Flags: review?(mark.finkle)
Attachment #437517 - Flags: review?(dougt)
Attachment #437517 - Flags: review?(bugmail)
(Assignee)

Comment 10

7 years ago
Created attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Also, on Qt port we need the "MozGestureMagnify" event to know when pinch gesture ends.
Attachment #438434 - Flags: review?(mark.finkle)
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

you probably can set outside of all fo the pinch->state() checks.
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

otherwise looks fine. please post another patch
Attachment #438434 - Flags: review?(mark.finkle) → review+
Comment on attachment 438434 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

wrong way.  i want a new patch.
Attachment #438434 - Flags: review+ → review-
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

I want Matt to have a look too
Attachment #437517 - Flags: feedback?(mbrubeck)
Still looking through this, just one note before I head to dinner:

> AnimatedZoom.prototype.browserToClientRect = function(r) {
>   let [l, t] = Browser.browserViewToClient(r.left, r.top);
>   let [r, b] = Browser.browserViewToClient(r.right, r.bottom);
>   return new Rect(l, t, r - l, b - t);
> }

Can you use Browser.browserViewToClientRect instead?
Comment on attachment 437517 [details] [diff] [review]
 Updated patch proposal

>+ * Portions created by the Initial Developer are Copyright (C) 2008

The year should be fixed, along with the list of contributors.

>+      let zoom = new AnimatedZoom(this._browserView);
>+      return zoom.animateTo(zoomRect);

Let's move these lines from Browser.zoomFromPoint and zoomToPoint into a new "Browser.animatedZoomTo" function, so we can easily use animated zoom in even more places (like Browser.zoom and FormHelper.zoom).

The rest of the patch looks good to me.
Attachment #437517 - Flags: feedback?(mbrubeck) → feedback-
Created attachment 439554 [details] [diff] [review]
Additional patch: More animated zoom

(In reply to comment #16)
> Let's move these lines from Browser.zoomFromPoint and zoomToPoint into a new
> "Browser.animatedZoomTo" function, so we can easily use animated zoom in even
> more places (like Browser.zoom and FormHelper.zoom).

Apply this additional patch on top of attachment 437517 [details] [diff] [review] to animate volume-rocker zoom and FormHelper zoom.
Attachment #439554 - Flags: review?(mark.finkle)
(Assignee)

Comment 18

7 years ago
Created attachment 440184 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Updated gesture finishing event catching. I hope I understood the comments to previous proposal correctly.
Attachment #438434 - Attachment is obsolete: true
Attachment #440184 - Flags: review?(dougt)
(Assignee)

Comment 19

7 years ago
Created attachment 440185 [details] [diff] [review]
Updated "animated zooming with pinch" patch

This patch contains all the suggested corrections so far, including Matt's additions. Also corrected magic numbers from pinch calculation.

I hope I got the contributor lists right. Please add any missing contributors if you know.
Attachment #437517 - Attachment is obsolete: true
Attachment #440185 - Flags: review?(mark.finkle)
Attachment #437517 - Flags: review?(mark.finkle)
Attachment #437517 - Flags: review?(blassey.bugs)
Attachment #439554 - Attachment is obsolete: true
Attachment #439554 - Flags: review?(mark.finkle)
Comment on attachment 440185 [details] [diff] [review]
Updated "animated zooming with pinch" patch

>diff -r 7a4d3865b316 chrome/content/AnimatedZoom.js

I'm on a mission to use JavaScript Code Modules more and <script src=""/> less. We pay a price for including a lot of JS during startup. This particular code may not be used during startup, so we could end up delay loading it using a lazy property in browser.js and Components.utils.import

I think the only part we need to add to the contructor is aBrowser, so you have access to the "Browser" object in the code module.

NOTE: for now, I think we could convert AnimatedZoom to a JS module in a followup bug.

>+AnimatedZoom.prototype._callback = function() {

>+    }
>+    else // last cycle already rendered final scaled image, now clean up
>+      this.finish();

When one side of an if/else requires "{}" we add them to both sides, whether it needs them or not.

>diff -r 7a4d3865b316 chrome/content/browser.js

>+// Simple gestures support

I wonder if this should be merged into InputHandler. There is some overlap. We are planning a InputHandler refactor for 2.0 and we should consider this code when we start.

>+  init: function GS_init(aAddListener) {
>+    const gestureEvents = ["SwipeGesture",
>+      "MagnifyGestureStart", "MagnifyGestureUpdate", "MagnifyGesture",
>+      "RotateGestureStart", "RotateGestureUpdate", "RotateGesture",
>+      "TapGesture", "PressTapGesture"];

Why do we add listeners for all of these gestures, but we only seem to handle the MozMagnifyXxx events

>+
>+    //Add or remove listener according to given parameter
>+    let addRemove = aAddListener ? window.addEventListener :
>+      window.removeEventListener;

This is too fancy for me. Call int init with a boolean to handle uninit too is a bit confusing. Let's just separate the init and unint calls.

Do we uninit in this patch anyway?

>+  _pinchStart: function GS_pinchStart(aEvent) {

>+        // hide element highlight
>+        for (let elem = aEvent.target; elem; elem = elem.parentNode)
>+          if (elem.customClicker) {
>+            elem.customClicker.panBegin();
>+            break;
>+          }

Since this is for content only, I assume, can we just grab the cutsomClicker directly?

document.getElementById("tile-container").customClicker

I guess customClicker.panBegin works for now, but I think we need a better way to hide the highlight. File a followup bug?

>diff -r 7a4d3865b316 chrome/content/browser.xul

>+    <html:canvas id="zoom-canvas" style="display: none; image-rendering: optimizeSpeed;" top="0" left="0" moz-opaque="true">
>+    </html:canvas>

You are adding this canvas, but somewhere in browser.xul we have a canvas used for the current "fuzzy" zoom (id="view-buffer", iirc). We should remove it now since all zooming is using the new canvas.

In general, this is a very nice patch
Attachment #440185 - Flags: review?(mark.finkle) → review-
Comment on attachment 440184 [details] [diff] [review]
Patch to send pinch gesture ending event on Qt port

Please don't add spaces on new lines:

+        
+        mLastPinchDistance = mTouchPointDistance;
     }

otherwise great.  I fixed this locally and push this patch to m-c:


http://hg.mozilla.org/mozilla-central/rev/58e9ce25c23f
Attachment #440184 - Flags: review?(dougt) → review+
This should also be updated to respect the "user-scalable" viewport metadata.  Bug 561815's patch adds a BrowserView.prototype.allowZoom getter you can use to see if zoom is enabled or disabled.
(Assignee)

Comment 23

7 years ago
Created attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

New version of the patch that catches up with default branch and implements the changes from previous review comments. More specifically:

- GestureHandler converted to GestureModule in InputHandler.js
- simply re-use existing view-buffer as drawing canvas
- respect BrowserView.allowZoom
- animation timer updated to make it a bit smoother
- prevent pinch gesture on top of XUL elements/views
- fixed pinch gesture algorithm to more intuitive behavior

Omitted:
- removing need to be loaded as <script src="...">
Attachment #440185 - Attachment is obsolete: true
Attachment #445306 - Flags: review?(mark.finkle)

Comment 24

7 years ago
The zooming behavior in this patch feels pretty great. However, I'm having issues with the page not updating while zoomed in. (zoomed in area shows and everything outside the area is checkerboard) Have you seen this issue? I'm hitting it in slashdot.org and kotaku.com .
(Assignee)

Comment 25

7 years ago
(In reply to comment #24)
> issues with the page not updating while zoomed in. (zoomed in area shows and
> everything outside the area is checkerboard) Have you seen this issue? I'm

Checkerboard becomes visible because we just take a snapshot of the page with 1.5 times the critical rectangle dimensions (look for the use of Rect.inflate), which of course is not enough to cover very big leaps outwards. With bigger values the size of bitmap data would be too much and could unnecessarily OOM resource limited devices.

If there would be a possibility to render smaller resolution snapshots as fast as we currently can say renderToCanvas(), please let me know, so we could always render enough overscan data to cover the whole zoomed-out area.
I'm not sure if this is what Michael means, but I'm seeing some cases where the page does not render (checkerboard does not go away) even after zooming stops.

Comment 27

7 years ago
(In reply to comment #26)
> I'm not sure if this is what Michael means, but I'm seeing some cases where the
> page does not render (checkerboard does not go away) even after zooming stops.

Yeah this is what I mean. Persistent checkerboard.
(Assignee)

Comment 28

7 years ago
Oh, ok. Are the remaining checkerboard areas possibly about the same size as TileManager's tiles? Because that's what I've sometimes seen, that some tiles are not rendered even after the last call to Browser.setVisibleRect() (which in the end *should* put us to the same state as the non-animated zoom did). But if I pan after such a situation a bit, the missing parts usually appear, so the rendered data probably is there but it just isn't painted correctly after setVisibleRect().

Comment 29

7 years ago
the checkerboard is just the background -- if a tile is missing nothing will be in there showing through.
(In reply to comment #28)
> But if I pan after such a situation a bit, the missing parts usually appear,
> so the rendered data probably is there but it just isn't painted correctly
> after setVisibleRect().

When this happens to me, panning does *not* cause the missing tiles to appear.  Zooming again will cause the current screen contents to be drawn, but there will still be missing tiles until a new page is loaded.
Comment on attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

>diff --git a/chrome/content/AnimatedZoom.js b/chrome/content/AnimatedZoom.js

>+function AnimatedZoom(aBrowserView) {
>+  this.bv = aBrowserView;
>+
>+  // Render a snapshot of the viewport contents around the visible rect
>+  let [w, h] = this.bv.getViewportDimensions();
>+  let viewportRect = new Rect(0, 0, w, h);
>+  this.zoomFrom = this.bv.getVisibleRect().translateInside(viewportRect);
>+  // try to cover 1.5 times the size of the current visible rect
>+  this.snapshotRect = this.bv.getVisibleRect().inflate(1.5);
>+  // sanitize the snapshot rectangle to fit inside viewport
>+  this.snapshotRect.translateInside(viewportRect).restrictTo(viewportRect).expandToIntegers();
>+  this.snapshot = this._createCanvas(this.snapshotRect.width, this.snapshotRect.height);
>+  let snapshotCtx = this.snapshot.getContext("2d");
>+  snapshotCtx.clearRect(0, 0, this.snapshotRect.width, this.snapshotRect.height);
>+  this.bv.renderToCanvas(this.snapshot, this.snapshotRect.width, this.snapshotRect.height, this.snapshotRect.clone());

nit: Adding a linebreak before commented lines make them easier to read.


>+AnimatedZoom.prototype._createCanvas = function(width, height) {
>+  let canvas = document.createElementNS(kXHTMLNamespaceURI, "canvas");
>+  canvas.setAttribute("width", String(width));
>+  canvas.setAttribute("height", String(height));

nit: The String() calls shouldn't be needed, right?

>+AnimatedZoom.prototype.finish = function() {
>+  try {
>+    // restore canvas state
>+    let ctx = Elements.viewBuffer.getContext("2d");
>+    ctx.mozImageSmoothingEnabled = this.defaultMozImageSmoothing;
>+    ctx.globalCompositeOperation = this.defaultCompositeOperation;
>+    // resume live rendering
>+    this.bv.resumeRendering();
>+    // if we actually zoomed somewhere, clean up the UI to normal
>+    if (this.zoomRect)
>+      Browser.setVisibleRect(this.zoomRect);

nit: linebreaks for readability

>diff --git a/chrome/jar.mn b/chrome/jar.mn
>   content/prompt/prompt.js             (content/prompt/prompt.js)
>+* content/AnimatedZoom.js              (content/AnimatedZoom.js)

Do we need this file to be preprocessed? Just wondering why you have the "*" at the beginning of the line?

Great patch!

r+, but let me know about the preprocessed flag. Whoever lands this patch, please add some linebreaks to the larger code blocks I identified.
Attachment #445306 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 32

7 years ago
(In reply to comment #31)
> >+* content/AnimatedZoom.js              (content/AnimatedZoom.js)
> 
> Do we need this file to be preprocessed? Just wondering why you have the "*" at
> the beginning of the line?

You are right, preprocessing is not needed. The star can be replaced by space, I just verified it.

Comment 33

7 years ago
Comment on attachment 445306 [details] [diff] [review]
Updated "animated zooming with pinch" patch 2

the patch is still broken, needs to be updated to not break rendering
Attachment #445306 - Flags: review+ → review-
(Assignee)

Comment 34

7 years ago
As a status update, the checkerboard problem is easily fixed by calling bv,resumeRendering(true) instead of just bv,resumeRendering() at AnimatedZoom.finish(). BUT, there is another issue that (at least) after pinch zoom the rendering kind of gets frozen, i.e. animations/flash are not updating. Panning a bit after that restarts rendering again, so I'm now trying to find out what else should we call at finish() to fix that.

Also, in next update I'll set the temporary canvas element's mozOpaque=true and implement clipped checkerboard filling in order to get into pixman fast path and have *much* better fps.
(Assignee)

Comment 35

7 years ago
Created attachment 449238 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed
Attachment #445306 - Attachment is obsolete: true
Attachment #449238 - Flags: review?(mark.finkle)
(Assignee)

Comment 36

7 years ago
Created attachment 449242 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Fixed garbled patch.. sorry about that.
Attachment #449238 - Attachment is obsolete: true
Attachment #449242 - Flags: review?(mark.finkle)
Attachment #449238 - Flags: review?(mark.finkle)
Comment on attachment 449242 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Jaakko, This patch seems to be an interdiff with a previous patch which is nice for checking new changes.

Can you also attach a full patch so I can test it?
(Assignee)

Comment 38

7 years ago
Created attachment 449580 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Ah, of course, here are differences against the default branch.
Attachment #449242 - Attachment is obsolete: true
Attachment #449580 - Flags: review?(mark.finkle)
Attachment #449242 - Flags: review?(mark.finkle)
Comment on attachment 449580 [details] [diff] [review]
Updated patch with checkerboard, render freezing and fps issues fixed

Works well for local pages. I see some rendering issues during the animation for remote pages though. Hard to tell where the problem is though.

Rendering remote pages isn't working 100% so it could be something in the system.

I unbitrotted with not much trouble.
Attachment #449580 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/cb09e0392c8a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

6 years ago
Assignee: nobody → jaakko.kiviluoto
You need to log in before you can comment on or make changes to this bug.