Bug 192116

Summary: Web Inspector: REGRESSION(r238602): Elements: deleting multiple DOM nodes doesn't select the nearest node after deletion
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://devinrousso.com/demo/WebKit/test.html
See Also: https://bugs.webkit.org/show_bug.cgi?id=192044
Bug Depends on: 192059    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Devin Rousso 2018-11-28 15:31:51 PST
* STEPS TO REPRODUCE:
1. open WebInspector
2. go to the Elements tab
3. select all <area>
4. press ⌫ to delete
 => no element is selected
Comment 1 Radar WebKit Bug Importer 2018-11-29 09:33:55 PST
<rdar://problem/46344339>
Comment 2 Matt Baker 2018-12-10 16:50:45 PST
Created attachment 357019 [details]
Patch
Comment 3 Matt Baker 2018-12-10 16:53:19 PST
It works pretty well, even for edge cases like having multiple selected end-tag TreeElements (</li> etc). I am seeing some asserts, and occasionally a new selected element cannot be found.

Looking into the above issues now, but this is largely reviewable.
Comment 4 Devin Rousso 2018-12-10 19:46:06 PST
Comment on attachment 357019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357019&action=review

r-, as I'd expect slightly different functionality

Having a Delete > Nodes is a good idea :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:520
> +        let selectedTreeElements = this.selectedTreeElements.filter(hasNoSelectedAncestors);

This wouldn't have the effect I'd expect.  If I've selected a parent and child, and press delete, I'd expect that they are no longer attached to the DOM tree _and_ that their parent-child relationship no longer exists.  This would be testable by having both nodes saved as $x values (e.g. `$1.parentNode !== $2 && $1.parentNode === null`).

Is the reason you're performing this check to ensure that the `WI.SelectionController` doesn't try to do funky things?  You're already `removeSelectedItems()` before removing each `WI.DOMTreeElement` individually, so I don't think that's an issue there.
Comment 5 Matt Baker 2018-12-12 17:04:19 PST
Comment on attachment 357019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357019&action=review

In addition to the issue you brought up, there is another bug I just noticed that should be resolved.

Given the following DOM with selected (*) nodes:

  <div>
*     <p>
*         <span>
              <b></b>
          </span>
      <p>
      <p></p>
  </div>

When SelectionController looks for a new index for select in preparation for the removal of the selected <p> and <span>, it does so unaware of the parent-child relationships involved. It will select the index for <b>, which is going to be removed with its parent <span>.

To resolve this, SelectionController.prototype.removeSelectedItems should use delegate methods `selectionControllerNextSelectableIndex` and `selectionControllerNextSelectableIndex` when determining an index to select. The delegate can skip children of items that are flagged for deletion.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:520
>> +        let selectedTreeElements = this.selectedTreeElements.filter(hasNoSelectedAncestors);
> 
> This wouldn't have the effect I'd expect.  If I've selected a parent and child, and press delete, I'd expect that they are no longer attached to the DOM tree _and_ that their parent-child relationship no longer exists.  This would be testable by having both nodes saved as $x values (e.g. `$1.parentNode !== $2 && $1.parentNode === null`).
> 
> Is the reason you're performing this check to ensure that the `WI.SelectionController` doesn't try to do funky things?  You're already `removeSelectedItems()` before removing each `WI.DOMTreeElement` individually, so I don't think that's an issue there.

I agree with your assessment. The reason for not removing a child if its parent is going to be removed is to avoid having to sort nodes prior to removing. Removing a node from a subtree that's no longer attached to the root is an error. I'll rework this so that nodes are removed bottom-up.
Comment 6 Matt Baker 2018-12-12 23:27:29 PST
Created attachment 357212 [details]
Patch
Comment 7 Devin Rousso 2018-12-13 12:31:04 PST
Comment on attachment 357212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:274
>      {
>      }

I'm amazed this made it through review...

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284
> +        let levelMap = new Map;

Rather than do all of this work, isn't it just as valid to remove the elements in reverse order?  The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.
Comment 8 Devin Rousso 2018-12-13 12:35:09 PST
Comment on attachment 357212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284
>> +        let levelMap = new Map;
> 
> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order?  The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.

Oh, this might have an issue if the user has selected the closing tag of an expanded element.  In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`).
Comment 9 Matt Baker 2018-12-13 12:39:46 PST
Comment on attachment 357212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review

>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284
>>> +        let levelMap = new Map;
>> 
>> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order?  The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.
> 
> Oh, this might have an issue if the user has selected the closing tag of an expanded element.  In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`).

Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement.
Comment 10 Matt Baker 2018-12-13 13:05:18 PST
Comment on attachment 357212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review

>>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284
>>>> +        let levelMap = new Map;
>>> 
>>> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order?  The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.
>> 
>> Oh, this might have an issue if the user has selected the closing tag of an expanded element.  In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`).
> 
> Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement.

Ah, but I think this simpler approach fails in the case where you have selected an element and the closing tag of its parent. If you have:

   <p>
       <span>
*          <b></b>
*      </span>
   </p>

and you delete <b> and </span>:

ondelete()
{
    if (!this._editable)
        return false;

    let selectedTreeElements = this.selectedTreeElements.map((element) => element.isCloseTag ? this.findTreeElememnt(element.node) : element);
    selectedTreeElements.reverse();

    this._selectionController.removeSelectedItems();

    for (let treeElement of selectedTreeElements)
        treeElement.remove();

    return true;
}

you would remove the parent before its child.
Comment 11 Matt Baker 2018-12-13 13:06:28 PST
Comment on attachment 357212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review

>>>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284
>>>>> +        let levelMap = new Map;
>>>> 
>>>> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order?  The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.
>>> 
>>> Oh, this might have an issue if the user has selected the closing tag of an expanded element.  In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`).
>> 
>> Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement.
> 
> Ah, but I think this simpler approach fails in the case where you have selected an element and the closing tag of its parent. If you have:
> 
>    <p>
>        <span>
> *          <b></b>
> *      </span>
>    </p>
> 
> and you delete <b> and </span>:
> 
> ondelete()
> {
>     if (!this._editable)
>         return false;
> 
>     let selectedTreeElements = this.selectedTreeElements.map((element) => element.isCloseTag ? this.findTreeElememnt(element.node) : element);
>     selectedTreeElements.reverse();
> 
>     this._selectionController.removeSelectedItems();
> 
>     for (let treeElement of selectedTreeElements)
>         treeElement.remove();
> 
>     return true;
> }
> 
> you would remove the parent before its child.

Disregard this comment.
Comment 12 Matt Baker 2018-12-13 13:22:07 PST
Created attachment 357248 [details]
Patch
Comment 13 Devin Rousso 2018-12-13 13:29:32 PST
Comment on attachment 357248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357248&action=review

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:287
> +            if (!levelMap.has(element)) {

This performs two lookups, which can be avoided.

    function getLevel(treeElement) {
        let level = levelMap.get(treeElement);
        if (isNaN(level)) {
            level = 0;
            let current = treeElement;
            while (current = current.parent)
                ++level;
            levelMap.set(treeElement, level);
        }
        return level;
    }

Style: you're reusing `level` as both a function name and variable name.
Style: since we're expecting `WI.TreeElement` and not DOM nodes, the parameter should be `treeElement` instead of `element`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:302
> +        // same DOMNode can both be selected.

NIT: I'd either include the `WI` or separate it into two names, for clarity as to whether you're referring to our model object or the builtin Node (e.g. `WI.DOMNode` vs DOM node).
Comment 14 Devin Rousso 2018-12-13 13:30:37 PST
I forgot to mention, but this case should _definitely_ have a test, as this is not an edge-case by any means.  Either add it to this patch, or to <https://webkit.org/b/192044>.
Comment 15 Matt Baker 2018-12-13 13:50:39 PST
Created attachment 357249 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2018-12-13 14:29:14 PST
Comment on attachment 357249 [details]
Patch for landing

Clearing flags on attachment: 357249

Committed r239178: <https://trac.webkit.org/changeset/239178>
Comment 17 WebKit Commit Bot 2018-12-13 14:29:15 PST
All reviewed patches have been landed.  Closing bug.