Closed
Bug 767678
Opened 11 years ago
Closed 10 years ago
Responsive Design mode has no close button
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox22 verified)
RESOLVED
FIXED
Firefox 22
Tracking | Status | |
---|---|---|
firefox22 | --- | verified |
People
(Reporter: Felipe, Assigned: beberveiga)
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 17 obsolete files)
5.72 KB,
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
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)?
Comment 1•11 years ago
|
||
(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.
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
I've created the close button but I don't know how to set button's position.
Attachment #684879 -
Flags: review?(paul)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #684880 -
Flags: review?(paul)
Comment 5•10 years ago
|
||
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.
Attachment #684879 -
Flags: review?(paul)
Updated•10 years ago
|
Attachment #684880 -
Flags: review?(paul)
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #687397 -
Flags: review?(paul)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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 :/
Attachment #687397 -
Flags: review?(paul)
Comment 9•10 years ago
|
||
The responsive mode will have a toggle button in the toolbox. That will make the situation better.
Comment 10•10 years ago
|
||
(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.
Comment 11•10 years ago
|
||
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.
Assignee: nobody → contact
Comment 12•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
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).
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #684879 -
Attachment is obsolete: true
Attachment #684880 -
Attachment is obsolete: true
Attachment #687397 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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 17•10 years ago
|
||
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 :)
Comment 18•10 years ago
|
||
+care
Assignee | ||
Comment 19•10 years ago
|
||
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.
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #701479 -
Attachment is obsolete: true
Attachment #701480 -
Attachment is obsolete: true
Attachment #701561 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #701562 -
Attachment is obsolete: true
Attachment #701563 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #701566 -
Attachment is obsolete: true
Attachment #701567 -
Attachment is obsolete: true
Attachment #701568 -
Attachment is obsolete: true
Attachment #701569 -
Attachment is obsolete: true
Attachment #701572 -
Flags: review?
Comment 28•10 years ago
|
||
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"
Attachment #701572 -
Flags: review?
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #701572 -
Attachment is obsolete: true
Attachment #701927 -
Flags: review?(paul)
Comment 30•10 years ago
|
||
Comment on attachment 701927 [details] [diff] [review] Now with the tooltiptext attribute. Optimizer, can you do a quick informal review?
Attachment #701927 -
Flags: review?(paul) → review?(scrapmachines)
Comment 31•10 years ago
|
||
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.
Attachment #701927 -
Flags: review?(scrapmachines) → review-
Comment 32•10 years ago
|
||
(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.
Comment 33•10 years ago
|
||
(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.
Comment 34•10 years ago
|
||
(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?
Status: NEW → ASSIGNED
Flags: needinfo?(hubert.sablonniere)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #701927 -
Attachment is obsolete: true
Attachment #702463 -
Flags: review?(paul)
Assignee | ||
Comment 36•10 years ago
|
||
Comment 37•10 years ago
|
||
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?
Flags: needinfo?(hubert.sablonniere)
Comment 38•10 years ago
|
||
Attachment #702502 -
Flags: ui-review?
Comment 39•10 years ago
|
||
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.
Attachment #702502 -
Flags: ui-review? → ui-review-
Comment 40•10 years ago
|
||
> 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 41•10 years ago
|
||
Comment on attachment 702463 [details] [diff] [review] Text improvements. Cancelling review until we come to an agreement.
Attachment #702463 -
Flags: review?(paul)
Comment 42•10 years ago
|
||
Willian, can I ask you to try with moving the button on the far left, with an actual close icon?
Comment 43•10 years ago
|
||
(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.
Comment 44•10 years ago
|
||
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.
Comment 45•10 years ago
|
||
Attachment #702463 -
Attachment is obsolete: true
Attachment #702465 -
Attachment is obsolete: true
Attachment #702502 -
Attachment is obsolete: true
Attachment #722768 -
Flags: review?(mratcliffe)
Comment 46•10 years ago
|
||
Comment on attachment 722768 [details] [diff] [review] patch v1 Review of attachment 722768 [details] [diff] [review]: ----------------------------------------------------------------- Lookin good
Attachment #722768 -
Flags: review?(mratcliffe) → review+
Updated•10 years ago
|
Whiteboard: [land-in-fx-team]
Comment 47•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/598d99bc4dd8
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 48•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/598d99bc4dd8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox22:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment 49•10 years ago
|
||
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.
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•