Last Comment Bug 668254 - Highlighter doesn't handle full page zoom
: Highlighter doesn't handle full page zoom
Status: VERIFIED FIXED
[minotaur][best: 1h; likely: 3h; wors...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P2 major (vote)
: Firefox 10
Assigned To: Kyle Simpson
:
Mentors:
: 672478 690936 (view as bug list)
Depends on:
Blocks: 663830 689939
  Show dependency treegraph
 
Reported: 2011-06-29 10:36 PDT by Dão Gottwald [:dao]
Modified: 2011-10-07 02:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix and tests (3.63 KB, patch)
2011-09-02 09:33 PDT, Kyle Simpson
rcampbell: review+
dcamp: feedback-
Details | Diff | Review
updated to address feedback from dcamp (4.69 KB, patch)
2011-09-05 14:32 PDT, Kyle Simpson
no flags Details | Diff | Review
updated to address feedback from dcamp (4.69 KB, patch)
2011-09-05 14:43 PDT, Kyle Simpson
rcampbell: review+
gavin.sharp: review+
Details | Diff | Review
fixed with more feedback from dcamp (4.78 KB, patch)
2011-09-21 15:25 PDT, Kyle Simpson
no flags Details | Diff | Review
[in-fx-team] Rebased and updated test. (4.10 KB, patch)
2011-09-27 17:46 PDT, Dave Camp (:dcamp)
dcamp: review+
Details | Diff | Review
[in-fx-team] Clarify use of content-scaled and highlighter-scaled rects. (1.95 KB, patch)
2011-09-27 17:48 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Review
[in-fx-team] zoomtestfix (7.00 KB, patch)
2011-09-28 12:00 PDT, Rob Campbell [:rc] (:robcee)
dcamp: review+
Details | Diff | Review

Description Dão Gottwald [:dao] 2011-06-29 10:36:32 PDT
Zoom in on a page, then start inspecting, or zoom while inspecting. The rectangles don't appear where they should.
Comment 1 Dave Camp (:dcamp) 2011-07-19 10:34:32 PDT
*** Bug 672478 has been marked as a duplicate of this bug. ***
Comment 2 Kyle Simpson 2011-09-02 09:33:21 PDT
Created attachment 557861 [details] [diff] [review]
fix and tests
Comment 3 Rob Campbell [:rc] (:robcee) 2011-09-02 11:33:10 PDT
Comment on attachment 557861 [details] [diff] [review]
fix and tests

yes!
Comment 4 Rob Campbell [:rc] (:robcee) 2011-09-02 11:35:34 PDT
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.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-09-02 11:35:59 PDT
hook in *there*. ugh.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-02 11:41:26 PDT
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 Dave Camp (:dcamp) 2011-09-02 11:49:07 PDT
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?
Comment 8 Rob Campbell [:rc] (:robcee) 2011-09-02 11:52:13 PDT
(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!
Comment 9 Dave Camp (:dcamp) 2011-09-02 11:53:53 PDT
Comment on attachment 557861 [details] [diff] [review]
fix and tests

If it should be storing the original, this patch is wrong.
Comment 10 Kyle Simpson 2011-09-05 14:32:11 PDT
Created attachment 558338 [details] [diff] [review]
updated to address feedback from dcamp
Comment 11 Kyle Simpson 2011-09-05 14:43:10 PDT
Created attachment 558344 [details] [diff] [review]
updated to address feedback from dcamp
Comment 12 Rob Campbell [:rc] (:robcee) 2011-09-06 14:53:49 PDT
Comment on attachment 558344 [details] [diff] [review]
updated to address feedback from dcamp

goodgood.
Comment 13 Kyle Simpson 2011-09-08 05:17:01 PDT
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`.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-08 05:20:26 PDT
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?
Comment 15 Kyle Simpson 2011-09-08 05:30:22 PDT
(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)?
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-08 05:44:15 PDT
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.
Comment 17 Kyle Simpson 2011-09-08 06:27:32 PDT
(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?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-08 06:31:16 PDT
Yes, with the highlighter applied in both cases.
Comment 19 Dave Camp (:dcamp) 2011-09-08 20:22:31 PDT
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.
Comment 20 Kyle Simpson 2011-09-21 15:25:25 PDT
Created attachment 561595 [details] [diff] [review]
fixed with more feedback from dcamp

fixed the test per dcamp, to use getBoundingClientRect() instead of offsetWidth/Height
Comment 21 Dave Camp (:dcamp) 2011-09-27 17:43:58 PDT
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.
Comment 22 Dave Camp (:dcamp) 2011-09-27 17:46:15 PDT
Created attachment 562925 [details] [diff] [review]
[in-fx-team] Rebased and updated test.

Rebased Kyle's patch, carrying over my r+.
Comment 23 Dave Camp (:dcamp) 2011-09-27 17:48:29 PDT
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.
Comment 24 Rob Campbell [:rc] (:robcee) 2011-09-28 07:30:44 PDT
filed bug 689939 to account for disappearing infobar during zoom.
Comment 25 Rob Campbell [:rc] (:robcee) 2011-09-28 07:51:04 PDT
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
Comment 26 Rob Campbell [:rc] (:robcee) 2011-09-28 07:56:03 PDT
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.
Comment 27 Rob Campbell [:rc] (:robcee) 2011-09-28 12:00:16 PDT
Created attachment 563135 [details] [diff] [review]
[in-fx-team] zoomtestfix

additional test fixer-upper.
Comment 28 Rob Campbell [:rc] (:robcee) 2011-09-29 06:32:27 PDT
Comment on attachment 562925 [details] [diff] [review]
[in-fx-team] Rebased and updated test.

https://hg.mozilla.org/integration/fx-team/rev/e38e2c197a9c
Comment 29 Rob Campbell [:rc] (:robcee) 2011-09-29 06:33:09 PDT
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
Comment 30 Rob Campbell [:rc] (:robcee) 2011-09-29 06:37:20 PDT
Comment on attachment 563135 [details] [diff] [review]
[in-fx-team] zoomtestfix

https://hg.mozilla.org/integration/fx-team/rev/f99307d15ab4
Comment 31 Rob Campbell [:rc] (:robcee) 2011-09-29 08:34:44 PDT
and checked in:

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

hopefully to clear some oranges.
Comment 32 Panos Astithas [:past] 2011-10-01 02:07:52 PDT
*** Bug 690936 has been marked as a duplicate of this bug. ***
Comment 34 Teodosia Pop 2011-10-07 02:33:25 PDT
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.

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