Created attachment 372732 [details] [Animated GIF] Bug Steps: 1. Add property "color: red" 2. Press Enter 3. Press Shift-Tab 4. Press Delete to delete "red" Actual: The entire property gets removed without giving a chance to enter a new value. Expected: Editing continues. Notes: The attached animated GIF runs Web Inspector with Style Editing Debug Mode (Debug -> Styles: Enable style editing debug mode). The red border on the right side of a style rule indicates that the style declaration is locked. After step 3, you can see it is no longer the case and that's the problem. This is a regression from Bug 198505 - REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues.
<rdar://problem/52042815>
Created attachment 372734 [details] Patch
Created attachment 372736 [details] [Animated GIF] Bug
Interestingly this isn't a problem when there is more than one property. Steps to Reproduce: 1. Add "color: red" 2. Press Enter 3. Add "text-align: center" 4. Press Enter 5. Shift-Tab three times => Property value "red" gets the focus 6. Press Delete => Carat positioned at empty property value, autocomplete menu shown
Comment on attachment 372734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372734&action=review This fixes the problem, I'd just like to have a better understanding of the underlying issue before r+ing. > Source/WebInspectorUI/ChangeLog:11 > + incorrectly set to false. This explanation is a bit confusing. Based on the change log for r246621, it looks like we now dispatch this event in cases where previously we did not: "When WI.CSSStyleDeclaration.prototype.update gets called after setting text, it exits early without firing WI.CSSStyleDeclaration.Event.PropertiesChanged." If I understand the code, the problem is that a handler which blurs the element is now being called, causing the property to be deleted. Why doesn't this happen in the case mentioned here: https://bugs.webkit.org/show_bug.cgi?id=199143#c4?
(In reply to Matt Baker from comment #4) > Interestingly this isn't a problem when there is more than one property. > > Steps to Reproduce: > > 1. Add "color: red" > 2. Press Enter > 3. Add "text-align: center" > 4. Press Enter > 5. Shift-Tab three times > => Property value "red" gets the focus > 6. Press Delete > => Carat positioned at empty property value, autocomplete menu shown Correct. Let's dig into step 5. 1st Shift-Tab: focused "center" -> focused=true blank property removed -> focused=false 2nd Shift-Tab: focused "text-align" -> focused=true 3rd Shift-Tab: focused "red" -> focused=true My patch makes it so on the 1st Shift-Tab unfocused property removal doesn't set focus to false.
Comment on attachment 372734 [details] Patch r=me, thanks for the detailed explanation. I'll leave it up to you whether to update the change log.
> If I understand the code, the problem is that a handler which blurs the > element is now being called, causing the property to be deleted. Not quite. The property is being deleted because of the full layout. When a style is being edited, it shouldn't do a full layout. It should call _updatePropertiesStatus instead. WI.CSSStyleDeclaration.Event.PropertiesChanged calls WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged _propertiesChanged _propertiesChanged(event) { if (this.editing && isNaN(this._pendingAddBlankPropertyIndexOffset)) this._updatePropertiesStatus(); else this.needsLayout(); } Because `focused` is incorrectly false, `this.editing` returns false, too: get editing() { return this._focused || this._inlineSwatchActive; } > Why doesn't this happen in the case mentioned here: > https://bugs.webkit.org/show_bug.cgi?id=199143#c4? Let me know if answered it above!
Comment on attachment 372734 [details] Patch Clearing flags on attachment: 372734 Committed r246816: <https://trac.webkit.org/changeset/246816>
All reviewed patches have been landed. Closing bug.
(In reply to Nikita Vasilyev from comment #8) > > If I understand the code, the problem is that a handler which blurs the > > element is now being called, causing the property to be deleted. > > Not quite. The property is being deleted because of the full layout. When a > style is being edited, it shouldn't do a full layout. It should call > _updatePropertiesStatus instead. > > WI.CSSStyleDeclaration.Event.PropertiesChanged calls > WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged > _propertiesChanged > > _propertiesChanged(event) > { > if (this.editing && isNaN(this._pendingAddBlankPropertyIndexOffset)) > this._updatePropertiesStatus(); > else > this.needsLayout(); > } > > Because `focused` is incorrectly false, `this.editing` returns false, too: > > get editing() > { > return this._focused || this._inlineSwatchActive; > } Ahh okay. I totally missed this. > > > Why doesn't this happen in the case mentioned here: > > https://bugs.webkit.org/show_bug.cgi?id=199143#c4? > > Let me know if answered it above!