Some version of editing to change the options available in the device list. The complexity is a bit unclear right now, since it will depend on the behavior we end up designing.

Clarifying purpose, given recent design discussions. This bug will add a modal to choose devices from a larger set that we maintain so they can be promoted to the smaller select box on the viewport. We are not going to offer addition of custom devices at this moment.

Part of the device list feature is enabling only a subset of devices in the viewport device selector at first. In bug 1247302, it was determined that the default enabled set should include: # Phone Apple iPhone 5s Apple iPhone 6s Apple iPhone 6Plus Google Nexus 4 Google Nexus 5 Google Nexus 6 Samsung Galaxy S5 Samsung Galaxy S6 Samsung Galaxy Note 3 Nokia Lumia 520 # Tablet Apple iPad Air 2 Apple iPad Mini 2 Google Nexus 7 Amazon Kindle Fire HDX 8.9 # Laptops Laptop (1366 x 768) Laptop (1920 x 1080) This work might occur in this bug, or another that has not yet been filed.

Comment on attachment 8737319 [details] [diff] [review] 1241720.patch [1.0] Seems like there are some weird overflowing issues at smaller sizes. (See overflow.png)

Comment on attachment 8737319 [details] [diff] [review] 1241720.patch [1.0] Review of attachment 8737319 [details] [diff] [review]: ----------------------------------------------------------------- Some things that seem incorrect compared to Helen's UX notes from bug 1241713: * Notes say "can still see top of viewport a bit" (I am not sure what this really supposed to mean... is the modal supposed to be off center?) * "The select box should display the devices alphabetically." With the basic device list, they were sorted in the device select drop down, but that is no longer true with this patch applied. Please add some browser tests to interact with the modal and verify the device drop down has the expected things. Good progress so far! ::: devtools/client/locales/en-US/responsive.properties @@ +11,5 @@ > # A good criteria is the language in which you'd find the best > # documentation on web development on the web. > +# LOCALIZATION NOTE (responsive.editDeviceList): option displayed in the device > +# selector > +responsive.editDeviceList=Edit list... Use the ellipsis character https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_Unicode_characters_over_their_ASCII_counterparts_when_possible ::: devtools/client/responsive.html/actions/index.js @@ +29,5 @@ > "RESIZE_VIEWPORT", > // Rotate the viewport. > "ROTATE_VIEWPORT", > + // Update the device display state in the device selector. > + "UPDATE_DEVICE_ENABLED", This becomes "...DISPLAYED" given the name change to the store state. ::: devtools/client/responsive.html/app.js @@ +27,5 @@ > onExit: PropTypes.func.isRequired, > }, > + getInitialState() { > + return { > + isDeviceModalOpen: false, Why do you want component state here? It could be part of the store, perhaps in `devices.isModalOpen`. ::: devtools/client/responsive.html/components/device-modal.js @@ +13,5 @@ > + > + displayName: "DeviceModal", > + > + propTypes: { > + isOpen: PropTypes.bool.isRequired, Maybe this sorts below `devices` in this list? @@ +90,5 @@ > + id: "device-close-button", > + className: "toolbar-button devtools-button", > + onClick: onDeviceModalClose, > + }), > + dom.div({ Nit: Move brace down and indent options object so that braces are on the same vertical line as the children @@ +98,5 @@ > + return dom.div( > + { > + className: "device-type", > + }, > + dom.header({ Nit: dom.header( { className: "device-header", }, type ), @@ +123,5 @@ > + }) > + ); > + }) > + ), > + dom.button({ Nit: dom.button( { id: "device-submit-button", onClick: this.onDeviceModalSubmit, }, getStr("responsive.done") ) ::: devtools/client/responsive.html/components/device-selector.js @@ +25,3 @@ > onResizeViewport, > } = this.props; > + if (target.value === EDIT_DEVICES) { Give this option an ID and check that, instead of testing the string value. ::: devtools/client/responsive.html/index.css @@ +306,5 @@ > +.device-modal-content { > + display: flex; > + flex-direction: column; > + flex-wrap: wrap; > + height: 550px; So let's say a few weeks from now we add two more phones to the DB. Now the modal does a strange scrolling thing and the done button obscures the new phones. Any thoughts on how to tweak the layout so it responds better when the device DB changes? @@ +345,5 @@ > +.device-input-checkbox { > + margin-right: 5px; > +} > + > +#device-submit-button { This seems like it needs a better text color with the dark theme. ::: devtools/client/responsive.html/index.js @@ +27,5 @@ > const { addViewport } = require("./actions/viewports"); > const { loadSheet } = require("sdk/stylesheet/utils"); > +const Services = require("Services"); > + > +const PresetPref = "devtools.responsive.html.defaultDeviceList"; I'd say use PRESET_PREF style for this const value. Wouldn't "displayedDeviceList" make more sense? Also, something like "DISPLAYED_DEVICES_PERF" for the name as well? @@ +75,5 @@ > return; > } > this.store.dispatch(action); > }, > initDevices() { Let's move initDevices, loadDeviceList, and onDeviceListUpdate to some new devices.js module that you can require() and use here. I'd like to keep this file reasonably focused to booting up the app, so I'd prefer to contain this device logic elsewhere especially since it's growing larger. What's a good place in the file tree for such a module? A sibling of this file? In a new directory like "io" (because read and writes stuff) or "effects" (name borrowed from functional programming framworks)? @@ +82,5 @@ > GetDevices().then(devices => { > for (let type of devices.TYPES) { > this.dispatch(addDeviceType(type)); > for (let device of devices[type]) { > if (device.os != "fxos") { Use an "early return" style here: if (device.os == "fxos") { continue; } to save one level of indentation. @@ +83,5 @@ > for (let type of devices.TYPES) { > this.dispatch(addDeviceType(type)); > for (let device of devices[type]) { > if (device.os != "fxos") { > + let newDevice = { I think this can be less verbose as something like: let newDevice = Object.assign({}, device, { enabled: ... }); @@ +91,5 @@ > + pixelRatio: device.pixelRatio, > + userAgent: device.userAgent, > + touch: device.touch, > + os: device.os, > + enabled: deviceList.hasOwnProperty(device.name) ? So, what's going to happen here after the following steps: 1. We have a device list with: * Foo (featured = false) * Bar (featured = false) * Bob (featured = true) 2. User uses modal to enable device Bar 3. We update the device list to be: * Foo (featured = true) * Bar (featured = false) * Bob (featured = false) Based on these steps, I would expect the device select list to contain on Foo and Bar at the end, but I feel like that's not what happens since we're saving the entire set in prefs. Would you expect a different outcome? Either way, we definitely need tests to cover this case of what happens after the device list changes featured states. @@ +113,5 @@ > + if (Services.prefs.prefHasUserValue(PresetPref)) { > + try { > + deviceList = JSON.parse(Services.prefs.getCharPref(PresetPref)); > + } catch (e) { > + // User pref is malformated. Nit: Let's not carry over typos from our Parisian team members ;) ::: devtools/client/responsive.html/types.js @@ +33,3 @@ > os: PropTypes.String, > + // Whether or not the device is displayed in the device selector > + enabled: PropTypes.bool, Your comment uses the word displayed, and I think that's a better word for what it means, so let's change it to "displayed" instead of "enabled".

Comment on attachment 8737319 [details] [diff] [review] 1241720.patch [1.0] Review of attachment 8737319 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/locales/en-US/responsive.properties @@ +11,5 @@ > # A good criteria is the language in which you'd find the best > # documentation on web development on the web. > +# LOCALIZATION NOTE (responsive.editDeviceList): option displayed in the device > +# selector > +responsive.editDeviceList=Edit list... Fixed. ::: devtools/client/responsive.html/actions/index.js @@ +29,5 @@ > "RESIZE_VIEWPORT", > // Rotate the viewport. > "ROTATE_VIEWPORT", > + // Update the device display state in the device selector. > + "UPDATE_DEVICE_ENABLED", Fixed. ::: devtools/client/responsive.html/app.js @@ +27,5 @@ > onExit: PropTypes.func.isRequired, > }, > + getInitialState() { > + return { > + isDeviceModalOpen: false, Fixed. ::: devtools/client/responsive.html/components/device-modal.js @@ +13,5 @@ > + > + displayName: "DeviceModal", > + > + propTypes: { > + isOpen: PropTypes.bool.isRequired, Fixed. @@ +90,5 @@ > + id: "device-close-button", > + className: "toolbar-button devtools-button", > + onClick: onDeviceModalClose, > + }), > + dom.div({ Fixed. @@ +98,5 @@ > + return dom.div( > + { > + className: "device-type", > + }, > + dom.header({ Fixed. @@ +123,5 @@ > + }) > + ); > + }) > + ), > + dom.button({ Fixed. ::: devtools/client/responsive.html/components/device-selector.js @@ +25,3 @@ > onResizeViewport, > } = this.props; > + if (target.value === EDIT_DEVICES) { So, I moved to using an ID-like string as the option value instead of "Edit list...". I prefer this approach since it is easier than going through <select>'s selectedIndex and getting the id of the option. ::: devtools/client/responsive.html/index.css @@ +306,5 @@ > +.device-modal-content { > + display: flex; > + flex-direction: column; > + flex-wrap: wrap; > + height: 550px; Need to give some more thoughts on this. @@ +345,5 @@ > +.device-input-checkbox { > + margin-right: 5px; > +} > + > +#device-submit-button { Fixed. ::: devtools/client/responsive.html/index.js @@ +27,5 @@ > const { addViewport } = require("./actions/viewports"); > const { loadSheet } = require("sdk/stylesheet/utils"); > +const Services = require("Services"); > + > +const PresetPref = "devtools.responsive.html.defaultDeviceList"; Fixed. @@ +75,5 @@ > return; > } > this.store.dispatch(action); > }, > initDevices() { Moves this to ./devices.js @@ +82,5 @@ > GetDevices().then(devices => { > for (let type of devices.TYPES) { > this.dispatch(addDeviceType(type)); > for (let device of devices[type]) { > if (device.os != "fxos") { Fixed. @@ +83,5 @@ > for (let type of devices.TYPES) { > this.dispatch(addDeviceType(type)); > for (let device of devices[type]) { > if (device.os != "fxos") { > + let newDevice = { Fixed. @@ +91,5 @@ > + pixelRatio: device.pixelRatio, > + userAgent: device.userAgent, > + touch: device.touch, > + os: device.os, > + enabled: deviceList.hasOwnProperty(device.name) ? Fixed. I used an array to hold the device names to be displayed, so updates to the device list will just see the featured property and add it into the array. @@ +113,5 @@ > + if (Services.prefs.prefHasUserValue(PresetPref)) { > + try { > + deviceList = JSON.parse(Services.prefs.getCharPref(PresetPref)); > + } catch (e) { > + // User pref is malformated. Fixed. ::: devtools/client/responsive.html/types.js @@ +33,3 @@ > os: PropTypes.String, > + // Whether or not the device is displayed in the device selector > + enabled: PropTypes.bool, Fixed.

Unit tests will come in a part 2.

Comment on attachment 8739521 [details] [diff] [review] 1241720.patch [2.0] Review of attachment 8739521 [details] [diff] [review]: ----------------------------------------------------------------- The top level notes from my previous review seem unfixed (or you did not reply to say why they are left the same): * Notes say "can still see top of viewport a bit" (I am not sure what this really supposed to mean... is the modal supposed to be off center?) * "The select box should display the devices alphabetically." With the basic device list, they were sorted in the device select drop down, but that is no longer true with this patch applied. Here are other remaining issues as well: * Device modal still appears to overflow and behave strangely if one more phone is added (you replied you would give some more thought to it) * At the moment, it seems I can't **add** devices to the device selector by checking them in the modal, but **removing** by unchecking them does work * If I close and re-open RDM after **adding** a device by checking it, then the entire device selector is blank after it re-opens ::: devtools/client/responsive.html/components/device-modal.js @@ +73,5 @@ > + onDeviceListUpdate(displayedDeviceList); > + onUpdateDeviceModalOpen(false); > + }, > + > + render() { React dev build logs: "Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `DeviceModal`. See https://fb.me/react-warning-keys for more information." @@ +99,5 @@ > + { > + className: "device-modal-content", > + }, > + devices.types.map(type => { > + return dom.div( It could be the device type elements (which have no key per type)... @@ +110,5 @@ > + }, > + type > + ), > + devices[type].map(device => { > + return dom.label( ...and / or the device elements (which have no key per device) @@ +117,5 @@ > + }, > + dom.input({ > + className: "device-input-checkbox", > + type: "checkbox", > + key: { What is the purpose of the `key` object here? ::: devtools/client/responsive.html/components/device-selector.js @@ +8,5 @@ > const { DOM: dom, createClass, PropTypes, addons } = > require("devtools/client/shared/vendor/react"); > > const Types = require("../types"); > +const OPEN_DEVICE_MODAL_ID = "OPEN_DEVICE_MODAL"; Let's call this *_VALUE since it's a value and not an ID. ::: devtools/client/responsive.html/devices.js @@ +12,5 @@ > + > +/** > + * Get the device catalog and load the devices onto the store. > + * > + * @param {Function} dispatch Nit: only one space after @param @@ +27,5 @@ > + continue; > + } > + > + let newDevice = Object.assign({}, device, { > + displayed: deviceList.includes(device.name) ? Good, this logic seems like it would handle future featured devices more correctly. However, we definitely want to have tests to be sure of what it will do when the device DB is changed. @@ +33,5 @@ > + !!device.featured, > + }); > + > + if (newDevice.displayed) { > + deviceList.push(newDevice.name); This doesn't seem like it will work correctly... We're inside a .then() here, and the only use of `deviceList` is outside the .then(). Do you need to call `updateDeviceList` again perhaps? I am not sure what you intended. @@ +68,5 @@ > + > +/** > + * Update the displayed device list preference with the given device list. > + * > + * @param {Array} devices Nit: only one space after @param ::: devtools/client/responsive.html/index.css @@ +6,5 @@ > */ > > .theme-light { > --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); > + --submit-button-hover-background-color: rgba(170,170,170,.2); Do these match any existing variables? You should double check with Helen on these colors, through ui-review / ni. @@ +17,5 @@ > } > > .theme-dark { > --box-shadow: 0 4px 4px 0 rgba(105, 105, 105, 0.26); > + --submit-button-hover-background-color: hsla(206,37%,4%,.2); Either use decimal or percents, not a mix in the same color expression. ::: devtools/client/responsive.html/test/unit/test_update_device_displayed.js @@ +7,5 @@ > + > +const { > + addDevice, > + addDeviceType, > + updateDeviceEnabled, enabled -> displayed

Comment on attachment 8739521 [details] [diff] [review] 1241720.patch [2.0] Review of attachment 8739521 [details] [diff] [review]: ----------------------------------------------------------------- * Notes say "can still see top of viewport a bit" (I am not sure what this really supposed to mean... is the modal supposed to be off center?) > I talked to Helen about this. There is still quite a bit of UX issues with the original proposal and part of this would be fixed when we absolutely position the global toolbar. Here are some of the edge cases that we still need to talk with the UX about the device modal: - Where would the device modal go if we have multi-viewports? - Should pointer events in the background still be allowed? - Do we have an overlay to cover everything up and force the focus to the modal? - For small viewports, the modal would be aligned below the midpoint of the screen if it was aligned to cover up the device selector We agreed to fix these UX issues in a followup. * "The select box should display the devices alphabetically." With the basic device list, they were sorted in the device select drop down, but that is no longer true with this patch applied. > The select box was not originally sorted alphabetically. It simply gets the device list and loads up the devices in order of the types. This should be fixed in a separate bug. Here are other remaining issues as well: * Device modal still appears to overflow and behave strangely if one more phone is added (you replied you would give some more thought to it) > I am added an overflow for the modal content. So, this case should be somewhat mitigated. I would still like to figure out how to make the modal dynamically sized in a follow up. * At the moment, it seems I can't **add** devices to the device selector by checking them in the modal, but **removing** by unchecking them does work > Should be fixed. * If I close and re-open RDM after **adding** a device by checking it, then the entire device selector is blank after it re-opens > Should be fixed. ::: devtools/client/responsive.html/components/device-modal.js @@ +73,5 @@ > + onDeviceListUpdate(displayedDeviceList); > + onUpdateDeviceModalOpen(false); > + }, > + > + render() { Fixed. @@ +99,5 @@ > + { > + className: "device-modal-content", > + }, > + devices.types.map(type => { > + return dom.div( Fixed. @@ +110,5 @@ > + }, > + type > + ), > + devices[type].map(device => { > + return dom.label( Fixed. @@ +117,5 @@ > + }, > + dom.input({ > + className: "device-input-checkbox", > + type: "checkbox", > + key: { Fixed. Removed key here. ::: devtools/client/responsive.html/components/device-selector.js @@ +8,5 @@ > const { DOM: dom, createClass, PropTypes, addons } = > require("devtools/client/shared/vendor/react"); > > const Types = require("../types"); > +const OPEN_DEVICE_MODAL_ID = "OPEN_DEVICE_MODAL"; Fixed. ::: devtools/client/responsive.html/devices.js @@ +12,5 @@ > + > +/** > + * Get the device catalog and load the devices onto the store. > + * > + * @param {Function} dispatch I prefer to leave 2 spaces to be make sure everything lines up correctly if there was a @returns {Object} in the following line. @@ +27,5 @@ > + continue; > + } > + > + let newDevice = Object.assign({}, device, { > + displayed: deviceList.includes(device.name) ? Added Tests. @@ +33,5 @@ > + !!device.featured, > + }); > + > + if (newDevice.displayed) { > + deviceList.push(newDevice.name); Fixed. @@ +68,5 @@ > + > +/** > + * Update the displayed device list preference with the given device list. > + * > + * @param {Array} devices Same reasoning as above. ::: devtools/client/responsive.html/index.css @@ +6,5 @@ > */ > > .theme-light { > --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); > + --submit-button-hover-background-color: rgba(170,170,170,.2); This matches the devtools tab hover and active colors. I tried to derive some variables out of it. Originally, I kept the values exactly to what is found in toolbars.css, so it can be found if a search was performed. ::: devtools/client/responsive.html/test/unit/test_update_device_displayed.js @@ +7,5 @@ > + > +const { > + addDevice, > + addDeviceType, > + updateDeviceEnabled, Fixed.

Comment on attachment 8742446 [details] [diff] [review] 1241720.patch [3.0] Review of attachment 8742446 [details] [diff] [review]: ----------------------------------------------------------------- :bgrins, can you review the new theme variables in here, specifically devtools/client/themes/toolbars.css and usages in other CSS files? The feature seems to be working well now and the tests look good, great work! The CSS variables are the only thing I am not sure about, so hopefully :bgrins can advise there. Also, needs a ui-review+ from Helen before landing, so I went ahead and requested that. Side note: I would really prefer future large-ish reviews like this in some kind of easy-to-interdiff format. Splinter's interdiff fails as usual... MozReview is great about this, maybe worth a try for next time (though I think you have to be careful not to rebase in between review rounds). I was able to compare the patches manually, so at least there's that option. (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #11) > Comment on attachment 8739521 [details] [diff] [review] > 1241720.patch [2.0] > > Review of attachment 8739521 [details] [diff] [review]: > ----------------------------------------------------------------- > > * Notes say "can still see top of viewport a bit" (I am not sure what this > really supposed to mean... is the modal supposed to be off center?) > > I talked to Helen about this. There is still quite a bit of UX issues with the original proposal and part of this would be fixed when we absolutely position the global toolbar. Here are some of the edge cases that we still need to talk with the UX about the device modal: > - Where would the device modal go if we have multi-viewports? > - Should pointer events in the background still be allowed? > - Do we have an overlay to cover everything up and force the focus to the > modal? > - For small viewports, the modal would be aligned below the midpoint of the > screen if it was aligned to cover up the device selector > > We agreed to fix these UX issues in a followup. Okay, please file a follow up bug for this before landing. > * "The select box should display the devices alphabetically." With the > basic device list, they were sorted in the device select drop down, but that > is no longer true with this patch applied. > > The select box was not originally sorted alphabetically. It simply gets the device list and loads up the devices in order of the types. This should be fixed in a separate bug. Same here, please file a follow up bug for this before landing. > Here are other remaining issues as well: > > * Device modal still appears to overflow and behave strangely if one more > phone is added (you replied you would give some more thought to it) > > I am added an overflow for the modal content. So, this case should be somewhat mitigated. I would still like to figure out how to make the modal dynamically sized in a follow up. Great, this seems like a decent short term fix at least. ::: devtools/client/jsonview/css/tabs.css @@ +141,5 @@ > } > > .theme-dark .tabs .tabs-menu-item:hover, > .theme-light .tabs .tabs-menu-item:hover { > + background-color: rgba(170, 170, 170, .2); /* --devtools-tab-hover */ Are the new variables not accessible here? There's another variable down in the next rule set, so it seems like it should be accessible... ::: devtools/client/responsive.html/devices.js @@ +16,5 @@ > + * > + * @param {Function} dispatch > + * Action dispatch function > + */ > +function initDevices(dispatch) { I think it's better to use Task.async for a case like this, so there's less `.then()` everywhere. It returns a promise, so that case will be handled as well. So, something like: let initDevices = Task.async(function* () { let deviceList = loadDeviceList(); let devices = yield GetDevices(); <use devices without .then()> updateDeviceList(deviceList); }); ::: devtools/client/responsive.html/index.css @@ +6,5 @@ > */ > > .theme-light { > --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); > + --submit-button-active-background-color: rgba(0,0,0,0.12); In toolbars.css, this appears to be used[1] as the background for .devtools-sidebar-tabs tabs > tab:hover. Should it be a variable? [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#588 @@ +16,5 @@ > } > > .theme-dark { > --box-shadow: 0 4px 4px 0 rgba(105, 105, 105, 0.26); > + --submit-button-active-background-color: hsla(206,37%,4%,.4); Isn't this --devtools-tab-hover-active?

Comment on attachment 8742446 [details] [diff] [review] 1241720.patch [3.0] A few nits: - Original design spec showed the global toolbar and a bit of the viewport peaking above the modal to ground the user. Can we get that back in with a margin-top of some sort? - For the <select>: can we do somewhere between 3-5px left-padding on the items? They hug the left side really tightly, looks like they need breathing room. - I think it'd be nice to have the modal 'appear' in some way. I'm asking Fang for some more info but if we could reverse-engineer the "slide-in (bottom)" effect from here I think it would add a lot: http://tympanus.net/Development/ModalWindowEffects/

:helenvholmes, could you also take a look at the theme variables here like you mentioned in the standup? If you feel comfortable reviewing those changes, we can drop the review from :bgrins queue.

Comment on attachment 8742446 [details] [diff] [review] 1241720.patch [3.0] Review of attachment 8742446 [details] [diff] [review]: ----------------------------------------------------------------- I think colors in devtools are getting kinda unwieldy, but this patch isn't the place to deal with that particular problem. You can leave Brian flagged for review if you want, but I'm hoping to chat with him about the larger-css-color-naming problem when he gets back. We also chatted about the potential for the viewport being larger than the modal and what should happen in that situation. I think if the viewport is (for whatever reason) very large, the modal should hug the top, the viewport should blur out, and pointer events for the viewport should freeze until the device modal is closed: http://cl.ly/1w43071f0R1u ::: devtools/client/jsonview/css/toolbar.css @@ +78,5 @@ > text-shadow: none; > margin: 1px 2px 1px 2px; > border: none; > border-radius: 0; > + background-color: rgba(170, 170, 170, .2); /* Splitter, --devtools-tab-hover */ I think this splitter might be the same as the previous splitter comment (same with line 93). The --devtools-tab-hover comment is probably descriptive enough. ::: devtools/client/responsive.html/index.css @@ +318,5 @@ > + */ > + > +.device-modal { > + border-radius: 2px; > + box-shadow: var(--box-shadow); Can we prefix this with --rdm-, or something of the like? I think this box-shadow might end up being shared in a lot of places too; might be a candidate to end up in variables.css. @@ +345,5 @@ > +} > + > +#device-close-button, > +#device-close-button::before { > + position: absolute; Any reason you're not positioning this with flexbox? I'm sure the position: absolute isn't doing any harm, just curious. ::: devtools/client/themes/toolbars.css @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* CSS Variables specific to the devtools toolbar that aren't defined by the themes */ > .theme-light { > + --devtools-tab-hover: rgba(170, 170, 170, .2); Can we prefix these with "toolbar" instead of "devtools"? I think "devtools" is a little too general. @@ +241,5 @@ > /* Text-only buttons */ > .theme-light .devtools-toolbarbutton[label]:not([text-as-image]):not([type=menu-button]), > .theme-light .devtools-toolbarbutton[data-text-only], > .theme-light #toolbox-buttons .devtools-toolbarbutton[text-as-image] { > + background-color: var(--devtools-tab-hover); /* Splitter */ I think this splitter comment needs to be removed; doesn't seem to be true anymore, and it's mapped to a variable so we can see the backtrace anyway.

Comment on attachment 8742446 [details] [diff] [review] 1241720.patch [3.0] Unflagged Brian for review for now—if Brian has additional suggestions when he's back next week we can address those outside of this patch.

https://hg.mozilla.org/mozilla-central/rev/be1fd1c7e43f https://hg.mozilla.org/mozilla-central/rev/a6623a30d98d

I managed to test this bug on Firefox 49.0a1 (2016-04-28) and on Windows 10 x86, Ubuntu 14.04 x86 and Mac OS X 10.9.5. While testing, I found a few issues: - while the modal is opened, the top of the viewport is correctly displayed only on Mac OS X. On Windows and Ubuntu OS's, the device list can be accessed while the large DB is opened. - On Ubuntu and Windows OS's, the scrollbars are not correctly positioned. The scrollbars should be displayed on the edge of the modal. The vertical scrollbar should be positioned under the 'X' button. - On windows, the horizontal scrollbar is unnecessary. - There are no floating scrollbars on Windows in the Device list or on the modal. - If in the device list are enabled a large no. of devices and the Edit list... button is selected, after closing the modal page and click the 'no device selected' button, the end of the list is displayed, instead of the top. Also, I think that it would be useful to have a Select/Deselect All button in the modal. I am attaching a screenshot with the issues and the differences between OS's. Ryan, should I log the issues described above?

(In reply to Mihai Boldan, QA [:mboldan] from comment #22) > - while the modal is opened, the top of the viewport is correctly displayed > only on Mac OS X. On Windows and Ubuntu OS's, the device list can be > accessed while the large DB is opened. Helen, maybe you have some input about this one? It looks like you suggested additional changes in comment 15: "We also chatted about the potential for the viewport being larger than the modal and what should happen in that situation. I think if the viewport is (for whatever reason) very large, the modal should hug the top, the viewport should blur out, and pointer events for the viewport should freeze until the device modal is closed: http://cl.ly/1w43071f0R1u" It doesn't seem like those were implemented. Should we have a follow up to continue to improve the behavior here?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23) > (In reply to Mihai Boldan, QA [:mboldan] from comment #22) > > - while the modal is opened, the top of the viewport is correctly displayed > > only on Mac OS X. On Windows and Ubuntu OS's, the device list can be > > accessed while the large DB is opened. > > Helen, maybe you have some input about this one? It looks like you > suggested additional changes in comment 15: > > "We also chatted about the potential for the viewport being larger than the > modal and what should happen in that situation. I think if the viewport is > (for whatever reason) very large, the modal should hug the top, the viewport > should blur out, and pointer events for the viewport should freeze until the > device modal is closed: http://cl.ly/1w43071f0R1u" > > It doesn't seem like those were implemented. Should we have a follow up to > continue to improve the behavior here? Filing a follow-up bug sounds like a good idea. Does that properly capture the issue Mihai is seeing, though?

(In reply to Mihai Boldan, QA [:mboldan] from comment #22) > - On Ubuntu and Windows OS's, the scrollbars are not correctly positioned. > The scrollbars should be displayed on the edge of the modal. The vertical > scrollbar should be positioned under the 'X' button. Yes, let's file an issue for this. > - On windows, the horizontal scrollbar is unnecessary. I think we should at least ensure the modal is wide enough so that horizontal scrolling isn't needed. We may have vertical scrolling if the list grows, but we could also try to avoid that too at least for the current device set. There's also bug 1266416 to investigate better sizing for the modal. > - There are no floating scrollbars on Windows in the Device list or on the > modal. I think that's okay, we only really want floating scrollbars in the viewport to avoid covering page content. If scrollbars appear in the modal or elsewhere, they should be "regular" scrollbars you expect on the OS. But hopefully we can avoid needed scrollbars for the modal as mentioned above. > - If in the device list are enabled a large no. of devices and the Edit > list... button is selected, after closing the modal page and click the 'no > device selected' button, the end of the list is displayed, instead of the > top. Ah, I see what you mean. Let's file an issue for this, to at least discuss what we'd like to do. > Also, I think that it would be useful to have a Select/Deselect All button > in the modal. I think we'll wait to see if we get more requests for this. We're assuming people only want a few "frequently used" devices to be enabled, so I'm not sure how many would use such buttons yet.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #24) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23) > > (In reply to Mihai Boldan, QA [:mboldan] from comment #22) > > > - while the modal is opened, the top of the viewport is correctly displayed > > > only on Mac OS X. On Windows and Ubuntu OS's, the device list can be > > > accessed while the large DB is opened. > > > > Helen, maybe you have some input about this one? It looks like you > > suggested additional changes in comment 15: > > > > "We also chatted about the potential for the viewport being larger than the > > modal and what should happen in that situation. I think if the viewport is > > (for whatever reason) very large, the modal should hug the top, the viewport > > should blur out, and pointer events for the viewport should freeze until the > > device modal is closed: http://cl.ly/1w43071f0R1u" > > > > It doesn't seem like those were implemented. Should we have a follow up to > > continue to improve the behavior here? > > Filing a follow-up bug sounds like a good idea. Does that properly capture > the issue Mihai is seeing, though? Well, it sounded like your suggestion would alter the appearance of what's seen behind the modal, which I think is what Mihai's describing? At the very least, it would make it be different, so we'd probably want him to take another look after such a change.

I filed bug 1269441 for the modal UI follow up work as discussed in comment 24.

I've logged Bug 1269739 and Bug 1269750 for the issues described in Comment 25. Since there were logged separate bugs for the found issues, I am marking this Bug Verified Fixed.