Keyboard shortcuts to enter Edit mode are precedented and users are asking for it: > My one humble suggestion: regarding discoverability, I think that pressing > the return key should behave like double-clicking in that the selected thing > should enter edit-mode. Making the inspector more keyboard accessible > would go a long ways towards matching the input style of a lot of programmers... Precedence: - Elements Tree => start editing attribute or textnode Missing: - DOM Storage => Edit Key/Value Suggested Behavior: If a datagrid is editable, then pushing Enter/Return on a row in the datagrid should start editing that row. Comment from User:
I'll take a look at this - the DataGrid callbacks need more work, but that can be held off until someone else besides local and session storage need them.
Created attachment 43397 [details] [PATCH] Enter to edit editable datagrids
Comment on attachment 43397 [details] [PATCH] Enter to edit editable datagrids dataGridNodeFromEvent needs renamed if it does not take an event. I personally like it taking an event, in case it needs it.
Comment on attachment 43397 [details] [PATCH] Enter to edit editable datagrids > + when enter was hit to confirm text, it would propogate back to the datagrid typo: s/propogate/propagate/ > + } else if (event.keyCode === 13) { > + if (this._editCallback) { > + handled = true; > + // The first child of the selected element is the <td class="0-column">, > + // and that's what we want to edit. > + this._startEditing(this.selectedNode._element.children[0]); > + } > } For handling the Enter key we have a function that was recently added by keishi, which does some special case checking. Its actually used later in the code: isEnterKey(event) Otherwise, I tend to use event.keyIdentifier when the key is a nice string (in this case "Enter") so that there is no need for the inevitable comment explaining what the keyCode number is. That seems to be the case in other places in the Web Inspector's code. I like the comment you've added here about the <td> =) Hmm, maybe throw the check for "this._editCallback" as an && in the surrounding if to reduce the depth of braces? I guess that is just a personal style choice. What you have is fine.
(In reply to comment #3) > (From update of attachment 43397 [details]) > dataGridNodeFromEvent needs renamed if it does not take an event. > > I personally like it taking an event, in case it needs it. All dataGridNodeForEvent does is take an event and use the target to get its enclosing tr. It doesn't do anything that would need the event, it only needs an event target. Would the patch be ok if I renamed the function dataGridNodeFromEventTarget? Or do you think it needs more refactoring or a different style solution than that? It seems like dataGridNodeFromEventTarget would be a fine solution (unless you have an idea for a different name, it's a little wordy), but I'm definitely open to suggestions here.
Comment on attachment 43397 [details] [PATCH] Enter to edit editable datagrids dataGridNodeFromNode would be a better name for the new dataGridNodeFromEvent.
Created attachment 43402 [details] [PATCH] Fix + Tim's and Joe's comments
(In reply to comment #7) > Created an attachment (id=43402) [details] > [PATCH] Fix + Tim's and Joe's comments Hehe, you still have the typo "propogate" which should be "propagate" in the ChangeLog. But that can be fixed when you commit.
(In reply to comment #8) > (In reply to comment #7) > > Created an attachment (id=43402) [details] [details] > > [PATCH] Fix + Tim's and Joe's comments > > Hehe, you still have the typo "propogate" which should be "propagate" in the > ChangeLog. But that can be fixed when you commit. Damn :(, I even had the typo in code and had to fix it there or else it was broken, but forgot about the ChangeLog. Yeah, that's fixed now and will be for the commit.
Landed in r51119. Is there more we want to do for this bug? Marking it as Resolved but reopen if there are other places we want this behavior.
Hi all, I'm the original author of the quote that Joseph used in the initial bug description here and I'm happy to say that it looks like this takes care of most everything. It's very cool to see a blog comment become a real bug and have a fix in within a day. Thanks for the notification Joseph! The only spot for improvement I can see is for somehow being able to allow for return/enter to enter edit mode of the Style Attribute of the selected DOM node when the right-side of the Elements inspector has focus.
> The only spot for improvement I can see is for somehow being able to allow for > return/enter to enter edit mode of the Style Attribute of the selected DOM node > when the right-side of the Elements inspector has focus. Unfortunately the Styles sidebar lacks a "selection" or "focus" unless you're already editing in it. Maybe this is something we should look into.