Add a custom user agent input in responsive design mode

RESOLVED FIXED in Firefox 64

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: ntim, Assigned: gl)

Tracking

unspecified
Firefox 64
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [multiviewport] [reserve-rdm] [ux])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Blocks: 1254386
Whiteboard: [multiviewport] [triage]
Blocks: 1172309
No longer blocks: 1254386
Depends on: 1254386
Summary: Make it more obvious that an UA is applied in new RDM → [UX] Make it more obvious that an UA is applied in new RDM
Whiteboard: [multiviewport] [triage] → [multiviewport][triage][ux]
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
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [multiviewport][triage][ux] → [multiviewport] [reserve-rdm] [ux]
Helen, any thoughts on this topic?
Flags: needinfo?(hholmes)
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.
Flags: needinfo?(hholmes)
See Also: → 1306198
See Also: → 1341134

Updated

Last year
Product: Firefox → DevTools
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
Assignee

Updated

9 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
Assignee

Updated

9 months ago
Summary: [UX] Make it more obvious that an UA is applied in new RDM → Add a custom user agent input in responsive design mode
Assignee

Comment 5

9 months ago
Posted patch 1297431.patch [1.0] (obsolete) — Splinter Review
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.
Attachment #9007491 - Flags: review?(rcaliman)
Assignee

Comment 7

9 months ago
Attachment #9007491 - Attachment is obsolete: true
Attachment #9007491 - Flags: review?(rcaliman)
Attachment #9007492 - Flags: review?(rcaliman)
Assignee

Comment 8

9 months ago
: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.
Flags: needinfo?(kdubost)
Reporter

Comment 9

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

Comment 10

9 months ago
(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.
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.
(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.
Flags: needinfo?(kdubost)
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; */
}
```
Attachment #9007492 - Flags: review?(rcaliman) → review+

Comment 14

9 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40bdf96e9fd
Add a custom user agent input in responsive design mode. r=rcaliman

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c40bdf96e9fd
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Reporter

Updated

8 months ago
Duplicate of this bug: 1319944
You need to log in before you can comment on or make changes to this bug.