Closed Bug 1267999 Opened 4 years ago Closed 4 years ago

Disabling APZ per page causes flashes

Categories

(Core :: Panning and Zooming, defect)

48 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified

People

(Reporter: petruta.rasa, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files)

Attached image flashes.gif
[Note]:
- This only happens when setting the pref apz.disable_for_scroll_linked_effects to True

[Affected versions]:
- Aurora 48.0a2 2016-04-26
- Nightly 49.0a1 2016-04-26

[Affected platforms]:
- Win 7 64-bit
- Mac OS X 10.9.5

[Steps to reproduce]:
1. Set pref apz.disable_for_scroll_linked_effects to True and restart the browser
2. Open https://docs.python.org/2/library/logging.html#handler-objects
3. Scroll the page and pay attention to the code samples while scrolling up and down (sections 15.7.1 and 15.7.10 from this page)

[Expected result]:
- No flashes

[Actual result]:
- Code sample area is flashing during scrolling

[Regression range]:
- Introduced by bug 1246290
This also happens if you disable APZ manually, and it's bug 1266161.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1266161
I can reproduce bug 1266161 when APZ is disabled, but not the issue described here.

Also, on page https://docs.python.org/2/library/logging.html#filter-objects, the black rectangles are not persistent.
I also think this is a different issue than bug 1266161. I'm seeing the same thing as Petruta said in comment 2.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
I note that with apz.highlight_checkerboarded_areas enabled, the region that flashes does so in the color of the pink highlight.
Whiteboard: [gfx-noted]
I'm going to have a look at this, as it's a fairly serious bug affecting apz.disable_for_scroll_linked_effects, and we'd like to have the option of turning that on during the 48 beta if necessary.
Assignee: nobody → botond
As hinted at by comment 4, the flashing area is a checkerboarding background color being drawn.

Basically, the affected region of the page contains a layer with "holes" - that is, a layer, whose visible region does not cover its entire bounds. This comes about due to three elements (the code snippets in section 15.7.10) having a z-index higher than the page's sidebar, and Layout layerizing them because it think they might overlap the sidebar during async scrolling (in fact they can't, but Layout isn't smart enough to tell). As the three elements have the same z-index, they are put into a single layer, but the surrounding content has a lower z-index than the sidebar, so they remain in the page's main layer. As a result, the layer for the elements has "holes" where the surrounding content shows through. [Thanks Markus for explaining this to me.]

With that in mind, there are two bugs related to checkerboarding background drawing here:

  1) In force-disable-APZ mode, we are mis-detecting a checkerboard where none occurs, and
     unnecessarily drawing the checkerboard background color.

  2) There's a general bug with drawing the checkerboard background for layers with holes
     in them. We draw it over the entire layer bounds, whenever the layer is opaque, but
     for a layer to be marked opaque it just needs to be opaque in its visible region - so
     a layer with holes can be marked opaque, and we draw over the holes!

(1) is a problem specific to force-disable-APZ mode.

(2) is a more general problem, but it's harder to see. For it to be visible, the edge of the displayport needs to be in the scrollport (so we are in fact checkerboarding), and the layer with the holes needs to be in the part of the scrollport that's covered by the displayport. This tends to be a very rare and transient occurrence.

I'm going to address (1) in this bug, and leave (2) for a follow-up.
The fix for (1) here is straightforward: if APZ is force-disabled for an APZC, it can't be checkerboarding, so there's no need to draw to even consider drawing a checkerboarding background rectangle for it.
Comment on attachment 8756630 [details]
MozReview Request: Bug 1267999 - An APZC cannot be checkerboarding if APZ is force-disabled for it. r=kats

https://reviewboard.mozilla.org/r/55296/#review52158

Nice and simple, I like it!
Attachment #8756630 - Flags: review?(bugmail.mozilla) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> [...] there are two bugs related to checkerboarding background
> drawing here:
> 
> [...]
> 
>   2) There's a general bug with drawing the checkerboard background for
> layers with holes
>      in them. We draw it over the entire layer bounds, whenever the layer is
> opaque, but
>      for a layer to be marked opaque it just needs to be opaque in its
> visible region - so
>      a layer with holes can be marked opaque, and we draw over the holes!
> 
> [...]
> 
> (2) is a more general problem, but it's harder to see. For it to be visible,
> the edge of the displayport needs to be in the scrollport (so we are in fact
> checkerboarding), and the layer with the holes needs to be in the part of
> the scrollport that's covered by the displayport. This tends to be a very
> rare and transient occurrence.
> 
> I'm going to [...] leave (2) for a follow-up.

(2) is sufficiently hard to notice that I don't think it's worth time fixing it right now.

Markus has a plan for re-working the handling of checkerboard background color drawing, by having Layout insert ColorLayers for backgrounds that usually get culled out in the compositor whenever we don't need them to be drawn, with a primary motivation being fixing bug 1243000, but which would also fix issue (2) here.
https://hg.mozilla.org/mozilla-central/rev/ddd9b902f9e1
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8756630 [details]
MozReview Request: Bug 1267999 - An APZC cannot be checkerboarding if APZ is force-disabled for it. r=kats

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1246290 (the "force disable APZ" mode).
  Note that the bug only manifests with this mode enabled.

[User impact if declined]:
  If we decide to enable this mode in 48 (an option we'd like to
  keep open), the user will see very noticeable flashes while
  scrolling certain pages.

[Describe test coverage new/current, TreeHerder]:
  Tested manually.

[Risks and why]: 
  Low risk, this is a well-understood one line change.

[String/UUID change made/needed]:
  None.
Attachment #8756630 - Flags: approval-mozilla-aurora?
Verified that there are no flashes on the test page with latest Nightly 49.0a1 2016-05-30 under Win 10 64-bit and Mac OS X 10.9.5.
Comment on attachment 8756630 [details]
MozReview Request: Bug 1267999 - An APZC cannot be checkerboarding if APZ is force-disabled for it. r=kats

Improve APZC and verified: taking it
Attachment #8756630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
On Aurora the IsApzForceDisabled function is still on FrameMetrics, not ScrollMetadata. Updated patch and re-landed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/c376a953a792
Flags: needinfo?(botond)
Verified as fixed using 48.0a2 2016-06-06 under Win 7 64-bit, Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.