Some UI control that lists devices. Upon choosing a device, any viewport settings associated with it (width, height, dPR, etc.) are applied.

User agent and touch simulation state are also known in the current device DB.

From Helen's designs in bug 1241713, I would expect this bug would involve: * Creating the smaller device menu that appears in the viewport toolbar * Applying device properties when a device is selected from that menu The panel for editing what devices appear in the smaller menu would be done over in bug 1241720.

Hey ntim, I am planning on working on Bug 1241720 - editable device list which is blocked by this bug. Do you have any WIP I can build off of, or do you mind if I take over the bug?

I haven't started, so feel free to take over.

Example use of the JSON device list API: https://github.com/nt1m/devtools-emulation/blob/master/chrome/panel.js#L59 devtools("devtools/shared/devices") = require("devtools/shared/devices")

- Added a store for Devices with the methods addDevice, addDeviceType - In the future, we will probably want to add a new attribute to the device type which specifies whether or not the device is enabled/disabled in the <select> drop menu. - I have hidden the <select> dropdown arrow and will ask Helen for a new icon to best emulate her designs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=54a2641b6ae3

This is the arrow I'm using twice in my designs (just much tinier, with a "whitesmoke" fill, and rotated).

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #7) > Created attachment 8724070 [details] > arrow.svg > > This is the arrow I'm using twice in my designs (just much tinier, with a > "whitesmoke" fill, and rotated). It appears to be blurry on Windows.

Comment on attachment 8723957 [details] [diff] [review] 1241714.patch Some small things: - Currently, if you have a device selected and you go to change the dimensions with the lower-righthand drag, the device becomes unselected on click. I think this is wrong; it should become unselected on first drag event. — I think we should follow up with :phlsa about <selects> and what his plans for them are. While I know we really want a custom select, they are really annoying to create.

Comment on attachment 8724983 [details] [diff] [review] 1241714.patch [1.0] - A small thing, but my designs actually have two arrows stacked on top of each other (one pointing up, one pointing down). You should be able to use the same arrow twice and use a transform to flip one of them and then make both of them smaller: http://cl.ly/1B3Y121U330i vs. http://cl.ly/3I3q2O3E2G0s I know that we were discussing forms.css and what might be possible to get that select box looking nicer. So, knowing that some of this might never come to fruition, here are some thoughts on what I think would make that select dropdown look nicer: - The select options expand to the arrow; if they expand only underneath "no device selected", I think that would look nicer. - The select is a little large, and the arrows feel oddly far away. Tightening up this spacing would help: http://cl.ly/0L1o3k24330r - Additionally, the select box should feel centered (even if it currently is, it doesn't feel like it is because of the icon on the right and the arrow being far away). Right now if I was working on this I would pull that arrow in, and move the whole select-content-area-whatever a tad to the right until it felt right. (Sorry this isn't an exact science.) - The options themselves are so so so tight. If we could increase their vertical spacing, that would also help: http://cl.ly/0L1o3k24330r - You're right that a) the black dotted line around the select box is ugly and additionally b) actually just all of the lines around everything is ugly, unfortunately. I know we have already discussed (despaired?) over what might be done about this over IRC and didn't come up with any good solutions—I'd suggest that once bgrins is back we ask him what we might do, since I would imagine he might have some better ideas for how we might make those selects look nicer. In standup tomorrow it's probably worth bringing up what we can't and can't do at the moment so that you can at least keep moving—we might consider opening up another bug to address these UI concerns since it seems like they need additional research beyond the scope of this patch and don't affect the functionality of the select box, they're just... annoying.

Seeing the select arrows in place, I think it might look nicer if they were spaced out a bit more.

Comment on attachment 8725300 [details] [diff] [review] 1241714.patch [2.0] I attached a new select-arrows.svg (with a comment), but other than that I think this is looking good!

Planning to review this first thing tomorrow, sorry for the delay.

Comment on attachment 8725331 [details] [diff] [review] 1241714.patch [3.0] Review of attachment 8725331 [details] [diff] [review]: ----------------------------------------------------------------- I did notice while reviewing that React is doing lots of unnecessary re-renders, but it does not seem specific to your changes. I'll try to investigate further. Also, I've worked out a way we can use a UA stylesheet to reset the remaining borders. I'll attach an example. ::: devtools/client/responsive.html/components/device-selector.js @@ +29,5 @@ > + > + for (let type of devices.types) { > + for (let device of devices[type]) { > + if (device.name === target.value) { > + onResizeViewport(device.width, device.height); We also want to apply: * touch simulation * dPR * UA from the device info. However, this bug is complex enough to add the basics and work out the list UI. Please file new bugs to apply each of these items from the selected device. It's possible it will get done in different bugs when we e.g. add touch support in general, but I think having specific bugs about applying for the device selected should ensure it doesn't get lost. @@ +53,5 @@ > + } > + > + let selectClass = "viewport-device-selector"; > + if (!viewport.device) { > + selectClass += " unselected"; Let's use a positive class "selected" and invert when it is set. @@ +66,5 @@ > + dom.option({ > + value: "", > + disabled: true, > + hidden: true, > + }, "no device selected"), This should move to a properties file for translation. The close button bug 1239464 adds an l10n module and strings file. ::: devtools/client/responsive.html/components/resizable-viewport.js @@ +99,5 @@ > > // Update the viewport store with the new width and height. > this.props.onResizeViewport(width, height); > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); The dimension inputs also need to reset the device when you commit the new value. ::: devtools/client/responsive.html/images/select-arrow.svg @@ +7,5 @@ > + use:not(:target) { > + display: none; > + } > + #light { > + fill: #393f4c; Are we able to set colors from our CSS file instead of embedded in the SVG? That's maybe more flexible? What do you think? Not sure which is better. ::: devtools/client/responsive.html/index.css @@ +98,5 @@ > + background-size: 7px; > + border: none; > + color: var(--viewport-active-color); > + height: 100%; > + padding: 0 16px 0 25px; Why is the padding unbalanced? Shouldn't the text be centered within the element? @@ +100,5 @@ > + color: var(--viewport-active-color); > + height: 100%; > + padding: 0 16px 0 25px; > + text-overflow: ellipsis; > + width: 150px; Should we allow the width to be flexible so there's more room for long device names? It seems like we should take advantage of flexbox sizing here. May not want to take up *all* free space, but perhaps most of it? @@ +136,5 @@ > padding: 0; > width: 16px; > height: 16px; > opacity: 0.8; > + position: absolute; Hmm, forcing these to the right does make flexible sizing on the device list harder... I'll think more about how we could do this. ::: devtools/client/responsive.html/manager.js @@ +6,5 @@ > > const promise = require("promise"); > const { Task } = require("resource://gre/modules/Task.jsm"); > const EventEmitter = require("devtools/shared/event-emitter"); > +const { GetDevices } = require("devtools/client/shared/devices"); Looking at the code for this module, it seems like we use `getJSON` to request the device list only once, save it to a pref, and then never check ever again. There is a `bypassCache` option to always load it, but that may be *too* often. For example, we do add-on background updates at most once per day. That seems like a fine frequency here as well. We should either fix this here, or file a follow up to work on it before enabling. @@ +196,5 @@ > let tabBrowser = this.tab.linkedBrowser; > let contentURI = tabBrowser.documentURI.spec; > tabBrowser.loadURI(TOOL_URL); > yield tabLoaded(this.tab); > + let devices = yield GetDevices(); I would like the manager.js file to do only the minimum amount of work that it must, because the current design is partly the cause of various known issues with our tab management, session restore, etc. It may change a lot soon. I think it would be better to do this in the index.js file of the tool, at the end of the init() method or some new method it calls, etc. Ordinarily, I'd be worried we need to ensure init() is totally done before we can call addInitialViewport, but since they each just dispatch actions, I would expect things to "just work" even if there ends up being a race. ::: devtools/client/responsive.html/test/unit/test_change_viewport_device.js @@ +37,5 @@ > + dispatch(changeDevice(0, "Firefox OS Flame")); > + > + viewport = getState().viewports[0]; > + equal(viewport.device, "Firefox OS Flame", > + "Changed to Firefox OS Flame device"); Please also check that the viewport is set to the expected size. ::: devtools/client/responsive.html/types.js @@ +32,5 @@ > + // Whether or not it is a touch device > + touch: PropTypes.bool, > + > + // Whether or not it is a firefox OS device > + firefoxOS: PropTypes.bool, I don't think we're planning to use this bit, so we can probably remove it here. Instead, there will be an "os" property, and we'll ignore any where os == "fxos". See the commit[1]. Also, since you are working on the modal as well, how are you planning to store the set of enabled devices? This reminds me we need to deploy an updated list with these changes, so I sent :janx an email about this. [1]: https://github.com/mozilla/simulated-devices/commit/0fa3a177281d4085689349f18ffb7869f6fa32a4 @@ +39,5 @@ > + > +/** > + * A list of devices and their types that can be displayed in the viewport. > + */ > +exports.devices = { How are you planning to handle only some of the devices from the DB being default enabled?

Comment on attachment 8725331 [details] [diff] [review] 1241714.patch [3.0] Review of attachment 8725331 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/responsive.html/components/device-selector.js @@ +53,5 @@ > + } > + > + let selectClass = "viewport-device-selector"; > + if (!viewport.device) { > + selectClass += " unselected"; Fixed. Used a positive class "selected" @@ +66,5 @@ > + dom.option({ > + value: "", > + disabled: true, > + hidden: true, > + }, "no device selected"), Fixed. Used added a new string. ::: devtools/client/responsive.html/components/resizable-viewport.js @@ +99,5 @@ > > // Update the viewport store with the new width and height. > this.props.onResizeViewport(width, height); > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); Fixed. Reset the device when we commit a new height/width value input. ::: devtools/client/responsive.html/images/select-arrow.svg @@ +7,5 @@ > + use:not(:target) { > + display: none; > + } > + #light { > + fill: #393f4c; The one way where we can fill in the background color was to use mask-image, but the mask feature doesn't seem to stable at the moment. ::: devtools/client/responsive.html/index.css @@ +98,5 @@ > + background-size: 7px; > + border: none; > + color: var(--viewport-active-color); > + height: 100%; > + padding: 0 16px 0 25px; Fixed. Changed to padding: 0 16px; @@ +100,5 @@ > + color: var(--viewport-active-color); > + height: 100%; > + padding: 0 16px 0 25px; > + text-overflow: ellipsis; > + width: 150px; Let's leave this as follow. It is tricky with how we have to position the select arrow, but we can probably use some percentage. ::: devtools/client/responsive.html/manager.js @@ +6,5 @@ > > const promise = require("promise"); > const { Task } = require("resource://gre/modules/Task.jsm"); > const EventEmitter = require("devtools/shared/event-emitter"); > +const { GetDevices } = require("devtools/client/shared/devices"); Let's file a follow up for this. @@ +196,5 @@ > let tabBrowser = this.tab.linkedBrowser; > let contentURI = tabBrowser.documentURI.spec; > tabBrowser.loadURI(TOOL_URL); > yield tabLoaded(this.tab); > + let devices = yield GetDevices(); Fixed. Added GetDevices in init() ::: devtools/client/responsive.html/test/unit/test_change_viewport_device.js @@ +37,5 @@ > + dispatch(changeDevice(0, "Firefox OS Flame")); > + > + viewport = getState().viewports[0]; > + equal(viewport.device, "Firefox OS Flame", > + "Changed to Firefox OS Flame device"); So, this test only checks for the state changes when we perform the action changeDevice which affects the <select> component. In the component, we manually call onResizeViewport with the device width and height. We would do this check when we test the UI. ::: devtools/client/responsive.html/types.js @@ +32,5 @@ > + // Whether or not it is a touch device > + touch: PropTypes.bool, > + > + // Whether or not it is a firefox OS device > + firefoxOS: PropTypes.bool, Fixed. Removed "firefoxOS" bit and added "os" string property. @@ +39,5 @@ > + > +/** > + * A list of devices and their types that can be displayed in the viewport. > + */ > +exports.devices = { We will probably need another object that maps the device name to a boolean indicating whether or not they are enabled. We may need to discuss this further if we also do an update of these preset defaults.

Rebased

Comment on attachment 8727613 [details] [diff] [review] 1241714.patch [5.0] Review of attachment 8727613 [details] [diff] [review]: ----------------------------------------------------------------- I filed bug 1254385, bug 1254386, and bug 1254388 to apply the add'l device params. I filed bug 1254390 to investigate flexible sizing for the device selector. I filed bug 1254392 to refresh the device DB. Overall the UX works nicely, thanks! ::: devtools/client/responsive.html/components/device-selector.js @@ +9,5 @@ > + require("devtools/client/shared/vendor/react"); > + > +const Types = require("../types"); > + > +module.exports = createClass({ Add the PureRenderMixin to this component to improve perf @@ +13,5 @@ > +module.exports = createClass({ > + > + propTypes: { > + devices: PropTypes.shape(Types.devices).isRequired, > + viewport: PropTypes.shape(Types.viewport).isRequired, Since you only need `viewport.device` inside, we don't want to pass around the full viewport. The reason is the viewport contains the width and height, so it is expensive to pass it in since it can lead to unnecessary re-rendering on resize. Have the outer component pull off `viewport.device` and pass it in as `selectedDevice`. @@ +18,5 @@ > + onChangeViewportDevice: PropTypes.func.isRequired, > + onResizeViewport: PropTypes.func.isRequired, > + }, > + > + displayName: "DeviceSelector", Move displayName above propTypes to match other components. @@ +30,5 @@ > + > + for (let type of devices.types) { > + for (let device of devices[type]) { > + if (device.name === target.value) { > + onResizeViewport(device.width, device.height); I wonder if it's better to do this work in the change device action creator instead... I'll keep thinking about it for the future, can't decide right now. It seems like it would be nice property, esp. for testing if you can assume that dispatching change device automatically resizes, so that there is less logic in this component. You can return a function[1] from an action creator to get access to dispatch and have the ability to dispatch more than once, instead of returning just one object. [1]: http://redux.js.org/docs/advanced/AsyncActions.html#async-action-creators ::: devtools/client/responsive.html/components/resizable-viewport.js @@ +99,5 @@ > > // Update the viewport store with the new width and height. > this.props.onResizeViewport(width, height); > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); Maybe null is more natural as "unselected"? Both are falsy... ::: devtools/client/responsive.html/components/viewport-dimension.js @@ +110,5 @@ > return; > } > > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); Maybe null is more natural as "unselected"? Both are falsy... ::: devtools/client/responsive.html/components/viewport-toolbar.js @@ +17,5 @@ > mixins: [ addons.PureRenderMixin ], > > propTypes: { > + devices: PropTypes.shape(Types.devices).isRequired, > + viewport: PropTypes.shape(Types.viewport).isRequired, This component also doesn't need the whole viewport, it's parent should pass in selectedDevice. ::: devtools/client/responsive.html/images/select-arrow.svg @@ +7,5 @@ > + use:not(:target) { > + display: none; > + } > + #light { > + fill: #dde1e4; Please document in comments which theme vars these are supposed to be. See my first patch in bug 1254261 as an example[1]. [1]: https://reviewboard.mozilla.org/r/38511/diff/1#0 ::: devtools/client/responsive.html/index.css @@ +179,5 @@ > + -moz-padding-start: 10px; > + -moz-padding-end: 5px; > +} > + > +.viewport-device-select > option:disabled { When does this get used? ::: devtools/client/responsive.html/index.js @@ +91,5 @@ > + > + GetDevices().then(devices => { > + for (let type of devices.TYPES) { > + bootstrap.dispatch(addDeviceType(type)); > + for (let device of devices[type]) { We want to ignore devices where "os" == "fxos". A bit hard to test, since the CDN is slow to update... The data is cached in devtools.devices.url_cache, maybe fiddle with it locally to make sure something gets ignored.

Comment on attachment 8727613 [details] [diff] [review] 1241714.patch [5.0] Review of attachment 8727613 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/responsive.html/components/device-selector.js @@ +9,5 @@ > + require("devtools/client/shared/vendor/react"); > + > +const Types = require("../types"); > + > +module.exports = createClass({ Fixed. Added PureRenderMixin @@ +13,5 @@ > +module.exports = createClass({ > + > + propTypes: { > + devices: PropTypes.shape(Types.devices).isRequired, > + viewport: PropTypes.shape(Types.viewport).isRequired, Fixed. Passed in selectedDevice @@ +18,5 @@ > + onChangeViewportDevice: PropTypes.func.isRequired, > + onResizeViewport: PropTypes.func.isRequired, > + }, > + > + displayName: "DeviceSelector", Fixed. Moved displayName @@ +30,5 @@ > + > + for (let type of devices.types) { > + for (let device of devices[type]) { > + if (device.name === target.value) { > + onResizeViewport(device.width, device.height); Waiting on a decision here. I am okay with both course of actions. ::: devtools/client/responsive.html/components/resizable-viewport.js @@ +99,5 @@ > > // Update the viewport store with the new width and height. > this.props.onResizeViewport(width, height); > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); Fixed. Used null. ::: devtools/client/responsive.html/components/viewport-dimension.js @@ +110,5 @@ > return; > } > > + // Change the device selector back to an unselected device > + this.props.onChangeViewportDevice(""); Fixed. Used null. ::: devtools/client/responsive.html/components/viewport-toolbar.js @@ +17,5 @@ > mixins: [ addons.PureRenderMixin ], > > propTypes: { > + devices: PropTypes.shape(Types.devices).isRequired, > + viewport: PropTypes.shape(Types.viewport).isRequired, Fixed. Used selectedDevice ::: devtools/client/responsive.html/images/select-arrow.svg @@ +7,5 @@ > + use:not(:target) { > + display: none; > + } > + #light { > + fill: #dde1e4; Fixed. Documented colour variables. ::: devtools/client/responsive.html/index.css @@ +179,5 @@ > + -moz-padding-start: 10px; > + -moz-padding-end: 5px; > +} > + > +.viewport-device-select > option:disabled { Fixed. Removed this unused css rule. ::: devtools/client/responsive.html/index.js @@ +91,5 @@ > + > + GetDevices().then(devices => { > + for (let type of devices.TYPES) { > + bootstrap.dispatch(addDeviceType(type)); > + for (let device of devices[type]) { Added a check for device.os != "fxos"

I had to back this out for tests hitting the network: https://treeherder.mozilla.org/logviewer.html#?job_id=7881605&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/4d61cc90ce23

This also appears to have caused https://treeherder.mozilla.org/logviewer.html#?job_id=7881422&repo=fx-team

Looks like the test head file should set some device data and / or we can check DevToolsUtils.testing to avoid the network

Couple of changes: - Reverted to this.props.onChangeViewportDevice(""); since null doesn't set the proper select value - Added a skip for parsing ua.css - Added a preset of devices to avoid hitting the network - I kept the colours for the select arrow as-is.

Comment on attachment 8730831 [details] [diff] [review] 1241714.patch [7.0] Review of attachment 8730831 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, but I am perplexed by how the color values were chosen. May be easy to resolve in person. ::: browser/base/content/test/general/browser_parsable_css.js @@ +27,5 @@ > // Highlighter CSS uses a UA-only pseudo-class, see bug 985597. > {sourceName: /highlighters\.css$/i, > errorMessage: /Unknown pseudo-class.*moz-native-anonymous/i}, > + // Responsive Design Mode CSS uses a UA-only pseudo-class, see Bug 1241714. > + {sourceName: /ua\.css$/i, Might be better to use a more specific path, seems like just the file name is somewhat generic, and could whitelist other files unexpectedly. ::: devtools/client/responsive.html/images/select-arrow.svg @@ +7,5 @@ > + use:not(:target) { > + display: none; > + } > + #light { > + fill: #dde1e4; /* --theme-splitter-color */ I am fine using this approach of setting the colors even though bug 1254261 has now diverged, since these aren't the same case as the buttons. How were these color values chosen? Why do we want to pull from different variables for each theme? ::: devtools/client/responsive.html/test/browser/head.js @@ +15,5 @@ > this); > > +const TEST_URI_ROOT = "http://example.com/browser/devtools/client/responsive.html/test/browser/"; > + > +Services.prefs.setCharPref("devtools.devices.url", For completeness, clear this pref in the clean up function below.

Comment on attachment 8730831 [details] [diff] [review] 1241714.patch [7.0] Oops, meant to set r- to hear more about color values first.

Comment on attachment 8730831 [details] [diff] [review] 1241714.patch [7.0] During our standup today, :gl, :helenvholmes and I decided adding new variable(s) as needed for this case would be best, so we can use the variables in both themes for consistency. The colors will still be embedded in the SVG for this case, but at least the variable names are commented next to them for searching. r+ assuming this change is made.

https://hg.mozilla.org/mozilla-central/rev/4eea3b603b0a https://hg.mozilla.org/mozilla-central/rev/dd6593bfbb59

I managed to test this RDM setting and found a few potential issues: - the arrows are not visible when the button is unselected - they should be darker - nothing happens when hoovering the mouse over the button - the button should be highlighted when hoovering the mouse over it - after selecting a device, the button remains highlighted - after selecting a device and changing the focus from the device selection button, in my opinion, the button should be gray again, since is not selected and highlighted when hoovering the cursor over it or when it's selected. Also I observed that if selecting any of the Samsung Galaxy devices, the RDM has the same dimension, 360x640. Since this devices have different displays, is this an expected result? Should I log this issues? Note that the tests were performed on WIndows 10 x86, Mac OS X 10.11 and Ubuntu 12.04 x86.

(In reply to Mihai Boldan, QA [:mboldan] from comment #41) > I managed to test this RDM setting and found a few potential issues: > - the arrows are not visible when the button is unselected - they should be > darker > - nothing happens when hoovering the mouse over the button - the button > should be highlighted when hoovering the mouse over it > - after selecting a device, the button remains highlighted - after selecting > a device and changing the focus from the device selection button, in my > opinion, the button should be gray again, since is not selected and > highlighted when hoovering the cursor over it or when it's selected. Let's ask Helen about these UX issues. > Also I observed that if selecting any of the Samsung Galaxy devices, the RDM > has the same dimension, 360x640. Since this devices have different displays, > is this an expected result? According to our device DB[1], they have the same size, so the RDM tool is working correctly. However, it could be that we have the wrong size data. One thing to note is that the sizes here are not the actual screen size in device pixels, but rather the "CSS pixels" that the device's browser reports. It's somewhat hard to find good data on these values unfortunately, but if ours is wrong please let us know! Filing an issue[2] on our device repo is a good way to report this. Looks at another data source[3], it appears S3 - S5 are believed to report the same CSS width / height values, for example. [1]: https://github.com/mozilla/simulated-devices/blob/master/devices.json#L261-L302 [2]: https://github.com/mozilla/simulated-devices/issues [3]: http://www.mydevice.io/devices/

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42) > (In reply to Mihai Boldan, QA [:mboldan] from comment #41) > > I managed to test this RDM setting and found a few potential issues: > > - the arrows are not visible when the button is unselected - they should be > > darker > > - nothing happens when hoovering the mouse over the button - the button > > should be highlighted when hoovering the mouse over it > > - after selecting a device, the button remains highlighted - after selecting > > a device and changing the focus from the device selection button, in my > > opinion, the button should be gray again, since is not selected and > > highlighted when hoovering the cursor over it or when it's selected. I agree with most of these points: - We definitely need a hover state. Let's go with #999797, var(----theme-body-color-inactive). This should affect the 'device list' string and the arrows. - When someone clicks out of the select list, the :active state doesn't seem like it's being removed, which is odd. Let's remove it. The dark gray persisting after you've chosen a device was intentional; makes it more obvious that the viewport has particular (device-) settings applied to it, so let's leave that as-is.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #43) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42) > > (In reply to Mihai Boldan, QA [:mboldan] from comment #41) > > > I managed to test this RDM setting and found a few potential issues: > > > - the arrows are not visible when the button is unselected - they should be > > > darker > > > - nothing happens when hoovering the mouse over the button - the button > > > should be highlighted when hoovering the mouse over it > > > - after selecting a device, the button remains highlighted - after selecting > > > a device and changing the focus from the device selection button, in my > > > opinion, the button should be gray again, since is not selected and > > > highlighted when hoovering the cursor over it or when it's selected. > I agree with most of these points: > > - We definitely need a hover state. Let's go with #999797, > var(----theme-body-color-inactive). This should affect the 'device list' > string and the arrows. > - When someone clicks out of the select list, the :active state doesn't seem > like it's being removed, which is odd. Let's remove it. > > The dark gray persisting after you've chosen a device was intentional; makes > it more obvious that the viewport has particular (device-) settings applied > to it, so let's leave that as-is. I have filed bug 1262829 to work on these things.

Hello Mihai, should this bug be marked as verified?

Hi Marco, Since the issues found while testing were logged in Bug 1262829 by Ryan, I am marking this bug Verified- Fixed. Thanks for the heads up.