Responsive Design mode has no close button

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Felipe, Assigned: Willian Gustavo Veiga)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(firefox22 verified)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 17 obsolete attachments)

5.72 KB, patch
miker
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

5 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

5 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

5 years ago
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.
Attachment #684879 - Flags: review?(paul)
(Assignee)

Comment 4

5 years ago
Created attachment 684880 [details] [diff] [review]
Close button text
Attachment #684880 - Flags: review?(paul)

Comment 5

5 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

5 years ago
Attachment #684880 - Flags: review?(paul)
(Assignee)

Comment 6

5 years ago
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.
Attachment #687397 - Flags: review?(paul)
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

5 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

5 years ago
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.

Comment 11

5 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

5 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

5 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

5 years ago
Created attachment 701479 [details] [diff] [review]
Label removed
Attachment #684879 - Attachment is obsolete: true
Attachment #684880 - Attachment is obsolete: true
Attachment #687397 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
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

5 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

5 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

5 years ago
+care
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
Created attachment 701562 [details]
CSS moved from common.css to browser.css
Attachment #701479 - Attachment is obsolete: true
Attachment #701480 - Attachment is obsolete: true
Attachment #701561 - Attachment is obsolete: true
(Assignee)

Comment 21

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

Comment 22

5 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

5 years ago
Created attachment 701566 [details] [diff] [review]
Back to a class. Id removed.
(Assignee)

Comment 24

5 years ago
Created attachment 701567 [details] [diff] [review]
browser.css - gnomestripe
Attachment #701562 - Attachment is obsolete: true
Attachment #701563 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 701568 [details] [diff] [review]
browser.css - pinstripe
(Assignee)

Comment 26

5 years ago
Created attachment 701569 [details] [diff] [review]
browser.css - winstripe
(Assignee)

Comment 27

5 years ago
Created attachment 701572 [details] [diff] [review]
Close icon changed. Now using the blue one.
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

5 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

5 years ago
Created attachment 701927 [details] [diff] [review]
Now with the tooltiptext attribute.
Attachment #701572 - Attachment is obsolete: true
Attachment #701927 - Flags: review?(paul)

Comment 30

5 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 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

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

5 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

5 years ago
Created attachment 702463 [details] [diff] [review]
Text improvements.
Attachment #701927 - Attachment is obsolete: true
Attachment #702463 - Flags: review?(paul)
(Assignee)

Comment 36

5 years ago
Created attachment 702465 [details]
Close button

Comment 37

5 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

5 years ago
Created attachment 702502 [details]
Proposition for the close button
Attachment #702502 - Flags: ui-review?

Comment 39

5 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

5 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

5 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

5 years ago
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.

Comment 44

5 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

5 years ago
Created attachment 722768 [details] [diff] [review]
patch v1
Attachment #702463 - Attachment is obsolete: true
Attachment #702465 - Attachment is obsolete: true
Attachment #702502 - Attachment is obsolete: true
Attachment #722768 - Flags: review?(mratcliffe)
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

5 years ago
Whiteboard: [land-in-fx-team]

Comment 47

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/598d99bc4dd8
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]

Comment 48

5 years ago
https://hg.mozilla.org/mozilla-central/rev/598d99bc4dd8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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.
status-firefox22: fixed → verified
You need to log in before you can comment on or make changes to this bug.