The default bug view has changed. See this FAQ.

Highlighter doesn't handle full page zoom

VERIFIED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P2
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: Kyle Simpson)

Tracking

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 1d][fixed-in-fx-team])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Zoom in on a page, then start inspecting, or zoom while inspecting. The rectangles don't appear where they should.
(Reporter)

Updated

6 years ago
Blocks: 663830

Updated

6 years ago
Duplicate of this bug: 672478

Updated

6 years ago
Summary: Inspector doesn't handle full page zoom → Highlighter doesn't handle full page zoom

Updated

6 years ago
Whiteboard: [minotaur]
Whiteboard: [minotaur] → [minotaur][best: 1h; likely: 3h; worst: 1d]
Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 1d] → [minotaur][best: 1h; likely: 3h; worst: 1d][p2]
Priority: -- → P2
Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 1d][p2] → [minotaur][best: 1h; likely: 3h; worst: 1d]

Updated

6 years ago
Assignee: nobody → getify
(Assignee)

Comment 2

6 years ago
Created attachment 557861 [details] [diff] [review]
fix and tests
Attachment #557861 - Flags: review?(rcampbell)
Comment on attachment 557861 [details] [diff] [review]
fix and tests

yes!
Attachment #557861 - Flags: review?(rcampbell) → review+
oh wait. Are we supposed to take into account changes to full page zoom when they occur? This patch won't do that as a user changes the zoom level of the page with a node selected. They'll have to rehighlight the node.

We should probably find the page zoom handlers and either hook in their or find some better place to do this. Sorry.
hook in *there*. ugh.
Status: NEW → ASSIGNED
Attachment #557861 - Flags: review+ → review-
Comment on attachment 557861 [details] [diff] [review]
fix and tests

Hmm, I was going to suggest using ZoomManager.zoom, but it seems to sometimes produce slightly different results - I guess getting screenPixelsPerCSSPixel directly is more accurate anyways.

Comment 7

6 years ago
This zooms the rect before calling highlightRect.  highlightRect stores the rect for later use in hit testing the underlying document (in tests).  Should that hit testing be done with the zoomed rect or the original rect?
(In reply to Dave Camp (:dcamp) from comment #7)
> This zooms the rect before calling highlightRect.  highlightRect stores the
> rect for later use in hit testing the underlying document (in tests). 
> Should that hit testing be done with the zoomed rect or the original rect?

Probably the original, since the underlying coordinates are still the "unzoomed" ones. I.e., the coords returned by getBoundingClientRect() are before the zoom has been applied. Hence the origin of this bug.

Also, as discussed in IRC, it seems that firing the zoom controls via menu or keyboard triggers a resize event which we're already listening for. I think this patch is good!
Attachment #557861 - Flags: review- → review+
Attachment #557861 - Flags: review?(gavin.sharp)

Comment 9

6 years ago
Comment on attachment 557861 [details] [diff] [review]
fix and tests

If it should be storing the original, this patch is wrong.
Attachment #557861 - Flags: feedback-
(Assignee)

Comment 10

6 years ago
Created attachment 558338 [details] [diff] [review]
updated to address feedback from dcamp
Attachment #558338 - Flags: review?(rcampbell)
(Assignee)

Comment 11

6 years ago
Created attachment 558344 [details] [diff] [review]
updated to address feedback from dcamp
Attachment #557861 - Attachment is obsolete: true
Attachment #558338 - Attachment is obsolete: true
Attachment #558344 - Flags: review?(rcampbell)
Attachment #558344 - Flags: review?(gavin.sharp)
Attachment #557861 - Flags: review?(gavin.sharp)
Attachment #558338 - Flags: review?(rcampbell)
Comment on attachment 558344 [details] [diff] [review]
updated to address feedback from dcamp

goodgood.
Attachment #558344 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 13

6 years ago
This is a continuation of "zoom" discussion from Bug #674871, comments 23, 25, 27-28, and 31-33. I posed a question on that thread if the new API `getQuads()` would provide post-zoom coordinates, as that API would be useful in creating a better/viable test for *this* bug. But to not hijack that thread with discussion about zoom, I'm moving the rest of the conversation back over to this thread.

roc-

I am already using `nsIDOMWindowUtils::screenPixelsPerCSSPixel` as the scaling factor to draw the highlighter boxes. That's not the issue. The issue is, when we want to run a test, to see if the boxes were in fact drawn to the same size/placement as the element they are overlayed on top of. 

Since we currently cannot ask the original element what his post-zoom coordinates are (only his pre-zoom coordinates are available), the only way to "test" is:

1. get the pre-zoom element coordinates, multiply them by the `screenPixelsPerCSSPixel` to *simulate* the post-zoom coordinates.
2. compare those simulated values to the actual dimensions of the highlighter boxes.

The problem is, this is a moot test. 

Because the highlighter box is calculated by the *exact* same multiplication, then the test is essentially saying a*b == a*b, which is of course only testing the javascript math engine. By literally duplicating code from the production code into the test, we aren't really testing that code at all. To truly test, we need an independent (2nd) way to verify the coordinates, but it appears at the moment there's only 1 way to get them: scaling with `screenPixelsPerCSSPixel`.
Have you tried using drawWindow to snapshot the chrome window and then compare some subregion of the drawWindow to a reference in a reftest-like manner, using WindowSnapshot.js?
(Assignee)

Comment 15

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Have you tried using drawWindow to snapshot the chrome window and then
> compare some subregion of the drawWindow to a reference in a reftest-like
> manner, using WindowSnapshot.js?

No, I haven't tried that approach yet. I discussed with several people using the reftests approach, and it seemed like from their perspective it wouldn't work for this case, so I didn't really pursue it much further.

I also wondered about doing some sort of screenshot'ing as you describe, but wasn't clear if the screenshot would capture only the highlighter layer, or only the content layer, or both, and also whether the screenshot would be post-zoom or pre-zoom.

In the reftest/screenshot case, wouldn't we have to compare with no zoom, to with zoom? If not, then what two screenshots would we compare? And if so, then how do you compare two screenshots which are fundamentally different (one is zoomed, one isn't)?
You can drawWindow the chrome window to get everything in the window, including your highlighter content and the Web content under it. reftests work like this, and it even works cross-process.

It's easy enough to create a Web content document which, when zoomed by a factor of two (for example), looks exactly like a Web content document with no zoom. Then I imagine it would be relatively easy to take a screenshot of the highlighter applied to each of the two documents and verify that the screenshots look the same.
(Assignee)

Comment 17

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> You can drawWindow the chrome window to get everything in the window,
> including your highlighter content and the Web content under it. reftests
> work like this, and it even works cross-process.

Thanks for that clarification.

> It's easy enough to create a Web content document which, when zoomed by a
> factor of two (for example), looks exactly like a Web content document with
> no zoom. Then I imagine it would be relatively easy to take a screenshot of
> the highlighter applied to each of the two documents and verify that the
> screenshots look the same.

So, to make sure I understand, you're saying manually create *two separate* documents, say one with a box 100px wide and another with a box 200px wide. take a screenshot of the second page (200px) with no zoom, and a screenshot of the first page(100px) at 2x zoom, and the compare the two screenshots?
Yes, with the highlighter applied in both cases.
Attachment #558344 - Flags: review?(gavin.sharp) → review+

Comment 19

6 years ago
This test is failing on my machine.  offsetWidth and offsetHeight return integers, but the highlighted element has a non-integer height.

Would probably be easiest to just use a predictably-sized box for this particular test rather than text whose size will depend on platform/fonts/etc.
(Assignee)

Comment 20

6 years ago
Created attachment 561595 [details] [diff] [review]
fixed with more feedback from dcamp

fixed the test per dcamp, to use getBoundingClientRect() instead of offsetWidth/Height
Attachment #558344 - Attachment is obsolete: true
Attachment #561595 - Flags: review?(dcamp)

Comment 21

6 years ago
Comment on attachment 561595 [details] [diff] [review]
fixed with more feedback from dcamp

This fix for the test looks fine.  Two problems have cropped up since the original patch was submitted:

1) We no longer animate transitions of the highlighter once the inspector is locked.  Very trivial fix, will attach an updated patch.
2) It didn't take long for _highlightRect to take on dual meanings - the original (which expects a content-scaled rect) and a new one (the infobar, assumes a highlighter-scaled rect).  Will attach a trivial followup, then land both of these together.
Attachment #561595 - Attachment is obsolete: true
Attachment #561595 - Flags: review?(dcamp)

Comment 22

6 years ago
Created attachment 562925 [details] [diff] [review]
[in-fx-team] Rebased and updated test.

Rebased Kyle's patch, carrying over my r+.
Attachment #562925 - Flags: review+

Comment 23

6 years ago
Created attachment 562926 [details] [diff] [review]
[in-fx-team] Clarify use of content-scaled and highlighter-scaled rects.

This just renames what was this._highlightRect (content-scaled rect) to this._contentRect, and saves the scaled rect as this._highlightRect.
Attachment #562926 - Flags: review?(rcampbell)
Attachment #562926 - Attachment is patch: true
Attachment #562926 - Flags: review?(rcampbell) → review+
Blocks: 689939
filed bug 689939 to account for disappearing infobar during zoom.
ran tests locally had the following error:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_highlighter.js | transparent veil box matches dimensions of element (2x zoom)
make: *** [mochitest-browser-chrome] Error 1
changed the tests to use is() instead of ok() received:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_highlighter.js | transparent veil box height matches width of element (2x zoom) - Got 76.80000305175781, expected 76.80001831054688

JS floats ftw.
Created attachment 563135 [details] [diff] [review]
[in-fx-team] zoomtestfix

additional test fixer-upper.
Attachment #563135 - Flags: review?(dcamp)

Updated

6 years ago
Attachment #563135 - Flags: review?(dcamp) → review+
Comment on attachment 562925 [details] [diff] [review]
[in-fx-team] Rebased and updated test.

https://hg.mozilla.org/integration/fx-team/rev/e38e2c197a9c
Attachment #562925 - Attachment description: Rebased and updated test. → [in-fx-team] Rebased and updated test.
Comment on attachment 562926 [details] [diff] [review]
[in-fx-team] Clarify use of content-scaled and highlighter-scaled rects.

https://hg.mozilla.org/integration/fx-team/rev/84f4b949e006
Attachment #562926 - Attachment description: Clarify use of content-scaled and highlighter-scaled rects. → [in-fx-team] Clarify use of content-scaled and highlighter-scaled rects.
Comment on attachment 563135 [details] [diff] [review]
[in-fx-team] zoomtestfix

https://hg.mozilla.org/integration/fx-team/rev/f99307d15ab4
Attachment #563135 - Attachment description: zoomtestfix → [in-fx-team] zoomtestfix
Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 1d] → [minotaur][best: 1h; likely: 3h; worst: 1d][fixed-in-fx-team]
and checked in:

https://hg.mozilla.org/integration/fx-team/rev/11cf2ae4b0a1

hopefully to clear some oranges.
Duplicate of this bug: 690936
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
(Reporter)

Comment 33

6 years ago
http://hg.mozilla.org/mozilla-central/rev/e38e2c197a9c
http://hg.mozilla.org/mozilla-central/rev/84f4b949e006
http://hg.mozilla.org/mozilla-central/rev/f99307d15ab4
http://hg.mozilla.org/mozilla-central/rev/11cf2ae4b0a1

Comment 34

6 years ago
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.