Closed Bug 1262782 Opened 8 years ago Closed 10 months ago

Allow resizing the area selected by measuring tool via keyboard

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox117 fixed)

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: sebo, Assigned: vinny.diehl)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [btpp-backlog])

Attachments

(1 file, 10 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
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
Triaging, filter on CLIMBING SHOES.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Keywords: good-first-bug
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
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
See Also: → 1382452
Component: Developer Tools: Inspector → Developer Tools: Measuring Tool
I intend to work on this bug, see what I can do. Cheers
Flags: needinfo?(pbrosset)
(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
Matthew, are you still interested in working on this? Do you need any help other than what has been said in previous comments?
Flags: needinfo?(pbrosset) → needinfo?(matthew.veltri)
Patrick, are you mentoring on this one?
Mentor: pbrosset
Flags: needinfo?(pbrosset)
Yes I can mentor this, thank you.
Flags: needinfo?(pbrosset)
Hi, sorry! Have been busy. I made a rough patch although it's implementation needs some refinement. Still, it's a start.
Flags: needinfo?(matthew.veltri)
Attached patch 1262782-patch-attempt.patch (obsolete) — Splinter Review
Here it is
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 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)
Attachment #8958493 - Flags: review?(pbrosset)
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.
Attachment #8958493 - Flags: review?(pbrosset)
>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.
(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!
Alright I updated the style, so it should abide by the standards.

I also implemented your suggestions
Attachment #8958493 - Attachment is obsolete: true
Whoops, I didn't pull before I made the patch, hold up I'll fix that
Attached patch Third attempt at patch (obsolete) — Splinter Review
There we go, that looks better. Cheers!
Attachment #8961981 - Attachment is obsolete: true
Attachment #8961990 - Flags: review?(pbrosset)
Attached patch 1262782 Patch Attempt 4 (obsolete) — Splinter Review
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.
Attachment #8961990 - Attachment is obsolete: true
Attachment #8961990 - Flags: review?(pbrosset)
Attachment #8962007 - Flags: review?(pbrosset)
Assignee: nobody → matthew.veltri
Status: NEW → ASSIGNED
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.
Attachment #8962007 - Flags: review?(pbrosset)
Product: Firefox → DevTools

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.

Assignee: matthew.veltri → nobody
Status: ASSIGNED → NEW

(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

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

Assignee: nobody → asishkrramuk
Status: NEW → ASSIGNED

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.

Flags: needinfo?(asishkrramuk)

Unassigning this bug.

Assignee: asishkrramuk → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(asishkrramuk)
Priority: P3 → P5

Hello, Can I take this bug ?

Flags: needinfo?(patrickbrosset+bugzilla)

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.

Mentor: patrickbrosset+bugzilla
Flags: needinfo?(patrickbrosset+bugzilla) → needinfo?(rcaliman)

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

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.

Assignee: nobody → aji.yash13
Status: NEW → ASSIGNED
Flags: needinfo?(rcaliman)
Priority: P5 → P3

@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

Component: Inspector: Highlighters → Inspector

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

Assignee: aji.yash13 → nobody
Status: ASSIGNED → NEW

Claim this bug. I wish to work on it

Severity: normal → S3

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.

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?

Assignee: nobody → vinny.diehl
Status: NEW → ASSIGNED

Depends on D183092

Attachment #9342931 - Attachment is obsolete: true

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

Attachment #8962007 - Attachment is obsolete: true
Attachment #9167262 - Attachment is obsolete: true
Attachment #9342949 - Attachment is obsolete: true
Attachment #9342950 - Attachment is obsolete: true
Attachment #9342951 - Attachment is obsolete: true
Attachment #9343029 - Attachment is obsolete: true
Attachment #9343029 - Attachment is obsolete: false

Depends on D183148

Attachment #9343135 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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

Duplicate of this bug: 1262785
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: