combobox invalidated when scrolled

RESOLVED FIXED in mozilla18

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

91 bytes, text/html
Details
fix
9.27 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
Created attachment 638176 [details]
testcase

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
Created attachment 638183 [details] [diff] [review]
fix
Attachment #638183 - Flags: review?(matt.woodrow)
Created attachment 638184 [details] [diff] [review]
Part 2: don't invalidate outside clip
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+
Blocks: 770061
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out as part of DLBI: https://hg.mozilla.org/mozilla-central/rev/2cf9546ee691
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 645184 [details] [diff] [review]
combobox invalidation mochitest
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

Updated

5 years ago
Blocks: 776695
Confirm that my test passes on trunk, fails 15 checks with DLBI.
Created attachment 645392 [details] [diff] [review]
form controls invalidation mochitest

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+
Created attachment 652465 [details] [diff] [review]
form controls invalidation mochitest
Attachment #645392 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=c02050c203b4
(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.
Backed out (see bug 539356 comment 337):
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f86e3a3240
https://hg.mozilla.org/mozilla-central/rev/32c4a08f8e6e
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Created attachment 671320 [details] [diff] [review]
Part 2: don't invalidate outside clip

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
Created attachment 671329 [details] [diff] [review]
form controls invalidation mochitest

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+
Created attachment 671607 [details] [diff] [review]
Part 2: don't invalidate outside clip v2
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
Created attachment 671652 [details] [diff] [review]
Part 2: don't invalidate outside clip v3
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
Created attachment 671659 [details] [diff] [review]
Part 2: don't invalidate outside clip v4


> 
> 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67ff8ae77e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e19d625511a0
Backed out the test change for failing on WinXP: https://hg.mozilla.org/integration/mozilla-inbound/rev/946b285bc14c
https://hg.mozilla.org/mozilla-central/rev/c67ff8ae77e2

Updated

5 years ago
Depends on: 855316
Depends on: 989403
You need to log in before you can comment on or make changes to this bug.