762846 - [responsive mode] the user should be able to create its own presets

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

We could add a "add preset…" menuitem at the end of the menulist.
The current code already look at a pref to see if there are any custom presets, so we only need the UI (menuitem + editor).

Comment 1

[hsablonniere]

Attachment: Responsive view preset creation

Comment 2

[hsablonniere]

(In reply to hsablonniere from comment #1)
> Created attachment 635107 [details]

I think creation of custom presets can be based on the current custom preset (obtained by resizing the window) since for now they only contain 2 infos : width and height.

Attachment 635107 [details] contains a linked mockup. This is probably too complicated but it could be a base for discussion.

Comment 3

[Paul Rouget [:paul]]

Thanks Hubert! I wasn't thinking about something like that at first, but I think it's an interesting (better) approach.

This is what I was thinking about:

1) In the menu, we add a "Edit presets" button that would bring a new window with a UI to create/delete/edit preset.

What you propose is:

2) We add a save/delete/edit button in the toolbar to save the current size.

pros/cons:

1)

pros:
- doesn't invade the toolbar.
- let you manually write the size.

cons:
- extra window
- extra window
- extra window
- "edit presets" button could be confused with "edit current size" (as in bug 762848)
- need to build these kind of awful "2 columns UIs"

2)

pros:
- no window
- no window
- no window
- easy to understand (not confusing)

cons:
- Toolbar invasion: the create/edit/delete buttons will be used very rarely. But they will be always visible. Bad.
- you have to reach the perfect size with the mouse. But that can be fixed by bug 762848

Maybe we could imagine moving these buttons inside the preset menu. Not sure though.

Comment 4

[Paul Rouget [:paul]]

Here is an idea:


- We keep the current UI.
- if the click on the dropdown menu,
  - if the current resolution is "Custom"
    - we add at the end of the menu a separator and a "Remember dimensions…" item
  - if the current resolution is a preset
    - we add at the end of the menu a separator and a "Forget preset" item
- if "Remember dimensions" is clicked
  - we just prompt for a name
  - we add the new preset, and add a "(aName)" at the end of the menuitem
- if "Forget preset"
  - we remove the related menuitem
  - switch to custom mode

There's is no way to "update" a preset, but saving as an existing named would update it.

Hubert, Kevin, what do you guys think?

It's not really powerful, but it's not intrusive (only one extra menu item and a prompt). The presets are saved in a pref, so it would pretty easy to build an addon that would do much more.

Comment 5

[Kevin Dangoor]

I like the compactness of what you're suggesting in comment 4. I worry hat it's a bit nonstandard, but it seems unlikely that anyone would be confused by it.

adding Brian Dils for opinion from a UX person.

Comment 6

[Paul Rouget [:paul]]

Let's start that way and focus on the inner mechanism first.

Hubert, I assign this bug to you and we'll see how it goes.
Please find me on IRC to talk about the implementation details.

Comment 7

[hsablonniere]

I'm also worried that it would feel a bit non standard. What about a mix of the two ideas?

* The last menu item you suggest could be a button just next to the menu.
* The update mecanism could simply be the one you described with the same name behaviour...
* The toolbar would not be really invaded since only one place for a button would be needed.

Comment 8

[Paul Rouget [:paul]]

I really don't think we want to have a button in the toolbar. This is a minor feature that we don't want to expose to all the users. People will try to click it (or click on it by mistake) and will mess with the presets.

Again, let's first implement the mechanism (via a button in the toolbar if you want) and then see how it can be integrated in the UI. Brian Dils (UX) will help us here.

Comment 9

[hsablonniere]

@paul

I'll implement it your way. No problem ;-)

I've started to dig the code and understand it. MDN+XUL is my new friend. I'll see the details with you on IRC.

Comment 10

[hsablonniere]

Attachment: First iteration (diff patch)

This first try lacks some stuffs :

* Maybe we need some sorting between the options in the list
* I don't ask for a custom name (with some kind of prompt) yet
* The delete button is there even if the user didn't save any preset (is it wrong?)
* The buttons should be moved into the list
* The code surely contains bad stuffs regarding general mozilla coding style

Waiting for your awesome review to iterate...

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 636192 [details] [diff] [review]
First iteration (diff patch)

(In reply to hsablonniere from comment #10)
> * Maybe we need some sorting between the options in the list
> * I don't ask for a custom name (with some kind of prompt) yet
> * The delete button is there even if the user didn't save any preset (is it
> wrong?)
> * The buttons should be moved into the list
> * The code surely contains bad stuffs regarding general mozilla coding style
> 
> Waiting for your awesome review to iterate...

I don't have anything to say about the code. Looks good :)

Next steps:
- sorting
- custom name

If we start sorting, maybe we want something a little more solid to hold the menuitem/preset pairs. We sill store the presets as arrays, but we could use a Map (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Map) to store them in memory (easier access). We should move from selectedIndex to selectedItem, and use the map to retrieve the preset. This is just an idea, you don't have to do it.

To trigger the prompt, use the nsIPromptService: https://developer.mozilla.org/en/nsIPromptService
> let res = {};
> Services.prompt.prompt(window, "window title", "question", res, null, {});
> let name = res.value;

Comment 12

[hsablonniere]

Are you sure it's safe to use ES6 Map right now? I'm not sure I understood right how you would use them to handle menuitem and presets...

Anyway, I agree on the fact that we need a better way to handle menuitem/preset pairs.

Working on prompt...

Comment 13

[hsablonniere]

Ok basic code for prompt is ready. A few questions :

For now the setMenuLabel method builds the label displayed in the list based on width and height. Can we directly use the key property of a preset or is it supposed to be used by for a different purpose.

If we have to use something else I would go for a label property on presets and setMenuLabel should be able to use it or to build a label like now.

Comment 14

[Paul Rouget [:paul]]

(In reply to hsablonniere from comment #12)
> Are you sure it's safe to use ES6 Map right now?

Yes.

> I'm not sure I understood right how you would use them to handle menuitem and presets...

Currently, when a menuitem is selected, we know which preset to activate because they have the same index. But if we start sorting, we are not sure that the index of the menuitem will be the same as in the array.

When we create a menuitem, we could link it to its preset:
init:
> this.menuitems = new Map();

building the menuitems:
> this.menuitems.set(menuitem, preset)

when a menuitem is clicked:
> let preset = this.menuitems.get(selectedItem)

> For now the setMenuLabel method builds the label displayed in the list based
> on width and height. Can we directly use the key property of a preset or is
> it supposed to be used by for a different purpose.

The key is a key. It happens to use the same formatting we use for the label.
But the formatting might change in the future.

We use the key to remember what preset was selected. Then when we start the Responsive Mode, we know which preset to use at first.

> If we have to use something else I would go for a label property on presets
> and setMenuLabel should be able to use it or to build a label like now.

We could have a label in the preset. It needs to be optional as we shipped a version of Firefox without labels (people might have stored presets already in the pref).

Comment 15

[Paul Rouget [:paul]]

Any update on this?

Comment 16

[hsablonniere]

Working on it right now. I'll be able to post a new patch during the week...

Comment 17

[hsablonniere]

Attachment: Second iteration

DONE :
* I've introduced the Map, the menulist and presets are now order by width/height.
* The user is prompted for a name when adding a preset. The name is used in the list.
* I changed the way we were refering to the custom preset and menuitem (always the first) to prevent spreading this particular infos everywhere in the code.
* The current selection is always retrieved using the menulist selection and the preset using that along with the map.
* I may added to much sets in prefs but it seems that closing firefox direclty doesn't call RUI_unload. Any ideas?

TODO:

* Validate the user input for names (do we? how?)
* Format the label the same way we do for custom...
* How do we want to handle the rotate state behaviour along restarts? It doesn't seems to work well :-(
* The buttons should be moved into the list
* Some refactoring

Waiting for your awesome review to iterate...

Comment 18

[Paul Rouget [:paul]]

Attachment: v0.2

rebased

Comment 19

[Paul Rouget [:paul]]

I'll played with the code. It feels right. I believe it's the right approach. Good work.

Some of these comments are not directly part of your work but might make the code a little better.

~~~~

>+    this.rotatebutton.setAttribute("tabindex", "1");
Why? 0 is fine I think.

~~~~

>     for (let i = 0; i < this.presets.length; i++) {
This should become:
> for (let preset of this.presets)

~~~~

You might want to get rid of this.customMenuItem. It's not really needed as you can simple do this.menuitems.get("custom")

~~~~

>+      if (aPreset.name != null) {
>+        size += ' (' + aPreset.name + ')';
>+      }

This needs to be localized. Something like that:
> let str = this.strings.formatStringFromName("responsiveUI.namedResolution", [size, aPreset.name], 2);
See: https://developer.mozilla.org/en/nsIStringBundle

You'll need to add a string there too:
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/responsiveUI.properties

Something like that:
> responsiveUI.namedResolution=%S (%S)

Same thing for:
>+    Services.prompt.prompt(null, "Responsive Design View", "Give a name to the " + w + "x" + h + " preset", newName, null, {});

~~~~

>+  sortPresets: function RUI_sortPresets(aPresetA, aPresetB) {
Don't add this function to RUI. Just add an anonymous (or inline) function to the sort call:
> this.presets.sort(function RUI_sortPresets(){});

~~~~

insertNewMenuitem can stay in the addPreset method.

~~~~

> Validate the user input for names (do we? how?)

I don't know.

> Format the label the same way we do for custom...

Yep.

> How do we want to handle the rotate state behaviour along restarts? It doesn't seems to work well :-(

What's the problem?

> The buttons should be moved into the list & Some refactoring

That should be the next step.

Thanks for the hard work :)

Comment 20

[hsablonniere]

Thanks for the remarks.

The add and remove buttons will be (alternatively) the first button with tabindex sets to 0 so the rotate button is now : this.rotatebutton.setAttribute("tabindex", "1");

I'll try to use more of new ECMA stuffs, let and for..of feel really nice :p

About the input validation, I don't think it's necessary.

I applied almost all your remarks. I'll work on the localization and moving the buttons into the list.

About the rotate button, I'll try to explain better if I still have issues/questions about it.

Thx.

Comment 22

[Jeremie Patonnier :Jeremie]

Attachment: UI Mockup proposal

Hi!

After Paul told me this bug were here, I quickly read it as well as bug 762848 and 767678

Here is a very rough proposal for an updated interface for the Responsive Design View :

I added a toolbar at the top to be consistent with others dev tools. It allows to add a close button to the left which can solve bug 767678

There is 2 input filed that display the width and height of the viewport, they can be edited by using the keyboard if needed which can solve bug 762848

The first button is a dropdown button that will display all the preset values for the viewport size.

The second button is an add/delete button : 
 1. if the value in the input file match an existing record, the button turn into a delete button (with a minus symbol for example), 
 2. if not, it's an add button (with a plus symbol for example)

The third button is just the rotate button. Note that hitting this button, will change the viewport as well as the values in the input fields.

Note that I also add a grip at the bottom of the viewport to allow changing the height with the mouse (which is not possible yet!)

So, that said, what do you think? ;)

Comment 23

[hsablonniere]

I like :

* the direct dual input of both height and width
* the small drop down icon
* the +/- button idea
* the rotate icon
* the bottom resize handle

I'm wondering about :

* The close button on the left, but maybe it's just a Mac thing
* 'w' and 'h' vs 'width' and 'height' in the inputs

We currently have a notion of name/label for preset. Your UI mockup doesn't display that. Do you think it could be added to the toolbar? Would it still be present in the drop down list?

I'm not sure how we should iterate. My current code has your +/ mecanism but not the double input. Maybe I could take care of bug 762848 at the same time.

Thx very much for sharing your ideas...

Comment 24

[Paul Rouget [:paul]]

Jeremie, I encourage you to file a "[Responsive Mode] interaction design v2" bug, where you can start exploring such ideas and gather feedback. You will then break this meta bug into implementation bugs.

In this bug, feel free to work with Hubert on a good intermediate (as in, not too intrusive) UI for this specific feature (adding/removing presets).

Comment 25

[Paul Rouget [:paul]]

(In reply to Jeremie Patonnier from comment #22)
> I added a toolbar at the top to be consistent with others dev tools. It
> allows to add a close button to the left which can solve bug 767678

We don't want to do that. We'd like to keep the current integration (as per UX discussion).

> There is 2 input filed that display the width and height of the viewport,
> they can be edited by using the keyboard if needed which can solve bug 762848
> 
> The first button is a dropdown button that will display all the preset
> values for the viewport size.

So then, you can't see the name of the preset (if it's a named preset).

> The second button is an add/delete button : 
>  1. if the value in the input file match an existing record, the button turn
> into a delete button (with a minus symbol for example), 
>  2. if not, it's an add button (with a plus symbol for example)

This feature (adding/removing presets) is probably not a feature most of the people will use. So it's better if we avoid adding a button for that.

Maybe a "Add preset" at the end of the Preset dropdown menu would act like the "+", and when a custom preset has been added, then we can show the "-" button.

> The third button is just the rotate button. Note that hitting this button,
> will change the viewport as well as the values in the input fields.

bug 761165

> Note that I also add a grip at the bottom of the viewport to allow changing
> the height with the mouse (which is not possible yet!)

bug 778174

Just in case you guys don't know: I gather all the Responsive Mode bugs into this bug: bug 749628 (see the dependency list).

Comment 26

[hsablonniere]

Ok paul. So what do I do with this bug? Should I proceed with the behaviour we discussed and the last remarks you gave me?

Comment 27

[Paul Rouget [:paul]]

(In reply to hsablonniere from comment #26)
> Ok paul. So what do I do with this bug? Should I proceed with the behaviour
> we discussed and the last remarks you gave me?

Yep

Comment 28

[hsablonniere]

I applied all your remarks except one :

I couldn't get rid of this.customMenuItem. I can't do this.menuitems.get("custom") since the this.menuitems is not a Map(String, Object). The keys are the menuitem objects...

I moved the buttons into the menupopup. I had to differentiate buttons from preset menu items, I hope it's the right way.

I also integrated changes from Michael Ratcliffe (74218d787b06).

I'll publish the patch tomorrow (long building running right now :p)

Comment 29

[hsablonniere]

Attachment: v0.3

Comment 30

[Paul Rouget [:paul]]

(just got back from holidays, I'll try to take a look a this this week)

Comment 31

[Paul Rouget [:paul]]

Instead of "Add preset":
> Remember preset
or
> Save as preset

Instead of "Remove preset":
> Forget preset

For the popup, I would do that instead:
String1:
> titleName this preset
String2:
> for example "myPhone"

Hubert, this looks like it's ready for a formal review (yeah!). But before that, we need some tests.

This is how I see it:
1) open Responsive Mode
2) resize to a custom size
3) save & enter name
4) select another preset
5) close Responsive Mode
6) open Responsive Mode
7) make sure the menuitem is present
8) select it
9) make sure the browser size is right
10) delete the saved preset and 2 other ones
11) close
12) open
13) make sure the presets list is right

To create this test:
create a new file in browser/devtools/responsivedesign/test
add the file name to the Makefile
read the other files there to understand how it works

To run your specific test:

TEST_PATH=browser/devtools/responsivedesign/test/yourfile.js make -C ~/xxx/mozilla/objdir mochitest-browser-chrome

To run all the responsive design tests:

TEST_PATH=browser/devtools/responsivedesign/test/ make -C ~/xxx/mozilla/objdir mochitest-browser-chrome

Comment 32

[Paul Rouget [:paul]]

Hubert, let me know if I can help.

Comment 33

[hsablonniere]

Sorry to reply so late. I'm on holidays. This means good weather = no work and rain = productive awesome mozilla contributions. For now it's mostly really good weather ;-)

Thanks for your review.

Comment 34

[hsablonniere]

I've made some progress. Harth helped me. I mocked the Service.prompt to ease the simulation.

I'm stuck on 8 but I don't really know why. Maybe it's the time. Will resume soon...

Comment 35

[Paul Rouget [:paul]]

(In reply to hsablonniere from comment #34)
> I'm stuck on 8 but I don't really know why. Maybe it's the time. Will resume
> soon...

menulist.selectedItem = anItem

or

menulist.selectedIndex = 42

Comment 36

[hsablonniere]

Attachment: [responsive mode] the user should be able to create its own presets

Comment 37

[hsablonniere]

I posted it with bzexport :p Let me know if I've done anything wrong.

Sometimes I'm not sure if something is necessary in the tests :
* calling this :
  let container = gBrowser.getBrowserContainer();
* executeSoon vs direct call?

So there may be too much stuff. Let me know.

Comment 38

[Paul Rouget [:paul]]

Going through try: https://tbpl.mozilla.org/?tree=Try&rev=96ee30eb6342

(reviewing right now)

Comment 39

[Paul Rouget [:paul]]

Comment on attachment 662684 [details] [diff] [review]
[responsive mode] the user should be able to create its own presets

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

Excellent!

Please fix these little things.

If the try turn green, I'll land that this week.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +350,3 @@
>        let menuitem = doc.createElement("menuitem");
> +      menuitem.setAttribute('ispreset', true);
> +      this.menuitems.set(menuitem, preset);

Double quote.

@@ +388,5 @@
>     * When a preset is selected, apply it.
>     */
>    presetSelected: function RUI_presetSelected() {
> +    if (this.menulist.selectedItem.getAttribute('ispreset') === 'true') {
> +      this.selectedItem = this.menulist.selectedItem;

double quote.

@@ +561,5 @@
> +    let selectedPreset = this.menuitems.get(this.selectedItem);
> +
> +    // If the selected preset is not the custom one, we select it and assign it the current dimensions
> +    if (!selectedPreset.custom) {
> +      // Be careful of the current rotate state

Obvious. Remove the comment.

@@ +574,2 @@
>      }
> +

extra space?

::: browser/devtools/responsivedesign/test/browser_responsiveui.js
@@ +49,5 @@
>        is(content.innerHeight, height, "preset " + c + ": dimension valid (height)");
>  
>        testOnePreset(c - 1);
>      }
> +    testOnePreset(instance.menulist.firstChild.childNodes.length - 4);

Can you add a comment to explain why it's 4?

::: browser/devtools/responsivedesign/test/browser_responsiveuiaddcustompreset.js
@@ +20,5 @@
> +    Services.prompt = {
> +      prompt: function(aParent, aDialogTitle, aText, aValue, aCheckMsg, aCheckState) {
> +        aValue.value = "Testing preset";
> +      }
> +    };

That's wild :)

::: browser/locales/en-US/chrome/browser/devtools/responsiveUI.properties
@@ +11,5 @@
>  # LOCALIZATION NOTE  (responsiveUI.rotate): label of the rotate button.
>  responsiveUI.rotate=rotate
>  
> +# LOCALIZATION NOTE  (responsiveUI.addPreset): label of the add preset button.
> +responsiveUI.addPreset=add preset

Add Preset

@@ +14,5 @@
> +# LOCALIZATION NOTE  (responsiveUI.addPreset): label of the add preset button.
> +responsiveUI.addPreset=add preset
> +
> +# LOCALIZATION NOTE  (responsiveUI.removePreset): label of the remove preset button.
> +responsiveUI.removePreset=remove preset

Remove Preset

@@ +22,5 @@
>  # current size of the page. For example: "400x600".
>  responsiveUI.customResolution=%S (custom)
> +
> +# LOCALIZATION NOTE  (responsiveUI.namedResolution): label of custom items with a name
> +# in the menulist of the toolbar. The preset gets its size and custom name displayed between parens.

Remove "The preset gets its size and custom name displayed between parens".

Comment 40

[hsablonniere]

Attachment: [responsive mode] the user should be able to create its own presets

Comment 41

[Paul Rouget [:paul]]

Hubert, as you can see here: https://tbpl.mozilla.org/?tree=Try&rev=96ee30eb6342
your prompt thing broke a test: https://tbpl.mozilla.org/php/getParsedLog.php?id=15523042&tree=Try

You forgot to reattach oldPrompt.

Comment 42

[hsablonniere]

Attachment: [patch] [responsive mode] the user should be able to create its own presets

Comment 43

[hsablonniere]

Sorry about that.

Comment 44

[Paul Rouget [:paul]]

(In reply to hsablonniere from comment #43)
> Sorry about that.

It's ok :) a "try server" is a place where we can run tests before landing. Exactly for this reasons. Nobody run all the tests.

Comment 45

[Paul Rouget [:paul]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=de2b7db8a17f

Comment 46

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=37d3286e67ba

Comment 47

[Paul Rouget [:paul]]

Thank you Hubert!

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/37d3286e67ba