Last Comment Bug 770001 - combobox invalidated when scrolled
: combobox invalidated when scrolled
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 2 votes (vote)
: mozilla18
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 855316 989403
Blocks: 546362 dlbi 770061 776695
  Show dependency treegraph
 
Reported: 2012-07-01 04:26 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2014-07-28 09:27 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (91 bytes, text/html)
2012-07-01 04:26 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
fix (9.27 KB, patch)
2012-07-01 07:01 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review
Part 2: don't invalidate outside clip (3.58 KB, patch)
2012-07-01 07:02 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review
combobox invalidation mochitest (3.70 KB, patch)
2012-07-23 21:05 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
form controls invalidation mochitest (4.82 KB, patch)
2012-07-24 11:17 PDT, Benoit Girard (:BenWa)
roc: review+
Details | Diff | Review
form controls invalidation mochitest (4.97 KB, patch)
2012-08-16 08:47 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Part 2: don't invalidate outside clip (12.47 KB, patch)
2012-10-14 22:38 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
form controls invalidation mochitest (5.79 KB, patch)
2012-10-14 23:54 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Review
Part 2: don't invalidate outside clip v2 (13.07 KB, patch)
2012-10-15 14:40 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Part 2: don't invalidate outside clip v3 (14.16 KB, patch)
2012-10-15 16:40 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Review
Part 2: don't invalidate outside clip v4 (14.24 KB, patch)
2012-10-15 17:01 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 04:26:20 PDT
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.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 06:42:18 PDT
The problem is clipping. The test *oldClip != aClip ignores the fact that the clips can be different due to scrolling.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 07:01:23 PDT
Created attachment 638183 [details] [diff] [review]
fix
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 07:02:06 PDT
Created attachment 638184 [details] [diff] [review]
Part 2: don't invalidate outside clip
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 07:03:11 PDT
Part 2 isn't necessary here, but seems simple and worth having. (We would have prevented this invalidation before DLBI.)
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-01 07:05:57 PDT
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.
Comment 6 Matt Woodrow (:mattwoodrow) 2012-07-01 16:14:08 PDT
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.
Comment 7 Jim Jeffery not reading bug-mail 1/2/11 2012-07-02 04:51:14 PDT
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)
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-02 17:57:06 PDT
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.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-03 18:01:48 PDT
Backed out as part of DLBI: https://hg.mozilla.org/mozilla-central/rev/2cf9546ee691
Comment 10 Benoit Girard (:BenWa) 2012-07-23 21:05:51 PDT
Created attachment 645184 [details] [diff] [review]
combobox invalidation mochitest
Comment 11 Benoit Girard (:BenWa) 2012-07-23 21:06:54 PDT
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
Comment 12 Benoit Girard (:BenWa) 2012-07-24 10:28:22 PDT
Confirm that my test passes on trunk, fails 15 checks with DLBI.
Comment 13 Benoit Girard (:BenWa) 2012-07-24 11:17:31 PDT
Created attachment 645392 [details] [diff] [review]
form controls invalidation mochitest

Fix tittle, adding more test that fail with DLBI
Comment 14 Matt Woodrow (:mattwoodrow) 2012-07-30 13:29:46 PDT
Comment on attachment 645392 [details] [diff] [review]
form controls invalidation mochitest

I'd prefer if roc reviewed html tests.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 14:50:41 PDT
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
Comment 16 Benoit Girard (:BenWa) 2012-08-16 08:47:26 PDT
Created attachment 652465 [details] [diff] [review]
form controls invalidation mochitest
Comment 17 Benoit Girard (:BenWa) 2012-08-16 08:48:52 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c02050c203b4
Comment 18 Benoit Girard (:BenWa) 2012-08-17 08:14:44 PDT
(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.
Comment 19 Timothy Nikkel (:tnikkel) 2012-08-17 08:52:42 PDT
Disable on Linux? Testing on Mac and Windows is better than no testing at all.
Comment 21 Ed Morley [:emorley] 2012-09-28 16:19:55 PDT
https://hg.mozilla.org/mozilla-central/rev/32c4a08f8e6e
Comment 22 Matt Woodrow (:mattwoodrow) 2012-10-14 22:38:24 PDT
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.
Comment 23 Matt Woodrow (:mattwoodrow) 2012-10-14 22:39:50 PDT
Uh, I mean bug 546362
Comment 24 Matt Woodrow (:mattwoodrow) 2012-10-14 23:54:43 PDT
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.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 02:31:47 PDT
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.
Comment 26 Matt Woodrow (:mattwoodrow) 2012-10-15 14:40:36 PDT
Created attachment 671607 [details] [diff] [review]
Part 2: don't invalidate outside clip v2
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 14:49:57 PDT
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
Comment 28 Matt Woodrow (:mattwoodrow) 2012-10-15 16:40:18 PDT
Created attachment 671652 [details] [diff] [review]
Part 2: don't invalidate outside clip v3
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 16:51:48 PDT
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
Comment 30 Matt Woodrow (:mattwoodrow) 2012-10-15 17:01:58 PDT
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.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-15 17:03:25 PDT
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.
Comment 32 Matt Woodrow (:mattwoodrow) 2012-10-15 18:12:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67ff8ae77e2
Comment 33 Matt Woodrow (:mattwoodrow) 2012-10-15 18:12:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e19d625511a0
Comment 34 Matt Woodrow (:mattwoodrow) 2012-10-15 21:00:40 PDT
Backed out the test change for failing on WinXP: https://hg.mozilla.org/integration/mozilla-inbound/rev/946b285bc14c
Comment 35 Ed Morley [:emorley] 2012-10-16 01:19:34 PDT
https://hg.mozilla.org/mozilla-central/rev/c67ff8ae77e2

Note You need to log in before you can comment on or make changes to this bug.