1210632 - [DevTools] New Measure tool could not be moved pointer to (0, 0) position

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[magicp]

Attachment: devtools-measure-left-top-position.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151001030236

Steps to reproduce:

1. Run Nightly 44.0a1
2. Open any pages (e.g. about:home)
3. Open Developer Tools
4. Available "Measure a portion of the page" in Toolbox Options
5. Enable Measure button
6. Move pointer[+] to left top (0, 0) position


Actual results:

Could not be moved pointer to (0, 0) position. could be moved until (0, 1) position.


Expected results:

Could be moved pointer to (0, 0) position.

Comment 1

[magicp]

Attachment: not-square(100x100).png

Not square

Comment 2

[Cato Auestad]

Attachment: bug-1210632.patch

Here is small patch which should take care of this bug.

Comment 3

[Cato Auestad]

Attachment: Measuretool Fixed 1

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8681665 [details] [diff] [review]
bug-1210632.patch

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

The patch fixes one issue, but we should uniform the calculation – and remove the redundancy of `0`.

::: devtools/server/actors/highlighters/measuring-tool.js
@@ +533,5 @@
>  
>          let { coords } = this;
>  
>          x = Math.min(innerWidth + scrollX - 1, Math.max(0 + scrollX, x));
> +        y = Math.min(innerHeight + scrollY, Math.max(0 + scrollY, y));

There was a reason behind that, but honestly I can't remember anymore: the original code seems to indicate that for `x` axis we decrement by 1 the max value, where for the `y` axis we increment by 1 the minimum value.

If we uniform that value we should probably do the same with the rest; so I would suggest something like:

    x = Math.min(innerWidth + scrollX, Math.max(scrollX, x));
    y = Math.min(innerHeight + scrollY, Math.max(scrollY, y));

Comment 5

[shounak]

Attachment: Move the pointer to (0,0) with the New Measure Tool

Comment 6

[:Gijs (he/him)]

Comment on attachment 8939159 [details] [diff] [review]
Move the pointer to (0,0) with the New Measure Tool

Shounak emailed me about this, so asking for review on their behalf to help move this patch along. :-)

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8939159 [details] [diff] [review]
Move the pointer to (0,0) with the New Measure Tool

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

Thanks for the patch! I'm really not the best reviewer here I haven't worked on the measuring tool. You should ask :pbro or :gl to review this patch.

However, testing this patch, I still can't move the pointer to (0,0), I can only move it to (1,0). So assume this patch still needs a bit tweaking.

Comment 8

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #7)
> only move it to (1,0). So assume this patch still needs a bit tweaking.

So I assume*

Comment 9

[shounak]

Attachment: I am able to move the pointer to (0,0)

I made the change according to the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1210632#c4

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8939525 [details]
I am able to move the pointer to (0,0)

Resetting the review? flags on the image attachment. The image looks like the problem is fixed. I'll take a look at the patch now.

Comment 11

[Tim Nguyen :ntim]

(In reply to shounak from comment #9)
> Created attachment 8939525 [details]
> I am able to move the pointer to (0,0)

I've tested the patch on macOS, so it might have something to do with it.

On macOS, with or without the patch, I can't move to (0,0).


Not sure about Windows however.

Comment 12

[Patrick Brosset <:pbro>]

I think that there might be an OS-dependent issue here.

I tested on macOS, and (without the patch), I can only set the start point at x=3, y=1 minimum. And even there, at x=3, my cursor has already turned into a horizontal resizer cursor, because I'm close to the edge of the browser, and there's an area there for me to click/hold in order to drag and resize the browser window.

Maybe this 3px area is wider or narrower on other OSes, but in any case, I don't know that we can really fix it.
In fact, try this test page:
data:text/html,<script>addEventListener("mousemove", e => console.log(`x=${e.clientX}, y=${e.clientY}`))</script>
and move your mouse very slowly toward the top left corner of the page, while looking at the console.
On macOS, I can only get x=3, y=0 maximum. I can never get a mousemove event all the way to x=0 or 1 or 2, because, by then, the mouse is in the resizing area of the OS window, and that seems to take precedence.

Now, with the patch, I'm able to go to x=3, y=0, which is already an improvement over what we have now, because at least we can start measuring from y=0.

Maybe attachment 8939525 [details] was captured on Windows or Linux (which I'm unable to test on now).

Comment 13

[Patrick Brosset <:pbro>]

I asked other people to test on Linux and Windows, and they managed to get to 0,0 on both these OSes with the test URL in my last comment.
So, I think we should disregard the mac-only x=3 problem here.

Comment 14

[Patrick Brosset <:pbro>]

Ok, sorry for the new comment, on macOS you can enter the full-screen mode (click on the green icon in the top left corner of the browser window), and there you can reach x=0,y=0 on the test page.
I tested the patch in full-screen too and was able to go to x=0,y=0 with the measuring tool too. So that's great.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8939159 [details] [diff] [review]
Move the pointer to (0,0) with the New Measure Tool

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

Not very familiar with the code, but this change looks good to me, and is in line with what Matteo suggested earlier, so I'm giving R+.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8681665 [details] [diff] [review]
bug-1210632.patch

Marking the old patch as obsolete now.
So, the next steps are: let's push this patch to TRY (Mozilla's CI environment) to make sure no tests fail as a result of the change. And when that's done, we can push the patch to a Mozilla integration branch.
Both are way easier to do nowadays thanks to the mozreview process. So I'll push your patch there first and trigger the TRY build from there next.

Comment 17

[Patrick Brosset <:pbro>]

Attachment: Bug 1210632 - Move the pointer to (0,0) with the New Measure Tool

Review commit: https://reviewboard.mozilla.org/r/209874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209874/

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8939562 [details]
Bug 1210632 - Move the pointer to (0,0) with the New Measure Tool

https://reviewboard.mozilla.org/r/209874/#review215428

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8939159 [details] [diff] [review]
Move the pointer to (0,0) with the New Measure Tool

The patch is now on mozreview, and the TRY push URL is the following one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2354b88b8c9bf646b703cf8ce4991567a52d076f
Let's wait until all tests have run and check that they are all "green".
If there are failures, let's look at them and see whether they are due to your changes or not.
(note, this can take a while).

Comment 20

[:Gijs (he/him)]

Patrick, is this ready to land? I *think* everything on try is older intermittents.

Comment 21

[Patrick Brosset <:pbro>]

Thanks Gijs for the reminder. I'll push this.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c9fc232f3ca
Move the pointer to (0,0) with the New Measure Tool r=pbro

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8467a81a2b5b
Backed out changeset 2c9fc232f3ca for eslint failure on /devtools/server/actors/highlighters/measuring-tool.js:522:66  r=backout on a CLOSED TREE

Comment 24

[:Gijs (he/him)]

(In reply to Pulsebot from comment #23)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/8467a81a2b5b
> Backed out changeset 2c9fc232f3ca for eslint failure on
> /devtools/server/actors/highlighters/measuring-tool.js:522:66  r=backout on
> a CLOSED TREE

D'oh. Sorry for misleading you - I missed the eslint failure. :-(

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8939562 [details]
Bug 1210632 - Move the pointer to (0,0) with the New Measure Tool

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209874/diff/1-2/

Comment 26

[Patrick Brosset <:pbro>]

Sorry for not seeing this. I'll make sure ESLint is happy and will push again soon.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/193679c98b6c
Move the pointer to (0,0) with the New Measure Tool r=pbro

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/193679c98b6c

Comment 29

[magicp]

This bug fix was verified in the latest Nightly build (20180111220102), also in RTL locales. Thanks!