762848 - [responsive mode] we need an "input" mechanism to set a size

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Paul Rouget [:paul]]

If the user wants to test a specific resolution, he has to use the mouse, and it's not easy to get a pixel-exact size.

Maybe a "edit" button in the menu or next to it. Maybe a textarea. Maybe +/- buttons.

Comment 1

[Kevin Dangoor]

At the very least, there can be a command line command to set responsive mode to a specific size.

I do agree that there should be visible UI for this, though

Comment 2

[hsablonniere]

I would go for 2 text input (maybe with spinners).

Those text input should update themselves as the user resize the view.

Comment 3

[André Miranda]

This is bug is really annoying, the user has to either struggle with the resizers or invoke a 'resize to' command.
I think spinners are more elegant than plain textfields validating if the string is a number.
The problem is that the guys at bug 344616 are discussing about input number since 2010 o.O

Comment 4

[Paul Rouget [:paul]]

I'd be happy to get such feature, but I have no idea of what this should look like. I really want to avoid clutter. So adding 2 new inputs is not an option.

Comment 5

[hsablonniere]

Any news on this. I'm still convinced about the two inputs ;-)

Comment 6

[Paul Rouget [:paul]]

We now can do precise resize by holding the control key while resizing. Other than that, I still don't know what this should look like.

Comment 7

[hsablonniere]

I didn't know about that :-(

Comment 8

[André Miranda]

I still believe that a more direct way to set dimensions is needed.
What about a dialog with 2 inputs (spinners)? Is that possible? If so would avoid clutter in the GUI.

Comment 9

[Paul Rouget [:paul]]

(In reply to André Miranda from comment #8)
> I still believe that a more direct way to set dimensions is needed.
> What about a dialog with 2 inputs (spinners)? Is that possible? If so would
> avoid clutter in the GUI.

Mockups usually help.

Comment 10

[André Miranda]

Attachment: Dialog Mockup

I went further and added some ideas to the mockup.
P.S.: Forgive me, but I'm terrible with mockups and graphics in general :)

Comment 11

[Paul Rouget [:paul]]

Sounds like a good option to me.

Comment 12

[André Miranda]

Attachment: Xul Mockup

I have prepared a XUL mockup, still presents some glitches but It's functional.
So, what do you guys think?

Comment 13

[Paul Rouget [:paul]]

(In reply to André Miranda from comment #12)
> Created attachment 826509 [details]
> Xul Mockup
> 
> I have prepared a XUL mockup, still presents some glitches but It's
> functional.
> So, what do you guys think?

I can't run this. Can you take a screenshot?

Comment 14

[André Miranda]

Attachment: Screenshot

Here you go.
If you're willing to run the XUL file, this add-on can do the job:
https://addons.mozilla.org/firefox/addon/remote-xul-manager/

Comment 15

[Paul Rouget [:paul]]

I guess we can go for something like that. We'd need to switch to a doorhanger though.

Comment 16

[André Miranda]

(In reply to Paul Rouget [:paul] from comment #15)
> I guess we can go for something like that. We'd need to switch to a
> doorhanger though.

I don't how PopupNotifications#show would represent all widgets(buttons, listbox, spacer...) as presented in the mockups. Could you give me a hint?

Comment 17

[Paul Rouget [:paul]]

I guess you want to use a doorhanger like the new menus in australis: http://i.imgur.com/ah4DLzs.png

I don't really know how that works. I guess you'll need to dig into the ux branch code or ping someone in #fx-team.

Come to IRC (#devtools), we can look at that together.

Comment 18

[André Miranda]

Attachment: sketch.patch

I've prepared a (ugly) sketch just to see how well the doorhanger idea would look like inside Firefox. Well, the patch is attatched, see for yourself and forgive me for such dirty code, I did it in a hurry.

While I was writing that code, and taking a look at the new menu and download notification code, I noticed that the interface code is separated from the "functional" code, pretty much like Android's Views(xml+java). Do you think it would be nice to do the same with the responsive mode? I think so, because code is starting to get bigger and trickier to describe a more elaborated UI.

Comment 19

[Rob Campbell [:rc] (:robcee)]

hey Paul,

This bug has a patch on it with a feedback request. Any thoughts?

Comment 20

[Paul Rouget [:paul]]

Comment on attachment 8355160 [details] [diff] [review]
sketch.patch

(In reply to André Miranda from comment #18)
> Created attachment 8355160 [details] [diff] [review]
> sketch.patch
> 
> I've prepared a (ugly) sketch just to see how well the doorhanger idea would
> look like inside Firefox. Well, the patch is attatched, see for yourself and
> forgive me for such dirty code, I did it in a hurry.
> 
> While I was writing that code, and taking a look at the new menu and
> download notification code, I noticed that the interface code is separated
> from the "functional" code, pretty much like Android's Views(xml+java). Do
> you think it would be nice to do the same with the responsive mode? I think
> so, because code is starting to get bigger and trickier to describe a more
> elaborated UI.

Indeed. We should have a more elaborated menu. With a list and the buttons like you did, with the text input.

Comment 21

[Jeremie Patonnier :Jeremie]

Attachment: Alternative proposal with simple editable menulist

Hi!

This is an alternative (and simpler) approach to this issue.
The list of preset is a XUL menulist. Such menu list can be editable.
So this patch make the preset list editable and handle the user inputs.

Paul can you review it? Once the patch feels okay, we'll see to add the required tests.

Comment 22

[Paul Rouget [:paul]]

Attachment: screenshot (linux)

I'll review this. In the meantime, look at this screenshot.

Comment 23

[Paul Rouget [:paul]]

Also, at this point, we might want to kill the " (custom)" string. It's not that useful, and it's annoying when editing text.

Comment 24

[Jeremie Patonnier :Jeremie]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #23)
> Also, at this point, we might want to kill the " (custom)" string. It's not
> that useful, and it's annoying when editing text.

Agreed and it's no big deal to get ride of it.

(BTW the screenshot is not working, I'm unable to see it... but I suspect a style issue, is that it?)

Comment 25

[Paul Rouget [:paul]]

Attachment: screenshot (linux)

Comment 26

[André Miranda]

Nice approach, but there some things to consider:
- The user can write any gibberish, just after "enter" it's sanitized.
  - Strangely, "800xa600" becomes 800x600, but "800x6a00" becomes 800x50(minimal size).
- Previously, if you clicked anywhere on the menulist, the preset list would pop up, now while hovering you get an annoying text cursor and you have to click on the rather small down arrow.
  - Something like a pencil icon inside the menulist "button" in order to make the current value editable would be better IMHO, but a custom widget is overkill.

Besides what has been said, while this approach is not as fancy as a doorhanger, it's indeed very simple and useful.

Regarding the " (custom)" string, I think something more unobtrusive would be better, maybe a small "*" somewhere inside or next to the menulist button.

Comment 27

[Paul Rouget [:paul]]

Comment on attachment 8447767 [details] [diff] [review]
Alternative proposal with simple editable menulist

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

You will need to write a test, and also make sure other tests run properly. To run the test:
> ./mach mochitest-devtools browser/devtools/responsivedesign

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +28,3 @@
>  const SLOW_RATIO = 6;
>  const ROUND_RATIO = 10;
> +const INPUT_PARSER = /[^\d]*(\d+)[^\d]+(\d+).*$/;

Let me break that down:
[^\d]* -> is that actually needed?
(\d+) -> yes
[^\d]+ -> what about just "x", we don't need to be too permissive
(\d+) -> yes
.*$ -> is that needed?

I haven't tested, but what about just:
/(\d+)x(\d+)/ ?

Also, use parseInt(value[1],10) and parseInt(value[2],10)

@@ +483,5 @@
> +      this.menulist.selectedItem = menuitem;
> +    }
> +
> +    this.customPreset.width = value[1];
> +    this.customPreset.height = value[2];

parseInt(x,10) this first

@@ +532,4 @@
>      } else if (aPreset.name != null && aPreset.name !== "") {
> +      size = this.strings.formatStringFromName("responsiveUI.namedResolution", [size, aPreset.name], 2);
> +    }
> +

Kill the "(custom)" string.

@@ +533,5 @@
> +      size = this.strings.formatStringFromName("responsiveUI.namedResolution", [size, aPreset.name], 2);
> +    }
> +
> +    aMenuitem.setAttribute("label", size);
> +

You might want to move the cursor to its previous position.

@@ +925,4 @@
>      this.ignoreY = false;
>      this.ignoreX = false;
>      this.isResizing = false;
> +    this.menulist.focus();

Is that required? Might be annoying.

::: browser/themes/shared/devtools/responsivedesign.inc.css
@@ +44,5 @@
>    color: inherit;
>  }
> +.devtools-responsiveui-menulist html|*.menulist-editable-input {
> +  -moz-appearance: none;
> +  color:inherit;

space before inherit.

@@ +51,5 @@
> +
> +.devtools-responsiveui-menulist html|*.menulist-editable-input::-moz-selection {
> +  background: hsla(212,7%,57%,.35);
> +}
> +

You need to add:

.devtools-responsiveui-menulist .menulist-editable-box {
  -moz-appearance: none;
  background-color: transparent;
}

to make it look good on Linux.

I haven't tested on Windows yet.

Comment 28

[Paul Rouget [:paul]]

(In reply to André Miranda from comment #26)
> Nice approach, but there some things to consider:
> - The user can write any gibberish, just after "enter" it's sanitized.
>   - Strangely, "800xa600" becomes 800x600, but "800x6a00" becomes
> 800x50(minimal size).

I suggested a more predictable regex.

> - Previously, if you clicked anywhere on the menulist, the preset list would
> pop up, now while hovering you get an annoying text cursor and you have to
> click on the rather small down arrow.

I don't think it's a problem.

>   - Something like a pencil icon inside the menulist "button" in order to
> make the current value editable would be better IMHO, but a custom widget is
> overkill.
> 
> Besides what has been said, while this approach is not as fancy as a
> doorhanger, it's indeed very simple and useful.
> 
> Regarding the " (custom)" string, I think something more unobtrusive would
> be better, maybe a small "*" somewhere inside or next to the menulist button.

I don't think it brings any useful information.

Comment 29

[Jeremie Patonnier :Jeremie]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #27)
> You will need to write a test, and also make sure other tests run properly.
> To run the test:
> > ./mach mochitest-devtools browser/devtools/responsivedesign

Okay, I'll do it, but I wish to make sure we agreed on the technical side first :)

> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +28,3 @@
> >  const SLOW_RATIO = 6;
> >  const ROUND_RATIO = 10;
> > +const INPUT_PARSER = /[^\d]*(\d+)[^\d]+(\d+).*$/;
> 
> Let me break that down:
> [^\d]* -> is that actually needed?
> (\d+) -> yes
> [^\d]+ -> what about just "x", we don't need to be too permissive
> (\d+) -> yes
> .*$ -> is that needed?
> 
> I haven't tested, but what about just:
> /(\d+)x(\d+)/ ?

Well, it is doable, but why being so restrictive?
Currently, the RegExp I'm suggesting can be understand as follow:
1. (Whatever that is not a digit)
2. (digits for width)
3. (Whatever that is not a digit)
4. (digits for height)
5. (whatever, including other digits that will be ignored)

This allows many input format (which is good to handle copy/past actually):
360x480
360:480
360/480
360 x 480
360px x 480px
w: 360 / h: 480

We currently reformat all of this to 360x480 but I don't see any reason to disallow more permissive format (it would also be possible to keep the user input rather than reformat it)

> @@ +483,5 @@
> > +      this.menulist.selectedItem = menuitem;
> > +    }
> > +
> > +    this.customPreset.width = value[1];
> > +    this.customPreset.height = value[2];
> 
> parseInt(x,10) this first

Oh right, yeah.

> @@ +532,4 @@
> >      } else if (aPreset.name != null && aPreset.name !== "") {
> > +      size = this.strings.formatStringFromName("responsiveUI.namedResolution", [size, aPreset.name], 2);
> > +    }
> > +
> 
> Kill the "(custom)" string.

RIP "(custom)" string :D

> @@ +533,5 @@
> > +      size = this.strings.formatStringFromName("responsiveUI.namedResolution", [size, aPreset.name], 2);
> > +    }
> > +
> > +    aMenuitem.setAttribute("label", size);
> > +
> 
> You might want to move the cursor to its previous position.

That could be tricky. I'll made some experiment in that area.

> @@ +925,4 @@
> >      this.ignoreY = false;
> >      this.ignoreX = false;
> >      this.isResizing = false;
> > +    this.menulist.focus();
> 
> Is that required? Might be annoying.

Yes, I just forget to remove it.

> @@ +51,5 @@
> > +
> > +.devtools-responsiveui-menulist html|*.menulist-editable-input::-moz-selection {
> > +  background: hsla(212,7%,57%,.35);
> > +}
> > +
> 
> You need to add:
> 
> .devtools-responsiveui-menulist .menulist-editable-box {
>   -moz-appearance: none;
>   background-color: transparent;
> }
> 
> to make it look good on Linux.
> 
> I haven't tested on Windows yet.

Okay, I haven't dig that much on styling yet.

Comment 30

[Jeremie Patonnier :Jeremie]

(In reply to André Miranda from comment #26)
> Nice approach, but there some things to consider:
> - The user can write any gibberish, just after "enter" it's sanitized.

Yes, that's the whole idea. The user can type anything, we'll make sure it will be a valid value in the end.

>   - Strangely, "800xa600" becomes 800x600, but "800x6a00" becomes
> 800x50(minimal size).

Yes, the current rules that applied only catch the first two series of digit to guess the user input. Se comment #29 for more details?

> - Previously, if you clicked anywhere on the menulist, the preset list would
> pop up, now while hovering you get an annoying text cursor and you have to
> click on the rather small down arrow.
>   - Something like a pencil icon inside the menulist "button" in order to
> make the current value editable would be better IMHO, but a custom widget is
> overkill.

It's possible to add a button to turn the menulist editable feature on and off.

Comment 31

[Paul Rouget [:paul]]

(In reply to Jeremie Patonnier :Jeremie from comment #30)
> It's possible to add a button to turn the menulist editable feature on and off.

Not sure we want that. I like the behavior of the current patch.

Comment 32

[Jeremie Patonnier :Jeremie]

Attachment: Alternative proposal with simple editable menulist + tests

Hi, 

Here's a patch update to address paul's comments and update/add tests

Comment 33

[Paul Rouget [:paul]]

Comment on attachment 8451555 [details] [diff] [review]
Alternative proposal with simple editable menulist + tests

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

Thank you! Can you do a push to try? (you have a commit access?)

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +467,3 @@
>      this.stack.appendChild(this.resizeBarH);
>    },
> +  handleManualInput: function RUI_handleManualInput() {

Add a comment above this function, and a line break.

@@ +529,5 @@
>     */
>    setMenuLabel: function RUI_setMenuLabel(aMenuitem, aPreset) {
>      let size = Math.round(aPreset.width) + "x" + Math.round(aPreset.height);
> +
> +    // We need to be sure the XBL binding is done before assigning any value

Better comment: ".inputField might be not reachable yet (async XBL loading)"

Comment 34

[André Miranda]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #28)
> (In reply to André Miranda from comment #26)
> >   - Something like a pencil icon inside the menulist "button" in order to
> > make the current value editable would be better IMHO, but a custom widget is
> > overkill.
> > 
> > Besides what has been said, while this approach is not as fancy as a
> > doorhanger, it's indeed very simple and useful.
> > 
> > Regarding the " (custom)" string, I think something more unobtrusive would
> > be better, maybe a small "*" somewhere inside or next to the menulist button.
> 
> I don't think it brings any useful information.

@Paul, please don't take me wrong, I really appreciate your work, but to state something is not useful without arguing is also not useful. Please, don't rule out an outsider's comment like that, it really discourages feedback. Once again don't take me wrong.

@Jeremie, Now I like your idea more than mine :)
We just need to figure out how to deal with updating preset. What do you have in mind?
I still believe a "*" to indicate that a preset has changed is valid. Another possibility is to not update a preset at all, just delete and create a new one. For example, "1024x768(15" display 4:3)", why would change the resolution of this preset?

Cheers

Comment 35

[Paul Rouget [:paul]]

(In reply to André Miranda from comment #34)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #28)
> > (In reply to André Miranda from comment #26)
> > >   - Something like a pencil icon inside the menulist "button" in order to
> > > make the current value editable would be better IMHO, but a custom widget is
> > > overkill.
> > > 
> > > Besides what has been said, while this approach is not as fancy as a
> > > doorhanger, it's indeed very simple and useful.
> > > 
> > > Regarding the " (custom)" string, I think something more unobtrusive would
> > > be better, maybe a small "*" somewhere inside or next to the menulist button.
> > 
> > I don't think it brings any useful information.
> 
> @Paul, please don't take me wrong, I really appreciate your work, but to
> state something is not useful without arguing is also not useful. Please,
> don't rule out an outsider's comment like that, it really discourages
> feedback. Once again don't take me wrong.

Like I said, I don't see how a star (or "custom") bring any useful information.
But I might be wrong. Tell me how a marker telling you that this is not a preset
is actually useful?

> @Jeremie, Now I like your idea more than mine :)
> We just need to figure out how to deal with updating preset. What do you
> have in mind?
> I still believe a "*" to indicate that a preset has changed is valid.

Why :) Don't "state something is useful without arguing" (sorry, just playing with your nerves ^^).

> Another possibility is to not update a preset at all, just delete and create
> a new one. For example, "1024x768(15" display 4:3)", why would change the
> resolution of this preset?


Isn't that what we do? We don't update any preset. We switch to a custom size.

Comment 36

[Paul Rouget [:paul]]

Jeremie, let's land that.

push to try: https://tbpl.mozilla.org/?tree=Try&rev=9cfce0041f62

Please address my comments and I'll land your patch.

Comment 37

[Jeremie Patonnier :Jeremie]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #36)
> Jeremie, let's land that.
> 
> push to try: https://tbpl.mozilla.org/?tree=Try&rev=9cfce0041f62

Thank for the push to try, I don't think I have the necessary right to do it :)

> Please address my comments and I'll land your patch.

No problem, I was in vacation last week, I'll do it this week.

Comment 38

[Jeremie Patonnier :Jeremie]

(In reply to André Miranda from comment #34)
> @Jeremie, Now I like your idea more than mine :)
> We just need to figure out how to deal with updating preset. What do you
> have in mind?

Nothing special actually. When someone edit the menulist, we store the value inside the custom preset (with the "custom" string on the label of that specific preset). If the user want to record that preset he just have to use the drop down list and chose the "add preset" option. This patch does not change the current preset management, it just add an editable feature, all the rest remain as it is.

> I still believe a "*" to indicate that a preset has changed is valid.
> Another possibility is to not update a preset at all, just delete and create
> a new one. For example, "1024x768(15" display 4:3)", why would change the
> resolution of this preset?

It's always possible to provide a more advance work-flow, but in that case, it needs to be carefully thought and experimented. The prototype you are working on is still a good way to handle more advance edition work-flow.

My patch try to solve the problem I face on a daily basis (being able to change the value of the responsive view with my keyboard to ease using presets larger than my screen). I tried to do this using the simplest possible way. That said it is still possible to improve this (a lot). Providing a better and more advance UX for the responsive mode interface is still needed but I just think it's better to add small enhancement one step at a time. Please feel free to enhance want I did :)

Comment 39

[André Miranda]

@Paul: ok, so let's just drop the "custom" or star thing. Now if the user selects a preset, for example '1024x768 (15" display)', and changes the view size to 1000x500, what will be display? just '1000x500'?
Well, let's see how this works once it makes its way to try :)

@Jeremie: Don't worry about something advanced or that seems fancy, your approach is great, let's Keep It Simple and Silly :P
As you said, to change the view size is really painful currently, and that's what this bug is meant to treat. As needs and ideas come up, we'll improve it.

Cheers.

Comment 40

[André Miranda]

s/try/master

Comment 41

[Paul Rouget [:paul]]

Jeremie, look at the try result. You broke something :)

> 10:21:03     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_parsable_css.js | Console message: [JavaScript Warning: "Unrecognized at-rule or error parsing at-rule '@namespace'." {file: "jar:file:///builds/slave/test/build/application/firefox/browser/omni.ja!/chrome/browser/skin/classic/browser/browser.css" line: 2879 column: 0 source: "@namespace html url("http://www.w3.org/1999/xhtml");"}]

> Thank for the push to try, I don't think I have the necessary right to do it :)

Can you ask for a commit access level 1? I'll vouch for you.

See https://www.mozilla.org/hacking/commit-access-policy/ and https://www.mozilla.org/hacking/committer/

Comment 42

[Jeremie Patonnier :Jeremie]

Attachment: Patch ready for check-in

Comment 43

[Jeremie Patonnier :Jeremie]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #36)
> Please address my comments and I'll land your patch.

Ok, all done, have fun :)

Comment 44

[Paul Rouget [:paul]]

try: https://tbpl.mozilla.org/?tree=Try&rev=6788b18aa73a

Comment 45

[Paul Rouget [:paul]]

Thank you Jeremie!

https://hg.mozilla.org/integration/fx-team/rev/35d54b6a84f6

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35d54b6a84f6

Comment 47

[André Miranda]

I just tested, it's simple and great!
Thanks Jeremie.

The only glitch I see is if the user enters something like "8a00x600", it resizes to 80x60. IMHO if the user input doesn't match '^(\d+)[^\d]+(\d+)$' then nothing should be done.
What do you guys think?

Comment 48

[Jeremie Patonnier :Jeremie]

(In reply to André Miranda from comment #47)
> The only glitch I see is if the user enters something like "8a00x600", it
> resizes to 80x60. IMHO if the user input doesn't match '^(\d+)[^\d]+(\d+)$'
> then nothing should be done.

Yes, the error handling system is a bit to simple right know. It could be better to simply get all digit string and if there are more or less than two then just do nothing.

That way it could allow permissive writing of the strings (again, this is good for copy/pasting) with a simple handling of mistyping.

Comment 50

[nagabalaji.qc]

[Blocking Requested - why for this release]: