We'd like to use VariableView as a JSON / manifest editor in the App Manager. We have lots of room on page to dedicate for this purpose, so we should come up with a larger layout / design for the VariablesView here.

Here's a general mockup that Darrin posted in bug 808371 comment 5.

(In reply to J. Ryan Stinnett [:jryans] from comment #2) > Created attachment 813248 [details] > Mockup v2 Looks good. Some comments: - the lines background colors might not be contrasted enough - the dashed green underline is a little bit too agressive - the tooltip arrow is an unknown ui pattern, and is very different from the error/warning boxes - property values horizontal position depends on the property name increment level, I think this is messy. Can we instead align all the values? Like in the xcode plist editor.

Since I am just posting Darrin's mockups to ensure they don't get lost, I'll flag him to make sure he sees these comments.

> - the lines background colors might not be contrasted enough Agreed, will adjust. > - the dashed green underline is a little bit too agressive Agreed, will adjust. > - the tooltip arrow is an unknown ui pattern, and is very different from the > error/warning boxes This is a good point, I will see what I can do. Perhaps removing the 'arrow' shape behind it is enough (just have the field type/desc. inline) > - property values horizontal position depends on the property name increment > level, I think this is messy. Can we instead align all the values? Like in > the xcode plist editor. XCode indents child properties as well (http://cl.ly/image/2W2e0q091x41)... I agree it's not as 'clean' but it conveys the hierarchy in a clearer way. I will revise the mockup and post something as soon as I'm ready.

Thanks jryans for pointing this out: Paul, I misread and didn't realize you were referring to aligning the *values*. I will see how this looks also, good suggestion.

Is something like this closer to what you had in mind Paul?

(In reply to Darrin Henein [:darrin] from comment #7) > Created attachment 817203 [details] > Mockup v3 > > Is something like this closer to what you had in mind Paul? Yes! We're trying to go away from 3D designs. Maybe the arrow can be flatter. Other than that, it looks good.

Dark variant of tooltip for Manifest Editor.

Light variant of tooltip for Manifest Editor.

I like both.

The dark version seems more readable/distringuishable from the background.

Yeah, I lean slightly towards dark as well, but both are quite nice!

I was thinking: floating panels are hard. What if there's not enough room on the right to show the arrow? Can we have instead an inline panel? (same way we show warning and errors)

The reason I have it as a tooltip is so it can still be visible when there are errors/warnings (arguably this is when you would need that information the most). I guess if we didn't need the description we could just have another column with the property type (i.e. String, Array, etc.)? I can mock that up to show what I mean.

Here is an option which shows the property type but no description... Will we have descriptions for each field? If so, I still think the popover/tooltip is the best approach.

Is the yellow highlight shown on "accessKeyLabel" on hover or on focus? What about the gray background on that entire row?

Here's a rough version of the main colors from the mockup. Note that the inline warnings / errors will be handled separately (bug 922148). I haven't been able to get the layout of indented names but aligned values to work yet. I tried setting a fixed width on the name and using negative margin after that, but this only works for 2 levels, not N levels. The only solutions that come to mind involve styling with JS or adding some kind of "level X" class, but then there would a maximum level. Benvie, do you see a CSS-only way to achieve this layout?

The following two rules should theoretically suffice: .variable-or-property > .title > .separator { -moz-box-flex: 1; } .variable-or-property > .title > .value { min-width: 40vw; } If additional icons or letters appear on the right, as an artifact of the fact that JSON objects might lack the proper object descriptors, add this as well: .variable-or-property > .title > spacer, .variable-or-property > .title > .variable-or-property-non-writable-icon, .variable-or-property > .title > .variable-or-property-frozen-label, .variable-or-property > .title > .variable-or-property-sealed-label, .variable-or-property > .title > .variable-or-property-non-extensible-label { display: none; } (we should probably have a single class defining all of those icons and labels, to keep the CSS simpler)

Didn't mean to clear this needinfo?

(In reply to Victor Porof [:vp] from comment #19) > The following two rules should theoretically suffice: > > .variable-or-property > .title > .separator { > -moz-box-flex: 1; > } > > .variable-or-property > .title > .value { > min-width: 40vw; > } I gave that a try, but no luck[1]. I could give the names a fixed width, and that brings the values close to alignment, but then child properties that are below the main level still have their values indented just like the names are. The main issue seems to be the indenting the names is achieved by indenting the entire container that hold the name, separator, value, etc. So, it's quite challenging to align all values while that is in effect. I can get the values aligned easily enough if I remove the indentation from the entire row... But I don't really see a great way to re-create the indentation effect on just the names (using CSS only). Victor, any other CSS approaches you can think of? [1]: https://dl.dropboxusercontent.com/s/g79v2vx1dcwhhhi/Manifest%20Editor%20Layout.png

> The main issue seems to be the indenting the names is achieved by indenting the entire container that hold the name, separator, value, etc. So, it's quite challenging to align all values while that is in effect. Indeed. Can we change the markup to just indent the first column?

(In reply to J. Ryan Stinnett [:jryans] from comment #21) > The main issue seems to be the indenting the names is achieved by indenting > the entire container that hold the name, separator, value, etc. So, it's > quite challenging to align all values while that is in effect. Indeed. Can we change the markup to just indent the first column? I think that would also benefit the normal variable view.

(In reply to J. Ryan Stinnett [:jryans] from comment #21) > (In reply to Victor Porof [:vp] from comment #19) > > The following two rules should theoretically suffice: > > > > .variable-or-property > .title > .separator { > > -moz-box-flex: 1; > > } > > > > .variable-or-property > .title > .value { > > min-width: 40vw; > > } > > I gave that a try, but no luck[1]. It will work if you add in the second part of the css in comment 19.

This works and doesn't require any special changes to the variables view.

Looks like this.

(In reply to Paul Rouget [:paul] from comment #22) > Indeed. Can we change the markup to just indent the first column? That's hard because the margin would have to be different for every indentation level. That means doing it in js. I don't like js.

This adds support for a generic "aligned values" mode, but does not enable it anywhere. This is similar to Victor's patch in attachment 820139 [details] [diff] [review], but also handles some hover issues.

Adds an "actions first" mode that moves the delete button (and soon the add button from bug 808371...) to the beginning of the row. Seemed like the cleanest way to go, rather than attempting some fancy CSS dance. I tried the CSS "order" property, but it didn't seem to work for me.

This enables the features from the first two parts, and overrides colors to match the mockup, at least for the elements that exist today. I used a pseudo-element for the remove icon mostly because Darrin is on PTO (I assume...), so we can swap in a real graphic later if we think it will improve the appearance.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=70cc4735787a

(In reply to J. Ryan Stinnett [:jryans] from comment #29) > I tried the CSS "order" property, but it didn't seem to work for me. Did you try -moz-box-ordinal-group ?

Comment on attachment 820684 [details] [diff] [review] Part 1: Aligned values option for Variables View Review of attachment 820684 [details] [diff] [review]: ----------------------------------------------------------------- Sweet! r+ with comments addressed. ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +2585,5 @@ > input.className = "plain " + aClassName; > input.setAttribute("value", initialString); > + if (!this._variablesView.alignedValues) { > + input.setAttribute("flex", "1"); > + } Nit: I think this could be more nicely written as > input.setAttribute("flex", +!this._variablesView.alignedValues); ::: browser/devtools/shared/widgets/widgets.css @@ +68,5 @@ > + > +.variables-view-container[aligned-values] *:not(:hover) .variables-view-delete { > + display: initial; > + visibility: hidden; > +} Make this always be the default, not only for aligned-values. This would fix another problem as well (besides the full-page design, this would fix an issue with scrollbars appearing on top of the delete button when trying to reach it, because changing the "display" causes a reflow). See the review in the next patch. ::: browser/themes/linux/devtools/widgets.css @@ +571,5 @@ > + -moz-box-flex: 1; > +} > + > +.variables-view-container[aligned-values] .title > .value { > + width: 70vw; It would be great if this size was made configurable. For example, 70vw won't work in the debugger because it's too wide. Definitely followup material though.

Comment on attachment 820686 [details] [diff] [review] Part 2: Actions first option for Variables View Review of attachment 820686 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +2413,5 @@ > if (ownerView.delete) { > let deleteNode = this._deleteNode = this.document.createElement("toolbarbutton"); > deleteNode.className = "plain variables-view-delete"; > + if (this._variablesView.actionsFirst) { > + deleteNode.setAttribute("ordinal", 0); Since in the previous patch "visibility: hidden" will be made the default behavior when not hovered, the ordinal when actionsFirst isn't true will need to be 1, to avoid always having some empty space on the right. I would write it as this: > deleteNode.setAttribute("ordinal", +!this._variablesView.actionsFirst);

(In reply to Victor Porof [:vp] from comment #33) > (In reply to J. Ryan Stinnett [:jryans] from comment #29) > > I tried the CSS "order" property, but it didn't seem to work for me. > > Did you try -moz-box-ordinal-group ? That does appear to work actually, assuming you first remove the "ordinal" attributes.

Carrying over Victor's r+ from attachment 820684 [details] [diff] [review]. (In reply to Victor Porof [:vp] from comment #34) > ::: browser/devtools/shared/widgets/VariablesView.jsm > @@ +2585,5 @@ > > input.className = "plain " + aClassName; > > input.setAttribute("value", initialString); > > + if (!this._variablesView.alignedValues) { > > + input.setAttribute("flex", "1"); > > + } > > Nit: I think this could be more nicely written as > > input.setAttribute("flex", +!this._variablesView.alignedValues); I find that much harder to read than the if block. A search of m-c only shows 2 uses of "+!" in JS, so I think I will leave this as-is. > ::: browser/devtools/shared/widgets/widgets.css > @@ +68,5 @@ > > + > > +.variables-view-container[aligned-values] *:not(:hover) .variables-view-delete { > > + display: initial; > > + visibility: hidden; > > +} > > Make this always be the default, not only for aligned-values. Done, seems nicer now! :) > ::: browser/themes/linux/devtools/widgets.css > @@ +571,5 @@ > > + -moz-box-flex: 1; > > +} > > + > > +.variables-view-container[aligned-values] .title > .value { > > + width: 70vw; > > It would be great if this size was made configurable. For example, 70vw > won't work in the debugger because it's too wide. Definitely followup > material though. Yep, I will also need to configure it assuming we do put the type info in it's own column. Filed bug 930040.

Carrying over Victor's r+ from attachment 820686 [details] [diff] [review]. (In reply to Victor Porof [:vp] from comment #35) > ::: browser/devtools/shared/widgets/VariablesView.jsm > @@ +2413,5 @@ > > if (ownerView.delete) { > > let deleteNode = this._deleteNode = this.document.createElement("toolbarbutton"); > > deleteNode.className = "plain variables-view-delete"; > > + if (this._variablesView.actionsFirst) { > > + deleteNode.setAttribute("ordinal", 0); > > Since in the previous patch "visibility: hidden" will be made the default > behavior when not hovered, the ordinal when actionsFirst isn't true will > need to be 1, to avoid always having some empty space on the right. Actually, to avoid the empty space, it's sufficient to just remove ordinal when actionsFirst is false, so that's what I've done. > I would write it as this: > > deleteNode.setAttribute("ordinal", +!this._variablesView.actionsFirst); Again, seems too cryptic to me.

Comment on attachment 820691 [details] [diff] [review] Part 3: Manifest editor layout and colors This is very nice. I'm wondering if some of these modifications should not be included by default in the Variable View… Can we have only one CSS file that we'd include in themes/*/devtools/widgets.css ? >+.manifest-editor .variables-view-delete, >+.manifest-editor .variables-view-delete:hover, >+.manifest-editor .variables-view-delete:active { >+ list-style-image: none; >+ -moz-image-region: initial; >+} `initial` … you're being fancy here :) I like this. >+.manifest-editor .variables-view-delete::before { >+ width: 12px; >+ height: 12px; >+ content: "-"; >+ text-align: center; >+ color: #FFF; >+ font-weight: bold; >+ font-size: 9px; >+ background-color: #FF6B00; >+ border-radius: 6px; >+ display: inline-block; >+} If you're waiting for Darrin to get the right icon, I'd prefer if you'd use /themes/shared/devtools/app-manager/images/remove.svg And can you make it so this button is not indented (might be hard). There are things missing compared to the design. I guess you filed (or will file) followups?

Comment on attachment 821047 [details] [diff] [review] Part 2: Actions first option for Variables View (v2) Review of attachment 821047 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +916,5 @@ > /** > + * Specifies if action buttons (like delete) should be placed at the beginning > + * or end of a line. > + */ > + actionsFirst: false, I thought about this a bit, and it won't be possible to change this property after the variables view initializes. If we were, for example, to expose this as a pref in the debugger (to experiment with different layouts and how people like them or not), this would have to be live. Do you have anything against approaching this exactly like alignedValues setter? (having an "actions-first" attribute on the container and setting -moz-box-ordinal-group in css).

In addition to comments below, I've changed all fonts to be monospace as discussed on IRC, to help align them better. (In reply to Paul Rouget [:paul] from comment #39) > I'm wondering if some of these modifications should not be included by > default in the Variable View… Yes, perhaps! I think it's nice that we can use the manifest editor to play around with some different options and iterate on them quickly without worrying as much about other Variables Views. Down the road we can talk about enabling them for all Variables Views if we want to. > Can we have only one CSS file that we'd include in > themes/*/devtools/widgets.css ? Yep, moved to browser/themes/shared/devtools/manifest-editor.inc.css. > >+.manifest-editor .variables-view-delete::before { > >+ width: 12px; > >+ height: 12px; > >+ content: "-"; > >+ text-align: center; > >+ color: #FFF; > >+ font-weight: bold; > >+ font-size: 9px; > >+ background-color: #FF6B00; > >+ border-radius: 6px; > >+ display: inline-block; > >+} > > If you're waiting for Darrin to get the right icon, I'd prefer if you'd use > /themes/shared/devtools/app-manager/images/remove.svg Copied to manifest-delete.svg to edit the fill color. > And can you make it so this button is not indented (might be hard). It does seem hard... Personally, I kind of like that they are indented myself. But if others would like them always aligned left, let's tackle that in a followup. > There are things missing compared to the design. I guess you filed (or will > file) followups? Yes, there are quite a few (see the dependency tree of bug 912912). I'm using this bug to get the initial appearance in place, and then will continue to refer to the mockup as further features are added.

Carrying over Victor's r+ from attachment 820684 [details] [diff] [review]. (In reply to Victor Porof [:vp] from comment #40) > ::: browser/devtools/shared/widgets/VariablesView.jsm > @@ +916,5 @@ > > /** > > + * Specifies if action buttons (like delete) should be placed at the beginning > > + * or end of a line. > > + */ > > + actionsFirst: false, > > I thought about this a bit, and it won't be possible to change this property > after the variables view initializes. If we were, for example, to expose > this as a pref in the debugger (to experiment with different layouts and how > people like them or not), this would have to be live. > > Do you have anything against approaching this exactly like alignedValues > setter? (having an "actions-first" attribute on the container and setting > -moz-box-ordinal-group in css). Seems like a good idea! I've updated the patch to do this.

Comment on attachment 821799 [details] [diff] [review] Part 3: Manifest editor layout and colors (v2) About manifest-delete.svg, can you not duplicate remove.svg, change its fill value, and in projects.css, change `.button-remove` opacity? (In reply to J. Ryan Stinnett [:jryans] from comment #41) > It does seem hard... Personally, I kind of like that they are indented > myself. But if others would like them always aligned left, let's tackle > that in a followup. I'd prefer if it could stay aligned. File a followup bug, a needinfo? Darrin.

Carrying over Paul's r+ from attachment 821799 [details] [diff] [review]. (In reply to Paul Rouget [:paul] from comment #43) > About manifest-delete.svg, can you not duplicate remove.svg, change its fill > value, and in projects.css, change `.button-remove` opacity? Okay, looks good to me. I've made this change, so now only one SVG file. > (In reply to J. Ryan Stinnett [:jryans] from comment #41) > > It does seem hard... Personally, I kind of like that they are indented > > myself. But if others would like them always aligned left, let's tackle > > that in a followup. > > I'd prefer if it could stay aligned. File a followup bug, a needinfo? Darrin. I've filed bug 930907 to resolve this.

https://hg.mozilla.org/integration/fx-team/rev/34f1f5b52bb7 https://hg.mozilla.org/integration/fx-team/rev/71ecbc0583e2 https://hg.mozilla.org/integration/fx-team/rev/d76790460c79

https://hg.mozilla.org/mozilla-central/rev/34f1f5b52bb7 https://hg.mozilla.org/mozilla-central/rev/71ecbc0583e2 https://hg.mozilla.org/mozilla-central/rev/d76790460c79