Closed Bug 770001 Opened 12 years ago Closed 12 years ago

combobox invalidated when scrolled

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: roc, Assigned: roc)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

testcase
(deleted), text/html
Details
fix
(deleted), patch
mattwoodrow
: review+
Details | Diff | Splinter Review
form controls invalidation mochitest
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Part 2: don't invalidate outside clip v4
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase (deleted) —
Turn on paint flashing and load the testcase, then scroll down a bit. The line on the page containing the <select> is invalidated on every scroll.
The problem is clipping. The test *oldClip != aClip ignores the fact that the clips can be different due to scrolling.
Assignee: nobody → roc
Attached patch fix (deleted) — — Splinter Review
Attachment #638183 - Flags: review?(matt.woodrow)
Attached patch Part 2: don't invalidate outside clip (obsolete) (deleted) — — Splinter Review
Attachment #638184 - Flags: review?(matt.woodrow)
Part 2 isn't necessary here, but seems simple and worth having. (We would have prevented this invalidation before DLBI.)
Side note: in InvalidateForLayerChange, in the same-layer case, why do we need to include mActiveScrolledRootPosition in offset/prevOffset to calculate shift? And why do we need to store mActiveScrolledRootPosition in oldGeometry? Because we know here the layer is the same, and if mActiveScrolledRootPosition has changed in the layer then CreateOrRecycleThebesLayer will have invalidated the whole layer.
Attachment #638183 - Flags: review?(matt.woodrow) → review+
Comment on attachment 638184 [details] [diff] [review] Part 2: don't invalidate outside clip Review of attachment 638184 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, though both patches should also probably modify FrameLayerBuilder::ProcessRemovedDisplayItems (called when a display item from a previous paint isn't in the current display list) as well. Let me think about comment 5 for a bit, I had a definite reason for writing it.
Attachment #638184 - Flags: review?(matt.woodrow) → review+
https://tbpl.mozilla.org/ is showing this as landed on m-c (merge) from m-i. I see the bug number on both m-i and m-c, but its not marked as having landed anywhere, or marked as 'fixed' ? Honestly I don't know what I'm looking for in the rainbow of color on the test case, so I for sure can't say if its fixed or not, but I am running the latest m-c hourly which supposedly does contain this patch. (according to tbpl)
I landed part 1: http://hg.mozilla.org/integration/mozilla-inbound/rev/2cf9546ee691 which got merged to central: http://hg.mozilla.org/mozilla-central/rev/2cf9546ee691 I didn't land part 2 because I suspect it of causing regressions in my local build, and it's just an optimization which we may not need yet. But this testcase should be fixed on central now.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Backed out as part of DLBI: https://hg.mozilla.org/mozilla-central/rev/2cf9546ee691
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch combobox invalidation mochitest (obsolete) (deleted) — — Splinter Review
Attachment #645184 - Flags: review?(matt.woodrow)
Comment on attachment 645184 [details] [diff] [review] combobox invalidation mochitest Review of attachment 645184 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/test_invalidation_combo.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test that images that are the only item in ThebesLayers get put into ImageLayers</title> Forgot to update the title. oops
Blocks: 776695
Confirm that my test passes on trunk, fails 15 checks with DLBI.
Attached patch form controls invalidation mochitest (obsolete) (deleted) — — Splinter Review
Fix tittle, adding more test that fail with DLBI
Attachment #645184 - Attachment is obsolete: true
Attachment #645184 - Flags: review?(matt.woodrow)
Attachment #645392 - Flags: review?(matt.woodrow)
Comment on attachment 645392 [details] [diff] [review] form controls invalidation mochitest I'd prefer if roc reviewed html tests.
Attachment #645392 - Flags: review?(matt.woodrow) → review?(roc)
Comment on attachment 645392 [details] [diff] [review] form controls invalidation mochitest Review of attachment 645392 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/test_invalidation_form.html @@ +69,5 @@ > + var shouldRepaint = posY % 10 == 0; > + > + if (lastPaintCount + 1 != window.mozPaintCount) { > + for (var i = 0; i < NO_REPAINT_ITEMS.length; i++) { > + dump(NO_REPAINT_ITEMS[i] + "\n"); remove the dump
Attachment #645392 - Flags: review?(roc) → review+
Attached patch form controls invalidation mochitest (obsolete) (deleted) — — Splinter Review
Attachment #645392 - Attachment is obsolete: true
(In reply to Benoit Girard (:BenWa) from comment #17) > https://tbpl.mozilla.org/?tree=Try&rev=c02050c203b4 We can't land this testcase because it fails on Linux.
Disable on Linux? Testing on Mac and Windows is better than no testing at all.
https://hg.mozilla.org/mozilla-central/rev/32c4a08f8e6e
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attached patch Part 2: don't invalidate outside clip (obsolete) (deleted) — — Splinter Review
Rebased your patch, adding clipping in a few more locations and made sure we don't clip everything out when mHaveClipRect == false. https://tbpl.mozilla.org/?tree=Try&rev=d47c84594e2b Fixes bug 456362 entirely.
Attachment #638184 - Attachment is obsolete: true
Attachment #671320 - Flags: review?(roc)
Uh, I mean bug 546362
Blocks: 546362
Simplified this a fair bit and added some code to make it easier to write invalidation tests.
Attachment #652465 - Attachment is obsolete: true
Attachment #671329 - Flags: review?(roc)
Comment on attachment 671320 [details] [diff] [review] Part 2: don't invalidate outside clip Review of attachment 671320 [details] [diff] [review]: ----------------------------------------------------------------- The rest looks OK ::: gfx/2d/BaseRect.h @@ +183,5 @@ > void MoveBy(T aDx, T aDy) { x += aDx; y += aDy; } > void MoveBy(const Point& aPoint) { x += aPoint.x; y += aPoint.y; } > + Sub MovedBy(const Point& aPoint) const { > + return Sub(x + aPoint.x, y + aPoint.y, width, height); > + } Why not just use operator+? ::: layout/base/FrameLayerBuilder.cpp @@ +2168,5 @@ > if (!oldLayer) { > // This item is being added for the first time, invalidate its entire area. > //TODO: We call GetGeometry again in AddThebesDisplayItem, we should reuse this. > + combined = aClip.mHaveClipRect ? aGeometry->ComputeInvalidationRegion().Intersect(aClip.NonRoundedIntersection()) : > + aGeometry->ComputeInvalidationRegion(); Define a helper method on aClip that takes a region parameter and intersects it with the clip rect if we have one, returning a region. That'll make this code much more bearable.
Attachment #671329 - Flags: review?(roc) → review+
Attached patch Part 2: don't invalidate outside clip v2 (obsolete) (deleted) — — Splinter Review
Attachment #671320 - Attachment is obsolete: true
Attachment #671320 - Flags: review?(roc)
Attachment #671607 - Flags: review?(roc)
Comment on attachment 671607 [details] [diff] [review] Part 2: don't invalidate outside clip v2 Review of attachment 671607 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +809,5 @@ > + nsRect rect = aRect; > + > + if (aClip.mHaveClipRect) { > + rect.Intersect(aClip.NonRoundedIntersection()); > + } nsRect rect = aClip.ApplyNonRoundedIntersection(aRect) @@ +2173,5 @@ > printf("Display item type %s(%p) added to layer %p!\n", aItem->Name(), aItem->GetUnderlyingFrame(), aNewLayer); > #endif > } else if (aItem->IsInvalid(invalid) && invalid.IsEmpty()) { > // Either layout marked item as needing repainting, invalidate the entire old and new areas. > + nsRegion oldInvalid = oldClip->ApplyNonRoun um @@ +2204,5 @@ > + nsRect temp = oldClip->NonRoundedIntersection(); > + temp.MoveBy(shift); > + clip.Or(clip, temp); > + hasClip = true; > + } This actually doesn't seem right. If one clip has a clip rect and the other one doesn't have a clip rect, we're clipping to the one that does have a clip rect. But I think we shouldn't clip at all. No clip rect should be treated as an infinite rect so 'clip' would be infinite. @@ +2285,5 @@ > + if (oldClip && oldClip->mHaveClipRect) { > + nsRect temp = oldClip->NonRoundedIntersection(); > + temp.MoveBy(aTopLeft - thebesData->mLastActiveScrolledRootOrigin); > + clip.Or(clip, temp); > + hasClip = true; We should share code with the previous hunk that computes a region containing the union of two clips (which could be infinite) @@ +3440,5 @@ > + return nsRegion(aRect); > + } > + > + nsRegion result = aRect; > + result.And(result, mClipRect); result.And(aRect, mClipRect); ::: layout/base/FrameLayerBuilder.h @@ +438,5 @@ > nsRect NonRoundedIntersection() const; > > + // Intersect the given rects with all rects in this clip, ignoring any > + // rounded corners. > + nsRegion ApplyNonRoundedIntersection(const nsRect& aRect) const; This should just return a rectangle
Attached patch Part 2: don't invalidate outside clip v3 (obsolete) (deleted) — — Splinter Review
Attachment #671607 - Attachment is obsolete: true
Attachment #671607 - Flags: review?(roc)
Attachment #671652 - Flags: review?(roc)
Comment on attachment 671652 [details] [diff] [review] Part 2: don't invalidate outside clip v3 Review of attachment 671652 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2127,5 @@ > + if (aOldClip) { > + aCombined = aOldClip->NonRoundedIntersection(); > + aCombined.MoveBy(aShift); > + } > + aCombined.Or(aCombined, aClip.NonRoundedIntersection()); Freaky. aCombined is an in-out parameter if aOldClip is nonnull, otherwise it's out-only? That seems too odd. I think it should be an out-parameter only. @@ +2286,5 @@ > } > + > + // We need to grab these before calling AddLayerDisplayItem because it will overwrite them. > + nsRegion clip; > + FrameLayerBuilder::Clip* oldClip = NULL; nullptr
> > Freaky. aCombined is an in-out parameter if aOldClip is nonnull, otherwise > it's out-only? That seems too odd. I think it should be an out-parameter > only. > I guess.. I was assuming it was always empty to begin with. I'll enforce that.
Attachment #671652 - Attachment is obsolete: true
Attachment #671652 - Flags: review?(roc)
Attachment #671659 - Flags: review?(roc)
Comment on attachment 671659 [details] [diff] [review] Part 2: don't invalidate outside clip v4 Review of attachment 671659 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2129,5 @@ > + aCombined.MoveBy(aShift); > + } else { > + aCombined = nsRegion(); > + } > + aCombined.Or(aCombined, aClip.NonRoundedIntersection()); Just move the Or into the "if" body.
Attachment #671659 - Flags: review?(roc) → review+
Depends on: 855316
Depends on: 989403
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: