Bug 31606 - Web Inspector: Enter/Return key should enter edit mode for Editable Fields
: Web Inspector: Enter/Return key should enter edit mode for Editable Fields
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-17 16:55 PST by
Modified: 2009-11-19 16:02 PST (History)


Attachments
[PATCH] Enter to edit editable datagrids (4.87 KB, patch)
2009-11-17 19:25 PST, Brian Weinstein
timothy: review-
bweinstein: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Fix + Tim's and Joe's comments (4.86 KB, patch)
2009-11-17 21:35 PST, Brian Weinstein
pfeldman: review+
bweinstein: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-17 16:55:11 PST
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:
------- Comment #1 From 2009-11-17 16:59:14 PST -------
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.
------- Comment #2 From 2009-11-17 19:25:57 PST -------
Created an attachment (id=43397) [details]
[PATCH] Enter to edit editable datagrids
------- Comment #3 From 2009-11-17 20:47:25 PST -------
(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.
------- Comment #4 From 2009-11-17 21:01:02 PST -------
(From update of attachment 43397 [details])
> +        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.
------- Comment #5 From 2009-11-17 21:18:53 PST -------
(In reply to comment #3)
> (From update of attachment 43397 [details] [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 #6 From 2009-11-17 21:22:40 PST -------
(From update of attachment 43397 [details])
dataGridNodeFromNode would be a better name for the new dataGridNodeFromEvent.
------- Comment #7 From 2009-11-17 21:35:51 PST -------
Created an attachment (id=43402) [details]
[PATCH] Fix + Tim's and Joe's comments
------- Comment #8 From 2009-11-17 21:59:29 PST -------
(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.
------- Comment #9 From 2009-11-17 22:00:59 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=43402) [details] [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.
------- Comment #10 From 2009-11-18 10:12:52 PST -------
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.
------- Comment #11 From 2009-11-19 09:30:10 PST -------
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.
------- Comment #12 From 2009-11-19 16:02:48 PST -------
> 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.