Network Throttling UI

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Responsive Design Mode
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
Firefox 52
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Once we have the back end to throttle data (bug 1244227), we'll need to implement some UI to toggle it and adjust the settings.

The user story (bug 1110207) gives a high level summary of what's desired and bug 1283445 will work out more details about the UX.
Priority: -- → P3
Whiteboard: [multiviewport][triage] → [multiviewport] [reserve-rdm]
Priority: P3 → P2
Created attachment 8772451 [details]
network-throttling-spec.png
Flags: qe-verify+
QA Contact: mihai.boldan
Priority: P2 → P3
Keywords: dev-doc-needed
Priority: P3 → P2
Priority: P2 → P1
(Assignee)

Updated

a year ago
Blocks: 1305769
(Assignee)

Updated

a year ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8799057 - Flags: ui-review?(hholmes)

Comment 4

a year ago
mozreview-review
Comment on attachment 8799057 [details]
Bug 1283453 - Add network throttling UI to RDM.

https://reviewboard.mozilla.org/r/84350/#review83008

::: devtools/client/responsive.html/data/network-throttling-profiles.js:18
(Diff revision 1)
> +/**
> + * Predefined network throttling profiles.
> + * Speeds are in bytes per second.  Latency is in ms.
> + */
> +/* eslint-disable key-spacing */
> +module.exports = [

I would put this in devtools/client/shared/ for consistency with devices.js. 

It would be even better if we had devtools/client/shared/emulation-profiles.js that takes care of managing devices and network throttling.

::: devtools/client/responsive.html/index.css:84
(Diff revision 1)
>  
> +select {
> +  -moz-appearance: none;
> +  background-color: var(--theme-toolbar-background);
> +  background-image: var(--viewport-selection-arrow);
> +  background-position: 100% 52%;

nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size).

::: devtools/client/responsive.html/index.css:89
(Diff revision 1)
> +  background-position: 100% 52%;
> +  background-repeat: no-repeat;
> +  background-size: 7px;
> +  border: none;
> +  color: var(--viewport-color);
> +  padding: 0 8px 0 8px;

nit: padding: 0 8px;

::: devtools/client/responsive.html/index.css:93
(Diff revision 1)
> +  color: var(--viewport-color);
> +  padding: 0 8px 0 8px;
> +  text-align: center;
> +  text-overflow: ellipsis;
> +  font-size: 11px;
> +  width: -moz-fit-content;

nit: width: -moz-fit-content is the default value for selects, it was probably set before to override width: 150px; that was accidently set.

::: devtools/client/responsive.html/index.css:117
(Diff revision 1)
> +select > option:hover,
> +select:hover > option:hover {

nit: I know it was there before, but 
select:hover > option:hover is redundant with select > option:hover

::: devtools/client/responsive.html/index.css:122
(Diff revision 1)
> +select > option:hover,
> +select:hover > option:hover {
> +  color: var(--viewport-active-color);
> +}
> +
> +select > option.divider {

nit: I would add pointer-events: none; to make sure this is un-clickable.

Comment 5

a year ago
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #4)
> Comment on attachment 8799057 [details]
> Bug 1283453 - Add network throttling UI to RDM.
> 
> https://reviewboard.mozilla.org/r/84350/#review83008
> 
> ::: devtools/client/responsive.html/data/network-throttling-profiles.js:18
> (Diff revision 1)
> > +/**
> > + * Predefined network throttling profiles.
> > + * Speeds are in bytes per second.  Latency is in ms.
> > + */
> > +/* eslint-disable key-spacing */
> > +module.exports = [
> 
> I would put this in devtools/client/shared/ for consistency with devices.js. 
> 
> It would be even better if we had
> devtools/client/shared/emulation-profiles.js that takes care of managing
> devices and network throttling.
meant network throttling profiles.

Comment 6

a year ago
mozreview-review
Comment on attachment 8799056 [details]
Bug 1283453 - Add network throttling to emulation actor.

https://reviewboard.mozilla.org/r/84348/#review83150

Thank you.  This looks good to me.

::: devtools/server/actors/emulation.js:22
(Diff revision 1)
> + * needed because it's possible to have multiple copies of this actor for a single page.
> + * When some instance of this actor changes a property, we want it to be able to restore
> + * that property to the way it was found before the change.
> + *
> + * A subtle aspect of the code below is that all get* methods must return non-undefined
> + * values, so that the absence of a previous value can be distinguised from the value for

Typo, "distinguished"
Attachment #8799056 - Flags: review?(ttromey) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8799057 [details]
Bug 1283453 - Add network throttling UI to RDM.

https://reviewboard.mozilla.org/r/84350/#review83088

::: devtools/client/responsive.html/components/network-throttling-selector.js:15
(Diff revision 1)
> +const Types = require("../types");
> +const { getStr } = require("../utils/l10n");
> +const throttlingProfiles = require("../data/network-throttling-profiles");
> +
> +module.exports = createClass({
> +  displayName: "NetworkThrottlingSelector",

We seem to either start with a empty new line or none in this case among our components. We should probably stick with one style for everything. No particular preference.

::: devtools/client/responsive.html/components/network-throttling-selector.js:29
(Diff revision 1)
> +  onSelectChange({ target }) {
> +    let {
> +      onChangeNetworkThrottling,
> +    } = this.props;
> +
> +    for (let profile of throttlingProfiles) {

We should probably have a check to see if "No throttling" is the value and do onChangeNetworkThrottling(false, ""); and return early before looping through the throttling profiles.

::: devtools/client/responsive.html/index.css:106
(Diff revision 1)
> +select:hover {
> +  background-image: var(--viewport-selection-arrow-hovered);
> +  color: var(--viewport-hover-color);
> +}
> +
> +select:focus {

We should pair select.selected, select:focus since the properties are both the same.

::: devtools/client/responsive.html/manager.js:377
(Diff revision 1)
>      this.tab = null;
>      this.inited = null;
>      this.toolWindow = null;
>      this.swap = null;
>  
>      // Close the debugger client used to speak with emulation actor

Should add a period to the end of the comment.

::: devtools/client/responsive.html/manager.js:431
(Diff revision 1)
>      if (event.origin !== "chrome://devtools") {
>        return;
>      }
>  
>      switch (event.data.type) {
> -      case "change-viewport-device":
> +      case "change-network-throtting": {

Haven't seen this style of using switch cases with embedding the logic of the case statements in { } break;. Why did we decide to switch the styles?

::: devtools/client/responsive.html/manager.js:469
(Diff revision 1)
> +      this.emulationFront.clearDPPXOverride();
>      }
>    },
>  
> -  updateTouchSimulation: Task.async(function* (enabled) {
> +  updateNetworkThrottling: Task.async(function* (enabled, profile) {
>      if (enabled) {

I would've preferred if we did:

if (!enabled) {
  yield this.emulationFront.clearNetworkThrottling();
}

and keep the reminder of the updateNetworkThrottling logic un-indented to make it look a bit cleaner.

This could be done in updateDPPX, updateTouchSimulation and updateUserAgent

::: devtools/client/responsive.html/types.js:117
(Diff revision 1)
> +/**
> + * Network throttling.
> + */
> +exports.networkThrottling = {
> +
> +  // Whether or not touch simulation is enabled

Whether or not networking throlling is enabled.
Attachment #8799057 - Flags: review?(gl) → review+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8799057 [details]
Bug 1283453 - Add network throttling UI to RDM.

https://reviewboard.mozilla.org/r/84350/#review83008

> I would put this in devtools/client/shared/ for consistency with devices.js. 
> 
> It would be even better if we had devtools/client/shared/emulation-profiles.js that takes care of managing devices and network throttling.

Ah, good idea.  I can imagine we'd want to use this from other tools as well, so makes sense!

> nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size).

After some quick tests, it seems like background-position already considers the size, so 50% might actually be what is desired?  In any case, calc(50% + 3.5px) did not appear to work for me.

> nit: padding: 0 8px;

Thanks, fixed.

> nit: width: -moz-fit-content is the default value for selects, it was probably set before to override width: 150px; that was accidently set.

Removed, thanks!

> nit: I know it was there before, but 
> select:hover > option:hover is redundant with select > option:hover

Thanks, fixed!

> nit: I would add pointer-events: none; to make sure this is un-clickable.

Docs of the disabled attribute say "If it is disabled, it does not accept clicks.", so I think it's okay as is.

Comment 9

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8799057 [details]
> Bug 1283453 - Add network throttling UI to RDM.
> 
> https://reviewboard.mozilla.org/r/84350/#review83008
> 
> > nit: background-position: 100% calc(50% + 3.5px) would be better (3.5px is half of the background-size).
> After some quick tests, it seems like background-position already considers
> the size, so 50% might actually be what is desired?  In any case, calc(50% +
> 3.5px) did not appear to work for me.
I'm fine with 50%, as long as we don't have any arbitrary values like 52%.

> > nit: I would add pointer-events: none; to make sure this is un-clickable.
> 
> Docs of the disabled attribute say "If it is disabled, it does not accept
> clicks.", so I think it's okay as is.
Oh, I didn't notice the disabled attribute, sorry! Should be fine without pointer-events then.
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8799057 [details]
Bug 1283453 - Add network throttling UI to RDM.

https://reviewboard.mozilla.org/r/84350/#review83088

> We seem to either start with a empty new line or none in this case among our components. We should probably stick with one style for everything. No particular preference.

I think I like the empty line, so I added it.

> We should probably have a check to see if "No throttling" is the value and do onChangeNetworkThrottling(false, ""); and return early before looping through the throttling profiles.

Okay, seems fine.

> We should pair select.selected, select:focus since the properties are both the same.

The current ordering seems to be intentional, since all 3 states stack on top of each other:

1. select.selected when some item is chosen and the feature is active (uses select.selected block)
2. select:hover when the select.selected is hovered (select:hover block takes precidence)
3. select:focus when the select.selected is focused (select:focus block takes precidence)

Now, I can't really remember if the visual effect this causes is actually intentional or not...  In any case, I'll add a comment about the ordering.

> Should add a period to the end of the comment.

Added.

> Haven't seen this style of using switch cases with embedding the logic of the case statements in { } break;. Why did we decide to switch the styles?

Yeah, I don't think we've used it before, I borrowed it from a C style I've seen.  It was needed because there are now two `enabled` variables in different parts of the switch statement, and case clauses don't count as separate blocks by default.  Adding braces makes the separate blocks.

I guess a more normal approach would be to make separate functions for each case clause...  I guess I'll change to that.

> I would've preferred if we did:
> 
> if (!enabled) {
>   yield this.emulationFront.clearNetworkThrottling();
> }
> 
> and keep the reminder of the updateNetworkThrottling logic un-indented to make it look a bit cleaner.
> 
> This could be done in updateDPPX, updateTouchSimulation and updateUserAgent

Okay, seems fine to me too.  Updated all of them.

> Whether or not networking throlling is enabled.

Fixed typo.
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8799056 [details]
Bug 1283453 - Add network throttling to emulation actor.

https://reviewboard.mozilla.org/r/84348/#review83150

> Typo, "distinguished"

Thanks, fixed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25fa682afdab
Comment on attachment 8799057 [details]
Bug 1283453 - Add network throttling UI to RDM.

This looks really good! I have one small request: 

Currently the space between the label and the "Responsive Design Mode |" looks a lot wider on one side than it does on the other. Can we tighten that?

Screenshot of current build vs. what I'd like: https://cl.ly/2U0A1s05261o
Attachment #8799057 - Flags: ui-review?(hholmes) → feedback+
Comment hidden (mozreview-request)
Created attachment 8800407 [details]
Spacing in UI

Thanks for the review!

Attached image shows how it looks now after I made some adjustments to the CSS.
Attachment #8800407 - Flags: ui-review?(hholmes)
Updated build-only try run for easier UI review:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed91133762d97506dafd2d75228669a1d113603
Attachment #8800407 - Flags: ui-review?(hholmes) → ui-review+

Comment 19

a year ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f289545ab7d
Add network throttling to emulation actor. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/56b16d2eaa77
Add network throttling UI to RDM. r=gl

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9f289545ab7d
https://hg.mozilla.org/mozilla-central/rev/56b16d2eaa77
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I still see the old RDM UI even with devtools.responsive.html.enabled set to true. Is there something else that needs to be done to see the new UI?

Sebastian
Flags: needinfo?(jryans)
Ah, my fault. I tried it on my e10s-disabled profile, which I have to keep Firebug running.
With e10s enabled it works fine.

Sebastian
Flags: needinfo?(jryans)
(Assignee)

Updated

a year ago
Blocks: 1309375
I managed to perform a few test around this bug and noticed some potential issues:
- On Ubuntu and Windows OS, the Network Throttling selection and the drop down button are not vertically aligned.
- on Ubuntu OS, the size of the text is not the same with the size of the "Responsive Design Mode" string, as on the other 2 OSs.
Here is a screenshot with the issues mentioned above: https://i.imgur.com/6WFikTY.png 

So just to be clear, the Edit button and the throttling options will be added/modified accordingly in Bug 1283445?

Please let me know if I should verify anything else around this fix.

The tests were performed on Firefox 52.0a1 (2016-10-20), under Windows 10x 64, Mac OS X 10.11.6 and under Ubuntu 16.04 x86.

Updated

a year ago
Flags: needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #23)
> I managed to perform a few test around this bug and noticed some potential
> issues:
> - On Ubuntu and Windows OS, the Network Throttling selection and the drop
> down button are not vertically aligned.
> - on Ubuntu OS, the size of the text is not the same with the size of the
> "Responsive Design Mode" string, as on the other 2 OSs.
> Here is a screenshot with the issues mentioned above:
> https://i.imgur.com/6WFikTY.png 

Okay, let's file a bug for these visual issues.  Probably one bug for both is enough.

> So just to be clear, the Edit button and the throttling options will be
> added/modified accordingly in Bug 1283445?

We don't have immediate plans to add the edit button and custom throttling yet.  There's a UX spec in bug 1283445 for it, but some new bug would be filed for the implementation.  For the moment, those options aren't available.

Thanks for testing!
Flags: needinfo?(jryans)
Bug 1312672 was filled for the issues described in Comment 23.
Since that the found issues were logged separately, I am marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-rdm]
status-firefox52: fixed → verified
Flags: qe-verify+

Updated

a year ago
Depends on: 1317413
-> https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Network_throttling
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.