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

a11y: use role=alert for messages from Django and JS #1641

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions umap/static/umap/alerts.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
[data-alert] {
box-sizing: border-box;
min-height: 46px;
line-height: 46px;
padding-left: 10px;
width: calc(100% - 500px);
position: absolute;
left: 250px; /* Keep save/cancel button accessible. */
right: 250px;
box-shadow: 0 1px 7px #999999;
background: none repeat scroll 0 0 rgba(20, 22, 23, 0.8);
font-weight: bold;
color: #fff;
font-size: 0.8em;
z-index: 1012;
border-radius: 2px;
visibility: visible;
top: 23px;
display: flex;
justify-content: space-between;
align-items: flex-start;
}
[data-alert][data-level="error"] {
background-color: #c60f13;
}
[data-alert] [data-close] {
color: #fff;
padding-right: 10px;
width: 100px;
line-height: 1;
margin: .5rem;
background-color: #202425;
font-size: .7rem;
}
54 changes: 54 additions & 0 deletions umap/static/umap/js/modules/alerts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
export default class Alerts {
constructor() {
this.alertNode = document.querySelector('[role="alert"]')
Copy link
Member

Choose a reason for hiding this comment

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

nit: Maybe alertNode could be a private property of the object?

const observer = new MutationObserver(this._callback.bind(this))
observer.observe(this.alertNode, { childList: true })
// On initial page load, we want to display messages from Django.
Array.from(this.alertNode.children).forEach(this._display.bind(this))
}

_callback(mutationList, observer) {
for (const mutation of mutationList) {
this._display(
[...mutation.addedNodes].filter((item) => item.tagName === 'P').pop()
)
almet marked this conversation as resolved.
Show resolved Hide resolved
}
}

_display(alert) {
const duration = alert.dataset?.duration || 3000
const level = alert.dataset?.level || 'info'
Copy link
Member

Choose a reason for hiding this comment

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

This repeats (in the code) the default behavior values. Maybe use a constant tied to the class instead?

const wrapper = document.createElement('div')
const alertHTML = alert.cloneNode(true).outerHTML
wrapper.innerHTML = `
<div data-level="${level}" data-alert data-toclose>
${alertHTML}
<button class="umap-close-link" type="button" data-close>
<i class="umap-close-icon"></i><span>${L._('Close')}</span>
</button>
</div>
`
const alertDiv = wrapper.firstElementChild
this.alertNode.after(alertDiv)
if (isFinite(duration)) {
Copy link
Member

Choose a reason for hiding this comment

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

Love using Infinity + isFinite 🤝

setTimeout(() => {
alertDiv.remove()
}, duration)
}
}

add(message, level = 'info', duration = 3000) {
this.alertNode.innerHTML = `
Copy link
Member

Choose a reason for hiding this comment

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

Is this replacing all the whole node? How does this plays with multiple alerts being added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed a concern, I was wondering if that case actually happens 🤔

<p data-level="${level}" data-duration="${duration}">
${message}
</p>
`
}
}

// TODISCUSS: this might be something we want somewhere else.
document.addEventListener('click', (event) => {
if (event.target.closest('[data-close]')) {
event.target.closest('[data-toclose]').remove()
}
})
12 changes: 11 additions & 1 deletion umap/static/umap/js/modules/global.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import * as L from '../../vendors/leaflet/leaflet-src.esm.js'
import URLs from './urls.js'
import Alerts from './alerts.js'
import Browser from './browser.js'
import { Request, ServerRequest, RequestError, HTTPError, NOKError } from './request.js'
// Import modules and export them to the global scope.
// For the not yet module-compatible JS out there.

// Copy the leaflet module, it's expected by leaflet plugins to be writeable.
window.L = { ...L }
window.U = { URLs, Request, ServerRequest, RequestError, HTTPError, NOKError, Browser }
window.U = {
Alerts,
URLs,
Request,
ServerRequest,
RequestError,
HTTPError,
NOKError,
Browser,
}
13 changes: 6 additions & 7 deletions umap/static/umap/js/modules/request.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Uses `L._`` from Leaflet.i18n which we cannot import as a module yet
import { DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js'
import Alert from './alerts.js'

export class RequestError extends Error {}

Expand Down Expand Up @@ -50,6 +51,7 @@ export class Request extends BaseRequest {
constructor(ui) {
super()
this.ui = ui
this.alerts = new Alert()
}

async _fetch(method, uri, headers, data) {
Expand Down Expand Up @@ -81,7 +83,7 @@ export class Request extends BaseRequest {
}

_onError(error) {
this.ui.alert({ content: L._('Problem in the response'), level: 'error' })
this.alerts.add(L._('Problem in the response'), 'error')
}

_onNOK(error) {
Expand Down Expand Up @@ -127,10 +129,10 @@ export class ServerRequest extends Request {
try {
const data = await response.json()
if (data.info) {
this.ui.alert({ content: data.info, level: 'info' })
this.alerts.add(data.info)
this.ui.closePanel()
} else if (data.error) {
this.ui.alert({ content: data.error, level: 'error' })
this.alerts.add(data.error, 'error')
return this._onError(new Error(data.error))
}
return [data, response, null]
Expand All @@ -145,10 +147,7 @@ export class ServerRequest extends Request {

_onNOK(error) {
if (error.status === 403) {
this.ui.alert({
content: message || L._('Action not allowed :('),
level: 'error',
})
this.alerts.add(message || L._('Action not allowed :('), 'error')
}
return [{}, error.response, error]
}
Expand Down
5 changes: 2 additions & 3 deletions umap/static/umap/js/umap.controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -782,8 +782,7 @@ const ControlsMixin = {
if (datalayer.hasDataVisible()) found = true
})
// TODO: display a results counter in the panel instead.
if (!found)
this.ui.alert({ content: L._('No results for these facets'), level: 'info' })
if (!found) this.alerts.add(L._('No results for these facets'))
}

const fields = keys.map((current) => [
Expand Down Expand Up @@ -1272,7 +1271,7 @@ U.Search = L.PhotonSearch.extend({
if (latlng.isValid()) {
this.reverse.doReverse(latlng)
} else {
this.map.ui.alert({ content: 'Invalid latitude or longitude', mode: 'error' })
this.map.alerts.add(L._('Invalid latitude or longitude'), 'error')
}
return
}
Expand Down
7 changes: 2 additions & 5 deletions umap/static/umap/js/umap.features.js
Original file line number Diff line number Diff line change
Expand Up @@ -686,10 +686,7 @@ U.Marker = L.Marker.extend({
const builder = new U.FormBuilder(this, coordinatesOptions, {
callback: function () {
if (!this._latlng.isValid()) {
this.map.ui.alert({
content: L._('Invalid latitude or longitude'),
level: 'error',
})
this.map.alerts.add(L._('Invalid latitude or longitude'), 'error')
builder.resetField('_latlng.lat')
builder.resetField('_latlng.lng')
}
Expand Down Expand Up @@ -886,7 +883,7 @@ U.PathMixin = {
items.push({
text: L._('Display measure'),
callback: function () {
this.map.ui.alert({ content: this.getMeasure(), level: 'info' })
this.map.alerts.add(this.getMeasure())
},
context: this,
})
Expand Down
11 changes: 5 additions & 6 deletions umap/static/umap/js/umap.importer.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,15 @@ U.Importer = L.Class.extend({
this.map.processFileToImport(file, layer, type)
}
} else {
if (!type)
return this.map.ui.alert({
content: L._('Please choose a format'),
level: 'error',
})
if (!type) {
this.map.alerts.add(L._('Please choose a format'), 'error')
return
}
if (this.rawInput.value && type === 'umap') {
try {
this.map.importRaw(this.rawInput.value, type)
} catch (e) {
this.ui.alert({ content: L._('Invalid umap data'), level: 'error' })
this.alerts.add(L._('Invalid umap data'), 'error')
console.error(e)
}
} else {
Expand Down
65 changes: 31 additions & 34 deletions umap/static/umap/js/umap.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ U.Map = L.Map.extend({
// After calling parent initialize, as we are doing initCenter our-selves
if (geojson.geometry) this.options.center = this.latLng(geojson.geometry)
this.urls = new U.URLs(this.options.urls)
this.alerts = new U.Alerts()

this.ui = new U.UI(this._container)
this.ui.on('dataloading', (e) => this.fire('dataloading', e))
Expand Down Expand Up @@ -393,7 +394,9 @@ U.Map = L.Map.extend({
icon: 'umap-fake-class',
iconLoading: 'umap-fake-class',
flyTo: this.options.easing,
onLocationError: (err) => this.ui.alert({ content: err.message }),
onLocationError: (err) => {
this.alerts.add(err.message, 'error')
},
})
this._controls.fullscreen = new L.Control.Fullscreen({
title: { false: L._('View Fullscreen'), true: L._('Exit Fullscreen') },
Expand Down Expand Up @@ -677,10 +680,10 @@ U.Map = L.Map.extend({
} catch (e) {
console.error(e)
this.removeLayer(tilelayer)
this.ui.alert({
content: `${L._('Error in the tilelayer URL')}: ${tilelayer._url}`,
level: 'error',
})
this.alerts.add(
`${L._('Error in the tilelayer URL')}: ${tilelayer._url}`,
'error'
)
// Users can put tilelayer URLs by hand, and if they add wrong {variable},
// Leaflet throw an error, and then the map is no more editable
}
Expand Down Expand Up @@ -712,10 +715,7 @@ U.Map = L.Map.extend({
} catch (e) {
this.removeLayer(overlay)
console.error(e)
this.ui.alert({
content: `${L._('Error in the overlay URL')}: ${overlay._url}`,
level: 'error',
})
this.alerts.add(`${L._('Error in the overlay URL')}: ${overlay._url}`, 'error')
}
},

Expand Down Expand Up @@ -842,10 +842,7 @@ U.Map = L.Map.extend({
if (this.options.umap_id) {
// We do not want an extra message during the map creation
// to avoid the double notification/alert.
this.ui.alert({
content: L._('The zoom and center have been modified.'),
level: 'info',
})
this.alerts.add(L._('The zoom and center have been modified.'))
}
},

Expand Down Expand Up @@ -889,12 +886,12 @@ U.Map = L.Map.extend({
processFileToImport: function (file, layer, type) {
type = type || L.Util.detectFileType(file)
if (!type) {
this.ui.alert({
content: L._('Unable to detect format of file {filename}', {
this.alerts.add(
L._('Unable to detect format of file {filename}', {
filename: file.name,
}),
level: 'error',
})
'error'
)
return
}
if (type === 'umap') {
Expand Down Expand Up @@ -946,10 +943,10 @@ U.Map = L.Map.extend({
self.importRaw(rawData)
} catch (e) {
console.error('Error importing data', e)
self.ui.alert({
content: L._('Invalid umap data in {filename}', { filename: file.name }),
level: 'error',
})
self.alerts.add(
L._('Invalid umap data in {filename}', { filename: file.name }),
'error'
)
}
}
},
Expand Down Expand Up @@ -1060,10 +1057,10 @@ U.Map = L.Map.extend({
const uri = this.urls.get('map_save', { map_id: this.options.umap_id })
const [data, response, error] = await this.server.post(uri, {}, formData)
if (!error) {
let duration = 3000,
alert = { content: L._('Map has been saved!'), level: 'info' }
let alertDuration = 3000
let alertMessage = L._('Map has been saved!')
if (!this.options.umap_id) {
alert.content = L._('Congratulations, your map has been created!')
alertMessage = L._('Congratulations, your map has been created!')
this.options.umap_id = data.id
this.permissions.setOptions(data.permissions)
this.permissions.commit()
Expand All @@ -1072,8 +1069,8 @@ U.Map = L.Map.extend({
data.permissions.anonymous_edit_url &&
this.options.urls.map_send_edit_link
) {
alert.duration = Infinity
alert.content =
alertDuration = Infinity
alertMessage =
L._(
'Your map has been created! As you are not logged in, here is your secret link to edit the map, please keep it safe:'
) + `<br>${data.permissions.anonymous_edit_url}`
Expand Down Expand Up @@ -1108,8 +1105,9 @@ U.Map = L.Map.extend({
if (history && history.pushState)
history.pushState({}, this.options.name, data.url)
else window.location = data.url
alert.content = data.info || alert.content
this.once('saved', () => this.ui.alert(alert))
this.once('saved', () => {
this.alerts.add(data.info || alertMessage, 'info', alertDuration)
})
this.ui.closePanel()
this.permissions.save()
}
Expand Down Expand Up @@ -1142,19 +1140,18 @@ U.Map = L.Map.extend({
},

star: async function () {
if (!this.options.umap_id)
return this.ui.alert({
content: L._('Please save the map first'),
level: 'error',
})
if (!this.options.umap_id) {
this.alerts.add(L._('Please save the map first'), 'error')
return
}
const url = this.urls.get('map_star', { map_id: this.options.umap_id })
const [data, response, error] = await this.server.post(url)
if (!error) {
this.options.starred = data.starred
let msg = data.starred
? L._('Map has been starred')
: L._('Map has been unstarred')
this.ui.alert({ content: msg, level: 'info' })
this.alerts.add(msg)
this.renderControls()
}
},
Expand Down