Bug 36965 – Web Inspector: start editing DOM and styles on click-and-pause.

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

[Pavel Feldman]

Also couple of bugfixes and startEditing utility improvement. Not sure how valuable this change is though.

[Pavel Feldman]

Created attachment 52301 [details]
[PATCH] Proposed change.

[Timothy Hatcher]

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.

[Pavel Feldman]

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!

[Timothy Hatcher]

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

[Pavel Feldman]

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.

[Timothy Hatcher]

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?

[Pavel Feldman]

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

[Timothy Hatcher]

Should the WebInspector.currentFocusElement part stay though?

[Pavel Feldman]

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

[Pavel Feldman]

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.

[Nikita Vasilyev]

Pavel, have you considered using custom event for that? I think it will make API cleaner. Just my two cents.

[Roland Takacs]

This patch is already in the trunk.