1421663 - Allow changing of custom viewport size in Responsive Design Mode with Arrow Keys

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, enhancement, P3)

Description

[mark]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171123161455

Steps to reproduce:

1) Enter Responsive Design Mode
2) Position cursor in vieport width custom value area
3) Attempt to use up/down arrows to change value (as in Chrome Dev Tools)


Actual results:

1) Nothing


Expected results:

2) Value should have incremented/decremented one pixel per key stroke (up/down as appropriate).

Without this ability, it is impossible to use an adhoc custom width/height from the keyboard; this breaks accessibility.

Comment 1

[mark]

CORRECTION:

Without the ability to increment or decrement the custom value with the arrow keys, it is very difficult to change the window size gradually by width or height only; dragging without changing the size of both width and height requires very fine motor skills!

I appreciate that setting a custom value can be done via the keyboard easily; changing the value incrementally, however, is the issue here.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for filing!  That seems like a nice enhancement idea.

I think this would be a good fit as a good first bug.

The ViewportDimension[1] component needs to check for up / down key events and react accordingly.  We may also want to carry over the modifier behavior that the inspector uses for its own inputs: increase by 10 when shift is pressed.

If you need help getting started, check out http://docs.firefox-dev.tools/getting-started/.

If you have any questions, please ask them here and set the needinfo? field to mentor.

[1]: https://searchfox.org/mozilla-central/source/devtools/client/responsive.html/components/ViewportDimension.js

Comment 3

[mark]

Thanks. I may have misunderstood something; am I 'expected' ('invited'?) to implement my suggestion, or is this now 'open' to the whole dev community? I've not background in developing Tools like this, nor have I done C+ for over a decade, and I suspect my JS skills may be lacking...but I may have a go anyway!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Anyone is welcome give it a try!  That portion wasn't meant to be directed to you specifically, apologies for the confusion.

Comment 5

[Abhinav Koppula]

Attachment: Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

Review commit: https://reviewboard.mozilla.org/r/205524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205524/

Comment 6

[Abhinav Koppula]

Hi Ryan,
I have given this a try and have also added small test for this behaviour.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review211572

Thanks for taking a look at this! :) I think it's quite close, just a few tweaks left.  Always great to see a test as well!

::: devtools/client/responsive.html/components/ViewportDimension.js:10
(Diff revision 1)
>  "use strict";
>  
>  const { Component } = require("devtools/client/shared/vendor/react");
>  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const {KeyCodes} = require("devtools/client/shared/keycodes");

Use `{ KeyCodes }` spacing style to match other lines.

::: devtools/client/responsive.html/components/ViewportDimension.js:41
(Diff revision 1)
>      this.onInputBlur = this.onInputBlur.bind(this);
>      this.onInputChange = this.onInputChange.bind(this);
>      this.onInputFocus = this.onInputFocus.bind(this);
>      this.onInputKeyUp = this.onInputKeyUp.bind(this);
>      this.onInputSubmit = this.onInputSubmit.bind(this);
> +    this.onInputKeyDown = this.onInputKeyDown.bind(this);

Move this above `onInputKeyUp` to keep it sorted.

::: devtools/client/responsive.html/components/ViewportDimension.js:112
(Diff revision 1)
> +   *        the key to check (can be a keyCode).
> +   * @param {...String} keys
> +   *        list of possible keys allowed.
> +   * @return {Boolean} true if the key matches one of the keys.
> +   */
> +  isKeyIn(key, ...keys) {

This is more of a utility function than something specific to this component.  Let's create a file like `utils/key.js` for this.

::: devtools/client/responsive.html/components/ViewportDimension.js:130
(Diff revision 1)
>      if (keyCode == 27) {
>        target.blur();
>      }
>    }
>  
> +  /**

This comment just restates the method name effectively, so I think we can omit the comment, unless there's more to say.

::: devtools/client/responsive.html/components/ViewportDimension.js:133
(Diff revision 1)
>    }
>  
> +  /**
> +   * Handle the input field's keydown event.
> +   */
> +  onInputKeyDown(event) {

We try to keep these methods sorted by name, so move this above `onInputKeyUp`.

::: devtools/client/responsive.html/components/ViewportDimension.js:136
(Diff revision 1)
> +   * Handle the input field's keydown event.
> +   */
> +  onInputKeyDown(event) {
> +    let { target } = event;
> +    let increment = this.getIncrement(event);
> +    if (increment != 0) {

Let's flip this to an early exit approach if there's nothing to do:

```
if (!increment) {
  return;
}
```

That generally leads to less nesting for actions after the check.

::: devtools/client/responsive.html/components/ViewportDimension.js:138
(Diff revision 1)
> +  onInputKeyDown(event) {
> +    let { target } = event;
> +    let increment = this.getIncrement(event);
> +    if (increment != 0) {
> +      target.value = parseInt(target.value, 10) + increment;
> +      this.onInputChange(event);

I think when you are adjust the size with arrows like this, you expect it to commit with each key press, so you can see the effect immediately.

So I think we need to add a call to `onInputSubmit()` as well.

::: devtools/client/responsive.html/components/ViewportDimension.js:145
(Diff revision 1)
> +  }
> +
> +  /**
> +   * Get the increment/decrement step to use for the provided key event.
> +   */
> +  getIncrement(event) {

Since this doesn't depend on any state of the component (it's a pure function), let's move this out of the component and place it above the component class as a regular function.

::: devtools/client/responsive.html/components/ViewportDimension.js:222
(Diff revision 1)
>            value: this.state.width,
>            onBlur: this.onInputBlur,
>            onChange: this.onInputChange,
>            onFocus: this.onInputFocus,
>            onKeyUp: this.onInputKeyUp,
> +          onKeyDown: this.onInputKeyDown,

Move above `onKeyUp`.

::: devtools/client/responsive.html/components/ViewportDimension.js:236
(Diff revision 1)
>            value: this.state.height,
>            onBlur: this.onInputBlur,
>            onChange: this.onInputChange,
>            onFocus: this.onInputFocus,
>            onKeyUp: this.onInputKeyUp,
> +          onKeyDown: this.onInputKeyDown,

Move above `onKeyUp`.

Comment 8

[Abhinav Koppula]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review211572

> I think when you are adjust the size with arrows like this, you expect it to commit with each key press, so you can see the effect immediately.
> 
> So I think we need to add a call to `onInputSubmit()` as well.

I have changed the signature of `onInputChange` to `onInputChange({ target }, callback)`, the reason being that setState is asynchronous and doesn't guarantee that the new state would be used in `onInputSubmit`.
In fact when I had my code as :
this.onInputChange(event);
this.onInputSubmit();
it didn't work at all. On pressing the up arrow key, the input stayed at 320 only, on debugging I found that after doing a setState when we are again using that value in `onInputSubmit` -> `onChangeSize`, it got the previous value of 320.

So now after passing a callback to setState in onInputChange, this functionality works well.
Let me know if this looks fine.

Comment 9

[Abhinav Koppula]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205524/diff/1-2/

Comment 10

[Abhinav Koppula]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review211572

Hi Ryan,
Thanks for the review. I have addressed the review comments.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review211980

Great, I think it feels very nice to use with this feature! :)

I think we need to tweak the test a bit now, so that it waits for the key events to commit the new size.  Let me know if you have questions!

::: devtools/client/responsive.html/test/browser/browser_device_width.js:71
(Diff revision 2)
> +
> +function* checkScreenWidthAndHeight(ui, dimensions) {
> +  let width = dimensions[0].value;
> +  let height = dimensions[1].value;
> +  let resized = waitForViewportResizeTo(ui, width, height);
> +  ui.setViewportSize({ width, height });

Hmm, doesn't this defeat the purpose of testing the key presses?  This will explicitly set the size to the dimensions provided, but the key events are supposed to commit the size themselves.

I think we should change the test flow to something like:

```
let resized = waitForViewportResizeTo(ui, width, height);
<press some keys>
yield resized;
```

Does that make sense?

::: devtools/client/responsive.html/utils/key.js:6
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +const { KeyCodes } = require("devtools/client/shared/keycodes");

Nit: Add a blank line after "use strict"

Comment 12

[Abhinav Koppula]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205524/diff/2-3/

Comment 13

[Abhinav Koppula]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review211980

> Hmm, doesn't this defeat the purpose of testing the key presses?  This will explicitly set the size to the dimensions provided, but the key events are supposed to commit the size themselves.
> 
> I think we should change the test flow to something like:
> 
> ```
> let resized = waitForViewportResizeTo(ui, width, height);
> <press some keys>
> yield resized;
> ```
> 
> Does that make sense?

Yes, makes sense. I have fixed the test.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8934649 [details]
Bug 1421663 - Allow changing of custom viewport size in RDM with arrow keys.

https://reviewboard.mozilla.org/r/205524/#review212528

Thanks, this looks ready to go! :) I assume you will run it on try, but please let me know if you need assistance with that.

Comment 15

[Abhinav Koppula]

TRY push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69c2365e131117105b6841e9a73188d851eeb477&selectedJob=150874161

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e602239c62f
Allow changing of custom viewport size in RDM with arrow keys. r=jryans

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6e602239c62f

Comment 18

[Chris Mills [:cmills]]

I have documented this, by:

* Adding some description to the RDM page; see the https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Setting_screen_size section
* Adding a note to the Fx59 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/59#Changes_for_web_developers

Let me know if that's OK, or if you would like to see further changes made. Thanks!

Comment 19

[Abhinav Koppula]

Looks fine.

Comment 20

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 59.0a1 (2017-11-29) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID   : 20180128191456
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Comment 21

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks Mohammad Maruf Rahman for testing this. I've also tested with Firefox 59.0b6 and Firefox 60.0a1(2018-02-02), under Ubuntu 16.04x64 and MacOS 10.10.5 and things are looking good.
Taking in consideration Comment 20, I'm marking this issue Verified Fixed.