Closed Bug 634431 Opened 9 years ago Closed 9 years ago

Flash of checkerboard at end of pan

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

defect
Not set

Tracking

(fennec2.0b5+)

VERIFIED FIXED
Tracking Status
fennec 2.0b5+ ---

People

(Reporter: stechz, Assigned: stechz)

Details

(Keywords: polish)

Attachments

(1 file, 3 obsolete files)

With nicing the process and content refresh getting faster, sometimes the displayport change and the scroll value change happen on separate renderings. This causes a flash of checkerboard.
Assignee: nobody → ben
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Keywords: polish
Summary: Flash at end of pan → Flash of checkerboard at end of pan
Attached patch Flash at end of pan (obsolete) — Splinter Review
Attachment #512633 - Flags: review?(mbrubeck)
Attachment #512633 - Attachment is obsolete: true
Attachment #512633 - Flags: review?(mbrubeck)
Attached patch Flash at end of pan (obsolete) — Splinter Review
Attachment #512638 - Flags: review?(mbrubeck)
Attachment #512638 - Attachment is obsolete: true
Attachment #512638 - Flags: review?(mbrubeck)
Attached patch Flash at end of pan (obsolete) — Splinter Review
Attachment #512656 - Flags: review?(mbrubeck)
Comment on attachment 512656 [details] [diff] [review]
Flash at end of pan

>+          _updateCSSViewport: function() {
>+            let rootScale = this.self.scale;
>+            let contentView = this._contentView;
>+            this._sendUpdateDisplayport(contentView.scrollX / rootScale,
>+                                        contentView.scrollY / rootScale);
>+          },

It would be slightly simpler (code-wise) to do the scaling in _sendUpdateDisplayport, since we're already scaling everything else in that method (unless you have a philosophical reason to pass these parameters using the scaled units).
Attachment #512656 - Flags: review?(mbrubeck) → review+
Comment on attachment 512656 [details] [diff] [review]
Flash at end of pan

There's a problem with this patch on http://people.mozilla.com/~mbrubeck/test/in-page-link.html - stechz says a fix is coming.
Attachment #512656 - Flags: review+
Attachment #512656 - Attachment is obsolete: true
Attachment #512662 - Flags: review?(mbrubeck)
Comment on attachment 512662 [details] [diff] [review]
Flash at end of pan

>+  _scrollOffset: new Point(0, 0),

nit: We could use {x: 0, y: 0} instead to avoid loading Geometry.jsm unnecessarily.

There's still a problem with this test case:
1. Open http://people.mozilla.com/~mbrubeck/test/in-page-link.html
2. Tap on "in-page link"
3. Press the Android back button or the escape key (not the "Back to top" link)

Expected results: The page scrolls back to the top.
Actual results: Nothing happens.
Attachment #512662 - Flags: review?(mbrubeck) → review-
Comment on attachment 512662 [details] [diff] [review]
Flash at end of pan

We figured out the fixes on IRC:

>         let scrollOffset = this.getScrollOffset(content);
>+        if (this._scrollOffset.x == scrollOffset.x && this._scrollOffset.y == scrollOffset.y)
>+          break;
>+

Set "this._scrollOffset = scrollOffset" right after this; otherwise, the back button won't work after following in-page links as in the previous comment.

>+          _updateCacheViewport: function() {
>+            // Do not update scroll values for content.
>+            this._sendUpdateDisplayport(NaN, NaN);
>+          },

NaN is not JSON-safe.  Use -1 instead.

r=mbrubeck with these changes.
Attachment #512662 - Flags: review- → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
tracking-fennec: ? → 2.0b5+
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre Fennec/4.0b5pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.