Last Comment Bug 566092 - Inspector highlight doesn't handle reflows correctly
: Inspector highlight doesn't handle reflows correctly
Status: RESOLVED FIXED
[minotaur][best: 1d; likely: 4d; wors...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: Firefox 12
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 566085 704484 716499 723091 (view as bug list)
Depends on: 703031
Blocks: 663830 703910
  Show dependency treegraph
 
Reported: 2010-05-14 20:16 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2012-04-17 09:59 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
patch v1 (2.25 KB, patch)
2012-01-10 13:17 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch - v1 - with test (4.88 KB, patch)
2012-01-13 21:58 PST, Paul Rouget [:paul]
rcampbell: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2010-05-14 20:16:25 PDT
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) 2010-08-20 09:24:41 PDT
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 2010-09-04 19:00:47 PDT
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Comment 3 Paul Rouget [:paul] 2011-07-22 02:23:31 PDT
Can't we use MozAfterPaint?
Comment 4 Rob Campbell [:rc] (:robcee) 2011-07-22 07:24:13 PDT
(In reply to comment #3)
> Can't we use MozAfterPaint?

that might work.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-07-25 17:17:36 PDT
tricky estimate. The MozAfterPaint experiment could Just Work or we might spend a bunch of time on it.
Comment 6 Mihai Sucan [:msucan] 2011-08-29 09:40:33 PDT
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 7 Paul Rouget [:paul] 2011-11-22 09:06:24 PST
*** Bug 704484 has been marked as a duplicate of this bug. ***
Comment 8 Paul Rouget [:paul] 2012-01-09 12:57:53 PST
*** Bug 716499 has been marked as a duplicate of this bug. ***
Comment 9 Paul Rouget [:paul] 2012-01-10 13:17:16 PST
Created attachment 587446 [details] [diff] [review]
patch v1
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-10 13:41:27 PST
Comment on attachment 587446 [details] [diff] [review]
patch v1

yup. we could add a simple test for this.
Comment 11 Dão Gottwald [:dao] 2012-01-10 13:43:11 PST
What's the point of the scroll event listener at this point?
Comment 12 Paul Rouget [:paul] 2012-01-10 14:02:18 PST
(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] 2012-01-10 18:58:00 PST
Apparently, that breaks a couple of tests: https://tbpl.mozilla.org/?tree=Try&rev=49785393e690
Comment 14 Paul Rouget [:paul] 2012-01-13 21:34:52 PST
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] 2012-01-13 21:58:28 PST
Created attachment 588598 [details] [diff] [review]
patch - v1 - with test
Comment 16 Paul Rouget [:paul] 2012-01-13 22:00:41 PST
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] 2012-01-17 04:27:28 PST
https://tbpl.mozilla.org/?tree=Try&rev=5d76eb6f3482
Comment 18 Paul Rouget [:paul] 2012-01-17 15:18:43 PST
Apparently, this is green enough.
Comment 19 Rob Campbell [:rc] (:robcee) 2012-01-23 06:53:42 PST
Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

looks good.
Comment 20 Rob Campbell [:rc] (:robcee) 2012-01-23 08:00:43 PST
https://hg.mozilla.org/integration/fx-team/rev/10825497af66
Comment 21 Tim Taubert [:ttaubert] 2012-01-24 06:10:40 PST
https://hg.mozilla.org/mozilla-central/rev/10825497af66
Comment 22 Rob Campbell [:rc] (:robcee) 2012-01-24 07:01:54 PST
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] 2012-01-25 18:27:34 PST
Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

[Triage Comment]
In support of a new FF10 feature - approved for Aurora 11.
Comment 24 Paul Rouget [:paul] 2012-02-01 06:42:03 PST
*** Bug 723091 has been marked as a duplicate of this bug. ***
Comment 25 Dão Gottwald [:dao] 2012-04-17 09:59:43 PDT
*** Bug 566085 has been marked as a duplicate of this bug. ***

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