Closed Bug 1157579 Opened 9 years ago Closed 9 years ago

Standalone Image vibrates with APZ enabled

Categories

(Core :: Panning and Zooming, defect)

40 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
firefox41 --- verified
firefox42 --- verified

People

(Reporter: Fanolian+BMO, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 7 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150422030206

Steps to reproduce:

0. Enable APZ
1. Load any image whose dimension is larger than the current window, e.g. https://upload.wikimedia.org/wikipedia/commons/thumb/1/18/OCR-A_char_Long_Vertical_Mark.svg/2000px-OCR-A_char_Long_Vertical_Mark.svg.png
2. Zoom in then unzoom


Actual results:

Image vibrates vigorously. Horizontal and vertical scrollbar keep flickering.


Expected results:

No vibration.
Blocks: 1154459
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can reproduce on windows and Linux but not OS X. Likely related to the non-overlay scrollbars. Also to clarify the "zoom in then unzoom" in the STR from comment 0 refer to just clicking on the image with the magnifying cursor on initial load, and then clicking again with un-magnifying cursor.
Summary: Standalone Image vibrates with APZ enabled in Windows → Standalone Image vibrates with APZ enabled
Whiteboard: [gfx-noted]
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
This fixes it, but needs more thought/testing/comments.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8598267 - Attachment is obsolete: true
Attachment #8598676 - Flags: review?(tnikkel)
Comment on attachment 8598676 [details] [diff] [review]
Patch

Oh wait I forgot the rule of three.
Attachment #8598676 - Flags: review?(tnikkel)
Attachment #8598676 - Attachment is obsolete: true
Attachment #8598691 - Flags: review?(tnikkel)
Comment on attachment 8598692 [details] [diff] [review]
Part 2 - Fix up visual scrollport size

This doesn't seem like it would be right.

TryLayout is called while we are determining if we need horizontal or vertical scrollbars. It uses the parameters aAssumeHScroll and aAssumeVScroll to try to layout the scroll frame with or without an H or V scrollbar as instructed, and returns if it was able to produce a conistent layout. So I don't think we can use the ScrollPositionClampingScrollPortSize that is currently set because we don't know what the state of scrollbars was when it was computed.

So instead can we just call the appropriate CalculateCompositionBounds/Size function in place of getting the last previously set ScrollPositionClampingScrollPortSize? That should produce the same value. We will need an option to tell the composition bounds calculation to not adjust for scrollbars. TryLayout can then adjust or not for scrollbar sizes.
Attachment #8598692 - Flags: review?(tnikkel)
Comment on attachment 8598691 [details] [diff] [review]
Part 1 - Refactor helper from the triplicated function

I moved this patch to bug 1160250.
Attachment #8598691 - Attachment is obsolete: true
Attachment #8598691 - Flags: review?(tnikkel)
Attached patch Patch v2 (obsolete) — Splinter Review
Updated patch as per tn's comments above. This seems to also fix the issue. Note that this applies on top of the patches in bug 1160250, although it could be rebased without it. I still need to test it locally on B2G to make sure it works fine there.
Attachment #8598692 - Attachment is obsolete: true
Hm, this patch regresses bug 1078373 on B2G, so something is not quite right.
Attached patch Patch v3 (obsolete) — Splinter Review
Here we go, this works on B2G and desktop.
Attachment #8600355 - Attachment is obsolete: true
Attachment #8600374 - Flags: review?(tnikkel)
Comment on attachment 8600374 [details] [diff] [review]
Patch v3

I think we want to subtract v/hScrollbarDesiredWidth from the scrollport size in TryLayout, whether the scrollport size is from the inside border size, or from the composition size.
Attached patch Patch v4 (obsolete) — Splinter Review
Ah I see what you meant now, makes sense.
Attachment #8600374 - Attachment is obsolete: true
Attachment #8600374 - Flags: review?(tnikkel)
Attachment #8600385 - Flags: review?(tnikkel)
Comment on attachment 8600385 [details] [diff] [review]
Patch v4

I think you just want the local resolution here, not the cumulative resolution.
Attachment #8600385 - Flags: review?(tnikkel) → review+
Attached patch Patch v5Splinter Review
Updated to use just the local resolution, carrying r+
Attachment #8600385 - Attachment is obsolete: true
Attachment #8600462 - Flags: review+
Rebased so that it applies without bug 1160250 and landed.
https://hg.mozilla.org/mozilla-central/rev/ef4588e3937d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Flags: qe-verify+
QA Contact: vasilica.mihasca
I was able to reproduce this issue on Firefox 40.0a1 (2015-04-22) using Windows 10 32-bit.

Verified fixed on Firefox 42.0a1 (2015-07-21/22) and Firefox 41.0a2 (2015-07-21/22) under Windows 10 32-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.