lib: Fix ListingTable nesting for non-expandable rows

`<Tr>` always needs to be wrapped into a `<Tbody>`.

Clean up the unnecessary special cases and handle them uniformly.

Adjust the tests to fix the selectors, they previously relied on the
buggy behaviour.

Also adjust the pixel tests -- tbodys are separated by some space.

pkg/lib/cockpit-components-table.jsx

@@ -250,10 +250,7 @@ export const ListingTable = ({
             </React.Fragment>
         );
 
-        if (row.expandedContent)
-            return <Tbody key={rowKey} isExpanded={isExpanded}>{rowPair}</Tbody>;
-        else
-            return rowPair;
+        return <Tbody key={rowKey} isExpanded={row.expandedContent && isExpanded}>{rowPair}</Tbody>;
     });
 
     return (
@@ -293,7 +290,7 @@ export const ListingTable = ({
                         })}
                     </Tr>
                 </Thead>}
-                {!isExpandable ? <Tbody>{rowsComponents}</Tbody> : rowsComponents}
+                {rowsComponents}
             </Table>
         </>
     );

test/common/storagelib.py

@@ -107,7 +107,7 @@ def devices_dropdown(self, title):
     # Content
 
     def content_row_tbody(self, index):
-        return "#detail-content > .pf-v5-c-card > div > table > tbody:nth-of-type(%d)" % index
+        return f"#detail-content > .pf-v5-c-card > div > table > tbody:nth-of-type({index})"
 
     def content_row_expand(self, index):
         b = self.browser
@@ -117,29 +117,20 @@ def content_row_expand(self, index):
             b.click(tbody + " tr td.pf-v5-c-table__toggle button")
             b.wait_visible(tbody + ".pf-m-expanded")
 
-    def content_row_action(self, index, title, isExpandable=True):
-        if isExpandable:
-            btn = self.content_row_tbody(index) + f" tr:first-child td button:contains({title})"
-        else:
-            btn = "#detail-content > .pf-v5-c-card > div > table > :nth-child(%d)" % index + f" td button:contains({title})"
+    def content_row_action(self, index, title):
+        btn = self.content_row_tbody(index) + f" tr:first-child td button:contains({title})"
         self.browser.click(btn)
 
     # The row might come and go a couple of times until it has the
     # expected content.  However, wait_in_text can not deal with a
     # temporarily disappearing element, so we use self.retry.
 
-    def content_row_wait_in_col(self, row_index, col_index, val, isExpandable=True, alternate_val=None):
-        if isExpandable:
-            col = self.content_row_tbody(row_index) + " tr:first-child > :nth-child(%d)" % (col_index + 1)
-        else:
-            col = "#detail-content > .pf-v5-c-card > div > table > :nth-child(%d)" % row_index + " > :nth-child(%d)" % (col_index + 1)
+    def content_row_wait_in_col(self, row_index, col_index, val, alternate_val=None):
+        col = self.content_row_tbody(row_index) + f" tr:first-child > :nth-child({col_index + 1}"
         wait(lambda: self.browser.is_present(col) and (val in self.browser.text(col) or (alternate_val and alternate_val in self.browser.text(col))))
 
-    def content_dropdown_action(self, index, title, isExpandable=True):
-        if isExpandable:
-            dropdown = self.content_row_tbody(index) + " tr td:last-child .pf-v5-c-dropdown"
-        else:
-            dropdown = "#detail-content > .pf-v5-c-card > div > table > :nth-child(%d)" % index + " td:last-child .pf-v5-c-dropdown"
+    def content_dropdown_action(self, index, title):
+        dropdown = self.content_row_tbody(index) + " tr td:last-child .pf-v5-c-dropdown"
         self.browser.click(dropdown + " button.pf-v5-c-dropdown__toggle")
         self.browser.click(dropdown + f" a:contains('{title}')")
 

test/verify/check-shell-keys

@@ -113,11 +113,11 @@ class TestKeys(testlib.MachineCase):
 
         # Add invalid key directly
         m.write("/home/user/.ssh/authorized_keys", "\nbad\n", append=True)
-        b.wait_in_text("#account-authorized-keys-list tr:last-child", "Invalid key")
+        b.wait_in_text("#account-authorized-keys-list tbody:last-child", "Invalid key")
         b.wait_js_func("ph_count_check", "#account-authorized-keys-list tr", 2)
 
         # Removing the key
-        b.click("#account-authorized-keys-list tr:last-child button")
+        b.click("#account-authorized-keys-list tbody:last-child button")
         b.wait_not_in_text("#account-authorized-keys", "Invalid key")
         b.wait_js_func("ph_count_check", "#account-authorized-keys-list tr", 1)
         data = m.execute("cat /home/user/.ssh/authorized_keys")
@@ -128,9 +128,9 @@ class TestKeys(testlib.MachineCase):
 
         # User can still see their keys
         login("user")
-        b.wait_in_text("#account-authorized-keys-list tr:first-child", "test-name")
+        b.wait_in_text("#account-authorized-keys-list tbody:first-child", "test-name")
 
-        b.click("#account-authorized-keys-list tr:first-child button")
+        b.click("#account-authorized-keys-list tbody:first-child button")
         b.wait_in_text("#account-authorized-keys", "no authorized public keys")
 
         self.allow_journal_messages('authorized_keys is not a public key file.')

test/verify/check-storage-mdraid

@@ -208,7 +208,7 @@ class TestStorageMdRaid(storagelib.StorageCase):
         self.content_row_wait_in_col(1, 1, part_prefix + "1")
 
         # Create second partition
-        self.content_row_action(2, "Create partition", isExpandable=False)
+        self.content_row_action(2, "Create partition")
         self.dialog({"type": "ext4",
                      "mount_point": "/foo2",
                      "name": "Two"})

test/verify/check-storage-mounting

@@ -559,7 +559,7 @@ class TestStorageMountingLUKS(storagelib.StorageCase):
                     secondary=True)
         self.content_row_wait_in_col(1, 3, "/run/foo")
 
-        self.content_row_action(2, "Create partition", isExpandable=False)
+        self.content_row_action(2, "Create partition")
         self.dialog({"type": "ext4",
                      "crypto": self.default_crypto_type,
                      "passphrase": "vainu-reku-toma-rolle-kaja",
@@ -622,8 +622,8 @@ class TestStorageMountingLUKS(storagelib.StorageCase):
 
         b.click('button:contains(Create partition table)')
         self.dialog_wait_open()
-        b.wait_text("#dialog tr:nth-child(1) td[data-label=Location]", "/run/foo/bar")
-        b.wait_text("#dialog tr:nth-child(2) td[data-label=Location]", "/run/foo")
+        b.wait_text("#dialog tbody:nth-of-type(1) td[data-label=Location]", "/run/foo/bar")
+        b.wait_text("#dialog tbody:nth-of-type(2) td[data-label=Location]", "/run/foo")
         self.dialog_apply()
         self.content_row_wait_in_col(1, 0, "Free space")
 

test/verify/check-storage-msdos

@@ -37,7 +37,7 @@ class TestStorageMsDOS(storagelib.StorageCase):
         # Format it with a DOS partition table
         b.click('button:contains(Create partition table)')
         self.dialog({"type": "dos"})
-        self.content_row_wait_in_col(1, 0, "Free space", isExpandable=False)
+        self.content_row_wait_in_col(1, 0, "Free space")
 
         # Create a primary partition
         self.content_row_action(1, "Create partition")
@@ -57,7 +57,7 @@ class TestStorageMsDOS(storagelib.StorageCase):
         self.dialog_wait_close()
 
         # Create a extended partition to fill the rest of the disk
-        self.content_row_action(2, "Create partition", isExpandable=False)
+        self.content_row_action(2, "Create partition")
         self.dialog_wait_open()
         self.dialog_set_val("type", "dos-extended")
         self.dialog_wait_not_present("name")
@@ -66,11 +66,11 @@ class TestStorageMsDOS(storagelib.StorageCase):
         self.dialog_apply()
         self.dialog_wait_close()
         self.content_row_wait_in_col(2, 2, "Extended partition")
-        self.content_row_wait_in_col(3, 1, "Free space", isExpandable=False)
+        self.content_row_wait_in_col(3, 1, "Free space")
 
         # Create logical partitions and check that "dos-extended" is
         # not offered.
-        self.content_row_action(3, "Create partition", isExpandable=False)
+        self.content_row_action(3, "Create partition")
         self.dialog_wait_open()
         b.wait_not_present("select option[value='dos-extended']")
         self.dialog_cancel()
@@ -81,7 +81,7 @@ class TestStorageMsDOS(storagelib.StorageCase):
         self.content_dropdown_action(2, "Delete")
         self.confirm()
 
-        self.content_row_wait_in_col(2, 1, "Free space", isExpandable=False)
+        self.content_row_wait_in_col(2, 1, "Free space")
 
 
 if __name__ == '__main__':

test/verify/check-storage-partitions

@@ -133,7 +133,7 @@ class TestStoragePartitions(storagelib.StorageCase):
                      "mount_point": "/foo1",
                      "size": 80 + nudge},
                     secondary=True)
-        self.content_row_action(2, "Create partition", isExpandable=False)
+        self.content_row_action(2, "Create partition")
         self.dialog({"type": "ext4",
                      "mount_point": "/foo2",
                      "size": 23},

test/verify/check-storage-used

@@ -86,7 +86,7 @@ ExecStart=/usr/bin/sleep infinity
 
         b.click('button:contains(Create partition table)')
         self.dialog_wait_open()
-        b.wait_visible("#dialog tr:first-child button:contains(Currently in use)")
+        b.wait_visible("#dialog tbody:first-of-type button:contains(Currently in use)")
         b.assert_pixels('#dialog', "format-disk")
         self.dialog_apply()
         self.dialog_wait_close()

test/verify/check-system-info

@@ -519,17 +519,17 @@ class TestSystemInfo(packagelib.PackageCase):
         # PCI
         b.wait_in_text(pci_selector + heading_selector, "PCI")
 
-        b.wait_in_text(pci_selector + ' tr:first-of-type td[data-label=Slot]', "0000:00:00.0")
+        b.wait_in_text(pci_selector + ' tbody:first-of-type td[data-label=Slot]', "0000:00:00.0")
 
         # sorted by device class by default; this makes some assumptions about QEMU devices
-        b.wait_in_text(pci_selector + ' tbody tr:first-of-type td[data-label=Class]', "Bridge")
-        b.wait_in_text(pci_selector + ' tbody tr:last-of-type td[data-label=Class]', "Unclassified")
+        b.wait_in_text(pci_selector + ' tbody:first-of-type td[data-label=Class]', "Bridge")
+        b.wait_in_text(pci_selector + ' tbody:last-of-type td[data-label=Class]', "Unclassified")
 
         # sort by model
         b.click(pci_selector + ' thead th:nth-child(2) button')
-        b.wait_in_text(pci_selector + ' tbody tr:first-of-type td[data-label=Model]', "440")
-        b.wait_in_text(pci_selector + ' tbody tr:last-of-type td[data-label=Model]', "Virtio SCSI")
-        b.wait_not_in_text(pci_selector + ' tbody tr:last-of-type td[data-label=Model]', "Unclassified")
+        b.wait_in_text(pci_selector + ' tbody:first-of-type td[data-label=Model]', "440")
+        b.wait_in_text(pci_selector + ' tbody:last-of-type td[data-label=Model]', "Virtio SCSI")
+        b.wait_not_in_text(pci_selector + ' tbody:last-of-type td[data-label=Model]', "Unclassified")
 
         # go back to system page
         b.click('.pf-v5-c-breadcrumb li:first-of-type')
@@ -557,7 +557,7 @@ class TestSystemInfo(packagelib.PackageCase):
 
         # PCI should be shown
         b.wait_in_text(pci_selector + heading_selector, "PCI")
-        b.wait_in_text(pci_selector + ' tr:first-of-type td[data-label=Slot]', "0000:00:00.0")
+        b.wait_in_text(pci_selector + ' tbody:first-of-type td[data-label=Slot]', "0000:00:00.0")
 
         # Check also variants when only some fields are present
         m.write("/sys/class/dmi/id/chassis_type", "10")
@@ -688,27 +688,27 @@ machine         : 8561
         distros_without_systemd_memory_dmi = ['rhel-8-7', 'rhel-8-8', 'rhel-8-9', 'centos-8-stream']
 
         # Test more specific memory data with a fake dmidecode
-        b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=ID]', "BANK 0: ChannelA-DIMM0")
-        b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Type]', "DDR4")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=ID]', "BANK 0: ChannelA-DIMM0")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Type]', "DDR4")
         if m.image in distros_without_systemd_memory_dmi:
-            b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Size]', "4 GB")
+            b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Size]', "4 GB")
         else:
-            b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Size]', "4 GiB")
-        b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=State]', "Present")
-        b.wait_text('#memory-listing tr:nth-of-type(1) td[data-label="Memory technology"]', "Unknown")
-        b.wait_text('#memory-listing tr:nth-of-type(1) td[data-label=Rank]', "Single rank")
-        b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Speed]', "2400 MT/s")
-
-        b.wait_in_text('#memory-listing tr:nth-of-type(2) td[data-label=ID]', "BANK 2: ChannelB-DIMM0")
-        b.wait_in_text('#memory-listing tr:nth-of-type(2) td[data-label=Type]', "DDR4")
+            b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Size]', "4 GiB")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=State]', "Present")
+        b.wait_text('#memory-listing tbody:nth-of-type(1) td[data-label="Memory technology"]', "Unknown")
+        b.wait_text('#memory-listing tbody:nth-of-type(1) td[data-label=Rank]', "Single rank")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Speed]', "2400 MT/s")
+
+        b.wait_in_text('#memory-listing tbody:nth-of-type(2) td[data-label=ID]', "BANK 2: ChannelB-DIMM0")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(2) td[data-label=Type]', "DDR4")
         if m.image in distros_without_systemd_memory_dmi:
-            b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Size]', "4 GB")
+            b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Size]', "4 GB")
         else:
-            b.wait_in_text('#memory-listing tr:nth-of-type(1) td[data-label=Size]', "4 GiB")
-        b.wait_in_text('#memory-listing tr:nth-of-type(2) td[data-label=State]', "Present")
-        b.wait_text('#memory-listing tr:nth-of-type(2) td[data-label="Memory technology"]', "Unknown")
-        b.wait_text('#memory-listing tr:nth-of-type(2) td[data-label=Rank]', "Single rank")
-        b.wait_in_text('#memory-listing tr:nth-of-type(2) td[data-label=Speed]', "2400 MT/s")
+            b.wait_in_text('#memory-listing tbody:nth-of-type(1) td[data-label=Size]', "4 GiB")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(2) td[data-label=State]', "Present")
+        b.wait_text('#memory-listing tbody:nth-of-type(2) td[data-label="Memory technology"]', "Unknown")
+        b.wait_text('#memory-listing tbody:nth-of-type(2) td[data-label=Rank]', "Single rank")
+        b.wait_in_text('#memory-listing tbody:nth-of-type(2) td[data-label=Speed]', "2400 MT/s")
 
     @ testlib.nondestructive
     def testCPUSecurityMitigationsDetect(self):

test/verify/check-system-services

@@ -345,17 +345,16 @@ WantedBy=default.target
         # returns index of first unit that isn't failed
         b.inject_js("""
                     function firstWorkingUnitPos() {
-                        const arr = document.getElementById('services-list').firstChild;
-                        for (var i = 0; i < arr.childElementCount; i++) {
-                            const child = arr.children[i];
-                            if (!child.classList.contains('service-unit-failed')) {
-                                return i + 1;
-                            }
+                        const services = document.getElementById('services-list');
+                        for (let i = 0; i < services.childElementCount; i++) {
+                            const tbody = services.children[i];
+                            if (!tbody.firstChild.classList.contains('service-unit-failed'))
+                                return i;
                         }
                     }
                   """)
         pos = b.eval_js('firstWorkingUnitPos();')
-        b.wait_text(f'#services-list > tbody > tr:nth-child({pos}) > th > div > div > a', 'test')
+        b.wait_text(f'#services-list > tbody:nth-child({pos + 1}) > tr > th > div > div > a', 'test')
         b.is_present('#test.service > svg.service-thumbtack-icon-color')
 
         # Unpin unit

test/verify/check-users

@@ -325,15 +325,15 @@ class TestAccounts(testlib.MachineCase):
 
         # test filters
         b.set_input_text("#accounts-filter input", "root")
-        b.wait_in_text("#accounts-list tbody tr:first-child td[data-label='Username']", "root")
+        b.wait_in_text("#accounts-list tbody:first-of-type td[data-label='Username']", "root")
         b.set_input_text("#accounts-filter input", "rOBeRt")
-        b.wait_in_text("#accounts-list tbody tr:first-child td[data-label='Username']", "robert")
+        b.wait_in_text("#accounts-list tbody:first-of-type td[data-label='Username']", "robert")
 
         uid = "1000"
         if "debian" in m.image or "ubuntu" in m.image:
             uid = "1001"
         b.set_input_text("#accounts-filter input", uid)
-        b.wait_in_text("#accounts-list tbody tr:first-child td[data-label='ID']", uid)
+        b.wait_in_text("#accounts-list tbody:first-of-type td[data-label='ID']", uid)
         b.set_input_text("#accounts-filter input", "spooky")
         b.wait_visible("#accounts div.pf-v5-c-empty-state")
 
@@ -346,7 +346,7 @@ class TestAccounts(testlib.MachineCase):
 
         def check_column_sort(query_selector, invert=False):
             # current account is always in the first row
-            b.wait_in_text("#accounts-list tbody tr:first-child td[data-label='Username']", "admin")
+            b.wait_in_text("#accounts-list tbody:first-of-type td[data-label='Username']", "admin")
             values = b.eval_js(f"getTextColumn(\"{query_selector}\")")
             for i in range(2, len(values)):
                 if values[i].isnumeric():
@@ -521,7 +521,7 @@ class TestAccounts(testlib.MachineCase):
         b.wait_js_cond('document.querySelector("#current-account-badge").previousSibling.getAttribute("href") === "#/jussi"')
 
         # The current account is the first in the list
-        b.wait_visible("#accounts-list > tbody :first-child #current-account-badge")
+        b.wait_visible("#accounts-list > tbody:first-of-type #current-account-badge")
 
         # Use [x] button on the group label to remove the account from group
         b.go("#/jussi")