Changeset 244155 in webkit


Ignore:
Timestamp:
Apr 10, 2019 3:44:50 PM (5 years ago)
Author:
Devin Rousso
Message:

Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
​https://bugs.webkit.org/show_bug.cgi?id=192711
<rdar://problem/46738990>

Reviewed by Timothy Hatcher.

Original patch by Matt Baker <Matt Baker>.

Source/WebInspectorUI:

  • UserInterface/Controllers/SelectionController.js:

(WI.SelectionController.prototype.removeSelectedItems):
When looking for a new item to select, start with the item preceding the
selection, instead of the item following the selection. This matches
pre-multiple selection behavior, as well as Mail and Xcode.

  • UserInterface/Views/DOMTreeElement.js:

(WI.DOMTreeElement.prototype.onexpand):
Drive-by fix: when a hidden node is selected, its selection area is drawn
with a height of 0px. Update the selection area once the hidden node's
parent is expanded. AFAIK, this has always been broken.

  • UserInterface/Views/DOMTreeOutline.js:

(WI.DOMTreeOutline.prototype.ondelete):
After a delete the SelectionController may have chosen a child of a
collapsed parent as the new selected item. If the item isn't the closing tag (e.g. after
deleting the last child), reveal it.

(WI.DOMTreeOutline.prototype.selectionControllerPreviousSelectableItem):

  • UserInterface/Views/TreeElement.js:

(WI.TreeElement.prototype.get previousSelectableSibling): Added.
(WI.TreeElement.prototype.get nextSelectableSibling): Added.

  • UserInterface/Views/TreeOutline.js:

(WI.TreeOutline.prototype.selectionControllerPreviousSelectableItem):
(WI.TreeOutline.prototype.selectionControllerNextSelectableItem):
Set skipUnrevealed to false, so that children of collapsed parent nodes
are considered when looking for an item to selected after a delete. Hidden TreeElements
are still ignored as they aren't selectable.

LayoutTests:

  • inspector/table/table-remove-rows.html:
  • inspector/table/table-remove-rows-expected.txt:
Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r244154 r244155  
     12019-04-10  Devin Rousso  <drousso@apple.com>
     2
     3        Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
     4        https://bugs.webkit.org/show_bug.cgi?id=192711
     5        <rdar://problem/46738990>
     6
     7        Reviewed by Timothy Hatcher.
     8
     9        Original patch by Matt Baker <mattbaker@apple.com>.
     10
     11        * inspector/table/table-remove-rows.html:
     12        * inspector/table/table-remove-rows-expected.txt:
     13
    1142019-04-10  Devin Rousso  <drousso@apple.com>
    215

  • trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt

    r239175 r244155  
    4545PASS: Should remove rows [3].
    4646
    47 -- Running test case: Table.RemoveSelectedRows.Multiple.SelectFollowing
    48 Given a Table with selected rows [0,2]:
    49  * Row 0
     47-- Running test case: Table.RemoveSelectedRows.Single.SelectPrecedingOverFollowing
     48Given a Table with selected rows [2]:
     49   Row 0
    5050   Row 1
    5151 * Row 2
    5252   Row 3
    5353Selection changed before removing rows:
    54  - Row 0
    55    Row 1
     54   Row 0
     55 * Row 1
    5656 - Row 2
    57  * Row 3
    58 PASS: Should remove rows [0,2].
     57   Row 3
     58PASS: Should remove rows [2].
    5959
    60 -- Running test case: Table.RemoveSelectedRows.Multiple.SelectHole
    61 Given a Table with selected rows [0,3]:
     60-- Running test case: Table.RemoveSelectedRows.Multiple.SelectFollowing
     61Given a Table with selected rows [0,1]:
    6262 * Row 0
    63    Row 1
     63 * Row 1
    6464   Row 2
    65  * Row 3
     65   Row 3
    6666Selection changed before removing rows:
    6767 - Row 0
    68  * Row 1
    69    Row 2
    70  - Row 3
    71 PASS: Should remove rows [0,3].
     68 - Row 1
     69 * Row 2
     70   Row 3
     71PASS: Should remove rows [0,1].
    7272
    7373-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPreceding
    … …  
    8484PASS: Should remove rows [2,3].
    8585
     86-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPrecedingOverFollowing
     87Given a Table with selected rows [1,2]:
     88   Row 0
     89 * Row 1
     90 * Row 2
     91   Row 3
     92Selection changed before removing rows:
     93 * Row 0
     94 - Row 1
     95 - Row 2
     96   Row 3
     97PASS: Should remove rows [1,2].
     98
     99-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPrecedingOverGap
     100Given a Table with selected rows [1,3]:
     101   Row 0
     102 * Row 1
     103   Row 2
     104 * Row 3
     105Selection changed before removing rows:
     106 * Row 0
     107 - Row 1
     108   Row 2
     109 - Row 3
     110PASS: Should remove rows [1,3].
     111
     112-- Running test case: Table.RemoveSelectedRows.Multiple.SelectGapOverFollowing
     113Given a Table with selected rows [0,2]:
     114 * Row 0
     115   Row 1
     116 * Row 2
     117   Row 3
     118Selection changed before removing rows:
     119 - Row 0
     120 * Row 1
     121 - Row 2
     122   Row 3
     123PASS: Should remove rows [0,2].
     124
    86125-- Running test case: Table.RemoveRow.NotCached
    87126Given a Table with selected rows [], remove row 999.

  • trunk/LayoutTests/inspector/table/table-remove-rows.html

    r239175 r244155  
    173173    addTestCase({
    174174        name: "Table.RemoveSelectedRows.Single.SelectFollowing",
    175         description: "Remove the selected row, causing the following row to be selected.",
     175        description: "Remove a selected row not preceded by a selectable row.",
    176176        rowIndexes: [0],
    177177    });
    … …  
    179179    addTestCase({
    180180        name: "Table.RemoveSelectedRows.Single.SelectPreceding",
    181         description: "Remove the selected row, causing the preceding row to be selected.",
     181        description: "Remove a selected row not followed by a selectable row.",
    182182        rowIndexes: [lastRowIndex],
    183183    });
    184184
    185185    addTestCase({
     186        name: "Table.RemoveSelectedRows.Single.SelectPrecedingOverFollowing",
     187        description: "Remove a selected row between two selectable rows.",
     188        rowIndexes: [lastRowIndex - 1],
     189    });
     190
     191    addTestCase({
    186192        name: "Table.RemoveSelectedRows.Multiple.SelectFollowing",
    187         description: "Remove selected rows, causing the row following the selection to be selected.",
    188         rowIndexes: [0, lastRowIndex - 1],
    189     });
    190 
    191     addTestCase({
    192         name: "Table.RemoveSelectedRows.Multiple.SelectHole",
    193         description: "Remove selected rows, causing the first deselected row inside the selection to be selected.",
    194         rowIndexes: [0, lastRowIndex],
     193        description: "Remove a contiguous selection not preceded by a selectable row.",
     194        rowIndexes: [0, 1],
    195195    });
    196196
    197197    addTestCase({
    198198        name: "Table.RemoveSelectedRows.Multiple.SelectPreceding",
    199         description: "Remove selected rows, causing the row preceding the selection to be selected.",
     199        description: "Remove a contiguous selection not followed by a selectable row.",
    200200        rowIndexes: [lastRowIndex - 1, lastRowIndex],
     201    });
     202
     203    addTestCase({
     204        name: "Table.RemoveSelectedRows.Multiple.SelectPrecedingOverFollowing",
     205        description: "Remove a contiguous selection between two selectable rows.",
     206        rowIndexes: [1, 2],
     207    });
     208
     209    addTestCase({
     210        name: "Table.RemoveSelectedRows.Multiple.SelectPrecedingOverGap",
     211        description: "Remove a non-contiguous selection preceded by a selectable row.",
     212        rowIndexes: [1, 3],
     213    });
     214
     215    addTestCase({
     216        name: "Table.RemoveSelectedRows.Multiple.SelectGapOverFollowing",
     217        description: "Remove a non-contiguous selection followed by a selectable row.",
     218        rowIndexes: [0, 2],
    201219    });
    202220

  • trunk/Source/WebInspectorUI/ChangeLog

    r244154 r244155  
     12019-04-10  Devin Rousso  <drousso@apple.com>
     2
     3        Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
     4        https://bugs.webkit.org/show_bug.cgi?id=192711
     5        <rdar://problem/46738990>
     6
     7        Reviewed by Timothy Hatcher.
     8
     9        Original patch by Matt Baker <mattbaker@apple.com>.
     10
     11        * UserInterface/Controllers/SelectionController.js:
     12        (WI.SelectionController.prototype.removeSelectedItems):
     13        When looking for a new item to select, start with the item preceding the
     14        selection, instead of the item following the selection. This matches
     15        pre-multiple selection behavior, as well as Mail and Xcode.
     16
     17        * UserInterface/Views/DOMTreeElement.js:
     18        (WI.DOMTreeElement.prototype.onexpand):
     19        Drive-by fix: when a hidden node is selected, its selection area is drawn
     20        with a height of 0px. Update the selection area once the hidden node's
     21        parent is expanded. AFAIK, this has always been broken.
     22
     23        * UserInterface/Views/DOMTreeOutline.js:
     24        (WI.DOMTreeOutline.prototype.ondelete):
     25        After a delete the `SelectionController` may have chosen a child of a
     26        collapsed parent as the new selected item. If the item isn't the closing tag (e.g. after
     27        deleting the last child), reveal it.
     28
     29        (WI.DOMTreeOutline.prototype.selectionControllerPreviousSelectableItem):
     30
     31        * UserInterface/Views/TreeElement.js:
     32        (WI.TreeElement.prototype.get previousSelectableSibling): Added.
     33        (WI.TreeElement.prototype.get nextSelectableSibling): Added.
     34
     35        * UserInterface/Views/TreeOutline.js:
     36        (WI.TreeOutline.prototype.selectionControllerPreviousSelectableItem):
     37        (WI.TreeOutline.prototype.selectionControllerNextSelectableItem):
     38        Set `skipUnrevealed` to false, so that children of collapsed parent nodes
     39        are considered when looking for an item to selected after a delete. Hidden `TreeElement`s
     40        are still ignored as they aren't `selectable`.
     41
    1422019-04-10  Devin Rousso  <drousso@apple.com>
    243

  • trunk/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js

    r244154 r244155  
    179179        let orderedSelection = Array.from(this._selectedItems).sort(this._comparator);
    180180
    181         // Try selecting the item following the selection.
    182         let lastSelectedItem = orderedSelection.lastValue;
    183         let itemToSelect = this._nextSelectableItem(lastSelectedItem);
     181        // Try selecting the item preceding the selection.
     182        let firstSelectedItem = orderedSelection[0];
     183        let itemToSelect = this._previousSelectableItem(firstSelectedItem);
    184184        if (!itemToSelect) {
    185             // If no item exists after the last item in the selection, try selecting
     185            // If no item exists before the first item in the selection, try selecting
    186186            // a deselected item (hole) within the selection.
    187             itemToSelect = orderedSelection[0];
     187            itemToSelect = firstSelectedItem;
    188188            while (itemToSelect && this.hasSelectedItem(itemToSelect))
    189189                itemToSelect = this._nextSelectableItem(itemToSelect);
    … …  
    191191            if (!itemToSelect || this.hasSelectedItem(itemToSelect)) {
    192192                // If the selection contains no holes, try selecting the item
    193                 // preceding the selection.
    194                 itemToSelect = this._previousSelectableItem(orderedSelection[0]);
     193                // following the selection.
     194                itemToSelect = this._nextSelectableItem(orderedSelection.lastValue);
    195195            }
    196196        }

  • trunk/Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js

    r244154 r244155  
    607607
    608608        this.updateTitle();
     609
     610        for (let treeElement of this.children)
     611            treeElement.updateSelectionArea();
    609612    }
    610613

  • trunk/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js

    r244154 r244155  
    336336        this._treeElementsToRemove = null;
    337337
     338        if (this.selectedTreeElement && !this.selectedTreeElement.isCloseTag()) {
     339            console.assert(this.selectedTreeElements.length === 1);
     340            this.selectedTreeElement.reveal();
     341        }
     342
    338343        return true;
     344    }
     345
     346    // SelectionController delegate overrides
     347
     348    selectionControllerPreviousSelectableItem(controller, item)
     349    {
     350        let treeElement = this.getCachedTreeElement(item);
     351        console.assert(treeElement, "Missing TreeElement for representedObject.", item);
     352        if (!treeElement)
     353            return null;
     354
     355        if (this._treeElementsToRemove) {
     356            // When deleting, force the SelectionController to check siblings in
     357            // the opposite direction before searching up the parent chain.
     358            if (!treeElement.previousSelectableSibling && treeElement.nextSelectableSibling)
     359                return null;
     360        }
     361
     362        return super.selectionControllerPreviousSelectableItem(controller, item);
    339363    }
    340364

  • trunk/Source/WebInspectorUI/UserInterface/Views/TreeElement.js

    r243242 r244155  
    185185    }
    186186
     187    get previousSelectableSibling()
     188    {
     189        let treeElement = this.previousSibling;
     190        while (treeElement && !treeElement.selectable)
     191            treeElement = treeElement.previousSibling;
     192        return treeElement;
     193    }
     194
     195    get nextSelectableSibling()
     196    {
     197        let treeElement = this.nextSibling;
     198        while (treeElement && !treeElement.selectable)
     199            treeElement = treeElement.nextSibling;
     200        return treeElement;
     201    }
     202
    187203    canSelectOnMouseDown(event)
    188204    {

  • trunk/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js

    r244154 r244155  
    856856            return null;
    857857
    858         const skipUnrevealed = true;
     858        const skipUnrevealed = false;
    859859        const stayWithin = null;
    860860        const dontPopulate = true;
    … …  
    875875            return null;
    876876
    877         const skipUnrevealed = true;
     877        const skipUnrevealed = false;
    878878        const stayWithin = null;
    879879        const dontPopulate = true;
Note: See TracChangeset for help on using the changeset viewer.