Bug 36965 - Web Inspector: start editing DOM and styles on click-and-pause.
Summary: Web Inspector: start editing DOM and styles on click-and-pause.
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 Feldman
URL:
Keywords:
Depends on:
Blocks: 36727 116924
  Show dependency treegraph
 
Reported: 2010-04-01 09:01 PDT by Pavel Feldman
Modified: 2013-05-29 01:21 PDT (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed change. (16.31 KB, patch)
2010-04-01 09:02 PDT, Pavel Feldman
timothy: review-
Details | Formatted Diff | Diff
[PATCH] Proposed change. (21.62 KB, patch)
2010-04-01 13:36 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-04-01 09:01:44 PDT
Also couple of bugfixes and startEditing utility improvement. Not sure how valuable this change is though.
Comment 1 Pavel Feldman 2010-04-01 09:02:46 PDT
Created attachment 52301 [details]
[PATCH] Proposed change.
Comment 2 Timothy Hatcher 2010-04-01 09:46:23 PDT
Comment on attachment 52301 [details]
[PATCH] Proposed change.


> +        this._clickAndPauseHelper = new ClickAndPauseHelper(this.listItemElement, this._clickAndPause.bind(this), true, true);

I think a better name for this would be ClickAndPauseGestureRecognizer.

> -        if (this.isEventWithinDisclosureTriangle(event))
> -            return;

Why isn't this needed anymore?

>      _dblclickSelector: function(event)
>      {
> +        this._clickAndPauseHelper.dblClick(event);

Why do you need to call dblClick when ClickAndPauseHelper listens for dblclick?


>      ondblclick: function(event)
>      {
> +        this._clickAndPauseHelper.dblClick(event);

Ditto.


> +function ClickAndPauseHelper(node, listener, externalClickDispatch, externalDblClickDispatch)

I'd like to see this in a new file. Utilities.js should just be DOM additions and simple functions. No state machines.

> +    if (!externalClickDispatch)
> +    if (!externalDblClickDispatch)

Oh I guess this is why dblClick needs called.

> +    click: function(e)

I think handleClick or processClick would be better names.

> +        this._clickX = e.pageX;
> +        this._clickY = e.pageY;

I don't think you need these, see below.

> +        this._timer = setTimeout(this._fireEvent.bind(this), 300);

You want to call reset before making a new timer, if the use clicks twice but isn't a double click.

> +    dblClick: function(e)

I think handleDoubleClick or processDoubleClick would be better names.

> +    _mouseMove: function(e)
> +    {
> +        if (this._clickX !== e.pageX || this._clickY !== e.pageY)
> +            this.reset();
> +    },

Resetting on any mouse movement is bad, since some people are twitchy with the mouse after a click. Either have some slop room (like 5-10 pixels in any direction) or remove this altogether. The Finder allows you to move the mouse any distance after the click, so I say remove the mousemove code.

We are not fully safe from click-drag to select, because you operate on click not mouse down. So you can click then quickly mouse down and darg and editing wont be prevented. So I thin you need to call reset on mousedown too. You will also need to make sure the mouseup after drag is not counded as a click, so it wont start the timer.
Comment 3 Pavel Feldman 2010-04-01 09:54:20 PDT
Thanks for the review. I'll comment on things that I think are right and will fix the rest.

(In reply to comment #2)
> (From update of attachment 52301 [details])
> 
> > +        this._clickAndPauseHelper = new ClickAndPauseHelper(this.listItemElement, this._clickAndPause.bind(this), true, true);
> 
> I think a better name for this would be ClickAndPauseGestureRecognizer.
> 

Ok.

> > -        if (this.isEventWithinDisclosureTriangle(event))
> > -            return;
> 
> Why isn't this needed anymore?

That's because we now get this event from TreeElement and this check has already been made.

> 
> >      _dblclickSelector: function(event)
> >      {
> > +        this._clickAndPauseHelper.dblClick(event);
> 
> Why do you need to call dblClick when ClickAndPauseHelper listens for dblclick?
> 

ClickAndPauseHelper is universal and you can tell it to register listeners for click and doubleclick or you'd like to reuse existing ones. It is important to be able to reuse existing ones in order to stop propagation exactly when it is needed.

> 
> >      ondblclick: function(event)
> >      {
> > +        this._clickAndPauseHelper.dblClick(event);
> 
> Ditto.

See above.

> 
> 
> > +function ClickAndPauseHelper(node, listener, externalClickDispatch, externalDblClickDispatch)
> 
> I'd like to see this in a new file. Utilities.js should just be DOM additions
> and simple functions. No state machines.
> 

Ok, although this is a small thingy. It looks alike startEditing, so we might want to extract them into some EditingUtilities.js. Ok?

> > +    if (!externalClickDispatch)
> > +    if (!externalDblClickDispatch)
> 
> Oh I guess this is why dblClick needs called.
> 

Yes.

> > +    click: function(e)
> 
> I think handleClick or processClick would be better names.
>

Ok.
 
> > +        this._clickX = e.pageX;
> > +        this._clickY = e.pageY;
> 
> I don't think you need these, see below.

I'll experiment with no reset on move.

> 
> > +        this._timer = setTimeout(this._fireEvent.bind(this), 300);
> 
> You want to call reset before making a new timer, if the use clicks twice but
> isn't a double click.
> 

Ok.

> > +    dblClick: function(e)
> 
> I think handleDoubleClick or processDoubleClick would be better names.
> 

Ok.

> > +    _mouseMove: function(e)
> > +    {
> > +        if (this._clickX !== e.pageX || this._clickY !== e.pageY)
> > +            this.reset();
> > +    },
> 
> Resetting on any mouse movement is bad, since some people are twitchy with the
> mouse after a click. Either have some slop room (like 5-10 pixels in any
> direction) or remove this altogether. The Finder allows you to move the mouse
> any distance after the click, so I say remove the mousemove code.
> 
> We are not fully safe from click-drag to select, because you operate on click
> not mouse down. So you can click then quickly mouse down and darg and editing
> wont be prevented. So I thin you need to call reset on mousedown too. You will
> also need to make sure the mouseup after drag is not counded as a click, so it
> wont start the timer.

I'll try to test and see where it can fail, thanks!
Comment 4 Timothy Hatcher 2010-04-01 10:09:33 PDT
(In reply to comment #3)

> > > +function ClickAndPauseHelper(node, listener, externalClickDispatch, externalDblClickDispatch)
> > 
> > I'd like to see this in a new file. Utilities.js should just be DOM additions
> > and simple functions. No state machines.
> > 
> 
> Ok, although this is a small thingy. It looks alike startEditing, so we might
> want to extract them into some EditingUtilities.js. Ok?

EditingUtilities.js is a good idea. Right now editing isn't in utilities.js either it is in inspector.js. But moving it all to EditingUtilities.js is good.
Comment 5 Pavel Feldman 2010-04-01 13:36:42 PDT
Created attachment 52329 [details]
[PATCH] Proposed change.

Addressed all the comments but could not get rid of the distance thing. Now only measures distance between mouse down and up, user can move mouse after the click and that won't affect the scheduled editing.
Comment 6 Timothy Hatcher 2010-04-01 13:50:52 PDT
Comment on attachment 52329 [details]
[PATCH] Proposed change.


> -    WebInspector.currentFocusElement = element;
> +    return {
> +        cancel: editingCancelled.bind(element),
> +        commit: editingCommitted.bind(element)
> +    };

What is this change for?
Comment 7 Pavel Feldman 2010-04-01 13:54:38 PDT
(In reply to comment #6)
> (From update of attachment 52329 [details])
> 
> > -    WebInspector.currentFocusElement = element;
> > +    return {
> > +        cancel: editingCancelled.bind(element),
> > +        commit: editingCommitted.bind(element)
> > +    };
> 
> What is this change for?

I was trying to implement edit with no delay in case user jumps from one attribute to another while editing single DOM element. At some point I needed to commit things fast, so I made startEditing return 'controller' that allows committing / canceling programmatically. I then reverted the change related to the instant edit because it did not work well,  but decided to leave this thing in place. It looks useful.
Comment 8 Timothy Hatcher 2010-04-01 13:58:17 PDT
Should the WebInspector.currentFocusElement part stay though?
Comment 9 Pavel Feldman 2010-04-02 08:43:51 PDT
Landing part 1.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/audits.css
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/treeoutline.js
Committed r57003
Comment 10 Pavel Feldman 2010-04-02 08:44:57 PDT
Comment on attachment 52329 [details]
[PATCH] Proposed change.

This prepares everything for the click and pause, but does not enable it just yet. I'd like to polish click and pause on sibling field while editing.
Comment 11 Nikita Vasilyev 2010-07-05 04:57:32 PDT
Pavel, have you considered using custom event for that? I think it will make API cleaner. Just my two cents.
Comment 12 Roland Takacs 2013-05-29 01:21:57 PDT
This patch is already in the trunk.