Full page design for VariablesView

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: WebIDE
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 12 obsolete attachments)

306.31 KB, image/png
Details
306.14 KB, image/png
Details
308.61 KB, image/png
Details
10.42 KB, patch
jryans
: review+
Details | Diff | Splinter Review
4.44 KB, patch
jryans
: review+
Details | Diff | Splinter Review
25.27 KB, image/png
Details
13.56 KB, patch
jryans
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 1

4 years ago
Created attachment 812262 [details]
Mockup

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

Updated

4 years ago
Attachment #812262 - Attachment description: manifest-editor-2-small.png → Mockup
(Assignee)

Comment 2

4 years ago
Created attachment 813248 [details]
Mockup v2
Attachment #812262 - Attachment is obsolete: true

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
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.
Flags: needinfo?(dhenein)
> - 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.
Created attachment 817203 [details]
Mockup v3

Is something like this closer to what you had in mind Paul?
Flags: needinfo?(dhenein) → needinfo?(paul)

Comment 8

4 years ago
(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.
Flags: needinfo?(paul)
Created attachment 817247 [details]
Mockup v4 Dark

Dark variant of tooltip for Manifest Editor.
Created attachment 817248 [details]
Mockup v4 Light

Light variant of tooltip for Manifest Editor.

Comment 11

4 years ago
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!

Comment 14

4 years ago
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.
Created attachment 818458 [details]
Mockup v5

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?
Flags: needinfo?(dhenein)
Created attachment 819302 [details] [diff] [review]
WIP v1

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)
Flags: needinfo?(dhenein)
Didn't mean to clear this needinfo?
Flags: needinfo?(dhenein)

Updated

4 years ago
Priority: P2 → P1
(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
Flags: needinfo?(vporof)

Comment 22

4 years ago
> 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?

Comment 23

4 years ago
(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.
Flags: needinfo?(vporof)
Created attachment 820139 [details] [diff] [review]
vview-full-page-design

This works and doesn't require any special changes to the variables view.
Attachment #820139 - Attachment is patch: true
Created attachment 820141 [details]
Screen Shot 2013-10-22 at 9.00.50 AM.png

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

Updated

4 years ago
Assignee: bbenvie → jryans
Created attachment 820684 [details] [diff] [review]
Part 1: Aligned values option for Variables View

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.
Attachment #819302 - Attachment is obsolete: true
Attachment #820139 - Attachment is obsolete: true
Attachment #820684 - Flags: review?(vporof)
(Assignee)

Updated

4 years ago
Attachment #820684 - Attachment description: Aligned values option for Variables View → Part 1: Aligned values option for Variables View
Created attachment 820686 [details] [diff] [review]
Part 2: Actions first option for Variables View

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.
Attachment #820686 - Flags: review?(vporof)
Created attachment 820691 [details] [diff] [review]
Part 3: Manifest editor layout and colors

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.
Attachment #820691 - Flags: review?(paul)
Created attachment 820693 [details]
Current Appearance.png
Attachment #813248 - Attachment is obsolete: true
Attachment #817203 - Attachment is obsolete: true
Attachment #820141 - Attachment is obsolete: true
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.
Attachment #820684 - Flags: review?(vporof) → review+
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);
Attachment #820686 - Flags: review?(vporof) → review+
Flags: needinfo?(dhenein)
(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.
Created attachment 821043 [details] [diff] [review]
Part 1: Aligned values option for Variables View (v2)

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.
Attachment #820684 - Attachment is obsolete: true
Attachment #821043 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 930040
Created attachment 821047 [details] [diff] [review]
Part 2: Actions first option for Variables View (v2)

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.
Attachment #820686 - Attachment is obsolete: true
Attachment #821047 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 925921

Updated

4 years ago
Depends on: 928144

Comment 39

4 years ago
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?
Attachment #820691 - Flags: review?(paul)
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).
Attachment #821047 - Flags: feedback?(jryans)
Created attachment 821799 [details] [diff] [review]
Part 3: Manifest editor layout and colors (v2)

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.
Attachment #820691 - Attachment is obsolete: true
Attachment #821799 - Flags: review?(paul)
Created attachment 821843 [details] [diff] [review]
Part 2: Actions first option for Variables View (v3)

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.
Attachment #821047 - Attachment is obsolete: true
Attachment #821843 - Flags: review+
Attachment #821047 - Flags: feedback?(jryans)
(Assignee)

Updated

4 years ago
Blocks: 930092

Comment 43

4 years ago
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.
Attachment #821799 - Flags: review?(paul) → review+
Created attachment 822178 [details]
Current Appearance v2.png
Attachment #820693 - Attachment is obsolete: true
Created attachment 822191 [details] [diff] [review]
Part 3: Manifest editor layout and colors (v3)

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.
Attachment #821799 - Attachment is obsolete: true
Attachment #822191 - Flags: review+
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
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/34f1f5b52bb7
https://hg.mozilla.org/mozilla-central/rev/71ecbc0583e2
https://hg.mozilla.org/mozilla-central/rev/d76790460c79
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.