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

Test for managing focus is failing #77

Open
#81
Open
Test for managing focus is failing#77
#81
@theinterned

Description

@theinterned
theinterned
opened on Feb 22, 2022 · edited by theinterned
Contributor

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

Activity

theinterned

theinterned commented on Feb 22, 2022

@theinterned
theinterned
on Feb 22, 2022 · edited by theinterned
ContributorAuthor

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

theinterned
added a commit that references this issue on Mar 1, 2022

Skip failing test for managing focus

6ac7293
theinterned
mentioned this in 2 pull requests on Mar 1, 2022
theinterned

theinterned commented on Mar 1, 2022

@theinterned
theinterned
on Mar 1, 2022
ContributorAuthor

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

theinterned commented on Mar 1, 2022

@theinterned
theinterned
on Mar 1, 2022 · edited by theinterned
ContributorAuthor

@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
linked a pull request that will close this issue on Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @theinterned

      Issue actions
        Test for managing focus is failing · Issue #77 · github/details-dialog-element