Bug 32460 - Regression: Web inspector deletes nodes when editing css
Summary: Regression: Web inspector deletes nodes when editing css
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords: Regression
: 32520 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-12-11 22:38 PST by David Beck
Modified: 2009-12-14 10:01 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] Proposed fix. (38.83 KB, patch)
2009-12-14 06:28 PST, Pavel Feldman
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Beck 2009-12-11 22:38:24 PST
When you try to edit css rules in the inspector and hit the backspace, the node you are editing gets deleted.
Comment 1 Daniel Bates 2009-12-12 21:59:24 PST
Confirmed in latest nightly (r52039).
Comment 2 Daniel Bates 2009-12-12 22:10:26 PST
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.
Comment 3 Pavel Feldman 2009-12-13 03:10:42 PST
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.
Comment 4 Pavel Feldman 2009-12-13 05:41:26 PST
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.
Comment 5 Timothy Hatcher 2009-12-13 05:57:26 PST
(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.
Comment 6 Pavel Feldman 2009-12-13 06:26:29 PST
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.
Comment 7 Pavel Feldman 2009-12-14 06:27:04 PST
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.
Comment 8 Pavel Feldman 2009-12-14 06:28:18 PST
Created attachment 44790 [details]
[PATCH] Proposed fix.
Comment 9 WebKit Review Bot 2009-12-14 06:33:01 PST
style-queue ran check-webkit-style on attachment 44790 [details] without any errors.
Comment 10 Pavel Feldman 2009-12-14 08:37:19 PST
*** Bug 32520 has been marked as a duplicate of this bug. ***
Comment 11 Timothy Hatcher 2009-12-14 09:05:35 PST
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
Comment 12 Pavel Feldman 2009-12-14 09:39:44 PST
(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
Comment 13 Pavel Feldman 2009-12-14 10:01:18 PST
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