668254 - Highlighter doesn't handle full page zoom

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Dão Gottwald [:dao]]

Zoom in on a page, then start inspecting, or zoom while inspecting. The rectangles don't appear where they should.

Comment 2

[Kyle Simpson]

Attachment: fix and tests

Comment 3

[Rob Campbell [:rc] (:robcee)]

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

yes!

Comment 4

[Rob Campbell [:rc] (:robcee)]

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)]

hook in *there*. ugh.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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)]

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)]

(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)]

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]

Attachment: updated to address feedback from dcamp

Comment 11

[Kyle Simpson]

Attachment: updated to address feedback from dcamp

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 558344 [details] [diff] [review]
updated to address feedback from dcamp

goodgood.

Comment 13

[Kyle Simpson]

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) (email my personal email if necessary)]

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]

(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) (email my personal email if necessary)]

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]

(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) (email my personal email if necessary)]

Yes, with the highlighter applied in both cases.

Comment 19

[Dave Camp (:dcamp)]

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]

Attachment: fixed with more feedback from dcamp

fixed the test per dcamp, to use getBoundingClientRect() instead of offsetWidth/Height

Comment 21

[Dave Camp (:dcamp)]

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)]

Attachment: [in-fx-team] Rebased and updated test.

Rebased Kyle's patch, carrying over my r+.

Comment 23

[Dave Camp (:dcamp)]

Attachment: [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)]

filed bug 689939 to account for disappearing infobar during zoom.

Comment 25

[Rob Campbell [:rc] (:robcee)]

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)]

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)]

Attachment: [in-fx-team] zoomtestfix

additional test fixer-upper.

Comment 28

[Rob Campbell [:rc] (:robcee)]

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)]

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)]

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)]

and checked in:

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

hopefully to clear some oranges.

Comment 33

[Dão Gottwald [:dao]]

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

[Teodosia Pop]

Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.