Bug 30554 - Web Inspector: Watched Expressions Buttons Do Not Match Inspector Styles
Summary: Web Inspector: Watched Expressions Buttons Do Not Match Inspector Styles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Podivilov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-19 23:42 PDT by Joseph Pecoraro
Modified: 2011-05-24 08:03 PDT (History)
5 users (show)

See Also:


Attachments
[IMAGE] Current Buttons (25.44 KB, image/png)
2009-10-19 23:42 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Proposed Buttons in Patch (26.00 KB, image/png)
2009-10-19 23:43 PDT, Joseph Pecoraro
no flags Details
[PATCH] Style Buttons in Sidebar Panes (2.11 KB, patch)
2009-10-19 23:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Another proposed images that saves a bit space. (103.99 KB, image/png)
2009-10-20 06:32 PDT, Pavel Feldman
no flags Details
patch with tentative code and css changes (3.29 KB, patch)
2009-11-12 10:35 PST, Patrick Mueller
pfeldman: review-
Details | Formatted Diff | Diff
patch with tentative code and css changes - with images this time (42.34 KB, patch)
2009-11-12 10:45 PST, Patrick Mueller
no flags Details | Formatted Diff | Diff
screen shot of title buttons (with lousy artwork) (42.87 KB, image/png)
2009-11-12 11:50 PST, Patrick Mueller
no flags Details
Patch. (15.94 KB, patch)
2011-02-08 05:05 PST, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Watch Expressions pane with the patch applied. (25.52 KB, image/png)
2011-02-08 05:12 PST, Pavel Podivilov
no flags Details
Hovered watch expression. (26.26 KB, image/png)
2011-02-08 05:13 PST, Pavel Podivilov
no flags Details
Patch. (13.87 KB, patch)
2011-04-15 04:17 PDT, Pavel Podivilov
pfeldman: review-
Details | Formatted Diff | Diff
Screenshot with patch applied. (22.15 KB, image/png)
2011-04-15 04:17 PDT, Pavel Podivilov
no flags Details
Patch. (16.65 KB, patch)
2011-05-23 02:55 PDT, Pavel Podivilov
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2009-10-19 23:42:11 PDT
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
Comment 1 Joseph Pecoraro 2009-10-19 23:42:43 PDT
Created attachment 41483 [details]
[IMAGE] Current Buttons
Comment 2 Joseph Pecoraro 2009-10-19 23:43:07 PDT
Created attachment 41484 [details]
[IMAGE] Proposed Buttons in Patch
Comment 3 Joseph Pecoraro 2009-10-19 23:46:30 PDT
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.
Comment 4 Timothy Hatcher 2009-10-20 06:15:29 PDT
Comment on attachment 41485 [details]
[PATCH] Style Buttons in Sidebar Panes

Nice!
Comment 5 Pavel Feldman 2009-10-20 06:32:42 PDT
Created attachment 41503 [details]
[IMAGE] Another proposed images that saves a bit space.
Comment 6 Joseph Pecoraro 2009-10-20 07:38:23 PDT
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?
Comment 7 Pavel Feldman 2009-10-20 07:44:52 PDT
> 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.
Comment 8 Joseph Pecoraro 2009-10-20 08:32:12 PDT
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.
Comment 9 Timothy Hatcher 2009-10-20 12:24:31 PDT
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.
Comment 10 Pavel Feldman 2009-10-28 12:33:41 PDT
(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).
Comment 11 Patrick Mueller 2009-10-28 13:11:02 PDT
(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.
Comment 12 Timothy Hatcher 2009-10-28 14:34:41 PDT
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.
Comment 13 Patrick Mueller 2009-11-12 10:35:04 PST
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.
Comment 14 Pavel Feldman 2009-11-12 10:44:54 PST
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?)
Comment 15 Patrick Mueller 2009-11-12 10:45:24 PST
Created attachment 43077 [details]
patch with tentative code and css changes - with images this time

forgot the images on the last patch
Comment 16 Timothy Hatcher 2009-11-12 10:48:49 PST
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.
Comment 17 Patrick Mueller 2009-11-12 11:50:26 PST
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
Comment 18 Patrick Mueller 2009-11-12 11:56:40 PST
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.
Comment 19 Timothy Hatcher 2009-11-12 12:00:23 PST
I think the CSS Joe added for the buttons was generic, and should stay incase we need buttons like those.
Comment 20 Timothy Hatcher 2009-11-12 12:00:53 PST
I'll make some images for add and reload.
Comment 21 Patrick Mueller 2009-11-12 14:15:04 PST
(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.
Comment 22 Timothy Hatcher 2009-11-12 14:43:24 PST
event.stopPropagation is correct. we need it in a few places for this very reason.
Comment 23 Timothy Hatcher 2009-11-17 14:05:48 PST
I still need to make image for this. I should have time later this week.
Comment 24 Patrick Mueller 2009-11-17 15:30:14 PST
(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.
Comment 25 Pavel Podivilov 2011-02-08 05:05:31 PST
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?
Comment 26 Pavel Podivilov 2011-02-08 05:12:40 PST
Created attachment 81623 [details]
Watch Expressions pane with the patch applied.
Comment 27 Pavel Podivilov 2011-02-08 05:13:22 PST
Created attachment 81624 [details]
Hovered watch expression.
Comment 28 Pavel Feldman 2011-02-25 03:02:06 PST
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?
Comment 29 Pavel Feldman 2011-03-06 04:25:50 PST
Ping. Do you intend to follow up?
Comment 30 Pavel Podivilov 2011-03-09 03:00:17 PST
(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.
Comment 31 Pavel Podivilov 2011-04-15 04:17:04 PDT
Created attachment 89766 [details]
Patch.
Comment 32 Pavel Podivilov 2011-04-15 04:17:48 PDT
Created attachment 89767 [details]
Screenshot with patch applied.
Comment 33 Pavel Podivilov 2011-04-15 04:23:17 PDT
(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.
Comment 34 Pavel Feldman 2011-05-12 13:12:17 PDT
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?
Comment 35 Pavel Podivilov 2011-05-23 02:55:33 PDT
Created attachment 94392 [details]
Patch.
Comment 36 Pavel Podivilov 2011-05-23 02:58:05 PDT
(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.
Comment 37 Pavel Podivilov 2011-05-24 08:03:32 PDT
Committed r87140: <http://trac.webkit.org/changeset/87140>