Eyedropper (Color picker) doesn't work properly if the scale of a page is not equal 100%

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: arni2033, Assigned: jj)

Tracking

({regression})

36 Branch
Firefox 43
regression

Firefox Tracking Flags

(firefox38 affected, firefox38.0.5 affected, firefox39 affected, firefox40 affected, firefox41 affected, firefox43 fixed, firefox-esr31 unaffected, relnote-firefox 43+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8625150 [details]
Eyedropper doesn't work properly if the scale of a page is not equal 100percent.webm

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

1. Change the scale of a page to 170%
2. Open devtools, press color picker button (You may need to go to preferences first, and check command-button-eyedropper option)

Result:
Looks like eyedropper only sees the smaller version of the page (scaled down to 100%), placed on the top left.
(Reporter)

Comment 1

3 years ago
Please also set See also -> bug 1176547
Bugzilla returns an error when I try to modify them
Component: Untriaged → Developer Tools: Graphic Commandline and Toolbar

Comment 2

3 years ago
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=43118280b457&tochange=a6775c581217

Regressed by: a6775c581217	Heather Arthur — Bug 1040653 - Make eyedropper work with e10s. r=mratcliffe
Blocks: 1040653
Status: UNCONFIRMED → NEW
status-firefox38: --- → affected
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → affected
Ever confirmed: true
Keywords: regression
Version: 38 Branch → 36 Branch
(Assignee)

Comment 3

3 years ago
I’m able to fix it on non-retina devices by doing the following:

	diff --git a/browser/devtools/eyedropper/eyedropper-child.js b/browser/devtools/eyedropper/eyedropper-child.js
	--- a/browser/devtools/eyedropper/eyedropper-child.js
	+++ b/browser/devtools/eyedropper/eyedropper-child.js
	@@ -1,20 +1,22 @@
	 /* This Source Code Form is subject to the terms of the Mozilla Public
	 * License, v. 2.0. If a copy of the MPL was not distributed with this
	 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
	 
	 addMessageListener("Eyedropper:RequestContentScreenshot", sendContentScreenshot);
	 
	 function sendContentScreenshot() {
	   let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
	+  let ratio = content.devicePixelRatio;
	   let width = content.innerWidth;
	   let height = content.innerHeight;
	-  canvas.width = width;
	-  canvas.height = height;
	+  canvas.width = width * ratio;
	+  canvas.height = height * ratio;
	   canvas.mozOpaque = true;
	   
	   let ctx = canvas.getContext("2d");
	   
	+  ctx.scale(ratio, ratio);
	   ctx.drawWindow(content, content.scrollX, content.scrollY, width, height, "#fff");
	   
	   sendAsyncMessage("Eyedropper:Screenshot", canvas.toDataURL());
	 }

Dividing `ratio` with 2.0 makes it work on retina-devices. Unfortunately I can’t find a way to make it work on both simultaneously:
The devicePixelRatio scales with both retina/non-retina as well as the current zoom-level. Is there a way to get the devices pixel ratio without the zoom-level?
Flags: needinfo?(jryans)
(In reply to Johan K. Jensen from comment #3)
> Dividing `ratio` with 2.0 makes it work on retina-devices. Unfortunately I
> can’t find a way to make it work on both simultaneously:
> The devicePixelRatio scales with both retina/non-retina as well as the
> current zoom-level. Is there a way to get the devices pixel ratio without
> the zoom-level?

Hmm, I am probably missing something here...  but if I zoom in on this bug page and then check devicePixelRatio, it seems to remain fixed at either 1 or 2 (non-retina vs. retina).  Are you seeing different behavior?
Flags: needinfo?(jryans) → needinfo?(jj)
(Assignee)

Comment 5

3 years ago
Yes, I’m seeing a different behavior. According to MDN, devicePixelRatio changes “[…] as the page is zoomed in the number of device pixels that one CSS pixel covers increases, and therefore the value of devicePixelRatio will also increase.” You should be able to open [1] and see the devicePixelRatio change as you zoom in and out with ⌘+ and ⌘- (or the Windows/Linux shortcut).

[1]: https://johankj.github.io/devicePixelRatio/
Flags: needinfo?(jj) → needinfo?(jryans)
(In reply to Johan K. Jensen from comment #5)
> Yes, I’m seeing a different behavior. According to MDN, devicePixelRatio
> changes “[…] as the page is zoomed in the number of device pixels that one
> CSS pixel covers increases, and therefore the value of devicePixelRatio will
> also increase.” You should be able to open [1] and see the devicePixelRatio
> change as you zoom in and out with ⌘+ and ⌘- (or the Windows/Linux shortcut).
> 
> [1]: https://johankj.github.io/devicePixelRatio/

Ah, I see.  The issue was that in my default profile, I had set "browser.zoom.full" to false, which causes the browser zoom to only affect text size, instead of scaling the entire page.  So, by resetting that back to default, I now see the behavior you describe.  Thanks for the test page!

It looks like the full zoom level is exposed on nsIDOMWindowUtils, so you may be able to use something like:

    let scale = content.devicePixelRatio / content.getInterface(Ci.nsIDOMWindowUtils).fullZoom;
Flags: needinfo?(jryans)
(Assignee)

Comment 8

3 years ago
Created attachment 8649264 [details] [diff] [review]
bug1176595_eyedropper_fix_for_scaled_page.patch

Awesome, jryans, the fullZoom-property was the solution.

This patch seems to solve the issue. (Although if you zoom while the eyedropper is active, it doesn’t get updated until you activate the eyedropper next. But is should be a rare issue.)
The above try-link is for this patch.
Assignee: nobody → jj
Status: NEW → ASSIGNED
Attachment #8649264 - Flags: review?(jryans)
Comment on attachment 8649264 [details] [diff] [review]
bug1176595_eyedropper_fix_for_scaled_page.patch

Review of attachment 8649264 [details] [diff] [review]:
-----------------------------------------------------------------

I thought you would need devicePixelRatio / fullZoom, not just fullZoom alone.  Is it correct as-is?

Also, this can be left to a follow up bug, but there is an event when fullZoom changes[1] that could let us fix up the eyedropper as it changes.

[1]: https://dxr.allizom.org/mozilla-central/rev/90d9b7c391d38ae118865bd87b5d011feee6dded/toolkit/content/browser-child.js?offset=0#440

::: browser/devtools/eyedropper/eyedropper-child.js
@@ +5,5 @@
>  addMessageListener("Eyedropper:RequestContentScreenshot", sendContentScreenshot);
>  
>  function sendContentScreenshot() {
>    let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +  let scale = content.getInterface(Components.interfaces.nsIDOMWindowUtils).fullZoom;

Common practice is to define something like:

    let { interfaces: Ci } = Components;

at the top of the file, since these properties are used so frequently.
Attachment #8649264 - Flags: review?(jryans)
(Reporter)

Updated

3 years ago
Blocks: 1176547
(Assignee)

Comment 11

3 years ago
Created attachment 8649391 [details] [diff] [review]
bug1176595_eyedropper_fix_for_scaled_page.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> I thought you would need devicePixelRatio / fullZoom, not just fullZoom
> alone.  Is it correct as-is?

Yeah, it is correct as-is.
On a retina device devicePixelRatio / fullZoom will always return 2 (and 1 on a non-retina).

> Also, this can be left to a follow up bug, but there is an event when
> fullZoom changes[1] that could let us fix up the eyedropper as it changes.
> 
> [1]:
> https://dxr.allizom.org/mozilla-central/rev/
> 90d9b7c391d38ae118865bd87b5d011feee6dded/toolkit/content/browser-child.
> js?offset=0#440

How would that be messaged?

> Common practice is to define something like:
> 
>     let { interfaces: Ci } = Components;
> 
> at the top of the file, since these properties are used so frequently.

Ahh... Tried to use the `const {Cc, Ci, Cu} = require("chrome");` but that didn’t work.
Attachment #8649264 - Attachment is obsolete: true
Attachment #8649391 - Flags: review?(jryans)
Comment on attachment 8649391 [details] [diff] [review]
bug1176595_eyedropper_fix_for_scaled_page.patch

Review of attachment 8649391 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Johan K. Jensen from comment #11)
> Created attachment 8649391 [details] [diff] [review]
> bug1176595_eyedropper_fix_for_scaled_page.patch
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> > I thought you would need devicePixelRatio / fullZoom, not just fullZoom
> > alone.  Is it correct as-is?
> 
> Yeah, it is correct as-is.
> On a retina device devicePixelRatio / fullZoom will always return 2 (and 1
> on a non-retina).

Great, it appears to work correctly here!

> > Also, this can be left to a follow up bug, but there is an event when
> > fullZoom changes[1] that could let us fix up the eyedropper as it changes.
> > 
> > [1]:
> > https://dxr.allizom.org/mozilla-central/rev/
> > 90d9b7c391d38ae118865bd87b5d011feee6dded/toolkit/content/browser-child.
> > js?offset=0#440
> 
> How would that be messaged?

I am not sure exactly, haven't thought it through yet.

> > Common practice is to define something like:
> > 
> >     let { interfaces: Ci } = Components;
> > 
> > at the top of the file, since these properties are used so frequently.
> 
> Ahh... Tried to use the `const {Cc, Ci, Cu} = require("chrome");` but that
> didn’t work.

Yeah, it's tricky...  that can be done in many DevTools files which are loaded by our CommonJS module loader.  This particular file isn't loaded that way.  To learn more about what's available in these frame scripts, see the MDN page[1].

[1]: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Frame_script_environment
Attachment #8649391 - Flags: review?(jryans) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88994e4a3207
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Reporter)

Updated

3 years ago
No longer blocks: 1176547
Release Note Request (optional, but appreciated)
[Why is this notable]:  User facing feature regression fixed
[Suggested wording]: Eyedropper tool works when page is zoomed
[Links (documentation, blog post, etc)]:

This seems worth a release note. If you have better suggested wording, please comment and needinfo me! Thanks.
status-firefox-esr38: affected → ---
relnote-firefox: --- → 43+
Flags: needinfo?(jj)
(Assignee)

Comment 16

3 years ago
That looks fine by me.
Flags: needinfo?(jj)

Updated

3 years ago
Duplicate of this bug: 1149143

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.