Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Modal Dialogue component #1668

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/govuk/all.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import CharacterCount from './components/character-count/character-count'
import Checkboxes from './components/checkboxes/checkboxes'
import ErrorSummary from './components/error-summary/error-summary'
import Header from './components/header/header'
import ModalDialogue from './components/modal-dialogue/modal-dialogue'
import Radios from './components/radios/radios'
import Tabs from './components/tabs/tabs'

Expand Down Expand Up @@ -50,6 +51,11 @@ function initAll (options) {
var $toggleButton = scope.querySelector('[data-module="govuk-header"]')
new Header($toggleButton).init()

var $modals = scope.querySelectorAll('[data-module="govuk-modal-dialogue"]')
nodeListForEach($modals, function ($modal) {
new ModalDialogue($modal).init()
})

var $radios = scope.querySelectorAll('[data-module="govuk-radios"]')
nodeListForEach($radios, function ($radio) {
new Radios($radio).init()
Expand Down
1 change: 1 addition & 0 deletions src/govuk/components/_all.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
@import "input/input";
@import "inset-text/inset-text";
@import "label/label";
@import "modal-dialogue/modal-dialogue";
@import "panel/panel";
@import "phase-banner/phase-banner";
@import "tabs/tabs";
Expand Down
Empty file.
161 changes: 161 additions & 0 deletions src/govuk/components/modal-dialogue/_modal-dialogue.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
@import "../../settings/all";
@import "../../tools/all";
@import "../../helpers/all";

@include govuk-exports("govuk/component/modal-dialogue") {
$govuk-dialogue-width: 640px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to use !default here so that the value can overridden.


.govuk-modal-dialogue,
.govuk-modal-dialogue__backdrop {
position: fixed;
z-index: 0;
Copy link

@nogginbox nogginbox Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase to 1000 so that this will appear above all other elements already in the site.

I had to do this when I've used this code as several elements floated on top of the backdrop.

Suggested change
z-index: 0;
z-index: 1000;

top: 0;
left: 0;
width: 100%;
height: 100%;
}

// Hide dialogue when closed
.govuk-modal-dialogue {
display: none;
}

// Show dialogue when opened
.govuk-modal-dialogue--open {
display: block;
}

// Wrapper to handle overflow scrolling
.govuk-modal-dialogue__wrapper {
box-sizing: border-box;
display: flex;
height: 100%;
@include govuk-responsive-padding(7, "top");
@include govuk-responsive-padding(7, "bottom");
overflow-y: auto;
align-items: flex-start; // sass-lint:disable no-duplicate-properties
align-items: safe center;
}

// HTML5 dialogue component
.govuk-modal-dialogue__box {
box-sizing: border-box;
display: block;
position: relative;
z-index: 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to increase this, just in the (non-ideal) case that the page has other competing elements. I notice that the GOV.UK Publishing equivalent sets this to 1000.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this 1001 so it is above the backdrop which also needs the z-index increasing.

Suggested change
z-index: 1;
z-index: 1001;

width: 90%;
margin: auto;
padding: 0;
overflow-y: auto;
Copy link
Member

@hannalaakso hannalaakso Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be okay as it is but just for reference, I notice that the GOV.UK Publishing example currently sets these to:

overflow-x: hidden;
overflow-y: scroll;

I don't think we currently have an example with page content in the main area so we need to add one (see #1668 (comment)) in order to check that the main page content is not scrollable and the modal is.

border: $govuk-focus-width solid govuk-colour("black");
background: govuk-colour("white");

// Add focus outline to dialogue
&:focus {
outline: $govuk-focus-width solid $govuk-focus-colour;
}

// Hide browser backdrop
&::backdrop {
display: none;
}
}

// Header with close button
.govuk-modal-dialogue__header {
@include govuk-clearfix;
@include govuk-responsive-margin(5, "bottom");
padding-bottom: $govuk-focus-width;
color: govuk-colour("white");
background-color: govuk-colour("black");
text-align: right;
}

// Inner content
.govuk-modal-dialogue__content {
@include govuk-font($size: 16);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to safely remove this font definition as I wanted the site's standard fonts to be used. We're a non gov body so can't use the standard GDS font and have overrides in place to not use them. This line being included broke our GDS font overrides.

Suggested change
@include govuk-font($size: 16);

@include govuk-responsive-padding(6);
padding-top: 0;
}

.govuk-modal-dialogue__description {
@include govuk-responsive-margin(4, "bottom");
}

// Remove bottom margins
.govuk-modal-dialogue__description:last-child,
.govuk-modal-dialogue__description > :last-child,
.govuk-modal-dialogue__content > :last-child {
margin-bottom: 0;
}

// Custom backdrop
.govuk-modal-dialogue__backdrop {
opacity: .8;
background: govuk-colour("black");
pointer-events: none;
touch-action: none;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this block should live below https://github.com/alphagov/govuk-frontend/pull/1668/files#diff-40be96fa6566e06e9d9d9ab4d1d3d831R8 to in order to keep the backdrop styles together.


// Crown icon
.govuk-modal-dialogue__crown {
display: block;
margin: 6px 0 0 6px;
@include govuk-responsive-margin(5, "left");
float: left;
}

// Heading
.govuk-modal-dialogue__heading:last-child {
margin-bottom: 0;
}

// Close button
.govuk-modal-dialogue__close {
$font-size: 36px;
$line-height: 1;

display: block;
width: auto;
min-width: 44px;
margin: 0;
padding: 2px 5px;
float: right;
color: govuk-colour("white");
background-color: govuk-colour("black");
box-shadow: none !important;
font-size: $font-size;
@if $govuk-typography-use-rem {
font-size: govuk-px-to-rem($font-size);
}
@include govuk-typography-weight-bold;
line-height: $line-height;

&:hover {
color: govuk-colour("black");
background-color: govuk-colour("yellow");
}

&:active {
top: 0;
}
}

// New dialogue width, inline button + link
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?

@include govuk-media-query($from: tablet) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the mobile/tablet experience meant to be similar to the GOV.UK Publishing modal?

Screen Shot 2019-12-10 at 16 52 02

This might be something for @Ash-Wilson to consider.

.govuk-modal-dialogue__content {
padding-top: 0;
}

.govuk-modal-dialogue__box {
width: percentage($govuk-dialogue-width / map-get($govuk-breakpoints, desktop));
}
}

// Fixed width
@include govuk-media-query($from: desktop) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?

.govuk-modal-dialogue__box {
width: $govuk-dialogue-width;
}
}
}
3 changes: 3 additions & 0 deletions src/govuk/components/modal-dialogue/macro.njk
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{% macro govukModalDialogue(params) %}
{%- include "./template.njk" -%}
{% endmacro %}
186 changes: 186 additions & 0 deletions src/govuk/components/modal-dialogue/modal-dialogue.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import { nodeListForEach } from '../../common'
import '../../vendor/polyfills/Element/prototype/classList'
import '../../vendor/polyfills/Function/prototype/bind'
import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation

function ModalDialogue ($module) {
this.$module = $module
this.$dialogBox = $module.querySelector('dialog')
this.$container = document.documentElement

// Check for browser support
this.hasNativeDialog = 'showModal' in this.$dialogBox

// Allowed focussable elements
this.focussable = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an 'inert' feature we could consider referring to when reviewing the focus trap functionality in this proposal:

https://github.com/WICG/inert

It ships in chromium, I'm not sure if it's a proper standard yet but worth thinking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sounds like a good enhancement.

You'd have to be very careful and keep the modal outside govuk-width-container or whatever the page wrapper element was.

<body>
    <div class="govuk-width-container" inert></div>
    <div data-module="govuk-modal-dialogue"></div>
</body>

It could go horribly wrong if the modal was inside the [inert] wrapper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The early prototype version of the modal that GOV.UK based theirs on used both the inert attribute and aria-hidden to make the <main> wrapper non-actionable and this tested well with screen reader users. The modal markup was outside <main> and below <body>.

Colin does have a valid point: we need to be clear in the guidance about where the modal markup is placed in the page content.

'button',
'[href]',
'input',
'select',
'textarea'
]
}

// Initialize component
ModalDialogue.prototype.init = function (options) {
this.options = options || {}

this.open = this.handleOpen.bind(this)
Copy link
Contributor

@NickColley NickColley Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these method binds should be in the constructor instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickColley I try to defer anything that uses more CPU cycles until it's actually needed (like when .init() gets run) but it's probably negligible so don't mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Colin 👍 We believe that the performance impact is negligible so we should make the change for consistency with other components.

this.close = this.handleClose.bind(this)
this.focus = this.handleFocus.bind(this)
this.focusTrap = this.handleFocusTrap.bind(this)
this.boundKeyDown = this.handleKeyDown.bind(this)

// Modal elements
this.$buttonClose = this.$dialogBox.querySelector('.govuk-modal-dialogue__close')
this.$focussable = this.$dialogBox.querySelectorAll(this.focussable.toString())
this.$focusableLast = this.$focussable[this.$focussable.length - 1]
this.$focusElement = this.options.focusElement || this.$dialogBox

if (this.$dialogBox.hasAttribute('open')) {
this.open()
}

this.initEvents()

return this
}

// Initialize component events
ModalDialogue.prototype.initEvents = function (options) {
if (this.options.triggerElement) {
this.options.triggerElement.addEventListener('click', this.open)
}

// Close dialogue on close button click
this.$buttonClose.addEventListener('click', this.close)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal but I wonder if the events inside this block could live inside init(), it'll be tidier once we move the variable definitions to the constructor.


// Open modal
ModalDialogue.prototype.handleOpen = function (event) {
if (event) {
event.preventDefault()
}

// Save last-focussed element
this.$lastActiveElement = document.activeElement

// Disable scrolling, show wrapper
this.$container.classList.add('govuk-!-scroll-disabled')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does govuk-!-scroll-disabled do anything? I can't see any styles created for it.

this.$module.classList.add('govuk-modal-dialogue--open')

// Close on escape key, trap focus
document.addEventListener('keydown', this.boundKeyDown, true)

// Optional 'onOpen' callback
if (typeof this.options.onOpen === 'function') {
this.options.onOpen.call(this)
}

// Skip open if already open
if (this.$dialogBox.hasAttribute('open')) {
return
}

// Show modal
this.hasNativeDialog
? this.$dialogBox.show()
: this.$dialogBox.setAttribute('open', '')

// Handle focus
this.focus()
}

// Close modal
ModalDialogue.prototype.handleClose = function (event) {
if (event) {
event.preventDefault()
}

// Skip close if already closed
if (!this.$dialogBox.hasAttribute('open')) {
return
}

// Hide modal
this.hasNativeDialog
? this.$dialogBox.close()
: this.$dialogBox.removeAttribute('open')

// Hide wrapper, enable scrolling
this.$module.classList.remove('govuk-modal-dialogue--open')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we save class names as variables in the constructor and reference them from there?

this.$container.classList.remove('govuk-!-scroll-disabled')

// Restore focus to last active element
this.$lastActiveElement.focus()

// Optional 'onClose' callback
if (typeof this.options.onClose === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to consider how we're documenting the JavaScript options for components, since we use data-attributes elsewhere instead of programmatic options.

See also: Proposal programmatic and data attribute api interface for components

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickColley Yeah good point.

Here's how we currently use this Modal API on our prototype:

var $dialogue = document.querySelector('[data-module="govuk-modal-dialogue"]')
var $dialogueButtonResume = document.querySelector('.govuk-modal-dialogue__continue')

if ($dialogue && $dialogueButtonResume) {
  var modal = new window.ModalDialogue($dialogue)
    .init({
      focusElement: $dialogueButtonResume,
      onClose: function () {
        // Extend session timeout here
      }
    })
}

this.options.onClose.call(this)
}

// Remove escape key and trap focus listener
document.removeEventListener('keydown', this.boundKeyDown, true)
}

// Lock scroll, focus modal
ModalDialogue.prototype.handleFocus = function () {
this.$dialogBox.scrollIntoView()
Copy link
Member

@hannalaakso hannalaakso Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the component is going to follow the mobile experience of the GOV.UK Publisher modal (see #1668 (comment)) then this might not be needed, since the modal will either have a fixed position or fill the viewport?

this.$focusElement.focus({ preventScroll: true })
}

// Ensure focus stays within modal
ModalDialogue.prototype.handleFocusTrap = function (event) {
Copy link
Member

@hannalaakso hannalaakso Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to understand more about the scenario in which focus might escape the modal. This function executes when the tab key is pressed, but the background should already be non-actionable so that none of the background elements can be tabbed to.

Fixing #1668 (comment) should help with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this point, what if a user navigates via landmarks? What is the impact on them when the modal is open.

var $focusElement

// Check for tabbing outside dialog
var hasFocusEscaped = document.activeElement !== this.$dialogBox

// Loop inner focussable elements
if (hasFocusEscaped) {
nodeListForEach(this.$focussable, function (element) {
// Actually, focus is on an inner focussable element
if (hasFocusEscaped && document.activeElement === element) {
hasFocusEscaped = false
}
})

// Wrap focus back to first element
$focusElement = hasFocusEscaped
? this.$dialogBox
: undefined
}

// Wrap focus back to first/last element
if (!$focusElement) {
if ((document.activeElement === this.$focusableLast && !event.shiftKey) || !this.$focussable.length) {
$focusElement = this.$dialogBox
} else if (document.activeElement === this.$dialogBox && event.shiftKey) {
$focusElement = this.$focusableLast
}
}

// Wrap focus
if ($focusElement) {
event.preventDefault()
$focusElement.focus({ preventScroll: true })
}
}

// Listen for key presses
ModalDialogue.prototype.handleKeyDown = function (event) {
var KEY_TAB = 9
var KEY_ESCAPE = 27

switch (event.keyCode) {
case KEY_TAB:
this.focusTrap(event)
break

case KEY_ESCAPE:
this.close()
break
}
}

export default ModalDialogue
Loading