UX Notes: - Entering responsive design and exiting responsive design should be a toggle by clicking on the phone button that sits in the command button area. I'll be attaching the icon a little later today, chatting with Shorlander about icons.

The legacy RD tool does not require a toolbox to be open when using it. If a command button is used to exit RD, does that mean we are implicitly requiring a toolbox to always be open in the new version? One argument for not requiring the toolbox is that it takes up lots of vertical space (when docked at the bottom) which covers page content, if you simple want to view the page at a specific size.

Ryan brought up a good point, so I've updated the UX notes. Transcribed here: UX Notes: - Entering responsive design and exiting responsive design should be a toggle by clicking on the phone button that sits in the command button area. - There is a _second_ way to leave the responsive design mode via the global tab bar 'x'. This is because a user can enter the responsive design mode independently of the developer tools. On exit, I think we should default to the Inspector as the open panel if the devtools are open.

I would split then this bug in two, then. I will keep this bug to add the button in the command button area; then create a follow up bug that implements the "X" on the global tab bar – and depends by its implementation.

MozReview-Commit-ID: A2ABbIJe6Fb

Comment on attachment 8720848 [details] [diff] [review] Close button for exiting the responsive design view Review of attachment 8720848 [details] [diff] [review]: ----------------------------------------------------------------- Overall it works well! A few styling things to work out for the next round. Has someone made a separate bug to update the command button icon? I think :helenvholmes mentioned there might be one...? ::: devtools/client/responsive.html/app.js @@ +32,3 @@ > } = this.props; > > + return dom.div(null, Let's use {} instead of null ::: devtools/client/responsive.html/components/global-toolbar.js @@ +19,5 @@ > + let { > + onExit, > + } = this.props; > + > + return dom.header({ I know it's somewhat unusual, but I think it works after you get used to it... Use the style from other components: return dom.header( { id: }, ... children ); @@ +23,5 @@ > + return dom.header({ > + id: "global-toolbar", > + className: "toolbar", > + }, > + "responsive design mode", Create a new devtools/client/locales/en-US/responsive.properties and place the string there. @@ +28,5 @@ > + dom.span({ > + className: "placeholder", > + }), > + dom.button({ > + id: "global-close-button", I think "exit" is better than "close" for this button. We can use "close" later when we need to close a single viewport. So, let's call this "global-exit-button". ::: devtools/client/responsive.html/images/moz.build @@ +4,5 @@ > # 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/. > > DevToolsModules( > + 'close.svg', Rename to exit.svg. ::: devtools/client/responsive.html/index.css @@ +5,5 @@ > * CSS Variables specific to the responsive design mode > */ > > .theme-light { > + --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); You need to update the old usage of --viewport-box-shadow @@ +16,5 @@ > * { > box-sizing: border-box; > } > > + Nit: remove extra blank line @@ +48,5 @@ > /* Individual viewports are inline elements, make sure they stay on a single > line */ > white-space: nowrap; > + > + /* Make sure the single viewports are centered */ What effect does this have? @@ +158,5 @@ > cursor: s-resize; > } > + > +/** > + * Global Toolbar Let's put this section above the viewports, since that's where the toolbar appears vertically on the page. @@ +179,5 @@ > + padding-right: 4px; > + margin-right: 4px; > +} > + > +#global-toolbar > .placeholder { My impression from the UX notes is that the global toolbar should be only as wide as needed to fit what it contains, so that's currently the tool name, divider line, and close button. Currently it's following the width of the viewport. So, the placeholder seems unnecessary here. @@ +183,5 @@ > +#global-toolbar > .placeholder { > + flex-grow: 1; > +} > + > +#global-toolbar .toolbar-button { Let's share whatever styles here are duplicated from .viewport-toolbar-button by using a common class. @@ +199,5 @@ > +#global-toolbar .toolbar-button:hover { > + opacity: 1; > +} > + > +#global-toolbar .toolbar-button:active { Probably wants the same styles as .viewport-toolbar-button:active @@ +204,5 @@ > + background-color: #468EE5; > +} > + > +#global-close-button { > + mask-image: url("./images/close.svg"); Change to mask. ::: devtools/client/responsive.html/index.js @@ +34,5 @@ > // See bug 1242057. > this.telemetry.toolOpened("responsive"); > let store = this.store = Store(); > + let app = App({ > + onExit: () => window.postMessage({type: "EXIT"}, "*"), Let's use "exit", like an event name. ::: devtools/client/responsive.html/manager.js @@ +196,5 @@ > tabBrowser.loadURI(TOOL_URL); > yield tabLoaded(this.tab); > let toolWindow = this.toolWindow = tabBrowser.contentWindow; > toolWindow.addInitialViewport(contentURI); > + addMessageListener(this); Hmm, the name `addMessageListener` confuses me a bit, since it's a global with a different meaning inside frame scripts. Maybe instead use `toolWindow.addEventListener("message", this)` and define a `handleEvent` on the object? ::: devtools/client/responsive.html/test/browser/browser.ini @@ +3,5 @@ > subsuite = devtools > support-files = > head.js > > +[browser_close_button.js] exit_button

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > Has someone made a separate bug to update the command button icon? I think > :helenvholmes mentioned there might be one...? Ideally the command button would be updated by bug 1174395, but we decided yesterday to use the png currently, so we won't block anything. I'll do that in this bug, but as separate patch. > ::: devtools/client/responsive.html/app.js > @@ +32,3 @@ > > } = this.props; > > > > + return dom.div(null, > Let's use {} instead of null I was tempted to do so initially, but I saw in the react tutorial `null`; I've updated the code. > I know it's somewhat unusual, but I think it works after you get used to > it... Use the style from other components: > > return dom.header( > { > id: > }, > ... children > ); But this style is only for react attributes, right? I saw that the creation of components still have the other indentation. Let me know if now the style is correct in the new patch. > Create a new devtools/client/locales/en-US/responsive.properties and place > the string there. I created the bundle directly in global-toolbar, but I guess we should create a separate module to import in any component needs localisation – for e.g. also the rotate button should have a tooltip. I was thinking to create a `l10n` module, but I'm not sure where to put it in the current structure. Any suggestion? We could also doing so in a follow up bug, I don't mind. Also, in the file's comment I used "Responsive Design Mode" even if the menu is not updated yet, but I think we agreed to use this name everywhere, right? > ::: devtools/client/responsive.html/images/moz.build > @@ +4,5 @@ > > # 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/. > > > > DevToolsModules( > > + 'close.svg', > > Rename to exit.svg. So, I renamed the button – agreed that exit would be better since is global – but I kept the `close.svg` name here. That's because we're going to have the same image for the close button for each viewport. Unless we're going to duplicate the assets, I think "close" it's a good name for both. > > .theme-light { > > + --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); > > You need to update the old usage of --viewport-box-shadow I actually removed intentionally the "viewport" prefix, I didn't know previously was without. That's because this shadow is not used only for the viewport, in fact we're using it for the global toolbar that is not related to that. So if we want to add a prefix, maybe "responsive" would be better? In the new patch I didn't modify this part. > > + /* Make sure the single viewports are centered */ > > What effect does this have? ensure that the viewports are centered, even if the global-toolbar grows more than the width of the viewport. To do so, I added the display and the justify-content. However that is not necessary anymore since the global-toolbar is not related to the viewport's with as before. > ::: devtools/client/responsive.html/index.js > @@ +34,5 @@ > > // See bug 1242057. > > this.telemetry.toolOpened("responsive"); > > let store = this.store = Store(); > > + let app = App({ > > + onExit: () => window.postMessage({type: "EXIT"}, "*"), > > Let's use "exit", like an event name. I used the `type` and the uppercase to mimic the action in case we wanted to make it as action in the future, but I updated to lowercase. > Hmm, the name `addMessageListener` confuses me a bit, since it's a global > with a different meaning inside frame scripts. Yeah, I wasn't sure too. > Maybe instead use `toolWindow.addEventListener("message", this)` and define > a `handleEvent` on the object? That's looks much better, thanks! As I mentioned in the stand up, I modified slightly the HTML structure, removing the <div id="app"> from the HTML, keeping only the <body>. That's because the `App` component is creating an additional <div> – that is the actual app – inside the <div id="app">. So we're going to end up with body > div#app > div > (content) so basically the anonymous div in the middle is redundant, and makes the styling a little bit more verbose. So I removed the div#app from the HTML, adding the id "app" to the <div> created by `App`, and using as mounted point the <body>.

MozReview-Commit-ID: LsiqLWiDmd

MozReview-Commit-ID: C08sHwjSMQ6

Comment on attachment 8722553 [details] [diff] [review] Close button for exiting the responsive design view Sorry for the mess! I forgot to fix the additional blank line and then I marked as obsoleted the wrong one.

Comment on attachment 8722553 [details] [diff] [review] Close button for exiting the responsive design view Review of attachment 8722553 [details] [diff] [review]: ----------------------------------------------------------------- Getting closer! :) I'd still like to see another round, and let's also get a ui-review from Helen (either now or on the next version). ::: devtools/client/locales/en-US/responsive.properties @@ +10,5 @@ > +# You want to make that choice consistent across the developer tools. > +# A good criteria is the language in which you'd find the best > +# documentation on web development on the web. > + > + Nit: remove second blank line @@ +13,5 @@ > + > + > +# LOCALIZATION NOTE (responsive.title): the title displayed in the global > +# toolbar > +responsive.title=Responsive Design Mode This text is all lowercase instead of title case in Helen's mockups. Please check with her about which to use. @@ +16,5 @@ > +# toolbar > +responsive.title=Responsive Design Mode > + > +# LOCALIZATION NOTE (responsive.exit): tooltip text of the exit button. > +responsive.exit=Leave Responsive Design Mode Maybe let's say "Exit" instead of "Leave", since we think of it as exit ourselves? Helen, any thoughts? I am not great at reviewing text. :) ::: devtools/client/responsive.html/components/global-toolbar.js @@ +7,5 @@ > +const { > + ViewHelpers > +} = require("resource://devtools/client/shared/widgets/ViewHelpers.jsm"); > +const STRINGS_URI = "chrome://devtools/locale/responsive.properties"; > +const L10N = new ViewHelpers.L10N(STRINGS_URI); Yes, let's make a l10n module in this patch. Since it's likely only going to be imported by UI components, I think it would be fine to have in the components directory. @@ +25,5 @@ > + let { > + onExit, > + } = this.props; > + > + return dom.header( I think we have so far used a consistent style for attributes set on the main DOM node returned by component. That object appears on separate line, with each property on it own line, just like you've done here, so this part seems correct to me. I based my mental style guide on memory components like https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/census-header.js, as I think it makes it easy to follow what is part of the parent vs. children. (I don't think we need the blank lines everywhere that are seen in the memory component example.) I believe the DevTools React group is working on style guides, so things will be simpler after that. @@ +31,5 @@ > + id: "global-toolbar", > + className: "toolbar", > + }, > + L10N.getStr("responsive.title"), > + dom.button( Breaking out the attributes object to separate lines does not matter as much when the node in question does not have children of its own, like this one. I think either of two following choices makes sense: dom.button( { id: "foo", ... }, <children would go here, if there were any> ) or (since there are no children in this case): dom.button({ id: "foo", ... }) so, either move the closing paren down to the next line, or move the object up to the opening paren, and un-indent one level. ::: devtools/client/responsive.html/images/snapshot.svg @@ +1,4 @@ > +<!-- 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/. --> > +<svg xmlns="http://www.w3.org/2000/svg" width="16px" height="16px" viewBox="0 0 16 16"> This file is not part of this bug. ::: devtools/client/responsive.html/index.css @@ +5,5 @@ > * CSS Variables specific to the responsive design mode > */ > > .theme-light { > + --box-shadow: 0 4px 4px 0 rgba(155, 155, 155, 0.26); This seems like a fine name for the moment. I don't think it needs "responsive" since it's only loaded in our responsive tool. However, you still need to update the usage in .resizable-viewport so it keeps the box shadow. @@ +40,5 @@ > + flex-direction: column; > +} > + > +/** > + * Common style for toolbars' button Nit: maybe say "toolbar buttons" @@ +68,5 @@ > +/** > + * Global Toolbar > + */ > + > +#global-toolbar { Looking at the mockups (https://invis.io/9G5R8XCYZ), it looks like there should be more spacing to the left of the "Responsive" text. Also, the spacing for the divider line should be same on both sides. Helen may have more precise details. @@ +107,5 @@ > /* Individual viewports are inline elements, make sure they stay on a single > line */ > white-space: nowrap; > + > + /* Make sure the single viewports are centered */ In your comments, you said this is no longer needed, so seems it should be removed. ::: devtools/client/responsive.html/index.js @@ +40,2 @@ > let provider = createElement(Provider, { store }, app); > + ReactDOM.render(provider, document.body); I did in fact place the app in the body just like this initially, but React adds a warning in the console about it. (You should enable developer mode of React with --enable-debug-js-modules in .mozconfig.) So, we should still keep some element, but it could have a different ID like "root", so that the app component can use "app". ::: devtools/client/responsive.html/test/browser/browser_exit_button.js @@ +2,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test viewports basics after opening, like size and location Update comment

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13) > > +# LOCALIZATION NOTE (responsive.title): the title displayed in the global > > +# toolbar > > +responsive.title=Responsive Design Mode > This text is all lowercase instead of title case in Helen's mockups. Please > check with her about which to use. True, I kept as convention the ResponsiveUI.properties; but the Helen's mockups is lowercase. > > +# LOCALIZATION NOTE (responsive.exit): tooltip text of the exit button. > > +responsive.exit=Leave Responsive Design Mode > > Maybe let's say "Exit" instead of "Leave", since we think of it as exit > ourselves? Helen, any thoughts? I am not great at reviewing text. :) Same as above: I just copied what we currently have in RDM. As soon as we agreed on these things, I will update the new patch.

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #14) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13) > > > > +# LOCALIZATION NOTE (responsive.title): the title displayed in the global > > > +# toolbar > > > +responsive.title=Responsive Design Mode This should be title case. > > This text is all lowercase instead of title case in Helen's mockups. Please > > check with her about which to use. > > True, I kept as convention the ResponsiveUI.properties; but the Helen's > mockups is lowercase. > > > > +# LOCALIZATION NOTE (responsive.exit): tooltip text of the exit button. > > > +responsive.exit=Leave Responsive Design Mode > > > > Maybe let's say "Exit" instead of "Leave", since we think of it as exit > > ourselves? Helen, any thoughts? I am not great at reviewing text. :) Let's do 'close'. I think people will understand what happens after hitting it once.

MozReview-Commit-ID: DLYZcpzRNJR

Comment on attachment 8723952 [details] [diff] [review] Close button for exiting the responsive design view I think I have addressed your comments. A couple of things: 1. I added the l10n module under `components/utils`; to ensure that all the first level module under `components` are, well, just components. 2. Instead of exporting directly the `L10N` instance created, I exported manually the methods of the object. I've done that so that we can have both `const L10N = require('./utils'); L10N.getStr(…)` and `const { getStr } = require('./utils');`. Since L10N is basically a singleton for us, it was making more sense in this way IMVHO. The reason I exported manually instead of iterate the object, it was to be explicit in the module about what it's exporting.

MozReview-Commit-ID: 9UPBhblioih

MozReview-Commit-ID: BnpLSuPOBLp

Comment on attachment 8724089 [details] [diff] [review] Close button for exiting the responsive design view Sorry again, I missed the last couple of your comments; this patch should be OK.

Comment on attachment 8724089 [details] [diff] [review] Close button for exiting the responsive design view Review of attachment 8724089 [details] [diff] [review]: ----------------------------------------------------------------- Just some small CSS tweaks, etc. left. Please also request ui-review and address any feedback from :helenvholmes. ::: devtools/client/responsive.html/index.css @@ +32,4 @@ > } > > #app { > height: 100%; #root also needs height: 100%, add it to the selector list with html and body above. @@ +62,5 @@ > +.toolbar-button:active { > + background-color: var(--theme-selection-background); > + opacity: 1; > +} > + Nit: remove extra blank line @@ +80,5 @@ > + display: inline-flex; > + -moz-user-select: none; > +} > + > +#global-toolbar > :first-child { I think it would easier to read this set of rules if we give this a class or ID instead of using :first-child. @@ +82,5 @@ > +} > + > +#global-toolbar > :first-child { > + border-right: 1px solid var(--theme-splitter-color); > + padding-right: 4px; This seems right at first given the button's 4px margin below, but the button contains embedded whitespace in the graphic. Using 6px here seems to balance it. Please also add: padding-left: 2px; padding-top: 1px; so in total: padding: 1px 6px 0 2px; or similar. I am trying to balance whitespace in both dimensions.

MozReview-Commit-ID: BnpLSuPOBLp

Comment on attachment 8724820 [details] [diff] [review] Close button for exiting the responsive design view Helen, let me know what you think, if there is anything UI related that needs to be addressed.

Comment on attachment 8724820 [details] [diff] [review] Close button for exiting the responsive design view From a UI perspective I don't really see anything to be changed here (since I believe we're updating image assets in other bugs.) However, I have a few UX notes: - Currently when you enter the responsive design mode, the developer tools close. I think that if you entered the responsive design mode with the developer tools, the developer tools should stay open. — Without the global tab bar, there's no way to leave responsive design mode, which is problematic: http://cl.ly/270l3F2B0Y1b Am I right to assume we're addressing this in another bug? — Cmd + Opt + M works, which is great

MozReview-Commit-ID: JUYjsiUUyEf

Comment on attachment 8725221 [details] [diff] [review] Command's icon for RDM Here the patch that replace the current command icon we have for RDM, as we discussed. Helen, I also attached the pngs I used as separate attachment, so you can also check them without applying the patch.

Comment on attachment 8724820 [details] [diff] [review] Close button for exiting the responsive design view A few of my comments got muddled by not applying the patch correctly, so here are some updated comments: - Currently when you enter the responsive design mode, the developer tools close. I think that if you entered the responsive design mode with the developer tools, the developer tools should stay open. (I said this before, but I still think it's true.) - Let's bump up the margins in the global toolbar to 8px instead of 4px to give everything a little bit more space. - The 'x' in the global toolbar feels a little large. I would recommend settings its width and height to 14px and re-centering it. (I notice it's using the .toolbar-button class which the rotate-button is also using; the rotate-button looks like it's a good size, so maybe apply the width/height on this class?: #global-toolbar .toolbar-button)

MozReview-Commit-ID: Fygjgc7FHzF

Comment on attachment 8725288 [details] [diff] [review] Close button for exiting the responsive design view After a chat with helen on IRC, we agreed on keeping the icons as is; just adding more horizontal padding on global toolbar.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #29) > - Currently when you enter the responsive design mode, the developer tools > close. I think that if you entered the responsive design mode with the > developer tools, the developer tools should stay open. (I said this before, > but I still think it's true.) Yes, this is a known issue. It's tracked in bug 1240912. (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #24) > — Without the global tab bar, there's no way to leave responsive design > mode, which is problematic: http://cl.ly/270l3F2B0Y1b Am I right to assume > we're addressing this in another bug? Is this comment outdated after you reapplied the patch? I am not sure what you mean about "without" the global tab bar. Does it disappear?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32) > (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #24) > > — Without the global tab bar, there's no way to leave responsive design > > mode, which is problematic: http://cl.ly/270l3F2B0Y1b Am I right to assume > > we're addressing this in another bug? > > Is this comment outdated after you reapplied the patch? I am not sure what > you mean about "without" the global tab bar. Does it disappear? No, this was a result of me not applying the patch properly and thus not seeing the global toolbar at all. Sorry for the confusion!

The try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=938fa65e434c&selectedJob=17545796 It seems blocked to 88% since a while, I don't think will proceed further. However, the issues doesn't seems related to this patch.

Needs rebasing

I rebased and landed.

Ryan, also the patch you marked as obsoleted needed to be checked in – not sure why was marked as obsolete. I'll ask for new checkin-needed and remove the obsolete flag from it.

Comment on attachment 8725221 [details] [diff] [review] Command's icon for RDM This patch needs to be landed too – only this one, since the other is already landed.

MozReview-Commit-ID: 1vc284i2cWF

Comment on attachment 8732187 [details] [diff] [review] Command's icon for RDM So, apparently the command toolbar's icons have padding in the image itself, that were missing in the new icons. The new icons are resized to considering such margin – notice, not all the image have the same margin, I stuck to the bare minimum that also the previous responsive mode icon had. Got already the ui r+ from Helen.

I managed to test the Close button from RDM. I confirm that the 'X' button and the 'Responsive Design Mode' button from Developer Tools are correctly displayed. Also I found an issue related to Close button. If RDM is enabled and the tab is detached from the current window, the 'X' button does not work any more and TypeError: tab.linkedBrowser is null; manager.js:215:9 error is displayed in Browser Console. Also if the 'Responsive Design Mode' button from Developer Tools is selected in order to disable RDM, the tool is not dismissed but the content from it it's erased. Only after the 'Responsive Design Mode' button from Developer Tools is clicked for the second time, the RDM tool is disabled. Other than the issue described above and the already known issue (Bug 1240912), no issues were found. Should I log this issue? Note that the tests were performed on Firefox 48.0a1 (2016-04-05) and on Windows 10 x86, Ubuntu 12.04 x86, Mac OS X 10.11.

(In reply to Mihai Boldan, QA [:mboldan] from comment #47) > Also I found an issue related to Close button. If RDM is enabled and the tab > is detached from the current window, the 'X' button does not work any more > and TypeError: tab.linkedBrowser is null; manager.js:215:9 error is > displayed in Browser Console. Yes, this is another oddity related to our how we currently integrate with the rest of the browser. Please file a bug for this issue. > Also if the 'Responsive Design Mode' button > from Developer Tools is selected in order to disable RDM, the tool is not > dismissed but the content from it it's erased. Only after the 'Responsive > Design Mode' button from Developer Tools is clicked for the second time, the > RDM tool is disabled. Ah interesting, this one is a bit more subtle. Yes, please file this issue. I would say file a separate bug for this one... it's somewhat related to first issue above, but it's not clear to me right now if they would get resolved together. Thanks!

I logged Bug 1262806 and Bug 1262818 for the issues described above, so I'm marking this issue Verified Fixed.