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

RESOLVED FIXED in Firefox 59

Status

RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: magicp.jp, Assigned: shounak.bhandekar)

Tracking

(Blocks: 1 bug, {good-first-bug})

44 Branch
Firefox 59
good-first-bug

Firefox Tracking Flags

(firefox44 affected, firefox45 affected, firefox46 affected, firefox47 affected, firefox59 verified)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Component: Untriaged → Developer Tools
(Reporter)

Comment 1

4 years ago
Not square
(Reporter)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Has STR: --- → yes
status-firefox44: --- → affected
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All

Comment 2

3 years ago
Posted patch bug-1210632.patch (obsolete) — Splinter Review
Here is small patch which should take care of this bug.
Attachment #8681665 - Flags: review?(zer0)

Comment 3

3 years ago
Posted image Measuretool Fixed 1
(Reporter)

Updated

3 years ago
status-firefox45: --- → affected
status-firefox46: --- → affected
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));
Attachment #8681665 - Flags: review?(zer0) → review-
(Reporter)

Updated

3 years ago
Blocks: 1152322
status-firefox47: --- → affected

Updated

2 years ago
Keywords: good-first-bug
Component: Developer Tools → Developer Tools: Measuring Tool
(Assignee)

Comment 5

a year ago

Comment 6

a year ago
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. :-)
Attachment #8939159 - Flags: review?(ntim.bugs)
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.
Attachment #8939159 - Flags: review?(ntim.bugs)
(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*
(Assignee)

Comment 9

a year ago
I made the change according to the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1210632#c4
Attachment #8939525 - Flags: review?(pbrosset)
Attachment #8939525 - Flags: review?(ntim.bugs)
(Assignee)

Updated

a year ago
Attachment #8939525 - Flags: review?(gl)
(Assignee)

Updated

a year ago
Attachment #8939159 - Flags: review?(pbrosset)
Attachment #8939159 - Flags: review?(gl)
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.
Attachment #8939525 - Flags: review?(pbrosset)
Attachment #8939525 - Flags: review?(ntim.bugs)
Attachment #8939525 - Flags: review?(gl)
(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.
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).
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.
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 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+.
Attachment #8939159 - Flags: review?(pbrosset) → review+
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.
Attachment #8681665 - Attachment is obsolete: true

Comment 18

a year ago
mozreview-review
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
Attachment #8939562 - Flags: review+
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).
Attachment #8939159 - Attachment is obsolete: true
Attachment #8939159 - Flags: review?(gl)

Comment 20

a year ago
Patrick, is this ready to land? I *think* everything on try is older intermittents.
Flags: needinfo?(pbrosset)
Thanks Gijs for the reminder. I'll push this.
Flags: needinfo?(pbrosset)

Comment 22

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c9fc232f3ca
Move the pointer to (0,0) with the New Measure Tool r=pbro

Updated

a year ago
Assignee: nobody → shounak.bhandekar

Comment 23

a year ago
Backout by dluca@mozilla.com:
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

a year ago
(In reply to Pulsebot from comment #23)
> Backout by dluca@mozilla.com:
> 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 hidden (mozreview-request)
Sorry for not seeing this. I'll make sure ESLint is happy and will push again soon.

Comment 27

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/193679c98b6c
Move the pointer to (0,0) with the New Measure Tool r=pbro

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/193679c98b6c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
(Reporter)

Comment 29

a year ago
This bug fix was verified in the latest Nightly build (20180111220102), also in RTL locales. Thanks!
status-firefox59: fixed → verified

Updated

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