Sebastian Zartner [:sebo] Reporter
	Description • 8 years ago

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

	Patrick Brosset <:pbro>
	Comment 1 • 8 years ago

Triaging, filter on CLIMBING SHOES.

	Tim Nguyen :ntim
	Comment 2 • 7 years ago

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

	Sebastian Zartner [:sebo] Reporter
	Comment 3 • 7 years ago

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

	matthew.veltri
	Comment 4 • 6 years ago

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

	Sebastian Zartner [:sebo] Reporter
	Comment 5 • 6 years ago

(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

	Patrick Brosset <:pbro>
	Comment 6 • 6 years ago

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

	Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo
	Comment 7 • 6 years ago

Patrick, are you mentoring on this one?

	Patrick Brosset <:pbro>
	Comment 8 • 6 years ago

Yes I can mentor this, thank you.

	matthew.veltri
	Comment 9 • 6 years ago

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

	matthew.veltri
	Comment 10 • 6 years ago

Here it is

	matthew.veltri
	Comment 11 • 6 years ago

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.

	Patrick Brosset <:pbro>
	Comment 12 • 6 years ago

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)

	Patrick Brosset <:pbro>
	Comment 13 • 6 years ago

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.

	matthew.veltri
	Comment 14 • 6 years ago

>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.

	Patrick Brosset <:pbro>
	Comment 15 • 6 years ago

(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!

	matthew.veltri
	Comment 16 • 6 years ago

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

I also implemented your suggestions

	matthew.veltri
	Comment 17 • 6 years ago

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

	matthew.veltri
	Comment 18 • 6 years ago

There we go, that looks better. Cheers!

	matthew.veltri
	Comment 19 • 6 years ago

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.

	Patrick Brosset <:pbro>
	Comment 20 • 6 years ago

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.

	Pulsebot
	Comment 43 • 10 months ago

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