Bug 50565 – Web Inspector: Enable CSS property editing name/value-wise (like Firebug does)

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

[Alexander Pavlov (apavlov)]

Currently, every CSS property is represented by a solid chunk of text containing the entire syntax. A more usable option is to have separate input fields both for the property name and value. Generally, many users want the CSS editing UX to be closer to the Firebug's.

[Alexander Pavlov (apavlov)]

Created attachment 75691 [details]
[PATCH] Suggested solution

[Joseph Pecoraro]

Comment on attachment 75691 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=75691&action=review

Not quite a thorough review. Normally I apply these kinds of UI change patches to
get a feel for how things work. It would help if you included a video, or include
a nice description in the ChangeLog! I'll try to apply this later today!

> WebCore/inspector/front-end/StylesSidebarPane.js:1471
> +        if (!selectElement)
> +            selectElement = this.nameElement; // no arguments passed in - edit the name element by default

Nit: Comment as a sentence. Capital and period.

> WebCore/inspector/front-end/StylesSidebarPane.js:1539
> +        }

^^ These two functions (nameFinishHandler and valueFinishHandler) have very similar
code. Only the first condition is different. They should share more code.

> WebCore/inspector/front-end/inspector.js:1998
> +    function keyDownEventListener(event) {

Nit: brace on next line.

[Joseph Pecoraro]

Arg, I didn't get around to applying this today. I'll update+build overnight, and
try to apply it tomorrow morning.

[Alexander Pavlov (apavlov)]

Created attachment 75824 [details]
[PATCH] Nits fixed

[WebKit Review Bot]

Attachment 75824 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 75691 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=75691&action=review

Alexander this patch feels really good! It is much easier to tab complete a property
and go into a value! I did manage to hit an ASSERT, but I'm not sure if it was related
to your changes, and I couldn't reproduce it. I would like to know if you think you
can remove the small, but noticeable, delay between tabs and the UI updating.

In case you think you can deduce how the ASSERT could happen, the ASSERT was:

  InspectorStyleSheet.cpp:192: (setPropertyText)
  if (overwrite) {
    ASSERT(index < allProperties.size());

I will r- for now so some of the above comments can be addressed, but I really like
the way this patch is going.


> WebCore/inspector/front-end/StylesSidebarPane.js:1744
>          // Make the Changes and trigger the moveToNextCallback after updating

Nit: Could you fix this old code and add a period on the comment.

> WebCore/inspector/front-end/StylesSidebarPane.js:1748
> +        if (userInput !== previousContent && !this._newProperty || shouldCommitNewProperty) { // only if something changed, or adding a new property and need to commit it

Nit: I don't think comment actually improves anything.
Also, the condition could use more parens to be explicit. For example:

  false && false || true    - is true, but requires the programmer to remember the precedence
  (false && false) || true  - is the same, but much easier to read.

> WebCore/inspector/front-end/StylesSidebarPane.js:1750
> +            var propertyText = blankInput || (this._newProperty && /^\s*$/.test(this.valueElement.textContent)) ? "" : (isEditingName ? (userInput.replace(/:/g, "") + ": " + this.valueElement.textContent) : (this.nameElement.textContent + ": " + userInput));

Call me old fashioned but nested ternaries is a bit messy. I think this 
would be much clearer as an if/else chain.

> WebCore/inspector/front-end/StylesSidebarPane.js:1792
> +                if (alreadyNew && !valueChanged && (isEditingName ^ moveDirection === "backward"))

Nice use of XOR here. I think this could benefit from either another set of parens
to make the precedence explicit:

  (isEditingName ^ (moveDirection === "backward"))

Or storing the move check into a local:

  var isMovingBackward = moveDirection === "backward";
  ... (isEditingName ^ isMovingBackward) ...

Similar to what you did above with `var moveToOther`

> WebCore/inspector/front-end/inspector.js:2017
> +        } else if (result && result.indexOf("move-") === 0) {
> +            moveDirection = result.substring(5);
> +            if (event.keyIdentifier !== "U+0009") {
> +                // Emulate "Tab" keydown event.
> +                var evt = document.createEvent("KeyboardEvent");
> +                evt.initKeyboardEvent("keydown", true, true, null, "U+0009", "");
> +                element.dispatchEvent(evt);
> +            }
> +        }

We don't normally emulate events. We should just be able to directly call the code
that the simulated keydown would run. Can you do that?

[WebKit Review Bot]

Attachment 75824 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75824 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75824 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Created attachment 75889 [details]
[PATCH] Comments addressed. Also added tabbing between all rules shown for the given element

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 75691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75691&action=review
> 
> Alexander this patch feels really good! It is much easier to tab complete a property
> and go into a value! I did manage to hit an ASSERT, but I'm not sure if it was related
> to your changes, and I couldn't reproduce it. I would like to know if you think you
> can remove the small, but noticeable, delay between tabs and the UI updating.

I will look into what's going on. The delay is also noticeable for me in a debug build.

> In case you think you can deduce how the ASSERT could happen, the ASSERT was:
> 
>   InspectorStyleSheet.cpp:192: (setPropertyText)
>   if (overwrite) {
>     ASSERT(index < allProperties.size());

Could it have been a series of very quick actions (possibly hitting some race)?
 
> I will r- for now so some of the above comments can be addressed, but I really like
> the way this patch is going.

All the comments were addressed in the patch just attached.

[Pavel Feldman]

Comment on attachment 75889 [details]
[PATCH] Comments addressed. Also added tabbing between all rules shown for the given element

View in context: https://bugs.webkit.org/attachment.cgi?id=75889&action=review

> WebCore/inspector/front-end/StylesSidebarPane.js:709
> +        var sectionArraysByPseudoId = WebInspector.panels.elements.sidebarPanes.styles.sections;

It might make sense to push it to the Section.js (i.e. implement generic nextSiblint / previousSibling there).

> WebCore/inspector/front-end/StylesSidebarPane.js:1567
> +            var isFieldInputTerminated = isEditingName ? (event.keyIdentifier === "U+00BA" && event.shiftKey) : (event.keyIdentifier === "U+00BA" && !event.shiftKey && shouldCommitValueSemicolon(event.target.textContent, event.target.selectionLeftOffset));

You should use keypress + WebInspector.KeyboardShortcut.Keys.*

> WebCore/inspector/front-end/inspector.js:1922
> +WebInspector.startEditing = function(element, committedCallback, cancelledCallback, context, config)

What is the difference between customFinishHandler and committedCallback? They should either all be in config or outside.

[Alexander Pavlov (apavlov)]

(In reply to comment #12)
> (From update of attachment 75889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75889&action=review
> 
> > WebCore/inspector/front-end/StylesSidebarPane.js:709
> > +        var sectionArraysByPseudoId = WebInspector.panels.elements.sidebarPanes.styles.sections;
> 
> It might make sense to push it to the Section.js (i.e. implement generic nextSiblint / previousSibling there).

Done.

> > WebCore/inspector/front-end/StylesSidebarPane.js:1567
> > +            var isFieldInputTerminated = isEditingName ? (event.keyIdentifier === "U+00BA" && event.shiftKey) : (event.keyIdentifier === "U+00BA" && !event.shiftKey && shouldCommitValueSemicolon(event.target.textContent, event.target.selectionLeftOffset));
> 
> You should use keypress + WebInspector.KeyboardShortcut.Keys.*

keypress is not generated for the Tab key

> > WebCore/inspector/front-end/inspector.js:1922
> > +WebInspector.startEditing = function(element, committedCallback, cancelledCallback, context, config)
> 
> What is the difference between customFinishHandler and committedCallback? They should either all be in config or outside.

Changed method signature.

[Alexander Pavlov (apavlov)]

Created attachment 75907 [details]
[PATCH] Comments from pfeldman addressed, WebInspector.startEditing() signature changed

[Joseph Pecoraro]

> I will look into what's going on. The delay is also noticeable for me in a debug build.

Thanks!


> > In case you think you can deduce how the ASSERT could happen, the ASSERT was:
> > 
> >   InspectorStyleSheet.cpp:192: (setPropertyText)
> >   if (overwrite) {
> >     ASSERT(index < allProperties.size());
> 
> Could it have been a series of very quick actions (possibly hitting some race)?

I think it is likely that it was a race condition. When it crashed I had just done
a shift tab after changing an invalid rule's value to the empty string.
I was just trying a bunch of different things =). However, I couldn't reproduce
this after a restart, which makes me think it might be a race condition.

[Joseph Pecoraro]

Comment on attachment 75907 [details]
[PATCH] Comments from pfeldman addressed, WebInspector.startEditing() signature changed

View in context: https://bugs.webkit.org/attachment.cgi?id=75907&action=review

> WebCore/ChangeLog:13
> +        For CSS property editing, the property name and value have become two fields separated
> +        by a colon (rather than one field containing the full property text.) A user can tab
> +        between the name and value fields forward and backward. A colon typed in the name field
> +        and a semicolon in the value field (unless found inside a string) act as a Tab and focus
> +        the next editable field (while applying the entire property value.) Once a rule boundary
> +        is reached, the editing starts for the next rule selector/previous style blank property.

When I applied this, typing a color or semicolon in those positions did not act as a tab.

> WebCore/inspector/front-end/StylesSidebarPane.js:1549
> +            // FIXME: the ":"/";" detection does not work for non-US layouts due to the event being keydown rather than keypress.

Is that true? I have a US layout and it wasn't working for me.

[Alexander Pavlov (apavlov)]

(In reply to comment #16)
> (From update of attachment 75907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75907&action=review
> 
> > WebCore/ChangeLog:13
> > +        For CSS property editing, the property name and value have become two fields separated
> > +        by a colon (rather than one field containing the full property text.) A user can tab
> > +        between the name and value fields forward and backward. A colon typed in the name field
> > +        and a semicolon in the value field (unless found inside a string) act as a Tab and focus
> > +        the next editable field (while applying the entire property value.) Once a rule boundary
> > +        is reached, the editing starts for the next rule selector/previous style blank property.
> 
> When I applied this, typing a color or semicolon in those positions did not act as a tab.
> 
> > WebCore/inspector/front-end/StylesSidebarPane.js:1549
> > +            // FIXME: the ":"/";" detection does not work for non-US layouts due to the event being keydown rather than keypress.
> 
> Is that true? I have a US layout and it wasn't working for me.

Hmm... Thanks for the report. You are obviously using a Mac, and I'm on Windows. My guess was that keycodes for the keydown event should roughly be the same but that may easily prove wrong - will look into that tomorrow.

[Alexander Pavlov (apavlov)]

Created attachment 76052 [details]
[PATCH] Fixed handling of ;/: on Mac

For some reason, Mac reports the ;/: keydown event keyIdentifier as U+003A rather than U+00BA.

[Alexander Pavlov (apavlov)]

(In reply to comment #17)
> (In reply to comment #16)
> > > WebCore/inspector/front-end/StylesSidebarPane.js:1549
> > > +            // FIXME: the ":"/";" detection does not work for non-US layouts due to the event being keydown rather than keypress.
> > 
> > Is that true? I have a US layout and it wasn't working for me.
> 
> Hmm... Thanks for the report. You are obviously using a Mac, and I'm on Windows. My guess was that keycodes for the keydown event should roughly be the same but that may easily prove wrong - will look into that tomorrow.

Fixed ":"/";" detection for Mac - using event.keyCode rather than event.keyIdentifier.

[Joseph Pecoraro]

(In reply to comment #18)
> Created an attachment (id=76052) [details]
> [PATCH] Fixed handling of ;/: on Mac
> 
> For some reason, Mac reports the ;/: keydown event keyIdentifier as U+003A rather than U+00BA.

Sounds like a bug! Could you file a bug on this?


> (In reply to comment #17)
> Fixed ":"/";" detection for Mac - using event.keyCode rather than event.keyIdentifier.

Nice! I'll check this tomorrow morning and likely approve the patch. Other reviewers
feel free to review this earlier, it looked good to me but I just wanted to verify this.

[Joseph Pecoraro]

Comment on attachment 76052 [details]
[PATCH] Fixed handling of ;/: on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=76052&action=review

r=me, with some comments below. Pavel might want to comment, since they were
originally his comments. Looks and feels slightly better than the first patch.
Thanks!

> WebCore/ChangeLog:13
> +        the next editable field (while applying the entire property value.) Once a rule boundary
> +        is reached, the editing starts for the next rule selector/previous style blank property.

Nit: Starting with "Once a rule..." could be a separate paragraph since it is separate behavior.
And you can simplify this by saying that you now allow for tabbing through all "editable" styles,
even across selector boundaries.

> WebCore/inspector/front-end/inspector.js:1922
> +// Available config fields:
> +// commitHandler: Function - handles editing "commit" outcome
> +// cancelHandler: Function - handles editing "cancel" outcome
> +// customFinishHandler: Function|undefined - custom finish handler for the editing session
> +// multiline: true|false - whether the edited element is multiline
> +WebInspector.startEditing = function(element, config, context)

Why couldn't context just be included in config? Or is config for entirely optional arguments?
This was Pavel's review comment, so I'd like to see what he thinks of your approach. If context
is required, I would have preferred the following signature:

  WebInspector.startEditing = function(element, context, options)

To have code that uses it look cleanly like:

  WebInspector.startEditing(elem, tagName, {
      multiline: true,
      commitHandler: handler
      ...
  });

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        M       LayoutTests/inspector/console-dir.html
        M       LayoutTests/inspector/styles-add-blank-property.html
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/BreakpointsSidebarPane.js
        M       WebCore/inspector/front-end/DataGrid.js
        M       WebCore/inspector/front-end/ElementsTreeOutline.js
        M       WebCore/inspector/front-end/MetricsSidebarPane.js
        M       WebCore/inspector/front-end/ObjectPropertiesSection.js
        M       WebCore/inspector/front-end/Section.js
        M       WebCore/inspector/front-end/SourceFrame.js
        M       WebCore/inspector/front-end/StylesSidebarPane.js
        M       WebCore/inspector/front-end/TextViewer.js
        M       WebCore/inspector/front-end/WatchExpressionsSidebarPane.js
        M       WebCore/inspector/front-end/inspector.css
        M       WebCore/inspector/front-end/inspector.js
        M       WebCore/inspector/front-end/treeoutline.js
Committed r73913