Spike Results: Enable native dialog for d2l-dialog-confirm by margaree · Pull Request #6760 · BrightspaceUI/core

margaree · 2026-04-07T20:50:12Z

Spike ticket: GAUD-7808

tldr it's quirky but possible; there are differences in how focus behaves between chrome vs others and potential screen reader issues but the situations can be handled with a recommended implementation.

Investigation notes:
Why we disabled it in the first place:

native dialogs were disabled in this PR fix: completely disable native dialogs due to Chrome issue #2524 due to issues related to elements having null offsetParents within the dialog and thus causing problems with calculating isFocusable
we ended up removing offsetParent from isFocusable here Fix dialog initial focus behaviour for Safari. #2963 so this particular issue isn't as much of a concern anymore (at least for confirm dialogs)
let me know if I'm missing another blocker here

Focus issues that now apply:

When the native confirm dialog is opened, the browser automatically focuses on the dialog content because it has tabindex="0" so by its calculations that's where to focus
BIG PROBLEM: we "can" post-initial-load move the focus to the "no" button (what we do with non-native) BUT this causes screen readers (VoiceOver, haven't yet tested others) to only read the "no" button and not the dialog context because that was interrupted by moving the focus (let me know if more clarity is needed)

Investigation notes:

According to w3c we should initially put focus on the "no" button
PROBLEM: Ideally we would do this with autofocus. However, currently only Chrome actually will use autofocus on a slotted element
MDN suggests that if dialog content is dynamically rendered, then perhaps putting autofocus on the dialog itself is more ideal (and perhaps we should do this for our general dialogs when we get here)

What a great compromise, right? We'll just put autofocus on the dialog and all our problems will be solved.

ha.

PROBLEM: Chrome behaves differently when autofocus is on a dialog and there are interactive elements within the dialog (see this blog post which has great comparisons; Demo 6 is the one applicable here). It puts focus on the content with tabindex 0. If we remove that remove that tabindex then it puts focus on the "yes" button (worse).

What I have done:
I have used the method in demo 11 which achieves the most consistency (all focus on the dialog initially, screen reader behaviour (VoiceOver so far, will test more) seems good. We do need to keep the tabindex=0 on the content as otherwise Safari doesn't actually read it as a description (though perhaps a more expert screen reader user than I could still navigate to it).

Option 2:
Put autofocus on the dialog (for Firefox and Safari) and also on the secondary button (for chrome). This would have inconsistencies between browsers and also is a bit hacky to do.

TODO:

Test with NVDA (prior testing has been done with VoiceOver)
Document current screen reader experience
Feature flag (if this seems like we'll move forward with it)

Screen reader experience:

Screen reader + Browser	Before	After (dialog.focus())
VoiceOver + Safari	"No, button, Confirm Title, web alert dialog"	"Confirm Title, web alert dialog"
VoiceOver + Chrome	"No, button, Confirm Title Are you sure you want more cookies?, alert dialog"	"Confirm Title Are you sure you want more cookies?, alert dialog"
NVDA + Firefox	"Confirm Title dialog Are you sure you want more cookies? Clickable No button Confirm Title button No"	"Confirm Title clickable heading level 2 Confirm Title Are you sure you want more cookies? Clickable button Yes clickable button No"

github-actions · 2026-04-07T20:51:19Z

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL	https://live.d2l.dev/prs/BrightspaceUI/core/pr-6760/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

margaree · 2026-04-08T14:40:48Z

I initially ran the native vdiffs for dialog-confirm with them not actually enabling native. The latest vdiff PR is with native enabled and shows the difference between our custom dialogs and the native dialog (mostly focus)

margaree · 2026-04-08T14:46:18Z

 // starting in Chrome 102, all elements inside <dialog>s that are inside shadow roots have null offsetParent
 // https://bugs.chromium.org/p/chromium/issues/detail?id=1331803
-window.D2L.DialogMixin.hasNative = false;
+window.D2L.DialogMixin.hasNative = (window.HTMLDialogElement !== undefined);


There look to be at least 2 cases that use DialogMixin and don't look to be setting preferNative so they would end up using native dialogs. We at least probably want to flag this change separately. We can evaluate those cases for if native dialogs can be enabled or if we should be setting preferNative to false for those cases.

With having this be a property this is no longer an issue. We can opt-in for those 2 places and test them out at that time.

dbatiste · 2026-04-08T15:57:42Z

I have used the method in demo 11 which achieves the most consistency (all focus on the dialog initially, screen reader behaviour (VoiceOver so far, will test more) seems good. We do need to keep the tabindex=0 on the content as otherwise Safari doesn't actually read it as a description (though perhaps a more expert screen reader user than I could still navigate to it).

Is there somewhere I can find demo 11? Is this just calling focus on the dialog, and not using autofocus?

I think ideally the content of the confirm gets announced immediately. Not sure about general dialog, but ideally the role and title are announced upon opening, and then the user can navigate the content how they want - virtual cursor or tabbing.

margaree · 2026-04-08T17:44:15Z

I have used the method in demo 11 which achieves the most consistency (all focus on the dialog initially, screen reader behaviour (VoiceOver so far, will test more) seems good. We do need to keep the tabindex=0 on the content as otherwise Safari doesn't actually read it as a description (though perhaps a more expert screen reader user than I could still navigate to it).

Is there somewhere I can find demo 11? Is this just calling focus on the dialog, and not using autofocus?

I think ideally the content of the confirm gets announced immediately. Not sure about general dialog, but ideally the role and title are announced upon opening, and then the user can navigate the content how they want - virtual cursor or tabbing.

Yup - it was in this blog.

In terms of announcing the content immediately: The only way to announce the content immediately in Safari + VoiceOver seems to be having the initial focus be the content (which would be the native dialog default behaviour due to the tabindex); it doesn't read the aria-describedby content (I feel like we've had issues with this elsewhere?). That does lead to duplication in the message in Chrome + VoiceOver and would be inconsistent with recommendations. If we were to do that, the initial message on opening would be:

Safari:

Chrome:

In Safari + VoiceOver we also aren't currently announcing the content immediately (though ideally we probably would):

dbatiste · 2026-04-15T14:26:24Z

+			/**
+			 * @ignore
+			 */
+			preferNative: { type: Boolean, attribute: 'prefer-native', reflect: true },


Does anything need to set, or do we rely on the prefer-native attribute? Just wondering if we want/need to keep this as much under wraps as possible.

We probably want to keep it under wraps

dbatiste · 2026-04-15T14:30:15Z

 	async updated(changedProperties) {
 		super.updated(changedProperties);
+
+		if (changedProperties.has('preferNative')) {


Are there any case where we would expect preferNative to change after the dialog constructor runs?

Aside from that, maybe this logic could go in willUpdate(changedProperties) { ... } so that useNative is set before render?

Likely not, it's more there as a just in case. willUpdate would make more sense. I'm also good to just remove this and can add back if needed.

Ok, I am good either way, but would move to willUpdate if we keep. It'd be really weird and potentially disruptive if it ever changed while open. I suppose if we draw the line now, then later on we won't be wondering if it ever changes once set.

dbatiste · 2026-04-15T18:40:44Z

 			_overflowBottom: { state: true },
 			_overflowTop: { state: true },
 			_parentDialog: { state: true },
+			_preferNative: { state: true },


Just raising in case it affects how you'd like to define this - the LMS fullscreen dialog appears to set preferNative = false. I think it probably does this to support launching LMS general (non-top-layer) dialogs nested within the fullscreen dialog. i.e. needed to ensure that nested non-top-layer LMS dialog stacks on top. I think it will still need to quietly opt out of native.

https://github.com/Brightspace/lms/blob/f9b23ff1941763b365a536f1d6168570f6eab8f4/lp/framework/web/D2L.LP.Web.UI/Legacy/MasterPages/Dialog/FullscreenDialogLegacy.js#L11

Ah interesting. With the current code in this PR this would still be fine, but if we ever set _preferNative to true by default in dialog-fullscreen.js then that would break. Perhaps then it should be declared as an attribute but ignored from the docs so that we can easily set it within the LMS.

Yeah. Via the attribute or setting it directly on the element in JavaScript. But yeah, wouldn't really want anyone else setting it.

dbatiste · 2026-04-16T14:03:45Z


+	firstUpdated(changedProperties) {
+		super.firstUpdated(changedProperties);
+		this._useNative = (window.D2L.DialogMixin.hasNative && this.preferNative);


I'm guessing that this is here (instead of the constructor) so that a consumer can:

const dialog = document.createElement('d2l-dialog'); dialog.preferNative = ...;

The unfortunate thing about firstUpdated() is that it runs after render(), so assigning to _useNative causes a second iteration render lifecycle.

We could move this to willUpdate() to avoid that. If we want to actively prevent someone from changing it after it's already assigned, we could add a check to not allow the update.

…ing _useNative

EdwinACL831 · 2026-04-17T16:38:11Z

+			 * @ignore
+			 * Do NOT use this
+			 */
+			preferNative: { type: Boolean, attribute: 'prefer-native' },


OK so just a question, if we do not want this attribute to be used, but we still expose it, why not making this a "protected" instance variable that is initialized in this mixin as needed, and overridden by web components that extends from it as well ?
That way we do not expose that attribute and also (as long as I understand the changes) it is initialized only in the constructor. Maybe I am missing more code context, but sounds like that is what we want to achieve maybe.

We will be setting it from the LMS code as well (context here). I had had it as this._preferNative and not an attribute prior to that, and I think in the LMS we technically could set that in the javascript, though to me that just feels like not a correct usage pattern (but I'm not totally opposed to doing that). I do think the risk of others setting this is likely low.

Oh I see, well in that case and IMHO what I would do is to remove the property, make it a private instance variable, and expose a setter for that code you and Dave were discussing about. That is only to be consistent with the intention of really not having to expose something we really don't want to as an HTML attribute. But if this results to be more cumbersome then, I guess adding the @ignore should do the trick. Just worried about people going through the code a noticing there is a property that they can set as an html attribute.

margaree added 6 commits April 6, 2026 13:29

Enable native dialog for d2l-dialog-confirm

24cf0ba

enable vdiffs for native dialogs for dialog-confirm and dialog-mixin

32cc493

tweak focus related changes

3230191

more implementation tweaks, related to chrome being different

d22b53a

fix feature flag default

c92c281

use the focus method for consistency

c69bbcd

github-actions Bot mentioned this pull request Apr 7, 2026

Updating vdiff goldens for PR 6760 #6762

Merged

github-actions Bot and others added 2 commits April 8, 2026 08:37

Updating vdiff goldens (#6762)

b7c3662

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

actually use native for the native tests

e9cb02b

github-actions Bot mentioned this pull request Apr 8, 2026

Updating vdiff goldens for PR 6760 #6768

Closed

margaree changed the title ~~[ignore for now] Enable native dialog for d2l-dialog-confirm~~ Spike Results: Enable native dialog for d2l-dialog-confirm Apr 8, 2026

margaree added 2 commits April 8, 2026 08:56

fix focus ring

851f7e7

fix lint

36148c7

margaree commented Apr 8, 2026

View reviewed changes

margaree requested a review from dbatiste April 8, 2026 14:47

margaree marked this pull request as ready for review April 10, 2026 14:53

margaree requested a review from a team as a code owner April 10, 2026 14:53

dbatiste reviewed Apr 14, 2026

View reviewed changes

Comment thread components/dialog/dialog-confirm.js Outdated

create preferNative property and use in dialog classes

44055c4

dbatiste reviewed Apr 15, 2026

View reviewed changes

margaree added 2 commits April 15, 2026 11:00

code review feedback: don't use updated

8d14b22

hide preferNative more

49f7824

dbatiste reviewed Apr 15, 2026

View reviewed changes

switch how preferNative is declared

cde6b8b

github-actions Bot mentioned this pull request Apr 16, 2026

Updating vdiff goldens for PR 6760 #6812

Draft

dbatiste reviewed Apr 16, 2026

View reviewed changes

code review feedback: use willUpdate instead of firstUpdated for sett…

4a12f84

…ing _useNative

EdwinACL831 reviewed Apr 17, 2026

View reviewed changes

Conversation

margaree commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 7, 2026

Uh oh!

margaree commented Apr 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatiste commented Apr 8, 2026

Uh oh!

margaree commented Apr 8, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatiste Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatiste Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdwinACL831 Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

margaree commented Apr 7, 2026 •

edited

Loading

dbatiste Apr 15, 2026 •

edited

Loading

dbatiste Apr 15, 2026 •

edited

Loading

EdwinACL831 Apr 17, 2026 •

edited

Loading