Bug 32460 – Regression: Web inspector deletes nodes when editing css

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

[David Beck]

When you try to edit css rules in the inspector and hit the backspace, the node you are editing gets deleted.

[Daniel Bates]

Confirmed in latest nightly (r52039).

[Daniel Bates]

Confirmed this is a regression. First appeared in the Mac nightly r51978. If I have some time, I will try to bisect the revisions to find the exact changset/look through recent trac changes.

[Pavel Feldman]

No need to bisect, regressed in r51961. The real reason is that Del/Backspace is being processed as a shortcut. Should be processed as an action on focused element instead.

[Pavel Feldman]

Investigation note:

The reason is that Backspace is processed in handleKeyEvent that is now being called from documents' global handler. So that if any control does not mute the event via preventDefault/stopPropagation, it gets into the default handler and is processed as a shortcut. As a result, any input field nukes selected element on backspace when there is nothing to erase.

I was not familiar with our focus / key handling code before, so I explored it a bit today and was quite surprised. In particular I learned that our tree outline component is not focusable by design. And divs are not focusable by design :) As a result, "Up" event on elements tree is being dispatched on "main-panels" target. I think it is very wrong - it should be dispatched on tree outline container instead.

Most of our panels / sidebars should be focusable and support keyboard handling. It'd be also nice to have proper focus traversal policy when going through the panel (like tab brings you from elements tree to the sidebar and iterates over the styles / metrics / etc. containers there).

Can we make tree outline focusable by means of tabIndex or something similar? We would then add explicit key listeners on right components and handle everything the right way. handleKeyEvent on Panel would transform into handleShortcut and would only be responsible for shortcuts, processing only unhandled events.

[Timothy Hatcher]

(In reply to comment #4)
> Can we make tree outline focusable by means of tabIndex or something similar?
> We would then add explicit key listeners on right components and handle
> everything the right way. handleKeyEvent on Panel would transform into
> handleShortcut and would only be responsible for shortcuts, processing only
> unhandled events.

Yes. But it is nice that up and down arrows work in the whole Elements panel.

[Pavel Feldman]

pfeldman: anyways, i think we should agree upon the focus / keyboard thing. cos i think up/down working in entire panel is not good if for example we make styles focusable and up/down is pressed there...

xenon: if up/down work in other areas sure, otherwise i think up/down should bubble to the panel and be handled by the tree

pfeldman: i want to make all trees traversable

xenon: ok

pfeldman: from the ui standpoint, imagine now there are several focusable components on elements panel. you start on elements tree. then you press tab and focus goes to the toolbar all buttons are focusable, but we can make tabIndex -1 for them and it will be kinda single traverse step. then it gets to "Styles" expandable compartment. Space triggers that compartment (collapses/expands). Press tab again, you get into the Styles contents where Up/Down do their local job.

xenon: sounds fine. but toolbar should be first, then the tree, then the sidebar items. toolbar buttons should all be seperate, like a real app.

pfeldman: this brings us to the question of the focus ui identification. For trees, blue background of the selected focused node is enough. But what about "Styles" expandable group. How to show it has a focus?

xenon: not sure. maybe the header is brighter/colored.

pfeldman: ok.

[Pavel Feldman]

Patch info: Now all the treeoutlines are focusable, sections and their titles are also focusable. Change contains couple of drive-by fixes such as canceling of the attribute edit nuking attr info and maintaining proper selection while deleting nodes.

[Pavel Feldman]

Created attachment 44790 [details]
[PATCH] Proposed fix.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44790 [details] without any errors.

[Pavel Feldman]

*** Bug 32520 has been marked as a duplicate of this bug. ***

[Timothy Hatcher]

Comment on attachment 44790 [details]
[PATCH] Proposed fix.

> -    _promptKeyDown: function(event)
> +    _onKeyDown: function(event)

I prefer the old function name or just "_keyDown". The "on" prefixes have always bugged me. This comment applies to the whole patch.


> -        // No need to call _updateTitle here, it will be called after the nodeValue is committed.
> +        // Need to restore attributes / node structure.
> +        this._updateTitle();

> -        // No need to call _updateTitle here, the editing code will revert to the original text.
> +        // Need to restore attributes structure.
> +        this._updateTitle();

Does this regress attribute tabbing? I removed _updateTitle from these places to make tabbing in attributes work in: http://trac.webkit.org/changeset/50303

[Pavel Feldman]

(In reply to comment #11)
> (From update of attachment 44790 [details])
> > -    _promptKeyDown: function(event)
> > +    _onKeyDown: function(event)
> 
> I prefer the old function name or just "_keyDown". The "on" prefixes have
> always bugged me. This comment applies to the whole patch.
> 

Done.

> 
> > -        // No need to call _updateTitle here, it will be called after the nodeValue is committed.
> > +        // Need to restore attributes / node structure.
> > +        this._updateTitle();
> 
> > -        // No need to call _updateTitle here, the editing code will revert to the original text.
> > +        // Need to restore attributes structure.
> > +        this._updateTitle();
> 
> Does this regress attribute tabbing? I removed _updateTitle from these places
> to make tabbing in attributes work in: http://trac.webkit.org/changeset/50303

Yes, https://bugs.webkit.org/show_bug.cgi?id=32512

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/CallStackSidebarPane.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/DataGrid.js
	M	WebCore/inspector/front-end/DatabaseQueryView.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/front-end/ElementsTreeOutline.js
	M	WebCore/inspector/front-end/Panel.js
	M	WebCore/inspector/front-end/PropertiesSection.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/SidebarPane.js
	M	WebCore/inspector/front-end/SourceFrame.js
	M	WebCore/inspector/front-end/TextPrompt.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/treeoutline.js
	M	WebCore/inspector/front-end/utilities.js
Committed r52099