Last Comment Bug 689939 - Infobar disappears when node fills top and bottom of screen during page zoom in highlighter
: Infobar disappears when node fills top and bottom of screen during page zoom ...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 12
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 668254
Blocks: 663830 694954
  Show dependency treegraph
 
Reported: 2011-09-28 07:29 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-12-29 09:36 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (4.94 KB, patch)
2011-12-07 07:23 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.1 (5.24 KB, patch)
2011-12-19 08:04 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.2 (7.88 KB, patch)
2011-12-19 08:45 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-09-28 07:29:25 PDT
STR:

1. Open Highlighter
2. Select a large node on the screen.
3. Hit ctrl/cmd-+ several times so the node reaches both top and bottom of the window

Expected results:

Infobar should move to the top of the available window space inside the node.

Actual:

Infobar disappears offscreen.
Comment 1 Paul Rouget [:paul] 2011-10-11 03:00:18 PDT
I can't reproduce. Can you give me an example?
Comment 2 Paul Rouget [:paul] 2011-10-11 06:07:08 PDT
That might be fixed once bug 690068 get fixed.
Comment 3 Dave Camp (:dcamp) 2011-10-17 08:52:13 PDT
I can reproduce this with a bugzilla body element, but only zooming out, not zooming in.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-10-17 08:58:47 PDT
Ditto, replace Cmd-+ in C#0 with Cmd-- and you can see this bug in action.

Dave suggested the reason might be that we're doing our bounds checking in the pre-scaled coordinate space.
Comment 5 Paul Rouget [:paul] 2011-10-25 07:26:05 PDT
Bug 690068 hasn't fixed this problem.
Comment 6 Dave Camp (:dcamp) 2011-10-27 08:52:01 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 7 Paul Rouget [:paul] 2011-12-07 07:23:00 PST
Created attachment 579683 [details] [diff] [review]
patch v1
Comment 8 Rob Campbell [:rc] (:robcee) 2011-12-12 06:37:03 PST
Comment on attachment 579683 [details] [diff] [review]
patch v1

       case "resize":
+        this.getZoomFactor(true);

are there any other cases where you'll want to send true to getZoomFactor?

Without that, we could do away with the cached value (this._zoom) and just call the lookup directly. It should be pretty fast and we'd be able to use a real getter.

I don't feel strongly about it though and this looks like it'll do the job nicely, so...
Comment 9 Paul Rouget [:paul] 2011-12-19 08:04:30 PST
Created attachment 582830 [details] [diff] [review]
patch v1.1

better?
Comment 10 Rob Campbell [:rc] (:robcee) 2011-12-19 08:22:27 PST
Comment on attachment 582830 [details] [diff] [review]
patch v1.1

you can probably reuse the test in browser_inspector_highlighter.js which already does some zoom calculation. Making it call your computeZoomFactor method will get it tested.

r+ with changes to the test to make that happen.
Comment 11 Paul Rouget [:paul] 2011-12-19 08:45:42 PST
Created attachment 582839 [details] [diff] [review]
patch v1.2

with test
Comment 12 Paul Rouget [:paul] 2011-12-19 08:49:25 PST
Thx for the r+. Updated the test as you asked.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-12-19 10:43:39 PST
-    let rect = this._highlightRect;
-    if (rect && this._highlighting) {
+    if (this._highlightRect) {
       let winHeight = this.win.innerHeight * this.zoom;
       let winWidth = this.win.innerWidth * this.zoom;
+
+      let rect = {top: this._highlightRect.top,
+                  left: this._highlightRect.left,
+                  width: this._highlightRect.width,
+                  height: this._highlightRect.height};

this didn't apply cleanly. Wondering if you had some other patch installed previously, as the let winHeight = … and winWidth lines were not there.

Added them.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-12-19 10:44:45 PST
ignore the above. wrong bug. :/
Comment 15 Rob Campbell [:rc] (:robcee) 2011-12-19 10:47:06 PST
and now I see why the above happened. :)
Comment 16 Rob Campbell [:rc] (:robcee) 2011-12-19 10:57:05 PST
https://hg.mozilla.org/integration/fx-team/rev/cf81dc222041
Comment 17 Tim Taubert [:ttaubert] 2011-12-20 00:56:01 PST
Backed out because of orange

https://hg.mozilla.org/integration/fx-team/rev/37b413b9c87b
Comment 18 Paul Rouget [:paul] 2011-12-22 06:33:43 PST
This was not the cause of the orange.
Comment 19 Rob Campbell [:rc] (:robcee) 2011-12-22 11:18:31 PST
https://hg.mozilla.org/integration/fx-team/rev/594b1a6f6a97
Comment 20 Tim Taubert [:ttaubert] 2011-12-29 09:36:58 PST
https://hg.mozilla.org/mozilla-central/rev/594b1a6f6a97

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