Bug 30554 – Web Inspector: Watched Expressions Buttons Do Not Match Inspector Styles

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

As from the discussion on the original bug report for adding Watched Expressions, the UI for the buttons (currently unstyled) should show its buttons like the Panel Enabler views:
https://bugs.webkit.org/show_bug.cgi?id=27514#c3

[Joseph Pecoraro]

Created attachment 41483 [details]
[IMAGE] Current Buttons

[Joseph Pecoraro]

Created attachment 41484 [details]
[IMAGE] Proposed Buttons in Patch

[Joseph Pecoraro]

Created attachment 41485 [details]
[PATCH] Style Buttons in Sidebar Panes

The styles are for any button inside a Sidebar Pane (below class name "pane"). Watched Expressions is the only Pane with Buttons at the moment.

[Timothy Hatcher]

Comment on attachment 41485 [details]
[PATCH] Style Buttons in Sidebar Panes

Nice!

[Pavel Feldman]

Created attachment 41503 [details]
[IMAGE] Another proposed images that saves a bit space.

[Joseph Pecoraro]

I like Pavel's buttons better! But I think his two new buttons should each have 1 or 2 pixels more padding (they look thin to me).  They would also work well in other places where space is vital:

- Event Listeners - for when we add deletion
- Styles - for delete an entire Selector?
- Breakpoints - for deleting a breakpoint right through the sidebar?


I could still add my patch in the anticipation of there being <button> elements in the sidebar pane.  But if there is no use case for buttons then it shouldn't be needed.

Pavel did you have a patch for that or was it a mock up in need of a patch?

[Pavel Feldman]

> Pavel did you have a patch for that or was it a mock up in need of a patch?

Gimp. I agree on the padding - I just screw up with pixels.

I think you should land what you have since it anyway is making watches look better.

[Joseph Pecoraro]

Landed Button Style Patch in http://trac.webkit.org/changeset/49858
r49858 = f0a2f56f41a37de17441048d105f47ad3226a4c9

I'll clear the flags and keep this open for proposed new style.

[Timothy Hatcher]

Yeah I like Pavel's buttons! I don't think we need minus on Styles, since we don't have a selected rule. Maybe we wnat minus on the individual rules.

But for now I think just adding plus next to the gear makes sense.

[Pavel Feldman]

(In reply to comment #9)
> Yeah I like Pavel's buttons! I don't think we need minus on Styles, since we
> don't have a selected rule. Maybe we wnat minus on the individual rules.
> 
> But for now I think just adding plus next to the gear makes sense.

I think we need Plus and Reload (for refresh).

[Patrick Mueller]

(In reply to comment #10)
> I think we need Plus and Reload (for refresh).

I like the toolbar buttons also, agree that + and reload would be the things to add.  The reload could in theory go in a gear menu, except that folks may want single click access if they want to keep hitting reload over and over to watch something progress while actively running.  Unless the gear menu is sticky, then I'd vote for a reload button instead.

Initial thougt on reload icon would be a circled arrow, but not sure if that can be rendered well in the given space.  Could be anything, really, if there's a tooltip.  Maybe an asterisk?  I seem to remember some chatter about adding a page reload button the inspector; could use the same shape for the two.

[Timothy Hatcher]

I think reload is fine to have as a top level button. And we should have room for a arrow circle. I can make images for it.

[Patrick Mueller]

Created attachment 43075 [details]
patch with tentative code and css changes

Took at stab at doing the real panel title toolbuttons.  I created some new CSS that's generic enough for other pane title toolbuttons, and changed the Watch Expressions code to use that.  Also added some truly horrific images.

The code is probably fine.  Need real artwork.  And settle on better css class names perhaps.

[Pavel Feldman]

Comment on attachment 43075 [details]
patch with tentative code and css changes

> +    function addButtonClicked(event) {
> +        event.stopPropagation();

What is this for?

(r- for lack of images, could you also provide a screenshot?)

[Patrick Mueller]

Created attachment 43077 [details]
patch with tentative code and css changes - with images this time

forgot the images on the last patch

[Timothy Hatcher]

Comment on attachment 43075 [details]
patch with tentative code and css changes

> +    var addButton = document.createElement("div");

USe a button element, so it is semantic. Then you dont need a super verbose classname, you can just do:

.pane > .title > button.action.add

> +    addButton.addStyleClass("tool-button");

I like action better. Drop the "-button" suffix and use a button element.

> +    addButton.addStyleClass("pane-title-tool-button-add");

Just needed to be "add".


> +    var refreshButton = document.createElement("div");
> +    refreshButton.addStyleClass("tool-button");
> +    refreshButton.addStyleClass("pane-title-tool-button-refresh");

Same as above.

[Patrick Mueller]

Created attachment 43086 [details]
screen shot of title buttons (with lousy artwork)

You'll have to use your imagination w/r/t the artwork.  

Guess we also need to decide on the order of the buttons.  I think actually it should be reversed - "add", then "refresh", in l->r order

[Patrick Mueller]

Also need to remember to remove the CSS for the old buttons, for the final
patch.  Looks like it's just the "watch-expressions-buttons-container" class. 
Could be some of the CSS that Joe added in his patch on 2009-10-19 also.

[Timothy Hatcher]

I think the CSS Joe added for the buttons was generic, and should stay incase we need buttons like those.

[Timothy Hatcher]

I'll make some images for add and reload.

[Patrick Mueller]

(In reply to comment #14)
> (From update of attachment 43075 [details])
> > +    function addButtonClicked(event) {
> > +        event.stopPropagation();
> 
> What is this for?

Without the stopPropagation(), the click event is subsequently passed to the title bar itself, which proceeds to collapse the open panel.

There may be another way to prevent this from happening (hints?).  Also, converting the buttons to actual button elements may end up working around the need for this, if there's a button-specific event I can handle.

It's weird, I've had to use stopPropagation() twice today, and I don't think I've ever had to use it before.

[Timothy Hatcher]

event.stopPropagation is correct. we need it in a few places for this very reason.

[Timothy Hatcher]

I still need to make image for this. I should have time later this week.

[Patrick Mueller]

(In reply to comment #23)
> I still need to make image for this. I should have time later this week.

While you're doing that one, here's another.  Bug 29309 .  I think I tried using the existing icon, converted to grayscale, and it looked pretty good.

[Pavel Podivilov]

Created attachment 81622 [details]
Patch.

* Move add and refresh buttons from section body to header.
* Show delete expression button only when expression is hovered.

Timothy, could you please help with refresh and delete icons?

[Pavel Podivilov]

Created attachment 81623 [details]
Watch Expressions pane with the patch applied.

[Pavel Podivilov]

Created attachment 81624 [details]
Hovered watch expression.

[Pavel Feldman]

Comment on attachment 81622 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=81622&action=review

One question, otherwise good.

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:222
> +    _mouseOut: function()

Can you use :hover style instead?

[Pavel Feldman]

Ping. Do you intend to follow up?

[Pavel Podivilov]

(In reply to comment #29)
> Ping. Do you intend to follow up?

Sure, Timothy is going to make nice icons for refresh and delete buttons.

[Pavel Podivilov]

Created attachment 89766 [details]
Patch.

[Pavel Podivilov]

Created attachment 89767 [details]
Screenshot with patch applied.

[Pavel Podivilov]

(In reply to comment #28)
> (From update of attachment 81622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81622&action=review
> 
> One question, otherwise good.
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:222
> > +    _mouseOut: function()
> 
> Can you use :hover style instead?

Using :hover would be quite tricky given that tree elements are li and delete button should be visible when pointer is outside the element. Also it would require twiddling pixel arithmetics used for tree expansion triangles.
I think JavaScript implementation is preferable here since it is straightforward and easy to understand.

[Pavel Feldman]

Comment on attachment 89766 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=89766&action=review

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:204
> +        this._lastMouseMoveEvent = e;

why storing events?

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:208
> +        this._hoveredElement = this.propertiesElement.firstChild;

are you looking for event.target here?

[Pavel Podivilov]

Created attachment 94392 [details]
Patch.

[Pavel Podivilov]

(In reply to comment #34)
> (From update of attachment 89766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89766&action=review
> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:204
> > +        this._lastMouseMoveEvent = e;
> 
> why storing events?

Done, storing last pageY position instead.

> 
> > Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:208
> > +        this._hoveredElement = this.propertiesElement.firstChild;
> 
> are you looking for event.target here?

Not exactly, event.target may be enclosing ol element, while I'm looking for li element corresponding to event.pageY.

[Pavel Podivilov]

Committed r87140: <http://trac.webkit.org/changeset/87140>