From c99dd2e0456ce462bba204692defa239d31f74ad Mon Sep 17 00:00:00 2001 From: Tan Li Hau Date: Tue, 14 Mar 2023 17:17:50 +0800 Subject: [PATCH 01/16] fix: binding group with if block (#8373) Fixes #8372 --------- Co-authored-by: Simon Holthausen --- src/compiler/compile/render_dom/Block.ts | 2 +- src/compiler/compile/render_dom/Renderer.ts | 4 +- .../render_dom/wrappers/Element/Binding.ts | 19 ++++++--- src/runtime/internal/dom.ts | 2 +- test/runtime-puppeteer/index.ts | 15 +++---- .../_config.js | 40 +++++++++++++++++++ .../main.svelte | 14 +++++++ .../_config.js | 34 ++++++++++++++++ .../main.svelte | 14 +++++++ 9 files changed, 128 insertions(+), 16 deletions(-) create mode 100644 test/runtime/samples/binding-input-group-if-gh-8372-1/_config.js create mode 100644 test/runtime/samples/binding-input-group-if-gh-8372-1/main.svelte create mode 100644 test/runtime/samples/binding-input-group-if-gh-8372-2/_config.js create mode 100644 test/runtime/samples/binding-input-group-if-gh-8372-2/main.svelte diff --git a/src/compiler/compile/render_dom/Block.ts b/src/compiler/compile/render_dom/Block.ts index c40dedc3b5..cb35673847 100644 --- a/src/compiler/compile/render_dom/Block.ts +++ b/src/compiler/compile/render_dom/Block.ts @@ -505,7 +505,7 @@ export default class Block { render_binding_groups() { for (const binding_group of this.binding_groups) { - binding_group.render(); + binding_group.render(this); } } } diff --git a/src/compiler/compile/render_dom/Renderer.ts b/src/compiler/compile/render_dom/Renderer.ts index ad2d1b092f..df1d68d5cb 100644 --- a/src/compiler/compile/render_dom/Renderer.ts +++ b/src/compiler/compile/render_dom/Renderer.ts @@ -27,8 +27,8 @@ export interface BindingGroup { contexts: string[]; list_dependencies: Set; keypath: string; - elements: Identifier[]; - render: () => void; + add_element: (block: Block, element: Identifier) => void; + render: (block: Block) => void; } export default class Renderer { diff --git a/src/compiler/compile/render_dom/wrappers/Element/Binding.ts b/src/compiler/compile/render_dom/wrappers/Element/Binding.ts index aa3d097ce3..e656d77672 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/Binding.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/Binding.ts @@ -153,7 +153,7 @@ export default class BindingWrapper { case 'group': { block.renderer.add_to_context('$$binding_groups'); - this.binding_group.elements.push(this.parent.var); + this.binding_group.add_element(block, this.parent.var); if ((this.parent as ElementWrapper).has_dynamic_value) { update_or_condition = (this.parent as ElementWrapper).dynamic_value_condition; @@ -323,7 +323,11 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B parent = parent.parent; } - const elements = []; + /** + * When using bind:group with logic blocks, the inputs with bind:group may be scattered across different blocks. + * This therefore keeps track of all the elements that have the same bind:group within the same block. + */ + const elements = new Map(); contexts.forEach(context => { renderer.add_to_context(context, true); @@ -343,8 +347,13 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B contexts, list_dependencies, keypath, - elements, - render() { + add_element(block, element) { + if (!elements.has(block)) { + elements.set(block, []); + } + elements.get(block).push(element); + }, + render(block) { const local_name = block.get_unique_name('binding_group'); const binding_group = block.renderer.reference('$$binding_groups'); block.add_variable(local_name); @@ -362,7 +371,7 @@ function get_binding_group(renderer: Renderer, binding: BindingWrapper, block: B ); } block.chunks.hydrate.push( - b`${local_name}.p(${elements})` + b`${local_name}.p(${elements.get(block)})` ); block.chunks.destroy.push( b`${local_name}.r()` diff --git a/src/runtime/internal/dom.ts b/src/runtime/internal/dom.ts index 99894d3d88..1f786d51b5 100644 --- a/src/runtime/internal/dom.ts +++ b/src/runtime/internal/dom.ts @@ -359,7 +359,7 @@ export function get_binding_group_value(group, __value, checked) { return Array.from(value); } -export function init_binding_group(group) { +export function init_binding_group(group: HTMLInputElement[]) { let _inputs: HTMLInputElement[]; return { /* push */ p(...inputs: HTMLInputElement[]) { diff --git a/test/runtime-puppeteer/index.ts b/test/runtime-puppeteer/index.ts index eba60544f6..efb7ac02e3 100644 --- a/test/runtime-puppeteer/index.ts +++ b/test/runtime-puppeteer/index.ts @@ -56,9 +56,7 @@ async function launchPuppeteer() { const assert = fs.readFileSync(`${__dirname}/assert.js`, 'utf-8'); -describe('runtime (puppeteer)', function() { - // Note: Increase the timeout in preparation for restarting Chromium due to SIGSEGV. - this.timeout(10000); +describe('runtime (puppeteer)', () => { before(async () => { svelte = loadSvelte(false); console.log('[runtime-puppeteer] Loaded Svelte'); @@ -75,7 +73,7 @@ describe('runtime (puppeteer)', function() { const failed = new Set(); - function runTest(dir, hydrate) { + function runTest(dir, hydrate, is_first_run) { if (dir[0] === '.') return; // MEMO: puppeteer can not execute Chromium properly with Node8,10 on Linux at GitHub actions. const { version } = process; @@ -254,11 +252,14 @@ describe('runtime (puppeteer)', function() { prettyPrintPuppeteerAssertionError(err.message); assertWarnings(); }); - }); + }).timeout(is_first_run ? 20000 : 10000); } + // Increase the timeout on the first run in preparation for restarting Chromium due to SIGSEGV. + let first_run = true; fs.readdirSync(`${__dirname}/samples`).forEach(dir => { - runTest(dir, false); - runTest(dir, true); + runTest(dir, false, first_run); + runTest(dir, true, first_run); + first_run = false; }); }); diff --git a/test/runtime/samples/binding-input-group-if-gh-8372-1/_config.js b/test/runtime/samples/binding-input-group-if-gh-8372-1/_config.js new file mode 100644 index 0000000000..bdc4da2d57 --- /dev/null +++ b/test/runtime/samples/binding-input-group-if-gh-8372-1/_config.js @@ -0,0 +1,40 @@ +export default { + async test({ assert, target, component, window }) { + const button = target.querySelector('button'); + const clickEvent = new window.Event('click'); + const changeEvent = new window.Event('change'); + + const [input1, input2] = target.querySelectorAll('input[type="checkbox"]'); + function validate_inputs(v1, v2) { + assert.equal(input1.checked, v1); + assert.equal(input2.checked, v2); + } + + assert.deepEqual(component.test, []); + validate_inputs(false, false); + + component.test = ['a', 'b']; + validate_inputs(true, true); + + input1.checked = false; + await input1.dispatchEvent(changeEvent); + assert.deepEqual(component.test, ['b']); + + input2.checked = false; + await input2.dispatchEvent(changeEvent); + assert.deepEqual(component.test, []); + + input1.checked = true; + input2.checked = true; + await input1.dispatchEvent(changeEvent); + await input2.dispatchEvent(changeEvent); + assert.deepEqual(component.test, ['b', 'a']); + + await button.dispatchEvent(clickEvent); + assert.deepEqual(component.test, ['b', 'a']); // should it be ['a'] only? valid arguments for both outcomes + + input1.checked = false; + await input1.dispatchEvent(changeEvent); + assert.deepEqual(component.test, []); + } +}; diff --git a/test/runtime/samples/binding-input-group-if-gh-8372-1/main.svelte b/test/runtime/samples/binding-input-group-if-gh-8372-1/main.svelte new file mode 100644 index 0000000000..71955b6b81 --- /dev/null +++ b/test/runtime/samples/binding-input-group-if-gh-8372-1/main.svelte @@ -0,0 +1,14 @@ + + + + + +{#if !hidden} + +{/if} + diff --git a/test/runtime/samples/binding-input-group-if-gh-8372-2/_config.js b/test/runtime/samples/binding-input-group-if-gh-8372-2/_config.js new file mode 100644 index 0000000000..a8d5a7137f --- /dev/null +++ b/test/runtime/samples/binding-input-group-if-gh-8372-2/_config.js @@ -0,0 +1,34 @@ +export default { + async test({ assert, target, component, window }) { + const button = target.querySelector('button'); + const clickEvent = new window.Event('click'); + const changeEvent = new window.Event('change'); + + const [input1, input2] = target.querySelectorAll('input[type="radio"]'); + function validate_inputs(v1, v2) { + assert.equal(input1.checked, v1); + assert.equal(input2.checked, v2); + } + + component.test = 'a'; + validate_inputs(true, false); + + component.test = 'b'; + validate_inputs(false, true); + + input1.checked = true; + await input1.dispatchEvent(changeEvent); + assert.deepEqual(component.test, 'a'); + + input2.checked = true; + await input2.dispatchEvent(changeEvent); + assert.deepEqual(component.test, 'b'); + + await button.dispatchEvent(clickEvent); + assert.deepEqual(component.test, 'b'); // should it be undefined? valid arguments for both outcomes + + input1.checked = true; + await input1.dispatchEvent(changeEvent); + assert.deepEqual(component.test, 'a'); + } +}; diff --git a/test/runtime/samples/binding-input-group-if-gh-8372-2/main.svelte b/test/runtime/samples/binding-input-group-if-gh-8372-2/main.svelte new file mode 100644 index 0000000000..9add22cacc --- /dev/null +++ b/test/runtime/samples/binding-input-group-if-gh-8372-2/main.svelte @@ -0,0 +1,14 @@ + + + + + +{#if !hidden} + +{/if} + From 127b61a4658c046f2a35136408b98fb832feaaf1 Mon Sep 17 00:00:00 2001 From: Jon Rouleau Date: Tue, 14 Mar 2023 04:46:49 -0500 Subject: [PATCH 02/16] fix: derived store restarting when unsubscribed from another store with a shared ancestor (#8368) Fixes #8364 --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- src/runtime/store/index.ts | 10 +++++++--- test/store/index.ts | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/runtime/store/index.ts b/src/runtime/store/index.ts index ee277c6216..67c2bcb15c 100644 --- a/src/runtime/store/index.ts +++ b/src/runtime/store/index.ts @@ -164,7 +164,7 @@ export function derived(stores: Stores, fn: Function, initial_value?: T): Rea const auto = fn.length < 2; return readable(initial_value, (set) => { - let inited = false; + let started = false; const values = []; let pending = 0; @@ -188,7 +188,7 @@ export function derived(stores: Stores, fn: Function, initial_value?: T): Rea (value) => { values[i] = value; pending &= ~(1 << i); - if (inited) { + if (started) { sync(); } }, @@ -197,12 +197,16 @@ export function derived(stores: Stores, fn: Function, initial_value?: T): Rea }) ); - inited = true; + started = true; sync(); return function stop() { run_all(unsubscribers); cleanup(); + // We need to set this to false because callbacks can still happen despite having unsubscribed: + // Callbacks might already be placed in the queue which doesn't know it should no longer + // invoke this derived store. + started = false; }; }); } diff --git a/test/store/index.ts b/test/store/index.ts index 7e8bdb2f64..920cabdc39 100644 --- a/test/store/index.ts +++ b/test/store/index.ts @@ -407,6 +407,25 @@ describe('store', () => { const d = derived(fake_observable, _ => _); assert.equal(get(d), 42); }); + + it('doesn\'t restart when unsubscribed from another store with a shared ancestor', () => { + const a = writable(true); + let b_started = false; + const b = derived(a, (_, __) => { + b_started = true; + return () => { + assert.equal(b_started, true); + b_started = false; + }; + }); + const c = derived(a, ($a, set) => { + if ($a) return b.subscribe(set); + }); + + c.subscribe(() => { }); + a.set(false); + assert.equal(b_started, false); + }); }); describe('get', () => { From 26c38e750c8b570183c32e9bdf32025f7291c15b Mon Sep 17 00:00:00 2001 From: Nguyen Tran <88808276+ngtr6788@users.noreply.github.com> Date: Tue, 14 Mar 2023 05:51:40 -0400 Subject: [PATCH 03/16] feat: add a11y `no-noninteractive-element-to-interactive-role` (#8167) Part of #820 --- .../content/docs/06-accessibility-warnings.md | 11 + src/compiler/compile/compiler_warnings.ts | 4 + src/compiler/compile/nodes/Element.ts | 11 +- src/compiler/compile/utils/a11y.ts | 61 +- .../input.svelte | 58 +- .../warnings.json | 26 +- .../input.svelte | 109 +++ .../warnings.json | 818 ++++++++++++++++++ 8 files changed, 1043 insertions(+), 55 deletions(-) create mode 100644 test/validator/samples/a11y-no-noninteractive-element-to-interactive-role/input.svelte create mode 100644 test/validator/samples/a11y-no-noninteractive-element-to-interactive-role/warnings.json diff --git a/site/content/docs/06-accessibility-warnings.md b/site/content/docs/06-accessibility-warnings.md index 4324ed7b75..bc793d80e7 100644 --- a/site/content/docs/06-accessibility-warnings.md +++ b/site/content/docs/06-accessibility-warnings.md @@ -277,6 +277,17 @@ Some HTML elements have default ARIA roles. Giving these elements an ARIA role t --- +### `a11y-no-noninteractive-element-to-interactive-role` + +[WAI-ARIA](https://www.w3.org/TR/wai-aria-1.1/#usage_intro) roles should not be used to convert a non-interactive element to an interactive element. Interactive ARIA roles include `button`, `link`, `checkbox`, `menuitem`, `menuitemcheckbox`, `menuitemradio`, `option`, `radio`, `searchbox`, `switch` and `textbox`. + +```sv + +

Button

+``` + +--- + ### `a11y-no-noninteractive-tabindex` Tab key navigation should be limited to elements on the page that can be interacted with. diff --git a/src/compiler/compile/compiler_warnings.ts b/src/compiler/compile/compiler_warnings.ts index d6b2dbc5aa..380ed5a6f0 100644 --- a/src/compiler/compile/compiler_warnings.ts +++ b/src/compiler/compile/compiler_warnings.ts @@ -119,6 +119,10 @@ export default { code: 'a11y-no-interactive-element-to-noninteractive-role', message: `A11y: <${element}> cannot have role '${role}'` }), + a11y_no_noninteractive_element_to_interactive_role: (role: string | boolean, element: string) => ({ + code: 'a11y-no-noninteractive-element-to-interactive-role', + message: `A11y: Non-interactive element <${element}> cannot have interactive role '${role}'` + }), a11y_role_has_required_aria_props: (role: string, props: string[]) => ({ code: 'a11y-role-has-required-aria-props', message: `A11y: Elements with the ARIA role "${role}" must have the following attributes defined: ${props.map(name => `"${name}"`).join(', ')}` diff --git a/src/compiler/compile/nodes/Element.ts b/src/compiler/compile/nodes/Element.ts index 0fdea77c16..a5dc74b256 100644 --- a/src/compiler/compile/nodes/Element.ts +++ b/src/compiler/compile/nodes/Element.ts @@ -24,14 +24,13 @@ import { Literal } from 'estree'; import compiler_warnings from '../compiler_warnings'; import compiler_errors from '../compiler_errors'; import { ARIARoleDefinitionKey, roles, aria, ARIAPropertyDefinition, ARIAProperty } from 'aria-query'; -import { is_interactive_element, is_non_interactive_roles, is_presentation_role, is_interactive_roles, is_hidden_from_screen_reader, is_semantic_role_element } from '../utils/a11y'; +import { is_interactive_element, is_non_interactive_element, is_non_interactive_roles, is_presentation_role, is_interactive_roles, is_hidden_from_screen_reader, is_semantic_role_element, is_abstract_role } from '../utils/a11y'; const aria_attributes = 'activedescendant atomic autocomplete busy checked colcount colindex colspan controls current describedby description details disabled dropeffect errormessage expanded flowto grabbed haspopup hidden invalid keyshortcuts label labelledby level live modal multiline multiselectable orientation owns placeholder posinset pressed readonly relevant required roledescription rowcount rowindex rowspan selected setsize sort valuemax valuemin valuenow valuetext'.split(' '); const aria_attribute_set = new Set(aria_attributes); const aria_roles = roles.keys(); const aria_role_set = new Set(aria_roles); -const aria_role_abstract_set = new Set(roles.keys().filter(role => roles.get(role).abstract)); const a11y_required_attributes = { a: ['href'], @@ -567,7 +566,7 @@ export default class Element extends Node { if (typeof value === 'string') { value.split(regex_any_repeated_whitespaces).forEach((current_role: ARIARoleDefinitionKey) => { - if (current_role && aria_role_abstract_set.has(current_role)) { + if (current_role && is_abstract_role(current_role)) { component.warn(attribute, compiler_warnings.a11y_no_abstract_role(current_role)); } else if (current_role && !aria_role_set.has(current_role)) { const match = fuzzymatch(current_role, aria_roles); @@ -607,8 +606,12 @@ export default class Element extends Node { if (is_interactive_element(this.name, attribute_map) && (is_non_interactive_roles(current_role) || is_presentation_role(current_role))) { component.warn(this, compiler_warnings.a11y_no_interactive_element_to_noninteractive_role(current_role, this.name)); } - }); + // no-noninteractive-element-to-interactive-role + if (is_non_interactive_element(this.name, attribute_map) && is_interactive_roles(current_role)) { + component.warn(this, compiler_warnings.a11y_no_noninteractive_element_to_interactive_role(current_role, this.name)); + } + }); } } diff --git a/src/compiler/compile/utils/a11y.ts b/src/compiler/compile/utils/a11y.ts index 60cb31c663..d0564b419e 100644 --- a/src/compiler/compile/utils/a11y.ts +++ b/src/compiler/compile/utils/a11y.ts @@ -7,7 +7,9 @@ import { import { AXObjects, AXObjectRoles, elementAXObjects } from 'axobject-query'; import Attribute from '../nodes/Attribute'; -const non_abstract_roles = [...roles_map.keys()].filter((name) => !roles_map.get(name).abstract); +const aria_roles = roles_map.keys(); +const abstract_roles = new Set(aria_roles.filter(role => roles_map.get(role).abstract)); +const non_abstract_roles = aria_roles.filter((name) => !abstract_roles.has(name)); const non_interactive_roles = new Set( non_abstract_roles @@ -40,6 +42,10 @@ export function is_interactive_roles(role: ARIARoleDefinitionKey) { return interactive_roles.has(role); } +export function is_abstract_role(role: ARIARoleDefinitionKey) { + return abstract_roles.has(role); +} + const presentation_roles = new Set(['presentation', 'none']); export function is_presentation_role(role: ARIARoleDefinitionKey) { @@ -65,7 +71,7 @@ export function is_hidden_from_screen_reader(tag_name: string, attribute_map: Ma const non_interactive_element_role_schemas: ARIARoleRelationConcept[] = []; elementRoles.entries().forEach(([schema, roles]) => { - if ([...roles].every((role) => non_interactive_roles.has(role))) { + if ([...roles].every((role) => role !== 'generic' && non_interactive_roles.has(role))) { non_interactive_element_role_schemas.push(schema); } }); @@ -82,6 +88,10 @@ const interactive_ax_objects = new Set( [...AXObjects.keys()].filter((name) => AXObjects.get(name).type === 'widget') ); +const non_interactive_ax_objects = new Set( + [...AXObjects.keys()].filter((name) => ['windows', 'structure'].includes(AXObjects.get(name).type)) +); + const interactive_element_ax_object_schemas: ARIARoleRelationConcept[] = []; elementAXObjects.entries().forEach(([schema, ax_object]) => { @@ -90,6 +100,14 @@ elementAXObjects.entries().forEach(([schema, ax_object]) => { } }); +const non_interactive_element_ax_object_schemas: ARIARoleRelationConcept[] = []; + +elementAXObjects.entries().forEach(([schema, ax_object]) => { + if ([...ax_object].every((role) => non_interactive_ax_objects.has(role))) { + non_interactive_element_ax_object_schemas.push(schema); + } +}); + function match_schema( schema: ARIARoleRelationConcept, tag_name: string, @@ -110,24 +128,31 @@ function match_schema( }); } -export function is_interactive_element( +export enum ElementInteractivity { + Interactive = 'interactive', + NonInteractive = 'non-interactive', + Static = 'static', +} + +export function element_interactivity( tag_name: string, attribute_map: Map -): boolean { +): ElementInteractivity { if ( interactive_element_role_schemas.some((schema) => match_schema(schema, tag_name, attribute_map) ) ) { - return true; + return ElementInteractivity.Interactive; } if ( + tag_name !== 'header' && non_interactive_element_role_schemas.some((schema) => match_schema(schema, tag_name, attribute_map) ) ) { - return false; + return ElementInteractivity.NonInteractive; } if ( @@ -135,10 +160,30 @@ export function is_interactive_element( match_schema(schema, tag_name, attribute_map) ) ) { - return true; + return ElementInteractivity.Interactive; } - return false; + if ( + non_interactive_element_ax_object_schemas.some((schema) => + match_schema(schema, tag_name, attribute_map) + ) + ) { + return ElementInteractivity.NonInteractive; + } + + return ElementInteractivity.Static; +} + +export function is_interactive_element(tag_name: string, attribute_map: Map): boolean { + return element_interactivity(tag_name, attribute_map) === ElementInteractivity.Interactive; +} + +export function is_non_interactive_element(tag_name: string, attribute_map: Map): boolean { + return element_interactivity(tag_name, attribute_map) === ElementInteractivity.NonInteractive; +} + +export function is_static_element(tag_name: string, attribute_map: Map): boolean { + return element_interactivity(tag_name, attribute_map) === ElementInteractivity.Static; } export function is_semantic_role_element(role: ARIARoleDefinitionKey, tag_name: string, attribute_map: Map) { diff --git a/test/validator/samples/a11y-no-interactive-element-to-noninteractive-role/input.svelte b/test/validator/samples/a11y-no-interactive-element-to-noninteractive-role/input.svelte index 80b4fd9410..5c9bfd86ed 100644 --- a/test/validator/samples/a11y-no-interactive-element-to-noninteractive-role/input.svelte +++ b/test/validator/samples/a11y-no-interactive-element-to-noninteractive-role/input.svelte @@ -71,36 +71,34 @@
- -
-x -
-
-
- -
-
-
-
- -

title

-

title

-

title

-

title

-
title
-
title
-
-x -
  • -
  • -