1297431 - Add a custom user agent input in responsive design mode

Status: RESOLVED FIXED

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

Description

[Tim Nguyen :ntim]

Currently, we silently change the UA on device change, we should be more obvious about it.
Also, the user should be able to change the UA independently from the device.

Comment 1

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

I imagine actually changing to custom non-device UA would be separate work.  So for this bug, we should focus on:

* potentially communicating that UA is changed to device's UA, but needs reload to fully affect the page
* potentially reminding the user somehow that a special UA is currently applied while a device is selected

Comment 2

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

Helen, any thoughts on this topic?

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

I think that in the case that a user changes device and the UA changes it's not important to communicate to the user the change. I think we should only worry about communicating UA changed in two instances:

1. The user changed the UA themselves, and has a custom UA.
2. The user had a custom UA, and then changed device, overriding their custom UA.

I think otherwise surfacing this kind of information to users who either don't care/don't understand what UA is is doing them a disservice.

Comment 4

[Victoria Wang [:victoria]]

https://mozilla.invisionapp.com/share/HQNDP3OZU8W#/

- Idea for a quick MVP to be hidden behind a flag 
- Potential fancier idea to develop later we need it - just filing it here to keep things together

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1297431.patch [1.0]

This adds a custom user agent input to the toolbar that is currently only enabled by default in nightly. Otherwise, this is toggle-able via the settings menu in the toolbar.

This also shows any of the applied UA by the device selector.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c20477bac7d43eb6f754ed7ec7e1886585f8b33

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1297431.patch [2.0]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

:karlcow, this might be of high interest to you. So, I am needinfoing you in case you want to take an early try at it and put any immediate feedback so I can continue to iterate on it. Otherwise, expect it in Nightly hopefully this week.

Comment 9

[Tim Nguyen :ntim]

Thanks for working on this! Works pretty nicely, one thing though, the 900px input on focus doesn’t really work well on smaller windows.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Tim Nguyen :ntim from comment #9)
> Thanks for working on this! Works pretty nicely, one thing though, the 900px
> input on focus doesn’t really work well on smaller windows.

Ya, I was mainly playing with this width to kinda figure out how long does it really need to be to see the entire user agent string and how much to display when there is actually a UA string applied and not focused. I could imagine a text area solution but that would enlarge the toolbar.

Comment 11

[Victoria Wang [:victoria]]

It just occurred to me that it would be cool if the width of the input was flexible to fill up remaining space, maybe with 10% left/right-margin built in so that it doesn't feel too crowded up there.

Comment 12

[Karl Dubost💡 :karlcow]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #8)
> :karlcow, this might be of high interest to you. So, I am needinfoing you in
> case you want to take an early try at it and put any immediate feedback so I
> can continue to iterate on it. Otherwise, expect it in Nightly hopefully
> this week.

Thanks Gabriel for the heads up. That's good news.
I will definitely play with this and comment back here when it lands on Nightly. 


One question:

On the left side, there is the Responsive Menu, where it is possible to select a predefined set of values including the UA.
What is the story for the users when  they have selected something on the left side, and they change the UA on the right side manually.

Comment 13

[Razvan Caliman [:rcaliman]]

Comment on attachment 9007492 [details] [diff] [review]
1297431.patch [2.0]

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

::: devtools/client/responsive.html/components/App.js
@@ +158,5 @@
>    onRemoveCustomDevice(device) {
> +    // If the custom device is currently selected on any of the viewports,
> +    // remove the device association and reset all the ui state.
> +    for (const viewport of this.props.viewports) {
> +      if (viewport.device == device.name) {

Please use triple equals for strict checks. Just in case :)

::: devtools/client/responsive.html/components/Toolbar.js
@@ +74,5 @@
> +      UserAgentInput({
> +        onChangeUserAgent,
> +        userAgent,
> +      }),
> +      dom.div({ className: "devtools-separator" }),

I'd prefer we don't use DOM nodes for decoration. 
Not a blocker now, but maybe a task to note for a future refactoring to replace these separator nodes with either a CSS border or 1px wide background-image.

::: devtools/client/responsive.html/components/UserAgentInput.js
@@ +32,5 @@
> +    this.onChange = this.onChange.bind(this);
> +    this.onKeyUp = this.onKeyUp.bind(this);
> +  }
> +
> +  componentWillReceiveProps(nextProps) {

I believe `componentWillReceiveProps` is on track to be deprecated: 
https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops

This isn't a blocker now, but it will need to be refactored when React gets eventually updated.

Recommendations for refactoring include using fully controlled components:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#preferred-solutions

@@ +49,5 @@
> +      };
> +    });
> +  }
> +
> +  onKeyUp({ target, keyCode }) {

This is neat. I never knew you could destructure like this in the method signature. Perhaps a short code comment will help avoid confusion for others and future us.

@@ +69,5 @@
> +          id: "user-agent-input",
> +          onChange: this.onChange,
> +          onKeyUp: this.onKeyUp,
> +          placeholder: getStr("responsive.customUserAgent"),
> +          ref: this.inputRef,

`inputRef` doesn't seem to be used anywhere in this component.

::: devtools/client/responsive.html/index.css
@@ +110,5 @@
> +  width: 120px;
> +}
> +
> +#user-agent-input:focus {
> +  width: 900px;

This `900px` makes the input be excessively wide for narrower viewports, pushing all controls to the right out of view. 

There is a CSS-only solution to use up all available space if you're willing to drop the `display: grid` on `#toolbar` for which I don't fully understand its role.

Here's an idea:
```
#toolbar {
  display: flex; // instead of grid
}

#toolbar-center-controls {
  flex: 1;
}

// And the following wholly replaces the CSS changes in this patch
// to highlight obsolete code if going with this suggestion.

#user-agent-label {
  display: flex;
  align-items: center;
  margin-inline-start: 3px;
  margin-inline-end: 3px;
  width: 300px;
}

#user-agent-label:focus-within {
  flex: 1;
}

#user-agent-input {
  margin-inline-start: 3px;
  width: 100%;
}

#user-agent-input[value=""] {
  /* width: 120px; */
}

#user-agent-input:focus {
  /* width: 900px; */
  /* transition: width 0.25s ease; */
}

#user-agent-input,
#user-agent-input[value=""]:focus {
  /* width: 300px; */
}
```

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40bdf96e9fd
Add a custom user agent input in responsive design mode. r=rcaliman

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c40bdf96e9fd