1262782 - Allow resizing the area selected by measuring tool via keyboard

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Sebastian Zartner [:sebo]]

The user should be able to resize the area selected by the measuring tool using keyboard shortcuts.

The shortcuts might be Shift + Up/Down/Left/Right where Shift + Up increases the height while Shift + Down decreases it and Shift + Right increases the width and Shift + Left decreases it.

Additionally, resizing might be influenced by modifiers. E.g. additionally pressing Ctrl may change the size by 10 or 100 pixels.

Sebastian

Comment 1

[Patrick Brosset <:pbro>]

Triaging, filter on CLIMBING SHOES.

Comment 2

[Tim Nguyen :ntim]

To fix this bug:
- Add a event listener for keydown events at [0]
- Handle that keydown event at [1] using event.key and event.shiftKey

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/measuring-tool.js#43
[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/measuring-tool.js#468

Comment 3

[Sebastian Zartner [:sebo]]

Thinking about this some more, I assume it may require some focus indication and maybe also some indication that the area can be resized via the keyboard.

Furthermore, it needs to be ensured that the shortcuts don't interfere with the ones defined on the page or chrome shortcuts. So, I'm not sure whether the ones mentioned in comment 0 can actually be used for this.

Sebastian

Comment 4

[matthew.veltri]

I intend to work on this bug, see what I can do. Cheers

Comment 5

[Sebastian Zartner [:sebo]]

(In reply to matthew.veltri from comment #4)
> I intend to work on this bug, see what I can do. Cheers

Sounds good! Please also have a look at my patch for bug 1152321 (which I still couldn't finish up, unfortunately). Some parts of it might be reused for this.
Also, feel free to take over bug 1152321, because I currently don't have the chance to work on it.

Sebastians

Comment 6

[Patrick Brosset <:pbro>]

Matthew, are you still interested in working on this? Do you need any help other than what has been said in previous comments?

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Patrick, are you mentoring on this one?

Comment 8

[Patrick Brosset <:pbro>]

Yes I can mentor this, thank you.

Comment 9

[matthew.veltri]

Hi, sorry! Have been busy. I made a rough patch although it's implementation needs some refinement. Still, it's a start.

Comment 10

[matthew.veltri]

Attachment: 1262782-patch-attempt.patch

Here it is

Comment 11

[matthew.veltri]

I just noticed that the formatting/style is a bit off for some reason. My IDE must not have the tabs set up to the right number of spaces or something. I'll look it over.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8958493 [details] [diff] [review]
1262782-patch-attempt.patch

Thank you for the patch! 
Let me mark it as R? so I don't forget to review it soon (most probably going to be early next week unfortunately)

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8958493 [details] [diff] [review]
1262782-patch-attempt.patch

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

Thanks for the patch.
It's a good start, but it needs a bit more work.
In particular, you forgot to add the event listener, so nothing happens right now. You need to add pageListenerTarget.addEventListener("keydown", this); in the constructor, and pageListenerTarget.removeEventListener("keydown", this); in the destroy method.

I have also made a couple of comments below in the code.

But it would also be good if you could change your editor to indent with 2 spaces like we do for the rest of the Mozilla code base.
For info, we have some docs about our coding style guide here: http://docs.firefox-dev.tools/contributing/javascript.html
And we use ESLint to check all of this: http://docs.firefox-dev.tools/contributing/eslint.html

Thanks! 
Feel free to tag me for a review when you have a new patch ready!

::: devtools/server/actors/highlighters/measuring-tool.js
@@ +559,5 @@
>          break;
> +      case "keydown":
> +	    // Modifier is the rate which the selection size is modified (in pixels).
> +        modifier = 1;
> +        if (event.shiftKey) {

Usually, we like flipping these conditions around, so that it's a short if at the beginning of the block, and the rest doesn't need to be indented:

// Only handle key events when shift is used too.
if (!event.shiftKey) {
  return;
}

This way, you can un-indent the rest of the block.

Also, I think we need to also check for arrow keys here.
What if none of the arrow keys were used, then we shouldn't update anything after, we should bail out. Otherwise we will be moving the measurement area to wherever the cursor is, which we don't want.

Therefore, you can probably add another condition to the if that checks that event.key is one of the 4 accepted values:
["ArrowUp", "ArrowDown", "ArrowLeft", "ArrowRight"].includes(event.key)

@@ +563,5 @@
> +        if (event.shiftKey) {
> +            if (event.ctrlKey) {
> +                modifier = 10;
> +            }
> +            if (event.keyCode == '38') {

Please event.key instead. It's better for maintaining the code later. Here for instance if would be:

event.key === "ArrowUp" 

This way, you can remove the comments as well.

Please do this with the other blocks too.

Comment 14

[matthew.veltri]

>In particular, you forgot to add the event listener, so nothing happens right now. You need to add >pageListenerTarget.addEventListener("keydown", this); in the constructor, and pageListenerTarget.removeEventListener("keydown", >this); in the destroy method.

Bizarre, I added it on my end and everything worked. I guess I messed up when creating the patch? In any case, I will re-add that.

>Otherwise we will be moving the measurement area to wherever the cursor is, which we don't want.

I actually did put a counter-measure in for that, by adding "if (this._isDragging)" surrounding this.setSize(x - coords.x, y - coords.y). Should I keep that in, or restore it and pursue your fix instead?

I'll fix the issues later this week.

Comment 15

[Patrick Brosset <:pbro>]

(In reply to matthew.veltri from comment #14)
> >Otherwise we will be moving the measurement area to wherever the cursor is, which we don't want.
> 
> I actually did put a counter-measure in for that, by adding "if
> (this._isDragging)" surrounding this.setSize(x - coords.x, y - coords.y).
> Should I keep that in, or restore it and pursue your fix instead?
Oh ok, you're right. I had actually removed this if when testing locally, because this was breaking another feature (the coordinates tooltip that follows the cursor around). So I think we should go with my suggestion for this particular issue.
Thanks!

Comment 16

[matthew.veltri]

Attachment: Second patch attempt for bug 1262782

Alright I updated the style, so it should abide by the standards.

I also implemented your suggestions

Comment 17

[matthew.veltri]

Whoops, I didn't pull before I made the patch, hold up I'll fix that

Comment 18

[matthew.veltri]

Attachment: Third attempt at patch

There we go, that looks better. Cheers!

Comment 19

[matthew.veltri]

Attachment: 1262782 Patch Attempt 4

Formatting issue on part I forgot about in previous patch. I looked it over but must have missed it because it was in the middle section... 

Anyway, sorry for the notification spam. Probably shouldn't be working on this overtired lol.

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8962007 [details] [diff] [review]
1262782 Patch Attempt 4

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

Thanks for the updated patch. This looks better.
I found a bug though, that we need to fix: if you draw the selection with the mouse *going up* (so click somewhere, then dragging towards the top of the page, and then releasing the mouse), then pressing shift+arrowUp is doing something weird where the selection becomes 0px high.
The same problem occurs when dragging to the left, and then using the left arrow key.
So we need some more code to account for the selection direction.

Also, it would be nice to move the keydown handling to another function. So in the switch/case, I would call this.handleKeyDown(event) and then move the code there to this new function.

Finally, as explained below, the if is breaking the position label cursor following.

::: devtools/server/actors/highlighters/measuring-tool.js
@@ +534,5 @@
>  
>          x = Math.min(innerWidth + scrollX, Math.max(scrollX, x));
>          y = Math.min(innerHeight + scrollY, Math.max(scrollY, y));
> +
> +        if (this._isDragging) {

I understand the reason for this if: avoiding to move the selection to wherever the mouse last was when starting to use the keyboard.
But, adding it actually breaks another feature: the position label does not follow the mouse around anymore.

I thought about this a little while, and concluded that we needed much more changes to fix those 2 things at once. Because currently we only have 1 set of coordinates, and we use it both for drawing the selection and the position label. Because we want to draw the label at the right place, next to the cursor, we set the coordinates to that position, but of course this, in turn, makes us draw the selection to that position too when using the arrow keys.
So we need to decouple the 2 logics: on one hand setting the coordinates for the selection (either when dragging or when using the keyboard), and on the other hand setting the coordinates for the position label. They can't be handled by just 1 logic anymore.

Comment 21

[Patrick Brosset <:pbro>]

Resetting the assignee field so others can pick this bug up, since it has been close to 1 year. Feel free to claim it again if you did intend to work on it after all.
Most of the work was done already, and not much has changed (if anything) in the measuring tool, so this patch should still apply without much problems.

Comment 22

[asish7295]

(In reply to Patrick Brosset <:pbro> from comment #21)

Resetting the assignee field so others can pick this bug up, since it has been close to 1 year. Feel free to claim it again if you did intend to work on it after all.
Most of the work was done already, and not much has changed (if anything) in the measuring tool, so this patch should still apply without much problems.

Hi, i am an outreachy applicant looking to contribute . I would like to work on it. It would be nice if you could assign it to me

Comment 23

[Patrick Brosset <:pbro>]

Sure, I'll assign it to you now. Thanks for wanting to work on it.
Make sure you ask any questions here if you need any help.
In the meantime, you can probably get started by reading the docs at https://docs.firefox-dev.tools to make sure you know how get the source code, build it, and make changes to it (and test those changes).
Also, the patch attached here on this bug would be a good start for you. You should try to download it locally and apply it to your repository. Please let me know if this doesn't work anymore (I'm fairly sure it will work fine because the file that this patch changes is almost never touched these days).

Comment 24

[Patrick Brosset <:pbro>]

Hi Asish, are you still planning on working on this bug? If you need more time, that is completely fine.
If, however, you do not plan on working on this, please let me know so it can be made available to others.

Comment 25

[Patrick Brosset <:pbro>]

Unassigning this bug.

Comment 26

[Ajitesh13]

Hello, Can I take this bug ?

Comment 27

[Patrick Brosset <:pbro>]

Yes I guess you can. I'm going to remove myself from the mentor field though, and forward your request to Razvan who, I think, would be a better person to put you on the right track here.

Comment 28

[Ajitesh13]

I am going to start working to create a patch by looking over the above discussion

Comment 29

[Razvan Caliman [:rcaliman]]

Hi Ajitesh13,

Sorry for the very late reply. We are currently in the process of updating the measuring tool and other highlighters to support multi-process tabs. This may or may not interfere with your work on this bug, depending on what you end up changing.

You are free to take this bug. I will assign it to you.

Comment 30

[Ajitesh13]

Attachment: Bug 1262782 - Allow resizing the area selected by measuring tool via keyboard r=rcaliman

Comment 31

[Ajitesh13]

@Razvan, I have a rough patch ready. shiftKey+arrowkeydown/arrowkeyup/arrowkeyupleft is working but, couldn't discover why shiftkey+arrowkeyright is not working. Please take a look at the patch and suggest changes

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 33

[teresiawachirakabura1]

Claim this bug. I wish to work on it

Comment 34

[Vinny Diehl]

I'd like to pick up this bug. A problem that I notice is that the measuring tool anchors in the corner from which the box was drawn. This makes it unclear which way the box is going to resize when you begin to hit the arrows, aside from the fact that the label renders at the opposite corner from the origin (which is IMO an insufficient indicator). Ways around this:

• We could change the color of the handler at the origin corner while the Shift key is held, perhaps to blue.
• We could pick a corner, such as the upper-left, to always use as the origin while resizing with the keyboard shortcuts.

I am leaning towards the former. Open to any suggestions.

Comment 35

[Vinny Diehl]

Attachment: WIP: Bug 1262782 - Allow resizing the area selected by measuring tool via keyboard

Comment 36

[Vinny Diehl]

I have a WIP patch which gets the keybinds working, passes lint, and decouples the mouse coordinates from the rectangle coordinates so that the rectangle does not snap to the mouse when resizing with a keybind.

Still looking for insight on the UX concern in my previous comment. Another thing that'd be nice would be keybinds to move the X/Y-position of the rectangle, so that full control of the rectangle is had from the keyboard. These could be on the raw arrow keys, or perhaps Alt+arrows. Thoughts?

Comment 37

[Vinny Diehl]

Attachment: Bug 1262782 - Allow resizing the area selected by measuring tool via keyboard r?#devtools-reviewers

Comment 38

[Vinny Diehl]

Attachment: Bug 1262782 - Highlight origin handler while shift key is held r?#devtools-reviewers

Depends on D183092

Comment 39

[Vinny Diehl]

Attachment: Bug 1262782 - Add keybinds to move the measuring tool via the arrow keys r?#devtools-reviewers

Depends on D183093

Comment 40

[Vinny Diehl]

I've submitted patches for review with tentative solutions to the issues I mentioned last night.

Comment 41

[Vinny Diehl]

Attachment: Bug 1262782 - Add keybinds to move/resize the measuring tool r?nchevobbe,#devtools-reviewers

Comment 42

[Vinny Diehl]

Attachment: Bug 1262782 - Clean up measuring tool implementation r?nchevobbe

Depends on D183148

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3df39b644daa
Add keybinds to move/resize the measuring tool r=nchevobbe,devtools-reviewers

Comment 44

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3df39b644daa

Comment 45

[Sebastian Zartner [:sebo]]

This is worth mentioning in https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/117.

Sebastian

Comment 46

[Brian Thomas Smith [bsmth]]

Docs additions for this can be tracked in the following GitHub issue: https://github.com/mdn/content/issues/28754