Don't move the eyedropper to the out of browser window by keyboard navigation

RESOLVED FIXED in Firefox 51

Status

P3
enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: magicp.jp, Assigned: ruturaj, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 51
good-first-bug

Firefox Tracking Flags

(firefox51 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160813030202

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Inspector
3. Run the eyedropper
4. Move the eyedropper to the out of browser window by keyboard navigation
Shift+[↑],[→],[↓],[←]


Actual results:

Can move the eyedropper to the out of browser (contents) window by keyboard navigation. It is impossible by mouse navigation.


Expected results:

Moving area same with mouse navigation.
(Reporter)

Updated

2 years ago
Has STR: --- → yes
status-firefox51: --- → affected
Component: Untriaged → Developer Tools: Inspector
(Reporter)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
This should be a fairly simple bug to fix if someone wants to attempt it.
Please make sure you go over the contribution docs first: https://developer.mozilla.org/en-US/docs/Tools/Contributing
The code that requires changing is: /devtools/server/actors/highlighters/eye-dropper.js and in particular the handleKeyDown method.
Mentor: pbrosset
Keywords: good-first-bug
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Severity: normal → enhancement
(Assignee)

Comment 3

2 years ago
Hey Patrick, I'll like to work on this while I get info on bug#1187980
Ok, I've assigned the bug to you, feel free to start looking into the file mentioned in comment 1.
You should see a handleKeyDown method there that you need to tweak.
Let me know if you have questions.
Assignee: nobody → ruturaj
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
While I'm at it, I'm unable to use console.log within handleKeyDown method, any guesses how I could get it ? In the Browser Console, it shows <Unavailable>

And strangely I'm unable to debug using Browser Toolbox.
Flags: needinfo?(pbrosset)
(In reply to Ruturaj Vartak from comment #5)
> While I'm at it, I'm unable to use console.log within handleKeyDown method,
> any guesses how I could get it ? In the Browser Console, it shows
> <Unavailable>
> 
> And strangely I'm unable to debug using Browser Toolbox.
That actually makes sense, let me explain: the file you are logging from and trying to debug runs in the devtools server process. Because of e10s [1] and also because devtools is able to debug things remotely, we have an architecture where the UI (the toolbox itself) and the things that actually inspect/debug/augment the page are in 2 separate processes.
Unfortunately, the Browser Toolbox only lets you debug the firefox main parent process, the one where the UI runs, so it's useful to debug the UI of devtools, but with it you can't debug any of the server devtools code that inspect/debug/augment the page.
The eye-dropper is one of these things, it is injected in the page, and therefore runs into the child content process, not the parent process.
To see the logs and to debug eye-dropper.js, you'll need the "Browser Content Toolbox". If you look into the same devtools menu where the Browser Toolbox is, you'll see it there too. 
You'll see that's a much simpler toolbox too, it only contains a console and a debugger, but hopefully that's all you'll need.

Alternatively, if you go in the firefox File menu and choose "New non-e10s window", then everything will run in the same process, and the normal Browser Toolbox will let you debug eye-dropper.js.

[1] https://wiki.mozilla.org/Electrolysis
Flags: needinfo?(pbrosset)
(Assignee)

Comment 7

2 years ago
Thanks !!! 
That was a really neat explanation for newbies like me ! :)
(Assignee)

Comment 8

2 years ago
Created attachment 8785269 [details] [diff] [review]
fix-1295010-1.patch

This is a first WIP patch, it has some console.log. I'll remove it in iterations of the patch.

This is how I'm trying to fix the issue
- save a global X and Y variable inside EyeDropper object
- update the global X and Y with moveTo method
- use these values and cross check them every keydown event with window's innerWidth and innerHeight
Attachment #8785269 - Flags: review?(pbrosset)
Comment on attachment 8785269 [details] [diff] [review]
fix-1295010-1.patch

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

I think there is a simpler solution that doesn't require introducing the global X and Y and the isKeyboardBrowsable function.

After we're done calculating the offset (as we did before), we can set the new values for the magnifierArea like so:

this.magnifiedArea.x = cap(this.magnifiedArea.x + offsetX, 0, this.win.innerWidth * this.pageZoom);
this.magnifiedArea.y = cap(this.magnifiedArea.y + offsetY, 0, this.win.innerHeight * this.pageZoom);

Where cap is a small helper function that could be something like:
function cap(nb, min, max) {
  return Math.max(min, Math.min(nb, max));
}

Then, draw (this.draw()), and then call moveTo exactly as we did before, no need for any changes there.
I tested this quickly, and I think this works fine, even when the page is zoomed. Let me know.
Attachment #8785269 - Flags: review?(pbrosset)
(Assignee)

Comment 10

2 years ago
Created attachment 8785926 [details] [diff] [review]
fix-1295010-2.patch

Hey Patrick,
Thanks ! 
Yes, your suggestion is way much simpler and seems to be working beautifully.
Attachment #8785269 - Attachment is obsolete: true
Attachment #8785926 - Flags: review?(pbrosset)
Comment on attachment 8785926 [details] [diff] [review]
fix-1295010-2.patch

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

Nice, thank you. Just a couple of comments below.
Also, could you please add a test for this new feature? So that we make sure to not regress it in the future.
In this case, it will be rather simple because we already have a test for this, you can just augment it to add more test cases to it.
Look at devtools\client\inspector\test\browser_inspector_highlighter-eyedropper-events.js , you'll see the MOVE_EVENTS_DATA that defines a series of test cases to be executed. You could add a few at the end that try to move the eyedropper past the edge of the viewport.

::: devtools/server/actors/highlighters/eye-dropper.js
@@ +421,5 @@
>      offsetX *= modifier;
>  
>      if (offsetX !== 0 || offsetY !== 0) {
> +      this.magnifiedArea.x = this.cap(this.magnifiedArea.x + offsetX, 0, this.win.innerWidth * this.pageZoom);
> +      this.magnifiedArea.y = this.cap(this.magnifiedArea.y + offsetY, 0, this.win.innerHeight * this.pageZoom);

Both these lines are longer than 90 chars, which is the limit we have defined in our ESLint rules. So this patch won't be able to land (since we run ESLint automatically). You'll need to wrap each of these lines on 2 lines.

You can have ESLint running locally by following this doc: https://wiki.mozilla.org/DevTools/CodingStandards

@@ +433,4 @@
>      }
>    },
>  
> +  cap(newOffset, min, max) {

Please move this to the bottom of this file, and out of the prototype object. This helper has no interest in accessing to 'this' and doesn't need to be a method on the class.

function cap(value, min, max) {
  return Math.max(min, Math.min(value, max));
}

And then, to call it: cap(...) instead of this.cap(...)
Attachment #8785926 - Flags: review?(pbrosset) → feedback+
(Assignee)

Comment 12

2 years ago
Created attachment 8786855 [details] [diff] [review]
fix-1295010-3.patch

Hey,
I've made changes as per your suggestions.

Test Cases: I've added the following
- Aligned X, Y using mouse-move
- Left 0 snapping
- Top 0 snapping
- Right max innerWidth snapping
- Bottom max innerHeight snapping: I'm unable to create a test case for bottom Y snapping. I don't know what I'm doing wrong.

Could you please help me understand where I'm going wrong in using window.innerHeight ?
Attachment #8785926 - Attachment is obsolete: true
Attachment #8786855 - Flags: review?(pbrosset)
Comment on attachment 8786855 [details] [diff] [review]
fix-1295010-3.patch

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

Changes in eye-dropper.js look great, thanks.
Thanks for adding these new test cases to the test too.

Indeed, the last check fails. But the reason is simple: you're using 'window' in this test to determine the width and height of the *content* window, however 'window' in a mochitest doesn't refer to the content window, but the parent *chrome* window of the browser itself.
So for instance (in my case, depends on the machine this runs on), window.innerHeight in the test is 1034px, while the actual innerHeight of the content window is 699px (because 335px are taken by devtools at the bottom of the browser window).

Luckily, you don't have that problem with innerWidth because the toolbox is at the bottom, not docked to the side, so the chrome and content window have the same width.

The problem with getting access to the content window is that you can't! Firefox is multi process and so the browser UI runs in a different process than the content inside of browser tabs. The only way this can work is via message passing. And with devtools you can also send requests via the devtools protocol.
So we can solve this, but we can't do this inside the MOVE_EVENTS_DATA array, because it exists before the test starts, and we haven't initialized the browser or the devtools protocol at that stage yet. So I think we're going to have to do a few changes to this array. I'll describe them here and let you implement. If you need more help, let me know.

Inside MOVE_EVENTS_DATA, any x or y value should be defined in 2 potential types. Either a number (like today), or a function:

{type: "mouse", x: 2, y: 2, expected: {x: 2, y: 2}} for example is a simple test case that already works today, but it should be possible to do something like this too:

{
  type: "mouse",
  x: 0,
  y: (width, height) => height - 5,
  expected: {
    x: 0
    y: (width, height) => height - 5
  }
}

So, any x or y can be a function, and if it is, it will receive 2 arguments: width and height (being the innerWidth and innerHeight of the content window), and should return a number.
Now, we'll need to change the test logic for this to work:

First of all in respondsToMoveEvents we should get the window width and height once and for all, we'll need them. Something like this should work:
let {width, height} = yield testActor.getBoundingClientRect("html");

The testActor is what we use to communicate from the test to the content process. And we're doing here is asking the size of the html element (maybe we'll need to add a css like html { height: 100%; } for this to work though). Don't hesitate to look into devtools\client\shared\test\test-actor.js to see what other methods you can call. The important part is that all of these methods are async, which is why we use the 'yield' keyword here.

Now, with this done, inside the for loop where we iterate over MOVE_EVENTS_DATA, you could resolve the various x and y values, with something like:

x = typeof x === "function" ? x(width, height) : x;
y = typeof y === "function" ? y(width, height) : y;
expected.x = typeof expected.x === "function" ? expected.x(width, height) : expected.x;
expected.y = typeof expected.y === "function" ? expected.y(width, height) : expected.y;

This way, if the values are already numbers, we use them directly, if they are functions instead, we call them with the width/height info we got before, and use the resulting values instead.

The rest should remain the same.
Attachment #8786855 - Flags: review?(pbrosset)
(Assignee)

Comment 14

2 years ago
phew !! Another great explanation for newbies :) !!! Lemme look into this.
(Assignee)

Comment 15

2 years ago
Created attachment 8787851 [details] [diff] [review]
fix-1295010-4.patch

Thanks for almost writing code for me. These are the changes
*As Suggested*
- Using coords from actors in arrow fn, and runtime eval
- Changed the HTML rendered in test to have html {width,height: 100%}

- returning testActor from getHighlighterHelperFor.mouse
- Giving extra desc member to MOVE_EVENTS_DATA as suggested in [1]

[1] https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Logs_and_comments
Attachment #8786855 - Attachment is obsolete: true
Attachment #8787851 - Flags: review?(pbrosset)
Comment on attachment 8787851 [details] [diff] [review]
fix-1295010-4.patch

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

Looks great.
I've pushed this to TRY so we can check if the test works on all platforms and if the code changes don't break other tests: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c25df15f4f66

I've also made one last comment about not changing the helper return value and just changing the test instead. See below.
Finally, the commit message on the change needs to be modified a little bit. I see this now:
Bug 1295010 - "Don't move the eyedropper to the out of browser window by keyboard navigation" [r=ruturaj]
It should be:
Bug <number> - <what you actually changed, not just the bug title>, r=<reviewer>
So:
Bug 1295010 - Snap the eyedropper to 0/0/width/height when moving with the keyboard; r=pbro

Once you've made those changes, you don't need to ask for review again, you can upload the new patch and mark it as R+ yourself.

::: devtools/client/inspector/test/head.js
@@ +530,5 @@
>          highlightedNode = null;
>          yield highlighter.finalize();
> +      },
> +
> +      testActor: testActor

getHighlighterHelperFor returns a helper that provides a few useful functions but the idea was that it wouldn't give you access to the testActor, but wrap whatever was needed.

In your case, you do need the testActor, so you could do this instead in browser_inspector_highlighter-eyedropper-events.js

let {inspector, testActor} = yield openInspectorForURL(
  "data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
let helper = yield getHighlighterHelperFor(HIGHLIGHTER_TYPE)({inspector, testActor});

This way you have access to the testActor, without having to change what getHighlighterHelperFor returns.
Attachment #8787851 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 17

2 years ago
Created attachment 8788388 [details] [diff] [review]
fix-1295010-5.patch

Hey Patrick,

Making the changes as suggested, also uploading a *hg-patchable-format* patch.
Sorry for asking another review, I hope this is what you wanted.
Attachment #8787851 - Attachment is obsolete: true
Attachment #8788388 - Flags: review?(pbrosset)
Attachment #8788388 - Flags: review?(pbrosset) → review+
Try build is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c25df15f4f66
Let's check this in.

Thanks Ruturaj!
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
Hey Patrick,
Thanks a lot !

Comment 20

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a6b6a93eb41a
Don't move the eyedropper to the out of browser window by keyboard navigation. r=pbro
Keywords: checkin-needed
(Assignee)

Comment 21

2 years ago
Hey Patrick,
Was the last patch attachment#8788388 [details] [diff] [review] getting checked in ?
Flags: needinfo?(pbrosset)
Yes, see comment 20, the last patch got checked into one of our integration repositories here:
https://hg.mozilla.org/integration/fx-team/rev/a6b6a93eb41a
Give it a couple of days and it will land in mozilla-central, which means it will be available in Nightly builds after that. The next "merge" date is next week I believe, which means that by next week, your fix will be in the next DevEdition build (and 6 weeks after this in beta, and another 6 weeks after this in Firefox proper).
Flags: needinfo?(pbrosset)
(Assignee)

Comment 23

2 years ago
Hey, it seems the one getting checked in is with testActor being returned [1]. You wanted another way, which was sent as attachment#8788388 [details] [diff] [review]

[1] https://hg.mozilla.org/integration/fx-team/rev/a6b6a93eb41a#l2.15
Flags: needinfo?(pbrosset)
Oh you're right. I think the patch that got checked in was the one I pushed to TRY, while I thought the last one attached here would be used instead.
I'll push a new patch here, on top of the old one.
Flags: needinfo?(pbrosset)
Keywords: leave-open

Comment 25

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/041a925171e4
Removed testActor from highlighterHelper in inspector tests; r=me
I've just pushed an interdiff to get your changes to look like what I had R+'d.
Keywords: leave-open

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6b6a93eb41a
https://hg.mozilla.org/mozilla-central/rev/041a925171e4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Comment 28

2 years ago
Thanks a lot guys!
(Reporter)

Comment 29

2 years ago
verified on latest Nightly Build ID (20160908030434)
Hi Ruturaj, Thanks for your work!
status-firefox51: fixed → verified

Updated

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