Devtools UI: Add node-specific actions into a ghost toolbar in DOM

Devtools UI: Add node-specific actions into a ghost toolbar in DOM

This change adds a button in the elements gutter which toggles the node-specific toolbar. This toolbar is triggered from this icon or markers when present. When markers are present, the icon is not visible and the toolbar is automatically opened.

This change also removes the elements toolbar completely by moving layout mode to the styles sidebar pane.

BUG=528387
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202482

[samli, 2015-09-04 18:20:56]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-09-04 18:20:57]

PTAL.

One known bug I don't know how to fix yet. When you collapse a parent element
such that the selected element is no longer visible, the toolbar should
disappear but it currently does not.

[paulirish, 2015-09-04 18:23:47]

Can you make a tracking bug and attach a screenshot/video?

I'd prefer if this started behind an experiment.

[samli, 2015-09-04 18:24:51]

On 2015/09/04 at 18:23:47, paulirish wrote:
> Can you make a tracking bug and attach a screenshot/video?
> 
> I'd prefer if this started behind an experiment.

It's behind the material design experiment, where the toolbar has been.

[samli, 2015-09-04 18:36:52]

On 2015/09/04 at 18:24:51, samli wrote:
> On 2015/09/04 at 18:23:47, paulirish wrote:
> > Can you make a tracking bug and attach a screenshot/video?
> > 
> > I'd prefer if this started behind an experiment.
> 
> It's behind the material design experiment, where the toolbar has been.

Video: https://youtu.be/-lMYNQim7ms
Screenshots: http://i.imgur.com/1dZaMtJ.png
http://i.imgur.com/D0yXYwx.png

[pfeldman, 2015-09-05 01:14:14]

Can we have the three dots icon there by default?

[paulirish, 2015-09-08 16:43:44]

On 2015/09/05 at 01:14:14, pfeldman wrote:
> Can we have the three dots icon there by default?

Agree, three horizontal dots. 
Might want to drop the background/border from them too, so they are floating
even more sublty.

[samli, 2015-09-09 18:48:56]

PTAL; see updated description for change in behavior

Screenshots:
http://i.imgur.com/pDNDRB7
http://i.imgur.com/BJaqLvk
http://i.imgur.com/ER2nsF5
http://i.imgur.com/rSvrJfa
http://i.imgur.com/ziXSgnN

[samli, 2015-09-09 18:51:55]

Screenshots:
http://i.imgur.com/pDNDRB7.png
http://i.imgur.com/BJaqLvk.png
http://i.imgur.com/ER2nsF5.png
http://i.imgur.com/rSvrJfa.png
http://i.imgur.com/ziXSgnN.png

[paulirish, 2015-09-09 19:07:34]

We're thinking the dots don't look actionable.

One attempt to fix (bg color lighter): http://i.imgur.com/lx5v1MJ.png


Pavel is wondering about moving the "..." icon to above the selected row. 
Sam, do you have a screenshot of what that looked like beforehand?

[samli, 2015-09-09 20:28:29]

On 2015/09/09 at 19:07:34, paulirish wrote:
> We're thinking the dots don't look actionable.
> 
> One attempt to fix (bg color lighter): http://i.imgur.com/lx5v1MJ.png
> 
> 
> Pavel is wondering about moving the "..." icon to above the selected row. 
> Sam, do you have a screenshot of what that looked like beforehand?

I don't sorry, but when I started with that, it looked very odd since its small
and fits better within that row.

I like the lighter bg color, since its more subtle and these are fairly uncommon
actions.

[samli, 2015-09-15 20:45:34]

ping, is there an issue with this? This is behind the material experiment.

[pfeldman, 2015-09-15 20:56:50]

https://codereview.chromium.org/1304173004/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://codereview.chromium.org/1304173004/diff/140001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsTreeOutline.js:446:
_createNodeToolbar: function()
I think this all needs to be done on the elements panel level since
ElementsTreeOutline is a reusable component that is instantiated for console,
event listeners, object property sections, etc. The actions we are looking at
are ElementsPanel-only.

[samli, 2015-09-16 21:36:22]

PTAL. Updated, also removes elements toolbar completely.

[pfeldman, 2015-09-16 21:53:14]

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:155:
_initializeActionsToolbar: function()
Move this above for a better diff.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:182:
_updateActionsToolbarButtons: function(node)
Move this back for a better diff.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:203: var actionsToolbar =
this._nodeActionsElement;
get those later - you don't need them for non-element nodes.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:211: if
(treeElement.selected && node.nodeType() === Node.ELEMENT_NODE &&
actionsToolbar._node !== node) {
this is already true.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:211: if
(treeElement.selected && node.nodeType() === Node.ELEMENT_NODE &&
actionsToolbar._node !== node) {
we use __node when we bind properties to elements.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:213: if
(this._forceShowActions) {
why do we need this flag?

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:244: if
(!this._actionsToolbarVisible())
ensureVisible bails about it actionsToolbar is not visible?

[samli, 2015-09-16 22:12:54]

Addressed all comments, PTAL.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:213: if
(this._forceShowActions) {
On 2015/09/16 at 21:53:14, pfeldman wrote:
> why do we need this flag?

This lets us auto-show the toolbar if there is a marker present. eg. if the
element is hidden and I reselect it, the toolbar opens and shows that the
element is hidden.

https://codereview.chromium.org/1304173004/diff/180001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:244: if
(!this._actionsToolbarVisible())
On 2015/09/16 at 21:53:14, pfeldman wrote:
> ensureVisible bails about it actionsToolbar is not visible?

Renamed.

[pfeldman, 2015-09-16 22:14:43]

Upload?

[samli, 2015-09-16 22:15:38]

On 2015/09/16 at 22:14:43, pfeldman wrote:
> Upload?

Oops, didn't press enter

[samli, 2015-09-16 23:25:38]

Done, did not add a clear button here.

[pfeldman, 2015-09-17 00:06:18]

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:166: if
(treeElement.selected && actionsToolbar.__node !== node) {
You just fetched the selected one, surely it is .selected.

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:222: * @param {?boolean}
toggled
we typically make params like this optional.

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsTreeElement.js (right):

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsTreeElement.js:282:
this.selectionElement.style.height = listItemElement.offsetHeight + "px";
Is this related to the dgozman's change rollout?

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsTreeElement.js:1201:
this._gutterContainer.classList.toggle("has-decorations",
this._decorationsElement.hasChildNodes());
Do this within setTitle and rename it?

[pfeldman, 2015-09-17 00:08:04]

lgtm % nits

[samli, 2015-09-17 18:08:04]

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:166: if
(treeElement.selected && actionsToolbar.__node !== node) {
On 2015/09/17 at 00:06:18, pfeldman wrote:
> You just fetched the selected one, surely it is .selected.

Done

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsPanel.js:222: * @param {?boolean}
toggled
On 2015/09/17 at 00:06:18, pfeldman wrote:
> we typically make params like this optional.

Done.

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
File Source/devtools/front_end/elements/ElementsTreeElement.js (right):

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsTreeElement.js:282:
this.selectionElement.style.height = listItemElement.offsetHeight + "px";
On 2015/09/17 at 00:06:18, pfeldman wrote:
> Is this related to the dgozman's change rollout?

Yeah, doesn't show up in my diff locally.

https://codereview.chromium.org/1304173004/diff/220001/Source/devtools/front_...
Source/devtools/front_end/elements/ElementsTreeElement.js:1201:
this._gutterContainer.classList.toggle("has-decorations",
this._decorationsElement.hasChildNodes());
On 2015/09/17 at 00:06:18, pfeldman wrote:
> Do this within setTitle and rename it?

Done.

[samli, 2015-09-17 18:08:06]

The CQ bit was checked by samli@chromium.org

[samli, 2015-09-17 18:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1304173004/#ps230001
(title: "Address nits")

[commit-bot: I haz the power, 2015-09-17 18:08:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304173004/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304173004/230001

[commit-bot: I haz the power, 2015-09-17 18:11:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-17 18:11:11]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[samli, 2015-09-17 21:08:43]

The CQ bit was checked by samli@chromium.org

[samli, 2015-09-17 21:08:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1304173004/#ps250001
(title: "Rebase; remove material breadcrumb from experiment")

[commit-bot: I haz the power, 2015-09-17 21:09:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304173004/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304173004/250001

[commit-bot: I haz the power, 2015-09-17 22:50:08]

Committed patchset #14 (id:250001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202482