Bug 111036 – Web Inspector: Empty attribute name/value pair added in Elements panel should be removed

• Status: RESOLVED
• Resolution: MOVED
• Component: Web Inspector (Deprecated)

[Vivek Galatage]

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.

[Vivek Galatage]

Created attachment 190646 [details]
Patch

[Pavel Feldman]

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.

[Vivek Galatage]

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.

[Vivek Galatage]

Created attachment 190914 [details]
Patch

[Pavel Feldman]

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);

[Vivek Galatage]

Created attachment 190917 [details]
Patch

[Vivek Galatage]

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.

[Pavel Feldman]

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.

[Vivek Galatage]

Created attachment 190919 [details]
Patch

[Vivek Galatage]

(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 :)

[Alexander Pavlov (apavlov)]

Comment on attachment 190919 [details]
Patch

Please add a test, as agreed upon over IRC.

[Vivek Galatage]

(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?

[Brian Burg]

Re-filed over here:

https://bugs.webkit.org/show_bug.cgi?id=139623

The bug does not occur for multiple spaces any more.