1276971 - Adding UI to display and change the current DPI / DPR setting

Status: VERIFIED FIXED

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

Description

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Image 2016-05-31 at 10.54.41 AM.png

We should display the current setting applied, and give to the user a way to change independently from the selected device, as we're doing for width and height.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

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

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Notice that this patch is made on top of bug 1302879 (landed) and bug 1303484 (pushed on fx-team).

Gabriel, I assigned this review as we discussed on IRC the last week – it took more time than expected since I had to solve the issues on the unit test (see the bugs above) –; since you also worked on the device selector and this is a bit similar, I think you're the right person to review this code; but if you can't let me know and I'll assign the review to jryans.

Thanks!

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e013f192dd68&selectedJob=28486326

Comment 4

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review82086

::: devtools/client/responsive.html/actions/index.js:37
(Diff revision 1)
> +  // when changing the monitor resolution, or when the window is dragged to a different
> +  // display with a different pixel ratio.
> +  "CHANGE_DISPLAY_PIXEL_RATIO",
> +
> +  // The pixel ratio of the viewport has changed. This may be triggered by the user
> +  // When changing the device displayed in the viewport, or when a pixel ratio is

s/When/when

::: devtools/client/responsive.html/actions/viewports.js:40
(Diff revision 1)
>    },
>  
>    /**
> +   * Change the viewport pixel ratio.
> +   */
> +  changeViewportPixelRatio(id, pixelRatio = 0) {

I don't think we need the default value for pixelRatio since we always provide a value.

::: devtools/client/responsive.html/app.js:37
(Diff revision 1)
>    displayName: "App",
>  
>    propTypes: {
>      devices: PropTypes.shape(Types.devices).isRequired,
>      location: Types.location.isRequired,
> +    displayPixelRatio: PropTypes.number.isRequired,

Move displayPixelRatio before location. I like to keep the propTypes listed in alphabetical order.

::: devtools/client/responsive.html/components/dpr-selector.js:5
(Diff revision 1)
> +/* 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/. */
> +
> + /* eslint-env browser */

Leading whitespace that should be removed.

::: devtools/client/responsive.html/components/dpr-selector.js:13
(Diff revision 1)
> +
> +const { DOM: dom, createClass, PropTypes, addons } =
> +  require("devtools/client/shared/vendor/react");
> +
> +const Types = require("../types");
> +const PIXEL_RATIO_PRESET = [1, 2, 3];

I would like to keep the requires together and have a new line that separates PIXEL_RATIO_PRESET. Swap Line 11 and 12.

::: devtools/client/responsive.html/components/dpr-selector.js:75
(Diff revision 1)
> +
> +    if (isDisabled) {
> +      selectorClass += " disabled";
> +    }
> +
> +    if (!PIXEL_RATIO_PRESET.includes(displayPixelRatio)) {

I think we should move this block after line 65 to keep the logic around building the hidden options closer together.

::: devtools/client/responsive.html/components/dpr-selector.js:97
(Diff revision 1)
> +      dom.select(
> +        {
> +          value: selectedPixelRatio,
> +          title: selectedPixelRatio,
> +          onChange: this.onSelectChange,
> +          disabled: isDisabled,

Move disabled above onChange. I prefer to keep the event handlers at the end.

::: devtools/client/responsive.html/components/viewport.js:21
(Diff revision 1)
>    displayName: "Viewport",
>  
>    propTypes: {
>      devices: PropTypes.shape(Types.devices).isRequired,
>      location: Types.location.isRequired,
> +    displayPixelRatio: PropTypes.number.isRequired,

Move displayPixelRatio before location.

::: devtools/client/responsive.html/components/viewport.js:74
(Diff revision 1)
>  
>    render() {
>      let {
>        devices,
>        location,
> +      displayPixelRatio,

Move displayPixelRatio before location.

::: devtools/client/responsive.html/components/viewport.js:97
(Diff revision 1)
>          className: "viewport",
>        },
>        ResizableViewport({
>          devices,
>          location,
> +        displayPixelRatio,

Move displayPixelRatio before location

::: devtools/client/responsive.html/components/viewports.js:20
(Diff revision 1)
>    displayName: "Viewports",
>  
>    propTypes: {
>      devices: PropTypes.shape(Types.devices).isRequired,
>      location: Types.location.isRequired,
> +    displayPixelRatio: PropTypes.number.isRequired,

Move displayPixelRatio before location

::: devtools/client/responsive.html/components/viewports.js:36
(Diff revision 1)
>  
>    render() {
>      let {
>        devices,
>        location,
> +      displayPixelRatio,

Move displayPixelRatio before location

::: devtools/client/responsive.html/components/viewports.js:57
(Diff revision 1)
>        viewports.map((viewport, i) => {
>          return Viewport({
>            key: viewport.id,
>            devices,
>            location,
> +          displayPixelRatio,

Move displayPixelRatio before location

::: devtools/client/responsive.html/index.css:195
(Diff revision 1)
>  }
>  
> +.viewport-device-selector {
> +  padding: 0 8px 0 8px;
> +}
> +.viewport-dpr-selector select {

Add a new line before this rule.

::: devtools/client/responsive.html/reducers.js:9
(Diff revision 1)
>  
>  "use strict";
>  
>  exports.devices = require("./reducers/devices");
>  exports.location = require("./reducers/location");
> +exports.displayPixelRatio = require("./reducers/display-pixel-ratio");

Move this before exports.location.

::: devtools/client/responsive.html/reducers/display-pixel-ratio.js:5
(Diff revision 1)
> +/* 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/. */
> +
> + /* eslint-env browser */

Leading whitespace that should be removed

::: devtools/client/responsive.html/reducers/display-pixel-ratio.js:14
(Diff revision 1)
> +const { CHANGE_DISPLAY_PIXEL_RATIO } = require("../actions/index");
> +const INITIAL_DISPLAY_PIXEL_RATIO = 0;
> +
> +let reducers = {
> +
> +  [CHANGE_DISPLAY_PIXEL_RATIO](_, action) {

We can replace action with { displayPixelRatio }

::: devtools/client/responsive.html/test/browser/browser_dpr_change.js:36
(Diff revision 1)
> +  yield testDefaults(ui);
> +  yield testChangingDevice(ui);
> +  yield testResetWhenResizingViewport(ui);
> +  yield testChangingDPR(ui);
> +/*
> +  // Here we're trying to emulate that RDM will works as expected in case we're switching

This whole block is commented out. Is this intended?

::: devtools/client/responsive.html/test/browser/head.js
(Diff revision 1)
>      "The device modal is displayed.");
>  }
>  
> -function switchDevice({ toolWindow }, value) {
> +function changeSelectValue({ toolWindow }, selector, value) {
>    return new Promise(resolve => {
> -    let selector = ".viewport-device-selector";

I would add an info() stating: 
Selecting "${value}" in "${selector}" selector

::: devtools/client/responsive.html/test/browser/head.js:258
(Diff revision 1)
>      select.value = value;
>      select.dispatchEvent(event);
>    });
>  }
>  
> +const switchDevice = (ui, name) =>

I think selectDevice and selectDPR would make this more clear.

::: devtools/client/responsive.html/test/unit/test_change_display_pixel_ratio.js:6
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test changing the pixel ratio of the display

I think this should just be: Test changing the display pixel ratio.

Also, missing a period at the end of the comment.

Comment 5

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

I wanted to clarify if this should've been moved into the global toolbar over the viewport toolbar since we are only working with a single viewport for the time being. Also, I think I understand the intention of keeping a separate state for the display pixel ratio, but wanted to clarify it with you. The display pixel ratio simply tracks the window's display pixel ratio, which makes sense when thinking about multiple viewports. I figured since we only have a single viewport we could keep track of the display pixel ratio in the viewport's pixel ratio, but I don't see it being a problem when thinking about multiple viewports. Just wanted to clarify the decision here.

Comment 6

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

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #5)
> I wanted to clarify if this should've been moved into the global toolbar
> over the viewport toolbar since we are only working with a single viewport
> for the time being.

My understanding is that we want to put things like this in the global toolbar for the moment, similar to the touch and screenshot buttons.  That's what I was planning to do for network throttling, at least.  It's a little strange since most of these things are viewport-specific, and we'll likely have to redo it for multiple viewports, but going with global for now at least seems to best match Helen's UX for single viewports.

Comment 7

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review82926

::: devtools/client/responsive.html/index.css:195
(Diff revision 1)
>  }
>  
> +.viewport-device-selector {
> +  padding: 0 8px 0 8px;
> +}
> +.viewport-dpr-selector select {

I also need to reuse this section of select style for the network throttling UI.  I think it would easier to just style all selects for now, instead of listing out all the class names?  That's what I've done locally for the moment in my network throttling patch.

Comment 8

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #5)
> > I wanted to clarify if this should've been moved into the global toolbar
> > over the viewport toolbar since we are only working with a single viewport
> > for the time being.
> 
> My understanding is that we want to put things like this in the global
> toolbar for the moment, similar to the touch and screenshot buttons.  That's
> what I was planning to do for network throttling, at least.  It's a little
> strange since most of these things are viewport-specific, and we'll likely
> have to redo it for multiple viewports, but going with global for now at
> least seems to best match Helen's UX for single viewports.

:zer0 and I talked about this again during today's meeting...  He brought up that the device menu and dPR are strongly linked because the dPR menu becomes disabled (per Helen's UX) when a device is chosen.  So, that would seem to justify keeping them together the same toolbar.

Once we get to having multiple viewports, it's an even stronger case for dPR as a per viewport setting, since each viewport could easily have their own value.  (It is however also true that other things currently in the global toolbar may want to be viewport specific.  It might be best to leave that to be resolved at a different time.  We'd likely need more space in the viewport toolbar if we're really going to fit all the options at the per-viewport level.)

Comment 9

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

Forgot to add earlier...  One challenge with the viewport toolbar currently is that it's smaller than the global toolbar (at least in total height, not sure about the content height once padding is subtracted...).  So we should just be careful to ensure the viewport toolbar still looks good with more things in it.  I assume Helen will check this during ui-review.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review82086

> I don't think we need the default value for pixelRatio since we always provide a value.

When we resetting the viewport, we don't. The same issue we had with the touch enabling; I just used the same approach as we did there.

> We can replace action with { displayPixelRatio }

True, but it wouldn't be consistent with the other reducers. We explicitly leave the `action` argument currently. If we want to change that, I think it would be better refactoring everything to be consistent.

> This whole block is commented out. Is this intended?

Yeah, it's a leftover sorry. I tried to test also multiple displays – simulating the same scenario – but it didn't work for several reasons – and it was a rough simulation anyway –; so I decided to remove those tests.

> I think selectDevice and selectDPR would make this more clear.

I applied the same refactoring once rebased, with the network throttling.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

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

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

Gabriel, I think I've addressed all your review comments, let me know in case.

Helen, as per comment 8, could you review this patch? Basically I followed your original design, since DPR dropdown is strongly connected to the device, I leave it at the right of the device  dropdown, instead of decouple them and put it in the global toolbar.

Comment 13

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review86220

::: devtools/client/responsive.html/app.js:165
(Diff revision 2)
>          onUpdateTouchSimulation,
>        }),
>        Viewports({
>          devices,
>          location,
> +        displayPixelRatio,

Move this before location

::: devtools/client/responsive.html/components/resizable-viewport.js:27
(Diff revision 2)
>    displayName: "ResizableViewport",
>  
>    propTypes: {
>      devices: PropTypes.shape(Types.devices).isRequired,
>      location: Types.location.isRequired,
> +    displayPixelRatio: PropTypes.number.isRequired,

MOve this before location

::: devtools/client/responsive.html/components/resizable-viewport.js:123
(Diff revision 2)
>  
>    render() {
>      let {
>        devices,
>        location,
> +      displayPixelRatio,

Move this before location.

::: devtools/client/responsive.html/index.css:242
(Diff revision 2)
> +  margin: 0 8px;
> +  -moz-user-select: none;
> +  color: var(--viewport-color);
> +  font-size: 11px;
> +}
> +

We should add some padding-right to the DPR "span" or "viewport-dpr-selector select" margin-left to separate the select and span. Maybe 2px.

::: devtools/client/responsive.html/index.css:260
(Diff revision 2)
> +}
> +
> +.viewport-dpr-selector > select > option {
> +  padding: 5px;
> +}
> +

We do lose the viewport-dpr-select active color in the DPR span when the select is open and option is active but our focus is outside the select and option. The DPR span should continue to span while the option dropdown is open despite where the mouse focus is.

::: devtools/client/responsive.html/index.js:87
(Diff revision 2)
>  });
>  
> +// Dispatch a `changeDisplayPixelRatio` action when the browser's pixel ratio is changing.
> +// This is usually triggered when the user changes the monitor resolution, or when the
> +// browser's window is dragged to a different display with a different pixel ratio.
> +function onDPRChange() {

Let's actually move the function block before window.addInitialViewport block to keep the logic closer together.

::: devtools/client/responsive.html/test/browser/head.js:235
(Diff revision 2)
>    ok(modal.classList.contains("opened") && !modal.classList.contains("closed"),
>      "The device modal is displayed.");
>  }
>  
> -function switchSelector({ toolWindow }, selector, value) {
> +function changeSelectValue({ toolWindow }, selector, value) {
> +  info(`Selectiong ${value} in ${selector}.`);

s/Selectiong/Selecting

Comment 14

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> :zer0 and I talked about this again during today's meeting...  He brought up
> that the device menu and dPR are strongly linked because the dPR menu
> becomes disabled (per Helen's UX) when a device is chosen.  So, that would
> seem to justify keeping them together the same toolbar.
> 
> Once we get to having multiple viewports, it's an even stronger case for dPR
> as a per viewport setting, since each viewport could easily have their own
> value.  (It is however also true that other things currently in the global
> toolbar may want to be viewport specific.  It might be best to leave that to
> be resolved at a different time.  We'd likely need more space in the
> viewport toolbar if we're really going to fit all the options at the
> per-viewport level.)

You're right that any answer I give here is going to need to be different once we have multiple viewports. Everything in the toolbar relates to the one viewport and view-versa at the moment. My proposal is to keep the dpr settings in the global toolbar; when inactive because a particular device is chosen, the inactive state as a hover label saying that "DPR automatically set by device". 

This way we can explain the relationship between the two things and keep the look of the current device toolbar area from getting too crowded.

I'll also make a note for the multi-viewport that we're probably going to want a different solution for that.

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

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

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

Some note about the new patch:

- Moved DPR dropdown in the global toolbar.

- Added tooltip in localization: when there is no device selected, the tooltip says “Device pixel ratio”, otherwise “DPR automatically set by <device name>). Notice that there is no localization for “DPR” since is intended as acronym as DPI etc, that also why I added the tooltip with a “localized” meaning.

- The DPR state is still in the viewport’s state, not in the global one.
In the app.js, I assumed we have only one viewport, and grab the info needed from its state.
I decided to do so for several reasons: as said, the device and the dpr are coupled, therefore if I had to move the DPR state from the viewport, I also had to do so for the device, at least the device’s name, in order to know if a device was selected or not.
To me, it would end up in a messier app state, than we have currently.
So I opted to have this workaround. We also have similar code elsewhere where we assume there is only one viewport, so I don’t think it’s a big deal; plus it keeps the “compatibility” for the multiviewport approach.

- The DPR Selector uses an internal state to add a class when the <select> is focused, so that also the “DPR” text keeps the right color even if the select is focused but the mouse is not over the component.

- This patch also fixes an issue where the browser_device_modal_submit was failing locally on OS X (not all machine, apparently). Now `openDeviceModal` uses synthesized events instead of `EventUtils`.

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5fa0357b5f2861d9aaef4ffc51a1ed76b0a21dd&selectedJob=30181023

Comment 18

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review89088

Comment 19

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review89094

::: devtools/client/responsive.html/app.js:150
(Diff revisions 2 - 3)
>        onUpdateDeviceModalOpen,
>        onUpdateTouchSimulation,
>      } = this;
>  
> +    let selectedPixelRatio = 0;
> +    let selectedDevice = "";

Put selectedDevice before selectedPixelRatio to be consistent with the order we assign the variables in the if statements that follow.

::: devtools/client/responsive.html/app.js:167
(Diff revisions 2 - 3)
> +        devices,
> +        displayPixelRatio,
>          networkThrottling,
> +        selectedDevice,
> +        selectedPixelRatio,
>          screenshot,

Move screenshot above selectedDevice

::: devtools/client/responsive.html/test/browser/head.js:275
(Diff revisions 2 - 3)
> +const selectNetworkThrottling = (ui, value) => Promise.all([
> +  once(ui, "network-throttling-changed"),
> +  changeSelectValue(ui, "#global-network-throttling-selector", value)
> +]);
>  
> -const selectNetworkThrottling = (ui, value) =>
> +const selectDPR = (ui, value) =>

This should go before selectNetworkThrottling to keep things alphabetically ordered.

::: devtools/client/locales/en-US/responsive.properties:79
(Diff revision 3)
> +# DevicePixelRatio (DPR) dropdown when is enabled.
> +responsive.devicePixelRatio=Device Pixel Ratio
> +
> +# LOCALIZATION NOTE (responsive.autoDPR): tooltip for the DevicePixelRatio
> +# (DPR) dropdown when is disabled because a device is selected.
> +# The argument (%1$S) is the selected device (e.g. iPhone 6) that set 

Trailing whitespace

::: devtools/client/locales/en-US/responsive.properties:83
(Diff revision 3)
> +# (DPR) dropdown when is disabled because a device is selected.
> +# The argument (%1$S) is the selected device (e.g. iPhone 6) that set 
> +# automatically the DPR value.
> +responsive.autoDPR=DPR automatically set by %1$S
> +
> +

Remove one of the new line EOF since we already have 2.

::: devtools/client/responsive.html/components/global-toolbar.js:13
(Diff revision 3)
>    require("devtools/client/shared/vendor/react");
>  
>  const { getStr } = require("../utils/l10n");
>  const Types = require("../types");
>  const NetworkThrottlingSelector = createFactory(require("./network-throttling-selector"));
> +const DPRSelector = createFactory(require("./dpr-selector"));

Move DPRSelector above NetworkThrottlingSelector to keep the factories in alphabetical order.

::: devtools/client/responsive.html/components/global-toolbar.js:24
(Diff revision 3)
> +    devices: PropTypes.shape(Types.devices).isRequired,
> +    displayPixelRatio: PropTypes.number.isRequired,
>      networkThrottling: PropTypes.shape(Types.networkThrottling).isRequired,
> +    selectedDevice: PropTypes.string.isRequired,
> +    selectedPixelRatio: PropTypes.number.isRequired,
>      screenshot: PropTypes.shape(Types.screenshot).isRequired,

Move screenshot above selectedDevice to keep the propTypes alphabetically ordered.

::: devtools/client/responsive.html/components/global-toolbar.js:42
(Diff revision 3)
> +      devices,
> +      displayPixelRatio,
>        networkThrottling,
> +      selectedDevice,
> +      selectedPixelRatio,
>        screenshot,

Move screenshot above selectedDevice.

Comment 20

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

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

Comment 21

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

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

https://reviewboard.mozilla.org/r/83262/#review89324

Overall it looks reasonable to me, thanks for working on it!  I'll forward my question below to :helenvholmes.

::: devtools/client/responsive.html/components/dpr-selector.js:61
(Diff revision 4)
> +
> +  onSelectChange({ target }) {
> +    this.props.onChangeViewportPixelRatio(+target.value);
> +  },
> +
> +  render() {

For the other menus (network throttling, device), we've been setting a "selected" class when an override is applied, which uses the "bolder" text color even when not focused.

Should we do the same thing for dPR when setting a custom value that differs from the device?

Comment 22

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

Helen, see my question in comment 21.

Comment 23

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)
> Should we do the same thing for dPR when setting a custom value that differs
> from the device?

To clarify since you can't change the dPR currently when a device is selected, I am suggesting we'd apply the "selected" class when a custom dPR value is chosen, so that would mean the case of "no specific device / flexible size with custom dPR".

Comment 24

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

> For the other menus (network throttling, device), we've been setting a "selected" class when an override is applied, which uses the "bolder" text color even when not focused.

> Should we do the same thing for dPR when setting a custom value that differs from the device?

I think differentiating is a good idea!

Comment 25

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8797594 [details]
Bug 1276971 - Adding UI to display and change the current DPI / DPR setting;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83262/diff/4-5/

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please mark pending issues as resolved in MozReview so this can autoland.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5992687fa68b
Adding UI to display and change the current DPI / DPR setting; r=gl,jryans

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5992687fa68b

Comment 29

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I used Fx 49.0a1, build ID:20160531030258, and Fx 52.0a1, build ID: 20161114043454 for the comparison.
The improvements are available, I checked the functionality of the pixel ratio and the multiple display density. No issues were found.I verified on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS. I am marking this issue as verified.  

Cheers!

Comment 30

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Setting_device_pixel_ratio