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.
Created attachment 75691 [details] [PATCH] Suggested solution
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.
Arg, I didn't get around to applying this today. I'll update+build overnight, and try to apply it tomorrow morning.
Created attachment 75824 [details] [PATCH] Nits fixed
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.
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?
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.
Created attachment 75889 [details] [PATCH] Comments addressed. Also added tabbing between all rules shown for the given element
(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.
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.
(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.
Created attachment 75907 [details] [PATCH] Comments from pfeldman addressed, WebInspector.startEditing() signature changed
> 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.
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.
(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.
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.
(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.
(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.
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 ... });
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