566092 - Inspector highlight doesn't handle reflows correctly

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[(no longer active)]

STR:

1. Go to a bug, and highlight the status combobox.
2. Click in the comment box to make it larger.

Screenshot: http://grab.by/4o1S

Comment 1

[Rob Campbell [:rc] (:robcee)]

We need a reflow event in order to fix this properly. We have a bug 453650 on file to create that.

Comment 2

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 3

[Paul Rouget [:paul]]

Can't we use MozAfterPaint?

Comment 4

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #3)
> Can't we use MozAfterPaint?

that might work.

Comment 5

[Rob Campbell [:rc] (:robcee)]

tricky estimate. The MozAfterPaint experiment could Just Work or we might spend a bunch of time on it.

Comment 6

[Mihai Sucan [:msucan]]

The STR provided in comment #0 is fixed by the patch from bug 566085.

Please let me know if that is sufficient or further action is needed to fix this bug. Thank you!

Comment 9

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 587446 [details] [diff] [review]
patch v1

yup. we could add a simple test for this.

Comment 11

[Dão Gottwald [:dao]]

What's the point of the scroll event listener at this point?

Comment 12

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #11)
> What's the point of the scroll event listener at this point?

From what I can see, the scroll events are fired much more frequently than the MozAfterPaint events.
Using only MozAfterPaint makes the scrolling animation too slow.

Comment 13

[Paul Rouget [:paul]]

Apparently, that breaks a couple of tests: https://tbpl.mozilla.org/?tree=Try&rev=49785393e690

Comment 14

[Paul Rouget [:paul]]

So I thought it might be a good idea to make some test before call highlight:

> let newRect = aEvent.boundingClientRect;
> if (this._oldAfterPainRect) {
>   // Are we getter the same rect again? (like a video being played for
>   // example)
>   if (this._oldAfterPainRect.top == newRect.top &&
>       this._oldAfterPainRect.left == newRect.left &&
>       this._oldAfterPainRect.width == newRect.width &&
>       this._oldAfterPainRect.height == newRect.height) {
>     break;
>   }
>
>   // Are the changes happening inside the current selection?
>   if (this._contentRect &&
>       newRect.top >= this._contentRect.top &&
>       newRect.left >= this._contentRect.left &&
>       newRect.width <= this._contentRect.width &&
>       newRect.height <= this._contentRect.height) {
>     break;
>   }
> } else {
>   this._oldAfterPainRect = {};
> }
> this._oldAfterPainRect.top = newRect.top;
> this._oldAfterPainRect.left = newRect.left;
> this._oldAfterPainRect.width = newRect.width;
> this._oldAfterPainRect.height = newRect.height;

But this is seriously slowing down the redraw. So I won't do that.

Comment 15

[Paul Rouget [:paul]]

Attachment: patch - v1 - with test

Comment 16

[Paul Rouget [:paul]]

Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

re-asking review because test added and based on the new highlighter code.

Comment 17

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=5d76eb6f3482

Comment 18

[Paul Rouget [:paul]]

Apparently, this is green enough.

Comment 19

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

looks good.

Comment 20

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/10825497af66

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/10825497af66

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

[Approval Request Comment]
Regression caused by (bug #): Not a regression. An improvement.
User impact if declined: Users won't get updated highlighter rectangles on document reflow. Their nodes will be all awry!
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Limited risk. This tested out fine on our systems.

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

[Triage Comment]
In support of a new FF10 feature - approved for Aurora 11.