I'm mostly a mouse user (I know...), so when I open the Responsive Design View through the menu I feel stuck in it because it doesn't have a close button like Inspect used to have (now part of the bottom toolbar), and then it takes me time to realize I can press Esc or go back to the menu and uncheck the responsive mode. Or are there plans to make the Responsive mode part of the inspector (i.e. always show the inspect toolbar for it and have a toggle like the 3d mode)?
(In reply to Felipe Gomes (:felipe) from comment #0) > Or are there plans to make the Responsive mode part of the inspector (i.e. > always show the inspect toolbar for it and have a toggle like the 3d mode)? Responsive Design View is separate from the page inspector because you can switch it on and browse around just seeing how things look without inspecting elements. The Developer Toolbar (currently behind devtools.toolbar.enabled pref, which will be on sometime soon) will have a popup menu that includes the Responsive Design View, so you can toggle the tool from that menu. There should also be a command to control it, but that doesn't exist yet. Given that Responsive Design View has a couple of buttons already, it seems reasonable to have a close button.
There is no buttons yet because it's not easy to keep the button on the top right corner (window & linux). The top right corner of the responsive-mode container can be hidden because off-screen (if the browser is bigger than firefox' window). So the button's position needs to be dynamically adjusted in JavaScript.
Created attachment 684879 [details] [diff] [review] Close button patch I've created the close button but I don't know how to set button's position.
Created attachment 684880 [details] [diff] [review] Close button text
Comment on attachment 684879 [details] [diff] [review] Close button patch > I don't know how to set button's position. The button should be on the top right corner. And this is the difficulty of this bug. I don't see how we can easily add a button there. Maybe we could add a floating button that is moved dynamically as we change the size of the browser.
Created attachment 687397 [details] [diff] [review] Close button "aligned" right I've improved the patch a little, but I think it's not what Paul meant. As I'm starting now, maybe I should get an easier bug to take care ... Thanks.
Why not add the the close button as a textual button (just like rotate) and just next to the rotate button. That way, it will be matching to the rest of the UI of responsive mode and also, the issue of dynamically adjusting the position of the close button would not arise ?
Comment on attachment 687397 [details] [diff] [review] Close button "aligned" right The problem with this approach is that the close button keeps moving as you resize the browser :/
The responsive mode will have a toggle button in the toolbox. That will make the situation better.
(In reply to Paul Rouget [:paul] from comment #9) > The responsive mode will have a toggle button in the toolbox. That will make > the situation better. better for sure, but not everyone will have the toolbox opened. There are shortcuts and menu entries to start responsive mode too.
I suggest that we just do what William did in his first attachment, but instead of using a close icon, we use the Responsive Mode icon we use in the toolbox: http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/devtools/toolbox.css#74 William, I assigned you, let me know if you can't work on this. Thanks.
I'd second Girish Sharma, the close button next to "rotate" would be always visible and not in a surprising location, but where all this view's UI elements are.
Like I said, we should just have a button there (where all the buttons are), but not a close button, but a checked responsive view button (like in the toolbox).
Created attachment 701479 [details] [diff] [review] Label removed
Created attachment 701480 [details] [diff] [review] Adds the icon to the button. I don't know the right place to put this CSS. I've tried to create an external CSS file called responsivedesign.css in MOZILLA_SRC_DIR/browser/devtools/responsivedesign/ but I didn't figure out how to link it with Responsive Design Mode. Please, send me your feedback. How would you handle this situation? Thank you.
Comment on attachment 701479 [details] [diff] [review] Label removed >diff -r 84c0cca250cf browser/devtools/responsivedesign/responsivedesign.jsm >--- a/browser/devtools/responsivedesign/responsivedesign.jsm Sat Jan 12 03:27:53 2013 -0800 >+++ b/browser/devtools/responsivedesign/responsivedesign.jsm Sat Jan 12 17:12:52 2013 -0200 >@@ -149,16 +149,17 @@ function ResponsiveUI(aWindow, aTab) > this.container.setAttribute("responsivemode", "true"); > this.stack.setAttribute("responsivemode", "true"); > > // Let's bind some callbacks. > this.bound_presetSelected = this.presetSelected.bind(this); > this.bound_addPreset = this.addPreset.bind(this); > this.bound_removePreset = this.removePreset.bind(this); > this.bound_rotate = this.rotate.bind(this); >+ this.close = this.close.bind(this); > this.bound_startResizing = this.startResizing.bind(this); > this.bound_stopResizing = this.stopResizing.bind(this); > this.bound_onDrag = this.onDrag.bind(this); > this.bound_onKeypress = this.onKeypress.bind(this); > > // Events > this.tab.addEventListener("TabClose", this); > this.tabContainer.addEventListener("TabSelect", this); >@@ -217,16 +218,17 @@ ResponsiveUI.prototype = { > this.stopResizing(); > > // Remove listeners. > this.mainWindow.document.removeEventListener("keypress", this.bound_onKeypress, false); > this.menulist.removeEventListener("select", this.bound_presetSelected, true); > this.tab.removeEventListener("TabClose", this); > this.tabContainer.removeEventListener("TabSelect", this); > this.rotatebutton.removeEventListener("command", this.bound_rotate, true); >+ this.closebutton.removeEventListener("command", this.close, true); > this.addbutton.removeEventListener("command", this.bound_addPreset, true); > this.removebutton.removeEventListener("command", this.bound_removePreset, true); > > // Removed elements. > this.container.removeChild(this.toolbar); > this.stack.removeChild(this.resizer); > this.stack.removeChild(this.resizeBar); > >@@ -287,16 +289,17 @@ ResponsiveUI.prototype = { > > /** > * Build the toolbar and the resizers. > * > * <vbox class="browserContainer"> From tabbrowser.xml > * <toolbar class="devtools-toolbar devtools-responsiveui-toolbar"> > * <menulist class="devtools-menulist"/> // presets > * <toolbarbutton tabindex="0" class="devtools-toolbarbutton" label="rotate"/> // rotate >+ * <toolbarbutton tabindex="1" class="devtools-toolbarbutton devtools-toolbarbutton-close"/> // close tabindex should be ="0". Is devtools-toolbarbutton-close needed? Can you use id="close-button"? > * </toolbar> > * <stack class="browserStack"> From tabbrowser.xml > * <browser/> > * <box class="devtools-responsiveui-resizehandle" bottom="0" right="0"/> > * <box class="devtools-responsiveui-resizebar" top="0" right="0"/> > * </stack> > * </vbox> > */ >@@ -329,18 +332,24 @@ ResponsiveUI.prototype = { > menupopup.appendChild(this.removebutton); > > this.rotatebutton = this.chromeDoc.createElement("toolbarbutton"); > this.rotatebutton.setAttribute("tabindex", "0"); > this.rotatebutton.setAttribute("label", this.strings.GetStringFromName("responsiveUI.rotate")); > this.rotatebutton.className = "devtools-toolbarbutton"; > this.rotatebutton.addEventListener("command", this.bound_rotate, true); > >+ this.closebutton = this.chromeDoc.createElement("toolbarbutton"); >+ this.closebutton.setAttribute("tabindex", "1"); "0" >+ this.closebutton.className = "devtools-toolbarbutton devtools-toolbarbutton-close"; See above. >+ this.closebutton.addEventListener("command", this.close, true); >+ > this.toolbar.appendChild(this.menulist); > this.toolbar.appendChild(this.rotatebutton); >+ this.toolbar.appendChild(this.closebutton); > > // Resizers > this.resizer = this.chromeDoc.createElement("box"); > this.resizer.className = "devtools-responsiveui-resizehandle"; > this.resizer.setAttribute("right", "0"); > this.resizer.setAttribute("bottom", "0"); > this.resizer.onmousedown = this.bound_startResizing; >
Comment on attachment 701480 [details] [diff] [review] Adds the icon to the button. This should be in "browser/themes/*stripe/browser.css": http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#3698 Also, the close button should have a checked=true attribute. Thank you for taking of that :)
+care
Created attachment 701561 [details] Close button using id="close-button" I've changed common.css to use #close-button but it's overridden by another style with the same id. "#close-button { list-style-image: url("chrome://browser/skin/devtools/command-responsivemode.png"); -moz-image-region: rect(0px, 16px, 16px, 0px); }" I can change this id to another one or use an "!important": "#close-button { list-style-image: url("chrome://browser/skin/devtools/command-responsivemode.png") !important; -moz-image-region: rect(0px, 16px, 16px, 0px); }" Which approach would you use? I appreciate your feedback. Thank you.
Created attachment 701562 [details] CSS moved from common.css to browser.css
Created attachment 701563 [details] [diff] [review] tabindex changed to 0, using id instead of a class, attribute checked = true.
(In reply to Willian Gustavo Veiga from comment #19) > Created attachment 701561 [details] > Close button using id="close-button" > > I've changed common.css to use #close-button but it's overridden by another > style with the same id. > > "#close-button { > list-style-image: > url("chrome://browser/skin/devtools/command-responsivemode.png"); > -moz-image-region: rect(0px, 16px, 16px, 0px); > }" > > I can change this id to another one or use an "!important": > > "#close-button { > list-style-image: > url("chrome://browser/skin/devtools/command-responsivemode.png") !important; > -moz-image-region: rect(0px, 16px, 16px, 0px); > }" > > Which approach would you use? > I appreciate your feedback. Thank you. Oh, of course, my bad, I forgot we were in the browser context. So in your latest patch, change the class into an id. You need to use a class.
Created attachment 701566 [details] [diff] [review] Back to a class. Id removed.
Created attachment 701567 [details] [diff] [review] browser.css - gnomestripe
Created attachment 701568 [details] [diff] [review] browser.css - pinstripe
Created attachment 701569 [details] [diff] [review] browser.css - winstripe
Created attachment 701572 [details] [diff] [review] Close icon changed. Now using the blue one.
Comment on attachment 701572 [details] [diff] [review] Close icon changed. Now using the blue one. Looks good. Because this button doesn't have text, it needs a tooltip. You need to use the "tooltip-text" attribute, and use a localized string. See how it's done there: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm#333 and http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/responsiveUI.properties Maybe the tooltip can say: "Leave responsive design mode"
Created attachment 701927 [details] [diff] [review] Now with the tooltiptext attribute.
Comment on attachment 701927 [details] [diff] [review] Now with the tooltiptext attribute. Optimizer, can you do a quick informal review?
Comment on attachment 701927 [details] [diff] [review] Now with the tooltiptext attribute. Review of attachment 701927 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that showing an icon that is always enabled while the responsive view is on is a good idea. May be using the close icon is a better choice ? @Paul, what do you think ? ::: browser/locales/en-US/chrome/browser/devtools/responsiveUI.properties @@ +16,5 @@ > responsiveUI.rotate=rotate > > +# LOCALIZATION NOTE (responsiveUI.close): label of the close button. > +responsiveUI.close=Leave Responsive Design Mode > + s/label/tooltip text Since this is tooltip, the R, D and M should not be all capital. Something like "Leave responsive design mode" I am not sure whether the word "mode" should be used or not. In the menu items, we use "view" as in Responsive Design View.
(In reply to Girish Sharma [:Optimizer] from comment #31) > Comment on attachment 701927 [details] [diff] [review] > Now with the tooltiptext attribute. > > Review of attachment 701927 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think that showing an icon that is always enabled while the > responsive view is on is a good idea. May be using the close icon is a > better choice ? > @Paul, what do you think ? See comment 13. > I am not sure whether the word "mode" should be used or not. In the menu > items, we use "view" as in Responsive Design View. Right! s/mode/view.
(In reply to Paul Rouget [:paul] from comment #32) > > I don't think that showing an icon that is always enabled while the > > responsive view is on is a good idea. May be using the close icon is a > > better choice ? > > @Paul, what do you think ? > > See comment 13. I know, but its just my thought. Having an always on button, on which user is supposed to click to toggle it off to turn the responsive mode off is weird. Furthermore, the off state of the icon will never be visible. But if you are okay with it, then its fine.
(In reply to Girish Sharma [:Optimizer] from comment #33) > I know, but its just my thought. Having an always on button, on which user > is supposed to click to toggle it off to turn the responsive mode off is > weird. Furthermore, the off state of the icon will never be visible. Ok - that's fair. Willian, can I ask you to attach a screenshot of your work? Hubert, can you tell me what you think about this?
Created attachment 702463 [details] [diff] [review] Text improvements.
Created attachment 702465 [details] Close button
Thanks for the screenshot. It helps. I agree with Girish on the "the always enabled icon". It's strange. Putting the close button in the upper right corner shouldn't be hard. Of course it's comlicated because of the reasons Paul mentioned and I agree. I think we have theses troubles because our toolbar is part of the window. When the resolution is too big, scrollbars appear and they scroll not only the responsive view but the preset list and the rotate button too. I say we create some kind of real toolbar like the one on the other tools. Just before tool unification they all got a close button on the right. Right now only the developer toolbar still has one. We solve two problems at the same time. On this gimped screenshot (excuse the quality), you can see what I mean. I added the scrollbars on purpose to show what part of the screen could be scrollable. What do you think?
Created attachment 702502 [details] Proposition for the close button
Comment on attachment 702502 [details] Proposition for the close button No. No need to come up with such a big toolbar just to fix this close button issue.
> I agree with Girish on the "the always enabled icon". It's strange. Ok. I can see why that would be strange. What if we put a close button in the top left corner? @all, what do you think?
Comment on attachment 702463 [details] [diff] [review] Text improvements. Cancelling review until we come to an agreement.
Willian, can I ask you to try with moving the button on the far left, with an actual close icon?
(In reply to Paul Rouget [:paul] from comment #42) > Willian, can I ask you to try with moving the button on the far left, with > an actual close icon? I think that is a nice tradeoff we can have. Any other choice is an overkill for such a small feature.
I agree that it seems like an overkill but because the icons are not on a toolbar if someone scrolls a big preview area, the buttons disappear. I still think it's a good to create a real fixed toolbar right now. It will be useful for direct width/height input.
Created attachment 722768 [details] [diff] [review] patch v1
Comment on attachment 722768 [details] [diff] [review] patch v1 Review of attachment 722768 [details] [diff] [review]: ----------------------------------------------------------------- Lookin good
https://hg.mozilla.org/integration/fx-team/rev/598d99bc4dd8
https://hg.mozilla.org/mozilla-central/rev/598d99bc4dd8
Verified as fixed on Firefox 22 beta 1 - the close button is located in the far left of the window. Verified on Windows 8, on Ubuntu 13.04 and on Mac OS X 10.8: Build ID: 20130514181517 Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0 In some circumstances, the whole toolbar disappears after scroll. I filed Bug 873092 to cover that issue.
or
