Closed Bug 762848 Opened 12 years ago Closed 10 years ago

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

Categories

(DevTools :: Responsive Design Mode, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: paul, Assigned: Jeremie)

References

Details

Attachments

(7 files, 2 obsolete files)

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.
Summary: [responsive mode] it should be possible to type a size → [responsive mode] we need an "input" mechanism to set a size
Blocks: 749628
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
I would go for 2 text input (maybe with spinners).

Those text input should update themselves as the user resize the view.
Component: Developer Tools → Developer Tools: Responsive Mode
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
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.
Any news on this. I'm still convinced about the two inputs ;-)
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.
I didn't know about that :-(
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.
(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.
Attached image 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 :)
Sounds like a good option to me.
Attached file Xul Mockup
I have prepared a XUL mockup, still presents some glitches but It's functional.
So, what do you guys think?
(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?
Attached image 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/
I guess we can go for something like that. We'd need to switch to a doorhanger though.
(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?
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.
Attached patch sketch.patchSplinter Review
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.
Attachment #8355160 - Flags: feedback?(paul)
hey Paul,

This bug has a patch on it with a feedback request. Any thoughts?
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.
Attachment #8355160 - Flags: feedback?(paul)
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.
Attachment #8447767 - Flags: review?(paul)
Attached image screenshot (linux) (obsolete) —
I'll review this. In the meantime, look at this screenshot.
Also, at this point, we might want to kill the " (custom)" string. It's not that useful, and it's annoying when editing text.
(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?)
Attached image screenshot (linux)
Attachment #8447963 - Attachment is obsolete: true
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 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.
Attachment #8447767 - Flags: review?(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.
(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.
(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.
Assignee: nobody → jeremie.patonnier
(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.
Hi, 

Here's a patch update to address paul's comments and update/add tests
Attachment #8447767 - Attachment is obsolete: true
Attachment #8451555 - Flags: review?(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)"
Attachment #8451555 - Flags: review?(paul) → review+
(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
(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.
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.
(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.
(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 :)
@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.
s/try/master
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/
(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 :)
Thank you Jeremie!

https://hg.mozilla.org/integration/fx-team/rev/35d54b6a84f6
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/35d54b6a84f6
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
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?
(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.
blocking-b2g: fugu? → ---
Flags: in-testsuite?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.