Inspector highlight doesn't handle reflows correctly

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: msucan)

Tracking

Trunk
Firefox 12
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 wontfix)

Details

(Whiteboard: [minotaur][best: 1d; likely: 4d; worst: 2w])

Attachments

(1 attachment, 1 obsolete attachment)

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
We need a reflow event in order to fix this properly. We have a bug 453650 on file to create that.

Updated

7 years ago
Severity: normal → blocker

Comment 2

7 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal

Updated

6 years ago
Whiteboard: [minotaur]

Comment 3

6 years ago
Can't we use MozAfterPaint?

Updated

6 years ago
Blocks: 663830
(In reply to comment #3)
> Can't we use MozAfterPaint?

that might work.
tricky estimate. The MozAfterPaint experiment could Just Work or we might spend a bunch of time on it.
Priority: -- → P2
Whiteboard: [minotaur] → [minotaur][best: 1d; likely: 4d; worst: 2w]
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Assignee: rcampbell → mihai.sucan
(Assignee)

Comment 6

6 years ago
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!

Updated

6 years ago
Duplicate of this bug: 704484

Updated

5 years ago
Duplicate of this bug: 716499

Updated

5 years ago
Blocks: 703910

Comment 9

5 years ago
Created attachment 587446 [details] [diff] [review]
patch v1

Updated

5 years ago
Attachment #587446 - Flags: review?(rcampbell)
Comment on attachment 587446 [details] [diff] [review]
patch v1

yup. we could add a simple test for this.
Attachment #587446 - Flags: review?(rcampbell) → review+
What's the point of the scroll event listener at this point?

Comment 12

5 years ago
(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

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

Comment 14

5 years ago
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.

Updated

5 years ago
Depends on: 703031

Comment 15

5 years ago
Created attachment 588598 [details] [diff] [review]
patch - v1 - with test

Updated

5 years ago
Attachment #587446 - Attachment is obsolete: true

Comment 16

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

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

Updated

5 years ago
Attachment #588598 - Flags: review?(rcampbell)

Comment 17

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

Comment 18

5 years ago
Apparently, this is green enough.
Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

looks good.
Attachment #588598 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/integration/fx-team/rev/10825497af66
Whiteboard: [minotaur][best: 1d; likely: 4d; worst: 2w] → [minotaur][best: 1d; likely: 4d; worst: 2w][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/10825497af66
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][best: 1d; likely: 4d; worst: 2w][fixed-in-fx-team] → [minotaur][best: 1d; likely: 4d; worst: 2w]
Target Milestone: --- → Firefox 12
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.
Attachment #588598 - Flags: approval-mozilla-aurora?
Comment on attachment 588598 [details] [diff] [review]
patch - v1 - with test

[Triage Comment]
In support of a new FF10 feature - approved for Aurora 11.
Attachment #588598 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox11: --- → wontfix

Updated

5 years ago
Duplicate of this bug: 723091

Updated

5 years ago
Duplicate of this bug: 566085
You need to log in before you can comment on or make changes to this bug.