Closed Bug 579808 Opened 13 years ago Closed 13 years ago

unpainted lines with MozTransform scale after scrolling


(Core :: Layout, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: karlt, Assigned: roc)



(Keywords: regression, Whiteboard: [hardblocker](?))


(5 files, 2 obsolete files)

Attached image screenshot

1. from <obj-path>/_tests/testing/mochitest,
   run python --test-path=modules/plugin/test/test_bug539565-2.html
   and wait for the test to finish.

2. Scroll to the right and slowly back to the left using the horizontal
   scrollbar thumb.

This seems to be a regression from bug 564991, but reproducing is a bit temperamental (seems to depend on window size) so perhaps there ways to produce this before that bug landed.
Note that as well as unpainted lines that cover the entire embed, there are also unpainted lines that are only in the blue div.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Karl, are you still seeing this?
Attached image more recent screenshot
(In reply to comment #2)
> Karl, are you still seeing this?

Still seeing this with 9d6b04fbec36.
The unpainted lines now seem to show uninitialized pixels.
I can
Assignee: nobody → roc
That test is pretty broken for me; scrolling around just fails to repaint the area where the plugin was.
Attached patch fix (obsolete) — Splinter Review
-- nsDisplayTransform::GetBounds/IsOpaque are wrong. IsOpaque returns true when the list reports as being opaque, but that doesn't mean the whole overflow area for the frame is opaque. The overflow area for the frame might not be covered by stuff in the list (even just because we had to take the union of rectangles to get the overflow area). So, compute the transform bounds as the transform of the list bounds.

-- PrintDisplayListTo needs to be more careful when printing the stored list for an nsDisplayTransform. We need to check whether the list DidComputeVisibility.

-- nsDisplayPlugin::GetBounds is wrong for plugins with layers, in two ways:
  -- When using a plugin with a layer, we need to set the size of the bounds to the size of the layer (as well as adjusting the offset to center the plugin layer within the plugin rect).
  -- If the plugin layer ImageContainer has no image, the plugin hasn't painted yet. We should leave the bounds as the size of the plugin content area instead of setting them to the layer image size (which will be 0,0 since we have no image). If we allow the size of the plugin content area to be set to 0,0, BuildLayer will not actually be called and the plugin never gets around to displaying properly.
Attachment #478632 - Flags: review?(tnikkel)
Benjamin, you might want this patch for your Windows work.
Comment on attachment 478632 [details] [diff] [review]

hmm, the last paragraph of comment #6 may be wrong. We do need to ensure NotifyPainted is triggered, but it's not a good idea to lie about the size of what we're going to paint. Probably we should move the call to NotifyPainted to nsObjectFrame::BuildDisplayList.
Attachment #478632 - Flags: review?(tnikkel)
Attached patch fix v2Splinter Review
Addresses previous comment. When we have no image, we allow the plugin display item to be 0x0 (it will actually be removed during ComputeVisibility and BuildLayer will never be called). But we move the code to call NotifyPaintWaiter and NotifyPainted from BuildLayer to BuildDisplayList, so it gets called anyway.
Attachment #478632 - Attachment is obsolete: true
Attachment #478634 - Flags: superreview?
Attachment #478634 - Flags: review?(tnikkel)
Attachment #478634 - Flags: superreview? → superreview?(romaxa)
Comment on attachment 478634 [details] [diff] [review]
fix v2

this patch breaking == 580160-1.html 580160-1-ref.html reftest
Comment on attachment 478634 [details] [diff] [review]
fix v2

r=me, of course you'll have to fix the reftest fail.
Attachment #478634 - Flags: review?(tnikkel) → review+
Whiteboard: [needs review]
Comment on attachment 478634 [details] [diff] [review]
fix v2

r-, due to reftest failure...
btw roc why do you ask superreview for this patch?
Attachment #478634 - Flags: review-
Whiteboard: [needs review]
The 580160-1.html testcase works for me if I load it and call doTest() manually. I presume we're taking the snapshot before the plugin has resized and repainted its layer.
My patch here actually causes a test failure in layout/reftests/transform/compound-1-ref.html (maybe in combination with other patches in my queue). The problem actually already exists and is nontrivial.

In FrameLayerBuilder in a couple of places we do
  nsIntRect itemVisibleRect =

This code is assuming that when the item draws, whatever it draws will be snapped to pixel boundaries. But this assumption is overoptimistic for a couple of reasons:
-- Some content, such as an element transformed by rotation, simply doesn't snap its drawing.
-- If the context has a tricky transform on it when we come to draw the content, we might not snap.

Using ToOutsidePixels here would be the conservative approach and it would give correct results. However it would mean that for content that *does* snap, but is not already aligned at pixel boundaries, the visible rect is bigger than the area that actually gets drawn, so a layer containing only that content could no longer be treated as opaque. That could be bad.

In BasicLayers we have a couple of places where we do gfxUtils::ClipToRegionSnapped that have related issues.
The 580160-1 reftest failure is an intermittent failure on trunk already. Bug 615417 has my explanation of the failure (or at least, one cause of the failure).
Even with bug 615417 fixed I'm still seeing failures in 580160-1.html with my patch stack. The problem seems to be in the reftest harness; we're not correctly waiting to get the correct-size plugin image before taking the snapshot.

The code in reftest.js controlling how a test waits for completion has become a mess. There are really three different conditions we can be waiting for: 'reftest-wait' to be removed from the root element's class, all paints to be flushed, and all plugin "explicit waiters" to be removed. Right now we wait for one of those conditions at a time and transition from waiting for one to waiting for another. A simpler and more robust design would be to keep checking all three conditions at once in one place and stop when they're all complete simultaneously. We register listeners for a change in each condition that all just call back to that single CheckTestEnd function.
Attached patch Part 2: fix harness (obsolete) — Splinter Review
This is simpler, and I hope clearer, and seems to fix the failures I've been seeing.
Attachment #495465 - Flags: review?(dbaron)
Comment on attachment 495465 [details] [diff] [review]
Part 2: fix harness

No, this needs some work.
Attachment #495465 - Attachment is obsolete: true
Attachment #495465 - Flags: review?(dbaron)
I'll move the reftest harness changes to a new bug --- bug 617152
Depends on: 617152
OK, turns out the test here fails on D3D10 because of bug 617242 (fix in bug).
Comment on attachment 478634 [details] [diff] [review]
fix v2

Patch a bit outdated (some parts are landed)... do we still need it?
I'm ok with this patch, if it is pass plugins reftests
Attachment #478634 - Flags: superreview?(romaxa) → superreview+
Keywords: regression
Attached patch updated patchSplinter Review
Some of this landed in bug 617512. I think the rest is ready to land.
Whiteboard: [needs landing]
Whiteboard: [needs landing] → [needs landing][hardblocker](?)
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing][hardblocker](?) → [hardblocker](?)
You need to log in before you can comment on or make changes to this bug.