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

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

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.

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.

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.

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.

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.

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.

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

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

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

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.

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

Any update on this?

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

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

rebased

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

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.

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? ;)

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

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

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

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?

(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

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)

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

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

Hubert, let me know if I can help.

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.

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

(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

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.

Going through try: https://tbpl.mozilla.org/?tree=Try&rev=96ee30eb6342 (reviewing right now)

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

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.

Sorry about that.

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

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

Thank you Hubert!