Steps: 1. Open the Elements panel 2. Try adding a new attribute using either way i.e. context menu, pressing tab key etc. 3. See that an editor is presented for editing the attribute name, value pair 4. Without any text change, simply commit the change by pressing either enter key or focus out event. 5. Repeat this for 4-5 times. Acutal outcome: The tree outline is shown with the empty spaces. Expected outcome: If the attribute name/value pair contents are empty, the list item in the elements tree outline shouldn't be shown with white spaces. Patch follows.
Created attachment 190646 [details] Patch
Comment on attachment 190646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190646&action=review > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1057 > - I try to not mix changes to semantics with formatting changes. > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1551 > + if (attributeName.trim().length === 0 && newText.trim().length === 0) You should move this check to above the representedObject.setAttribute call to further emphasize you don't call setAttribute in that case. Also, moveToNextAttribute should be the last line of this method since it is a completion callback.
Comment on attachment 190646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190646&action=review Thank you Pavel. >> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1057 >> - > > I try to not mix changes to semantics with formatting changes. Its my Sublime settings to auto trim the trailing spaces which caused these changes. Sure I will disable them. >> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1551 >> + if (attributeName.trim().length === 0 && newText.trim().length === 0) > > You should move this check to above the representedObject.setAttribute call to further emphasize you don't call setAttribute in that case. Also, moveToNextAttribute should be the last line of this method since it is a completion callback. Sure.
Created attachment 190914 [details] Patch
Comment on attachment 190914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190914&action=review > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1588 > + if (attributeName.trim().length === 0 && newText.trim().length === 0) it'd be great if it was showing that setAttribute is not called: if (!attributeName.trim() && !newText.trim()) { element.removeSelf(); moveToNextAttributeIfNeeded.call(this); return; } if (oldText !== newText) { this.representedObject.setAttribute(attributeName, newText, moveToNextAttributeIfNeeded.bind(this)); return; } moveToNextAttributeIfNeeded.call(this);
Created attachment 190917 [details] Patch
Comment on attachment 190917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190917&action=review > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1590 > + if (!emptyNameValuePair && (oldText !== newText)) I understand, we use the early bailout, but I thought this is mostly the first block which would be hit most of the times. Hence added the '!' case first.
Comment on attachment 190917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190917&action=review >> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1590 >> + if (!emptyNameValuePair && (oldText !== newText)) > > I understand, we use the early bailout, but I thought this is mostly the first block which would be hit most of the times. Hence added the '!' case first. To my taste, these nested conditions referring to the emptyNameValuePair are the opposite of readability. The three guards that I was suggesting are way more user-friendly.
Created attachment 190919 [details] Patch
(In reply to comment #8) > To my taste, these nested conditions referring to the emptyNameValuePair are the opposite of readability. The three guards that I was suggesting are way more user-friendly. Agreed. I realized it after submitting the patch. Sorry for these many versions :)
Comment on attachment 190919 [details] Patch Please add a test, as agreed upon over IRC.
(In reply to comment #11) > (From update of attachment 190919 [details]) > Please add a test, as agreed upon over IRC. I was not able to draft a test case correctly for the above case. Can we land this change without the test case as its only a UI related minor feature?
Re-filed over here: https://bugs.webkit.org/show_bug.cgi?id=139623 The bug does not occur for multiple spaces any more.