Skip to content
This repository has been archived by the owner on Sep 22, 2022. It is now read-only.

Test for managing focus is failing #77

Open
theinterned opened this issue Feb 22, 2022 · 3 comments · May be fixed by #81
Open

Test for managing focus is failing #77

theinterned opened this issue Feb 22, 2022 · 3 comments · May be fixed by #81

Comments

@theinterned
Copy link
Contributor

theinterned commented Feb 22, 2022 •
edited

https://github.com/github/details-dialog-element/runs/5240152268?check_suite_focus=true

This test appears to have been failing since https://github.com/github/details-dialog-element/actions/runs/1766435305 but it looks like older runs may have been failing silently as tests were not running. See for example this run with missing logs https://github.com/github/details-dialog-element/runs/3652343581?check_suite_focus=true
@theinterned
Copy link
Contributor Author

theinterned commented Feb 22, 2022 •
edited

Discovered this in #72. I spent a bit of time trying to debug but was unsuccessful and bailed.

theinterned added a commit that referenced this issue Mar 1, 2022
@theinterned
Skip failing test for managing focus
6ac7293 
We should fix this test, but skipping for now.

Tracked in #77
This was referenced Mar 1, 2022
Merged
Merged
@theinterned
Copy link
Contributor Author

With a git bisect @koddsson and I isolated this to #48 it appears that tests were passing at the time so this is likely a change to Chrome since then.

1d61bff9eadeb536120d9dd0cefdb44fbdba4d1a is the first bad commit
commit 1d61bff9eadeb536120d9dd0cefdb44fbdba4d1a
Author: Mu-An Chiou <me@muanchiou.com>
Date:   Tue Nov 5 17:03:40 2019 -0500

    Add hidden button in details
    Tested by 'manages focus'

 test/test.js | 1 +
 1 file changed, 1 insertion(+)
bisect run success

Untitled on  HEAD (1d61bff) (BISECTING) [?] is 📦 v3.0.11 via  v16.13.2 took 1m59s 
❯ git show
commit 1d61bff9eadeb536120d9dd0cefdb44fbdba4d1a (HEAD, refs/bisect/bad)
Author: Mu-An Chiou <me@muanchiou.com>
Date:   Tue Nov 5 17:03:40 2019 -0500

    Add hidden button in details
    Tested by 'manages focus'

diff --git a/test/test.js b/test/test.js
index 17ba54a..7b24cd3 100644
--- a/test/test.js
+++ b/test/test.js
@@ -29,6 +29,7 @@ describe('details-dialog-element', function() {
 29 ⋮ 29 │             <button data-button>Button</button>
 30 ⋮ 30 │             <button hidden>hidden</button>
 31 ⋮ 31 │             <div hidden><button>hidden</button></div>
    ⋮ 32 │+            <details><button>Button in closed details</button></details>
 32 ⋮ 33 │             <button ${CLOSE_ATTR}>Goodbye</button>
 33 ⋮ 34 │           </details-dialog>
 34 ⋮ 35 │         </details>

@theinterned
Copy link
Contributor Author

theinterned commented Mar 1, 2022 •
edited

@koddsson and I traced this to the visibile function:

details-dialog-element/src/index.ts

Lines 36 to 42 in 2aa3a08

function visible(el: Target): boolean {
return (
!el.hidden &&
(!(el as Disableable).type || (el as Disableable).type !== 'hidden') &&
(el.offsetWidth > 0 || el.offsetHeight > 0)
)
}

This function fails to score elements that are dependents of a closed details as not visible.

Why? Previously (and in other browsers) elements that were dependents of a closed details had no bounding rect: ie el.offsetWidth and el.offsetHeight both returned 0. However, a recent change in Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=1276028) means that descendants of a closed details now do have bounding rects.

Solutions

There are a few options:

1. Add code to check if the element is a child of a closed dialog.

we can see this approach in effect in the focus-trap library:

https://github.com/focus-trap/focus-trap/blob/0a5ce1bfd913edc4410aadfc98b1583de9d034cf/docs/demo-bundle.js#L406-L411

var isDirectSummary = matches.call(node, 'details>summary:first-of-type');
var nodeUnderDetails = isDirectSummary ? node.parentElement : node;

if (matches.call(nodeUnderDetails, 'details:not([open]) *')) {
  return true;
}

2. Update the CSS of descendants of a closed dialog

@koddsson asked about our specific use case in the chromium issue and received a useful suggestion to simply set the descendants of a closed details to display: none:

details:not([open]) > :not(summary) {
  display: none;
}

@koddsson koddsson linked a pull request Mar 8, 2022 that will close this issue
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Check if elements is in a closed details element github/details-dialog-element
1 participant
@theinterned