Bug 199143

<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!