Bug 177012

Created attachment 321902 [details] Patch

Created attachment 321905 [details] [Animated GIF] With patch applied

Comment on attachment 321902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321902&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:300 > +WI.SpreadsheetSelectorField = class SpreadsheetSelectorField This class is likely to change as I'll need to use a similar component for editable names and values.

Comment on attachment 321902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321902&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:300 >> +WI.SpreadsheetSelectorField = class SpreadsheetSelectorField > > This class is likely to change as I'll need to use a similar component for editable names and values. After talking to Matt offline, we agreed that I should take a closer look at the existing EditingSupport.js.

Comment on attachment 321902 [details] Patch After investigating EditingSupport.js, I encountered several issues: - Unnecessary tabIndex manipulations, that prevents proper focusing order. - Option-Arrow keys functionality, that can't be turned off at this time. In addition to this, EditingSupport.js is very old and uses patterns we don't use for our recent code (such as passing config instance with event listeners). I'm changing my patch back to r?. I think we need to rewrite EditingSupport at some point, but I don't want to be blocked by it right now.

Comment on attachment 321902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321902&action=review This looks really good. Applied patch locally and it works really well, even with selectors that wrap to multiple lines. Other than a couple comments, I think two things need to be address before landing, so r- for now: - After editing stops, there shouldn’t be a visible focus ring left around the selector element. - A rule selector that doesn’t match anything after being edited will disappear. This is unexpected, and there is no way to find the rule again. Instead, the selector should become grayed out. > Source/WebInspectorUI/ChangeLog:7 > + field editable. I noticed clicking a property name/value does not select the text, it just begins editing. We should probably change that too (but can be another patch unless it's a small change). > Source/WebInspectorUI/ChangeLog:12 > + Shift-Tab should commit changes and navigate to the last rule's property value, if there's one. I think these four lines would be better as a list: Keyboard behavior while editing: - Enter should commit changes. - Escape should... > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:397 > + this._selectorElement.selectText(); When is this called? In testing I never saw this code run. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:337 > + element.selectText(); If SpreadsheetSelectorField was responsible for selecting the element's text, you could remove the `selectText` utility function and move that code here.

(In reply to Matt Baker from comment #6) > Comment on attachment 321902 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321902&action=review > > - A rule selector that doesn’t match anything after being edited will > disappear. This is unexpected, and there is no way to find the rule again. > Instead, the selector should become grayed out. I agree that disappearing selectors aren't ideal. I spent last 5 hours trying to fix it. - WI.CSSRule.Event.SelectorChanged fires BEFORE CSSRule gets updated with a new selector. The current styles sidebar doesn't rely on WI.CSSRule.Event.SelectorChanged event and it doesn't try to re-parse the selector. As the result, edited selectors in the current styles sidebar are always grayed out. - In the current styles sidebar, rules with edited selectors stay visible until focus moves away from the CSS rule. It should stay visible until another DOM node is selected. This has been surprisingly difficult task. I'd prefer to address it as a follow up in Bug 177471 - Web Inspector: Styles Redesign: Selector editing quality improvements.

(In reply to Matt Baker from comment #6) > Comment on attachment 321902 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321902&action=review > > This looks really good. Applied patch locally and it works really well, even > with selectors that wrap to multiple lines. > > Other than a couple comments, I think two things need to be address before > landing, so r- for now: > - After editing stops, there shouldn’t be a visible focus ring left around > the selector element. Agreed. This is being addressed in Bug 177711 - Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc). >> Source/WebInspectorUI/ChangeLog:7 >> + field editable. > > I noticed clicking a property name/value does not select the text, it just begins editing. We should probably change that too (but can be another patch unless it's a small change). Being addressed in Bug 177711 - Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc). >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:337 >> + element.selectText(); > > If SpreadsheetSelectorField was responsible for selecting the element's text, you could remove the `selectText` utility function and move that code here. It's used in for property names and values as well in my follow up patch (Bug 177711).

Comment on attachment 321902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321902&action=review >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:397 >> + this._selectorElement.selectText(); > > When is this called? In testing I never saw this code run. This is the old styles sidebar. I should've not touched it!

Created attachment 322334 [details] Patch (In reply to Matt Baker from comment #6) > Other than a couple comments, I think two things need to be address before > landing, so r- for now: > - After editing stops, there shouldn’t be a visible focus ring left around > the selector element. I'll address it right after this patch in Bug 177711 - Web Inspector: Styles Redesign: Add support for keyboard navigation (Tab, Shift-Tab, Enter, Esc. > - A rule selector that doesn’t match anything after being edited will > disappear. This is unexpected, and there is no way to find the rule again. > Instead, the selector should become grayed out. - I'll address it in Bug 177471 - Web Inspector: Styles Redesign: Selector editing quality improvements, after I address fundamental UI components such as autocomplete and ability to add new CSS properties.

Created attachment 322419 [details] Patch I inlined Element.prototype.selectText and Event.prototype.stop for now.

Comment on attachment 322419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322419&action=review > Source/WebInspectorUI/ChangeLog:15 > + Reviewed by NOBODY (OOPS!). This should come before the summary. > Source/WebInspectorUI/ChangeLog:29 > + Split layout into _renderOrigin and _renderSelector, so selector field can be updated separetly separately > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:116 > + }, this); Since the event listener is automatically removed, you can use bind and make it one line: this._style.ownerRule.singleFireEventListener(WI.CSSRule.Event.SelectorChanged, this._renderSelector.bind(this)); > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:327 > + Remove blank line. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:329 > + Same here. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:335 > + element.textContent = element.textContent; The comment explains the effect, but what's the reason for doing this in the first place? I don't see this pattern used anywhere else. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:346 > + this._eventsInitialized = true; These should be unregistered in `stopEditing`, or registered in the constructor if it really doesn't matter (set and forget). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:396 > + this.stopEditing(); I'd add a blank line after this. The editing state change feels like the most important things about this function, and should stand out. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:407 > + Add a blank line before this, for the reason mentioned above.

Comment on attachment 322419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322419&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:116 >> + }, this); > > Since the event listener is automatically removed, you can use bind and make it one line: > > this._style.ownerRule.singleFireEventListener(WI.CSSRule.Event.SelectorChanged, this._renderSelector.bind(this)); I can avoid bind altogether: this._style.ownerRule.singleFireEventListener(WI.CSSRule.Event.SelectorChanged, this._renderSelector, this); >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:335 >> + element.textContent = element.textContent; > > The comment explains the effect, but what's the reason for doing this in the first place? I don't see this pattern used anywhere else. It's either this or CSS rules to undo all the possible styles of elements inside. This is more bulletproof. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:346 >> + this._eventsInitialized = true; > > These should be unregistered in `stopEditing`, or registered in the constructor if it really doesn't matter (set and forget). I'll move it to the constructor.

Created attachment 322487 [details] Patch

Comment on attachment 322487 [details] Patch r=me

Comment on attachment 322487 [details] Patch Clearing flags on attachment: 322487 Committed r222799: <http://trac.webkit.org/changeset/222799>

All reviewed patches have been landed. Closing bug.