Closed
Bug 1157579
Opened 9 years ago
Closed 9 years ago
Standalone Image vibrates with APZ enabled
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
VERIFIED
FIXED
mozilla40
People
(Reporter: Fanolian+BMO, Assigned: kats)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 7 obsolete files)
5.07 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•9 years ago
|
||
Pushlog if forced to enable the e10s and apz: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0dbdf8b62e2f&tochange=420b643efff1 Triggered by: Bug 1137267
Assignee | ||
Comment 2•9 years ago
|
||
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.
Blocks: apz-windows, apz-linux
Summary: Standalone Image vibrates with APZ enabled in Windows → Standalone Image vibrates with APZ enabled
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bugmail.mozilla
Assignee | ||
Comment 3•9 years ago
|
||
This fixes it, but needs more thought/testing/comments.
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8598267 -
Attachment is obsolete: true
Attachment #8598676 -
Flags: review?(tnikkel)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8598676 [details] [diff] [review] Patch Oh wait I forgot the rule of three.
Attachment #8598676 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8598676 -
Attachment is obsolete: true
Attachment #8598691 -
Flags: review?(tnikkel)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8598692 -
Flags: review?(tnikkel)
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
Hm, this patch regresses bug 1078373 on B2G, so something is not quite right.
Assignee | ||
Comment 13•9 years ago
|
||
Here we go, this works on B2G and desktop.
Attachment #8600355 -
Attachment is obsolete: true
Attachment #8600374 -
Flags: review?(tnikkel)
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
Updated to use just the local resolution, carrying r+
Attachment #8600385 -
Attachment is obsolete: true
Attachment #8600462 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Rebased so that it applies without bug 1160250 and landed.
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef4588e3937d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Flags: qe-verify+
QA Contact: vasilica.mihasca
Comment 21•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: qe-verify+ → qe-verify-
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•