Summary: | REGRESSION(r246621): Web Inspector: Styles: property may get removed when editing after deleting value | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-06-23 23:29:07 PDT
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! |