767678 - Responsive Design mode has no close button

Status: RESOLVED FIXED

Description

[:Felipe Gomes (needinfo me!)]

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

[Kevin Dangoor]

(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

[Paul Rouget [:paul]]

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.

Comment 3

[Willian Gustavo Veiga]

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.

Comment 4

[Willian Gustavo Veiga]

Created attachment 684880 [details] [diff] [review]
Close button text

Comment 5

[Paul Rouget [:paul]]

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.

Comment 6

[Willian Gustavo Veiga]

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.

Comment 7

[Girish Sharma [:Optimizer]]

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

[Paul Rouget [:paul]]

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 :/

Comment 9

[Paul Rouget [:paul]]

The responsive mode will have a toggle button in the toolbox. That will make the situation better.

Comment 10

[Girish Sharma [:Optimizer]]

(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

[Paul Rouget [:paul]]

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.

Comment 12

[Manuel Strehl]

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

[Paul Rouget [:paul]]

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).

Comment 14

[Willian Gustavo Veiga]

Created attachment 701479 [details] [diff] [review]
Label removed

Comment 15

[Willian Gustavo Veiga]

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 16

[Paul Rouget [:paul]]

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

[Paul Rouget [:paul]]

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

[Paul Rouget [:paul]]

+care

Comment 19

[Willian Gustavo Veiga]

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.

Comment 20

[Willian Gustavo Veiga]

Created attachment 701562 [details]
CSS moved from common.css to browser.css

Comment 21

[Willian Gustavo Veiga]

Created attachment 701563 [details] [diff] [review]
tabindex changed to 0, using id instead of a class, attribute checked = true.

Comment 22

[Paul Rouget [:paul]]

(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.

Comment 23

[Willian Gustavo Veiga]

Created attachment 701566 [details] [diff] [review]
Back to a class. Id removed.

Comment 24

[Willian Gustavo Veiga]

Created attachment 701567 [details] [diff] [review]
browser.css - gnomestripe

Comment 25

[Willian Gustavo Veiga]

Created attachment 701568 [details] [diff] [review]
browser.css - pinstripe

Comment 26

[Willian Gustavo Veiga]

Created attachment 701569 [details] [diff] [review]
browser.css - winstripe

Comment 27

[Willian Gustavo Veiga]

Created attachment 701572 [details] [diff] [review]
Close icon changed. Now using the blue one.

Comment 28

[Paul Rouget [:paul]]

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"

Comment 29

[Willian Gustavo Veiga]

Created attachment 701927 [details] [diff] [review]
Now with the tooltiptext attribute.

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 701927 [details] [diff] [review]
Now with the tooltiptext attribute.

Optimizer, can you do a quick informal review?

Comment 31

[Girish Sharma [:Optimizer]]

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.

Comment 32

[Paul Rouget [:paul]]

(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

[Girish Sharma [:Optimizer]]

(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

[Paul Rouget [:paul]]

(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?

Comment 35

[Willian Gustavo Veiga]

Created attachment 702463 [details] [diff] [review]
Text improvements.

Comment 36

[Willian Gustavo Veiga]

Created attachment 702465 [details]
Close button

Comment 37

[hsablonniere]

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?

Comment 38

[hsablonniere]

Created attachment 702502 [details]
Proposition for the close button

Comment 39

[Paul Rouget [:paul]]

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.

Comment 40

[Paul Rouget [:paul]]

> 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

[Paul Rouget [:paul]]

Comment on attachment 702463 [details] [diff] [review]
Text improvements.

Cancelling review until we come to an agreement.

Comment 42

[Paul Rouget [:paul]]

Willian, can I ask you to try with moving the button on the far left, with an actual close icon?

Comment 43

[Girish Sharma [:Optimizer]]

(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

[hsablonniere]

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

[Paul Rouget [:paul]]

Created attachment 722768 [details] [diff] [review]
patch v1

Comment 46

[Michael Ratcliffe [:miker] [:mratcliffe]]

Comment on attachment 722768 [details] [diff] [review]
patch v1

Review of attachment 722768 [details] [diff] [review]:
-----------------------------------------------------------------

Lookin good

Comment 47

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/598d99bc4dd8

Comment 48

[Paul Rouget [:paul]]

https://hg.mozilla.org/mozilla-central/rev/598d99bc4dd8

Comment 49

[Simona Badau [Away. Please needinfo? Alin Deac [:alindeac]]]

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.