Infobar disappears when node fills top and bottom of screen during page zoom in highlighter

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: paul)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

6 years ago
I can't reproduce. Can you give me an example?
(Assignee)

Comment 2

6 years ago
That might be fixed once bug 690068 get fixed.
(Assignee)

Updated

6 years ago
Blocks: 663830

Comment 3

6 years ago
I can reproduce this with a bugzilla body element, but only zooming out, not zooming in.
(Reporter)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Bug 690068 hasn't fixed this problem.

Comment 6

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2
(Assignee)

Comment 7

6 years ago
Created attachment 579683 [details] [diff] [review]
patch v1
(Assignee)

Updated

6 years ago
Attachment #579683 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Blocks: 694954
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 8

6 years ago
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...
Attachment #579683 - Flags: review?(rcampbell) → review+
(Reporter)

Updated

6 years ago
Whiteboard: [needs-test]
(Assignee)

Comment 9

6 years ago
Created attachment 582830 [details] [diff] [review]
patch v1.1

better?
(Assignee)

Updated

6 years ago
Attachment #582830 - Flags: review?(rcampbell)
(Reporter)

Comment 10

6 years ago
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.
Attachment #582830 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 582839 [details] [diff] [review]
patch v1.2

with test
(Assignee)

Updated

6 years ago
Attachment #582830 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Thx for the r+. Updated the test as you asked.
Whiteboard: [needs-test] → [land-in-fx-team]
(Reporter)

Comment 13

6 years ago
-    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.
(Reporter)

Comment 14

6 years ago
ignore the above. wrong bug. :/
(Reporter)

Comment 15

6 years ago
and now I see why the above happened. :)
(Reporter)

Comment 16

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/cf81dc222041
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Backed out because of orange

https://hg.mozilla.org/integration/fx-team/rev/37b413b9c87b
Whiteboard: [fixed-in-fx-team]

Updated

6 years ago
Target Milestone: --- → Firefox 12
(Assignee)

Comment 18

6 years ago
This was not the cause of the orange.
Whiteboard: [land-in-fx-team]
(Reporter)

Comment 19

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/594b1a6f6a97
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/594b1a6f6a97
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.