From 4abe6f2a8f94d364b7a5c795be4cd7fb5cd174c0 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 22 Nov 2023 12:07:50 -0600 Subject: [PATCH 01/13] Make anchor behave like native anchor when contenteditable --- .../nimble-components/src/anchor/index.ts | 29 +++ .../nimble-components/src/anchor/styles.ts | 4 + .../nimble-components/src/anchor/template.ts | 9 +- .../src/anchor/tests/anchor.spec.ts | 204 ++++++++++++------ 4 files changed, 173 insertions(+), 73 deletions(-) diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index 9ddffbca88..a9e15c46d5 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -34,6 +34,35 @@ export class Anchor extends AnchorBase { */ @attr public appearance: AnchorAppearance; + + /** + * @internal + */ + public container?: HTMLElement; + + public override connectedCallback(): void { + super.connectedCallback(); + this.updateContentEditable(); + } + + /** + * @internal + * We want our anchor to behave like a native anchor when in a content-editable + * region, i.e. not operable or focusable. The most reliable way to achieve this is + * to synchronize our shadow DOM's content-editable state with that of the host. + * We must look at the host's read-only isContentEditable property, which factors + * in any inherited value. Unfortunately, there is no way for us to detect when its + * value has changed, so the best we can do is to re-sync on specific events. + * This has shortcomings, e.g. if isContentEditable goes from true to false, our + * anchor will remain un-tabable until a mouseenter triggers a re-sync. + */ + public updateContentEditable(): void { + if (this.isContentEditable) { + this.container!.setAttribute('contenteditable', ''); + } else { + this.container!.removeAttribute('contenteditable'); + } + } } // FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here. diff --git a/packages/nimble-components/src/anchor/styles.ts b/packages/nimble-components/src/anchor/styles.ts index 8c225dee73..9d1f1a9c0e 100644 --- a/packages/nimble-components/src/anchor/styles.ts +++ b/packages/nimble-components/src/anchor/styles.ts @@ -20,6 +20,10 @@ export const styles = css` font: ${linkFont}; } + .top-container { + display: contents; + } + [part='start'] { display: none; } diff --git a/packages/nimble-components/src/anchor/template.ts b/packages/nimble-components/src/anchor/template.ts index 862228536f..ff2ecc3b51 100644 --- a/packages/nimble-components/src/anchor/template.ts +++ b/packages/nimble-components/src/anchor/template.ts @@ -9,7 +9,10 @@ import type { Anchor } from '.'; export const template: FoundationElementTemplate< ViewTemplate, AnchorOptions -> = (_context, definition) => html` = (_context, definition) => html``; diff --git a/packages/nimble-components/src/anchor/tests/anchor.spec.ts b/packages/nimble-components/src/anchor/tests/anchor.spec.ts index 3a16cee610..4597c93169 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.spec.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.spec.ts @@ -4,23 +4,7 @@ import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { fixture, Fixture } from '../../utilities/tests/fixture'; import { getSpecTypeByNamedList } from '../../utilities/tests/parameterized'; -async function setup(): Promise> { - return fixture(html``); -} - describe('Anchor', () => { - let element: Anchor; - let connect: () => Promise; - let disconnect: () => Promise; - - beforeEach(async () => { - ({ element, connect, disconnect } = await setup()); - }); - - afterEach(async () => { - await disconnect(); - }); - it('should export its tag', () => { expect(anchorTag).toBe('nimble-anchor'); }); @@ -29,66 +13,144 @@ describe('Anchor', () => { expect(document.createElement('nimble-anchor')).toBeInstanceOf(Anchor); }); - it('should set the "control" class on the internal control', async () => { - await connect(); - expect(element.control!.classList.contains('control')).toBe(true); - }); + describe('element only', () => { + async function setup(): Promise> { + return fixture(html``); + } + let element: Anchor; + let connect: () => Promise; + let disconnect: () => Promise; - it('should set the `part` attribute to "control" on the internal control', async () => { - await connect(); - expect(element.control!.part.contains('control')).toBe(true); - }); + beforeEach(async () => { + ({ element, connect, disconnect } = await setup()); + }); - const attributeNames: { name: string }[] = [ - { name: 'download' }, - { name: 'href' }, - { name: 'hreflang' }, - { name: 'ping' }, - { name: 'referrerpolicy' }, - { name: 'rel' }, - { name: 'target' }, - { name: 'type' }, - { name: 'aria-atomic' }, - { name: 'aria-busy' }, - { name: 'aria-controls' }, - { name: 'aria-current' }, - { name: 'aria-describedby' }, - { name: 'aria-details' }, - { name: 'aria-disabled' }, - { name: 'aria-errormessage' }, - { name: 'aria-expanded' }, - { name: 'aria-flowto' }, - { name: 'aria-haspopup' }, - { name: 'aria-hidden' }, - { name: 'aria-invalid' }, - { name: 'aria-keyshortcuts' }, - { name: 'aria-label' }, - { name: 'aria-labelledby' }, - { name: 'aria-live' }, - { name: 'aria-owns' }, - { name: 'aria-relevant' }, - { name: 'aria-roledescription' } - ]; - describe('should reflect value to the internal control', () => { - const focused: string[] = []; - const disabled: string[] = []; - for (const attribute of attributeNames) { - const specType = getSpecTypeByNamedList( - attribute, - focused, - disabled - ); - // eslint-disable-next-line @typescript-eslint/no-loop-func - specType(`for attribute ${attribute.name}`, async () => { - await connect(); + afterEach(async () => { + await disconnect(); + }); + + it('should set the "control" class on the internal control', async () => { + await connect(); + expect(element.control!.classList.contains('control')).toBe(true); + }); - element.setAttribute(attribute.name, 'foo'); - await waitForUpdatesAsync(); + it('should set the `part` attribute to "control" on the internal control', async () => { + await connect(); + expect(element.control!.part.contains('control')).toBe(true); + }); - expect(element.control!.getAttribute(attribute.name)).toBe( - 'foo' + const attributeNames: { name: string }[] = [ + { name: 'download' }, + { name: 'href' }, + { name: 'hreflang' }, + { name: 'ping' }, + { name: 'referrerpolicy' }, + { name: 'rel' }, + { name: 'target' }, + { name: 'type' }, + { name: 'aria-atomic' }, + { name: 'aria-busy' }, + { name: 'aria-controls' }, + { name: 'aria-current' }, + { name: 'aria-describedby' }, + { name: 'aria-details' }, + { name: 'aria-disabled' }, + { name: 'aria-errormessage' }, + { name: 'aria-expanded' }, + { name: 'aria-flowto' }, + { name: 'aria-haspopup' }, + { name: 'aria-hidden' }, + { name: 'aria-invalid' }, + { name: 'aria-keyshortcuts' }, + { name: 'aria-label' }, + { name: 'aria-labelledby' }, + { name: 'aria-live' }, + { name: 'aria-owns' }, + { name: 'aria-relevant' }, + { name: 'aria-roledescription' } + ]; + describe('should reflect value to the internal control', () => { + const focused: string[] = []; + const disabled: string[] = []; + for (const attribute of attributeNames) { + const specType = getSpecTypeByNamedList( + attribute, + focused, + disabled ); - }); + // eslint-disable-next-line @typescript-eslint/no-loop-func + specType(`for attribute ${attribute.name}`, async () => { + await connect(); + + element.setAttribute(attribute.name, 'foo'); + await waitForUpdatesAsync(); + + expect(element.control!.getAttribute(attribute.name)).toBe( + 'foo' + ); + }); + } + }); + }); + + describe('inner anchor isContentEditable', () => { + async function setup(): Promise> { + return fixture( + html`
` + ); } + let element: HTMLDivElement; + let anchor: Anchor; + let connect: () => Promise; + let disconnect: () => Promise; + + beforeEach(async () => { + ({ element, connect, disconnect } = await setup()); + anchor = element.firstElementChild as Anchor; + }); + + afterEach(async () => { + await disconnect(); + }); + + it('is false by default', async () => { + await connect(); + const innerAnchor = anchor.shadowRoot!.querySelector('a')!; + expect(innerAnchor.isContentEditable).toBeFalse(); + }); + + it('is true when container has contenteditable before connecting', async () => { + element.setAttribute('contenteditable', ''); + await connect(); + const innerAnchor = anchor.shadowRoot!.querySelector('a')!; + expect(innerAnchor.isContentEditable).toBeTrue(); + }); + + it('is true when container gets contenteditable after connecting, then anchor gets mouseenter event', async () => { + await connect(); + element.setAttribute('contenteditable', ''); + await waitForUpdatesAsync(); + const innerAnchor = anchor.shadowRoot!.querySelector('a')!; + innerAnchor.dispatchEvent(new MouseEvent('mouseenter')); + expect(innerAnchor.isContentEditable).toBeTrue(); + }); + + it('is true when container gets contenteditable after connecting, then anchor gets focus event', async () => { + await connect(); + element.setAttribute('contenteditable', ''); + await waitForUpdatesAsync(); + const innerAnchor = anchor.shadowRoot!.querySelector('a')!; + innerAnchor.dispatchEvent(new Event('focus')); + expect(innerAnchor.isContentEditable).toBeTrue(); + }); + + it('is false when container loses contenteditable, then anchor gets mouseenter event', async () => { + element.setAttribute('contenteditable', ''); + await connect(); + element.removeAttribute('contenteditable'); + const innerAnchor = anchor.shadowRoot!.querySelector('a')!; + innerAnchor.dispatchEvent(new MouseEvent('mouseenter')); + expect(innerAnchor.isContentEditable).toBeFalse(); + }); }); }); From 7d01ff4caef921b678e272aaa848af9954de937f Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Wed, 22 Nov 2023 12:08:06 -0600 Subject: [PATCH 02/13] Change files --- ...le-components-be72d648-96c1-403d-80e8-2a68c3752207.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@ni-nimble-components-be72d648-96c1-403d-80e8-2a68c3752207.json diff --git a/change/@ni-nimble-components-be72d648-96c1-403d-80e8-2a68c3752207.json b/change/@ni-nimble-components-be72d648-96c1-403d-80e8-2a68c3752207.json new file mode 100644 index 0000000000..dd7415a680 --- /dev/null +++ b/change/@ni-nimble-components-be72d648-96c1-403d-80e8-2a68c3752207.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Make anchor behave like native anchor when contenteditable", + "packageName": "@ni/nimble-components", + "email": "7282195+m-akinc@users.noreply.github.com", + "dependentChangeType": "patch" +} From 778a2c6b9a6352f27ceaf5af2ecf6d033d2f8a0d Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Tue, 28 Nov 2023 17:43:48 -0600 Subject: [PATCH 03/13] Move non-null assertion --- packages/nimble-components/src/anchor/index.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index a9e15c46d5..68feaec713 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -38,7 +38,7 @@ export class Anchor extends AnchorBase { /** * @internal */ - public container?: HTMLElement; + public container!: HTMLElement; public override connectedCallback(): void { super.connectedCallback(); @@ -58,9 +58,9 @@ export class Anchor extends AnchorBase { */ public updateContentEditable(): void { if (this.isContentEditable) { - this.container!.setAttribute('contenteditable', ''); + this.container.setAttribute('contenteditable', ''); } else { - this.container!.removeAttribute('contenteditable'); + this.container.removeAttribute('contenteditable'); } } } From 866d97bd3912f30eb0758a7f180e672d67e61a41 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Tue, 28 Nov 2023 18:06:55 -0600 Subject: [PATCH 04/13] Added link to FAST issue --- packages/nimble-components/src/anchor/index.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index 68feaec713..ed1b53c390 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -55,6 +55,10 @@ export class Anchor extends AnchorBase { * value has changed, so the best we can do is to re-sync on specific events. * This has shortcomings, e.g. if isContentEditable goes from true to false, our * anchor will remain un-tabable until a mouseenter triggers a re-sync. + * + * Ideally, proper support for contenteditable should come from FAST. + * I have filed bug https://github.com/microsoft/fast/issues/6870 to them. + * If/when it is fixed, we can remove our logic. */ public updateContentEditable(): void { if (this.isContentEditable) { From ab9b7dd8a786d6fe55fe10f6b2734bfa71828056 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 30 Nov 2023 12:34:33 -0600 Subject: [PATCH 05/13] Handle contenteditable via explicit attribute --- .../anchor/nimble-anchor.directive.ts | 10 + .../tests/nimble-anchor.directive.spec.ts | 37 ++++ .../Components/NimbleAnchor.razor | 1 + .../Components/NimbleAnchor.razor.cs | 6 + .../Unit/Components/NimbleAnchorTests.cs | 16 ++ .../nimble-components/src/anchor/index.ts | 33 +-- .../nimble-components/src/anchor/template.ts | 4 +- .../src/anchor/tests/anchor.spec.ts | 197 +++++++----------- .../src/anchor/tests/anchor.stories.ts | 7 + 9 files changed, 161 insertions(+), 150 deletions(-) diff --git a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts index 12d4e02762..1b9cba43c9 100644 --- a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts +++ b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts @@ -33,6 +33,16 @@ export class NimbleAnchorDirective extends NimbleAnchorBaseDirective { this.renderer.setProperty(this.elementRef.nativeElement, 'underlineHidden', toBooleanProperty(value)); } + public get contentEditable(): boolean { + return this.elementRef.nativeElement.contenteditable; + } + + // Renaming because property should have camel casing, but attribute should not + // eslint-disable-next-line @angular-eslint/no-input-rename + @Input('contenteditable') public set contentEditable(value: BooleanValueOrAttribute) { + this.renderer.setProperty(this.elementRef.nativeElement, 'contenteditable', toBooleanProperty(value)); + } + public constructor(renderer: Renderer2, elementRef: ElementRef) { super(renderer, elementRef); } diff --git a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts index 1cede0e2f6..3ebda415ff 100644 --- a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts +++ b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts @@ -101,6 +101,11 @@ describe('Nimble anchor', () => { expect(directive.underlineHidden).toBeFalse(); expect(nativeElement.underlineHidden).toBeFalse(); }); + + it('has expected defaults for contentEditable', () => { + expect(directive.contentEditable).toBeFalse(); + expect(nativeElement.contenteditable).toBeFalse(); + }); }); describe('with template string values', () => { @@ -116,6 +121,7 @@ describe('Nimble anchor', () => { type="${type1}" appearance="prominent" underline-hidden + contenteditable > ` @@ -184,6 +190,11 @@ describe('Nimble anchor', () => { expect(directive.underlineHidden).toBeTrue(); expect(nativeElement.underlineHidden).toBeTrue(); }); + + it('will use template string values for contentEditable', () => { + expect(directive.contentEditable).toBeTrue(); + expect(nativeElement.contenteditable).toBeTrue(); + }); }); describe('with property bound values', () => { @@ -199,6 +210,7 @@ describe('Nimble anchor', () => { [type]="type" [appearance]="appearance" [underlineHidden]="underlineHidden" + [contentEditable]="contentEditable" > ` @@ -215,6 +227,7 @@ describe('Nimble anchor', () => { public type = type1; public appearance: AnchorAppearance = appearance1; public underlineHidden = true; + public contentEditable = true; } let fixture: ComponentFixture; @@ -330,6 +343,17 @@ describe('Nimble anchor', () => { expect(directive.underlineHidden).toBeFalse(); expect(nativeElement.underlineHidden).toBeFalse(); }); + + it('can be configured with property binding for contentEditable', () => { + expect(directive.contentEditable).toBeTrue(); + expect(nativeElement.contenteditable).toBeTrue(); + + fixture.componentInstance.contentEditable = false; + fixture.detectChanges(); + + expect(directive.contentEditable).toBeFalse(); + expect(nativeElement.contenteditable).toBeFalse(); + }); }); describe('with attribute bound values', () => { @@ -345,6 +369,7 @@ describe('Nimble anchor', () => { [attr.type]="type" [attr.appearance]="appearance" [attr.underline-hidden]="underlineHidden" + [attr.contenteditable]="contentEditable" > ` @@ -361,6 +386,7 @@ describe('Nimble anchor', () => { public type = type1; public appearance: AnchorAppearance = appearance1; public underlineHidden = true; + public contentEditable = true; } let fixture: ComponentFixture; @@ -476,5 +502,16 @@ describe('Nimble anchor', () => { expect(directive.underlineHidden).toBeFalse(); expect(nativeElement.underlineHidden).toBeFalse(); }); + + it('can be configured with attribute binding for contentEditable', () => { + expect(directive.contentEditable).toBeTrue(); + expect(nativeElement.contenteditable).toBeTrue(); + + fixture.componentInstance.contentEditable = false; + fixture.detectChanges(); + + expect(directive.contentEditable).toBeFalse(); + expect(nativeElement.contenteditable).toBeFalse(); + }); }); }); diff --git a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor index 6a426d7260..2a942cdfb4 100644 --- a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor +++ b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor @@ -9,6 +9,7 @@ target="@Target" type="@Type" underline-hidden="@UnderlineHidden" + contenteditable="@ContentEditable" appearance="@Appearance.ToAttributeValue()" @attributes="AdditionalAttributes"> @ChildContent diff --git a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs index 9d5d59fe6d..8faf679f1b 100644 --- a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs +++ b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs @@ -10,6 +10,12 @@ public partial class NimbleAnchor : NimbleAnchorBase [Parameter] public bool? UnderlineHidden { get; set; } + /// + /// Whether the anchor should behave like it is in an editable region. + /// + [Parameter] + public bool? ContentEditable { get; set; } + /// /// The appearance of the anchor. /// diff --git a/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs b/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs index 7fa9e48005..a350f36c29 100644 --- a/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs +++ b/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs @@ -41,6 +41,22 @@ public void AnchorAppearance_AttributeIsSet(AnchorAppearance value, string expec Assert.Contains(expectedMarkup, anchor.Markup); } + [Fact] + public void AnchorContentEditable_AttributeIsSet() + { + var anchor = RenderWithPropertySet(x => x.ContentEditable, true); + + Assert.Contains("contenteditable", anchor.Markup); + } + + [Fact] + public void AnchorUnderlineHidden_AttributeIsSet() + { + var anchor = RenderWithPropertySet(x => x.UnderlineHidden, true); + + Assert.Contains("underline-hidden", anchor.Markup); + } + private IRenderedComponent RenderWithPropertySet(Expression> propertyGetter, TProperty propertyValue) { var context = new TestContext(); diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index ed1b53c390..3a98ae1fc2 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -36,37 +36,16 @@ export class Anchor extends AnchorBase { public appearance: AnchorAppearance; /** - * @internal - */ - public container!: HTMLElement; - - public override connectedCallback(): void { - super.connectedCallback(); - this.updateContentEditable(); - } - - /** - * @internal - * We want our anchor to behave like a native anchor when in a content-editable - * region, i.e. not operable or focusable. The most reliable way to achieve this is - * to synchronize our shadow DOM's content-editable state with that of the host. - * We must look at the host's read-only isContentEditable property, which factors - * in any inherited value. Unfortunately, there is no way for us to detect when its - * value has changed, so the best we can do is to re-sync on specific events. - * This has shortcomings, e.g. if isContentEditable goes from true to false, our - * anchor will remain un-tabable until a mouseenter triggers a re-sync. + * @public + * @remarks + * HTML Attribute: contenteditable * * Ideally, proper support for contenteditable should come from FAST. * I have filed bug https://github.com/microsoft/fast/issues/6870 to them. - * If/when it is fixed, we can remove our logic. + * If/when it is fixed, we can remove this workaround. */ - public updateContentEditable(): void { - if (this.isContentEditable) { - this.container.setAttribute('contenteditable', ''); - } else { - this.container.removeAttribute('contenteditable'); - } - } + @attr({ mode: 'boolean' }) + public contenteditable = false; } // FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here. diff --git a/packages/nimble-components/src/anchor/template.ts b/packages/nimble-components/src/anchor/template.ts index ff2ecc3b51..698e3aee59 100644 --- a/packages/nimble-components/src/anchor/template.ts +++ b/packages/nimble-components/src/anchor/template.ts @@ -10,8 +10,8 @@ export const template: FoundationElementTemplate< ViewTemplate, AnchorOptions > = (_context, definition) => html`
${ /* Start and End slot templates inlined to avoid extra whitespace. diff --git a/packages/nimble-components/src/anchor/tests/anchor.spec.ts b/packages/nimble-components/src/anchor/tests/anchor.spec.ts index 4597c93169..c18432bb60 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.spec.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.spec.ts @@ -4,7 +4,23 @@ import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { fixture, Fixture } from '../../utilities/tests/fixture'; import { getSpecTypeByNamedList } from '../../utilities/tests/parameterized'; +async function setup(): Promise> { + return fixture(html``); +} + describe('Anchor', () => { + let element: Anchor; + let connect: () => Promise; + let disconnect: () => Promise; + + beforeEach(async () => { + ({ element, connect, disconnect } = await setup()); + }); + + afterEach(async () => { + await disconnect(); + }); + it('should export its tag', () => { expect(anchorTag).toBe('nimble-anchor'); }); @@ -13,144 +29,85 @@ describe('Anchor', () => { expect(document.createElement('nimble-anchor')).toBeInstanceOf(Anchor); }); - describe('element only', () => { - async function setup(): Promise> { - return fixture(html``); - } - let element: Anchor; - let connect: () => Promise; - let disconnect: () => Promise; - - beforeEach(async () => { - ({ element, connect, disconnect } = await setup()); - }); + it('should set the "control" class on the internal control', async () => { + await connect(); + expect(element.control!.classList.contains('control')).toBe(true); + }); - afterEach(async () => { - await disconnect(); - }); + it('should set the `part` attribute to "control" on the internal control', async () => { + await connect(); + expect(element.control!.part.contains('control')).toBe(true); + }); - it('should set the "control" class on the internal control', async () => { - await connect(); - expect(element.control!.classList.contains('control')).toBe(true); - }); + const attributeNames: { name: string }[] = [ + { name: 'download' }, + { name: 'href' }, + { name: 'hreflang' }, + { name: 'ping' }, + { name: 'referrerpolicy' }, + { name: 'rel' }, + { name: 'target' }, + { name: 'type' }, + { name: 'aria-atomic' }, + { name: 'aria-busy' }, + { name: 'aria-controls' }, + { name: 'aria-current' }, + { name: 'aria-describedby' }, + { name: 'aria-details' }, + { name: 'aria-disabled' }, + { name: 'aria-errormessage' }, + { name: 'aria-expanded' }, + { name: 'aria-flowto' }, + { name: 'aria-haspopup' }, + { name: 'aria-hidden' }, + { name: 'aria-invalid' }, + { name: 'aria-keyshortcuts' }, + { name: 'aria-label' }, + { name: 'aria-labelledby' }, + { name: 'aria-live' }, + { name: 'aria-owns' }, + { name: 'aria-relevant' }, + { name: 'aria-roledescription' } + ]; + describe('should reflect value to the internal control', () => { + const focused: string[] = []; + const disabled: string[] = []; + for (const attribute of attributeNames) { + const specType = getSpecTypeByNamedList( + attribute, + focused, + disabled + ); + // eslint-disable-next-line @typescript-eslint/no-loop-func + specType(`for attribute ${attribute.name}`, async () => { + await connect(); - it('should set the `part` attribute to "control" on the internal control', async () => { - await connect(); - expect(element.control!.part.contains('control')).toBe(true); - }); + element.setAttribute(attribute.name, 'foo'); + await waitForUpdatesAsync(); - const attributeNames: { name: string }[] = [ - { name: 'download' }, - { name: 'href' }, - { name: 'hreflang' }, - { name: 'ping' }, - { name: 'referrerpolicy' }, - { name: 'rel' }, - { name: 'target' }, - { name: 'type' }, - { name: 'aria-atomic' }, - { name: 'aria-busy' }, - { name: 'aria-controls' }, - { name: 'aria-current' }, - { name: 'aria-describedby' }, - { name: 'aria-details' }, - { name: 'aria-disabled' }, - { name: 'aria-errormessage' }, - { name: 'aria-expanded' }, - { name: 'aria-flowto' }, - { name: 'aria-haspopup' }, - { name: 'aria-hidden' }, - { name: 'aria-invalid' }, - { name: 'aria-keyshortcuts' }, - { name: 'aria-label' }, - { name: 'aria-labelledby' }, - { name: 'aria-live' }, - { name: 'aria-owns' }, - { name: 'aria-relevant' }, - { name: 'aria-roledescription' } - ]; - describe('should reflect value to the internal control', () => { - const focused: string[] = []; - const disabled: string[] = []; - for (const attribute of attributeNames) { - const specType = getSpecTypeByNamedList( - attribute, - focused, - disabled + expect(element.control!.getAttribute(attribute.name)).toBe( + 'foo' ); - // eslint-disable-next-line @typescript-eslint/no-loop-func - specType(`for attribute ${attribute.name}`, async () => { - await connect(); - - element.setAttribute(attribute.name, 'foo'); - await waitForUpdatesAsync(); - - expect(element.control!.getAttribute(attribute.name)).toBe( - 'foo' - ); - }); - } - }); + }); + } }); describe('inner anchor isContentEditable', () => { - async function setup(): Promise> { - return fixture( - html`
` - ); - } - let element: HTMLDivElement; - let anchor: Anchor; - let connect: () => Promise; - let disconnect: () => Promise; + let innerAnchor: HTMLAnchorElement; beforeEach(async () => { - ({ element, connect, disconnect } = await setup()); - anchor = element.firstElementChild as Anchor; - }); - - afterEach(async () => { - await disconnect(); - }); - - it('is false by default', async () => { await connect(); - const innerAnchor = anchor.shadowRoot!.querySelector('a')!; - expect(innerAnchor.isContentEditable).toBeFalse(); - }); - - it('is true when container has contenteditable before connecting', async () => { - element.setAttribute('contenteditable', ''); - await connect(); - const innerAnchor = anchor.shadowRoot!.querySelector('a')!; - expect(innerAnchor.isContentEditable).toBeTrue(); + innerAnchor = element.shadowRoot!.querySelector('a')!; }); - it('is true when container gets contenteditable after connecting, then anchor gets mouseenter event', async () => { - await connect(); - element.setAttribute('contenteditable', ''); - await waitForUpdatesAsync(); - const innerAnchor = anchor.shadowRoot!.querySelector('a')!; - innerAnchor.dispatchEvent(new MouseEvent('mouseenter')); - expect(innerAnchor.isContentEditable).toBeTrue(); + it('is false by default', () => { + expect(innerAnchor.isContentEditable).toBeFalse(); }); - it('is true when container gets contenteditable after connecting, then anchor gets focus event', async () => { - await connect(); + it('is true when contenteditable set on host element', async () => { element.setAttribute('contenteditable', ''); await waitForUpdatesAsync(); - const innerAnchor = anchor.shadowRoot!.querySelector('a')!; - innerAnchor.dispatchEvent(new Event('focus')); expect(innerAnchor.isContentEditable).toBeTrue(); }); - - it('is false when container loses contenteditable, then anchor gets mouseenter event', async () => { - element.setAttribute('contenteditable', ''); - await connect(); - element.removeAttribute('contenteditable'); - const innerAnchor = anchor.shadowRoot!.querySelector('a')!; - innerAnchor.dispatchEvent(new MouseEvent('mouseenter')); - expect(innerAnchor.isContentEditable).toBeFalse(); - }); }); }); diff --git a/packages/nimble-components/src/anchor/tests/anchor.stories.ts b/packages/nimble-components/src/anchor/tests/anchor.stories.ts index a1dea3bfc2..acd979812d 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.stories.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.stories.ts @@ -15,6 +15,7 @@ interface AnchorArgs { label: string; href: string; underlineHidden: boolean; + contenteditable: boolean; appearance: keyof typeof AnchorAppearance; } @@ -39,6 +40,7 @@ const metadata: Meta = { Click on the <${anchorTag} href=${x => (x.href !== '' ? x.href : null)} ?underline-hidden=${x => x.underlineHidden} + ?contenteditable=${x => x.contenteditable} appearance=${x => x.appearance} >${x => x.label} to navigate. `), @@ -56,12 +58,17 @@ const metadata: Meta = { control: { type: 'radio' }, description: 'Set to `prominent` to make the anchor appear in a different color than normal text.' + }, + contenteditable: { + description: + 'Set this when the anchor is within an editable region (i.e. element/hierarchy with [contenteditable](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable)). Whereas native elements can automatically adapt their behavior when within a `contenteditable` element, the `nimble-anchor` requires this attribute be explicitly set. When set, the anchor cannot be focused or operated.' } }, args: { label: 'link', href: 'https://nimble.ni.dev', underlineHidden: false, + contenteditable: false, appearance: 'default' } }; From 10e5dcef35164d45680593676f394cc292d26ecd Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 30 Nov 2023 12:36:09 -0600 Subject: [PATCH 06/13] Change files --- ...imble-angular-af4cc1a5-1c53-4eac-8cba-b38dcd8fe376.json | 7 +++++++ ...nimble-blazor-21d307d5-1145-4e6c-8be4-dd65b8ca424c.json | 7 +++++++ 2 files changed, 14 insertions(+) create mode 100644 change/@ni-nimble-angular-af4cc1a5-1c53-4eac-8cba-b38dcd8fe376.json create mode 100644 change/@ni-nimble-blazor-21d307d5-1145-4e6c-8be4-dd65b8ca424c.json diff --git a/change/@ni-nimble-angular-af4cc1a5-1c53-4eac-8cba-b38dcd8fe376.json b/change/@ni-nimble-angular-af4cc1a5-1c53-4eac-8cba-b38dcd8fe376.json new file mode 100644 index 0000000000..58e26763f8 --- /dev/null +++ b/change/@ni-nimble-angular-af4cc1a5-1c53-4eac-8cba-b38dcd8fe376.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Add contentEditable property to anchor directive", + "packageName": "@ni/nimble-angular", + "email": "7282195+m-akinc@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/change/@ni-nimble-blazor-21d307d5-1145-4e6c-8be4-dd65b8ca424c.json b/change/@ni-nimble-blazor-21d307d5-1145-4e6c-8be4-dd65b8ca424c.json new file mode 100644 index 0000000000..03bceea747 --- /dev/null +++ b/change/@ni-nimble-blazor-21d307d5-1145-4e6c-8be4-dd65b8ca424c.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Add ContentEditable property to anchor", + "packageName": "@ni/nimble-blazor", + "email": "7282195+m-akinc@users.noreply.github.com", + "dependentChangeType": "patch" +} From fe7827b27e4cd4a378575c048777ebe230e1b64b Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 12:08:19 -0600 Subject: [PATCH 07/13] Change contenteditable to match built-in attribute/property --- .../anchor/nimble-anchor.directive.ts | 8 +-- .../tests/nimble-anchor.directive.spec.ts | 34 +++++----- .../nimble-components/src/anchor/index.ts | 5 +- .../nimble-components/src/anchor/template.ts | 2 +- .../src/anchor/tests/anchor.spec.ts | 65 +++++++++++++++++-- .../src/anchor/tests/anchor.stories.ts | 10 +-- 6 files changed, 90 insertions(+), 34 deletions(-) diff --git a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts index 1b9cba43c9..7e3681ed95 100644 --- a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts +++ b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/nimble-anchor.directive.ts @@ -33,14 +33,14 @@ export class NimbleAnchorDirective extends NimbleAnchorBaseDirective { this.renderer.setProperty(this.elementRef.nativeElement, 'underlineHidden', toBooleanProperty(value)); } - public get contentEditable(): boolean { - return this.elementRef.nativeElement.contenteditable; + public get contentEditable(): string { + return this.elementRef.nativeElement.contentEditable; } // Renaming because property should have camel casing, but attribute should not // eslint-disable-next-line @angular-eslint/no-input-rename - @Input('contenteditable') public set contentEditable(value: BooleanValueOrAttribute) { - this.renderer.setProperty(this.elementRef.nativeElement, 'contenteditable', toBooleanProperty(value)); + @Input('contenteditable') public set contentEditable(value: string) { + this.renderer.setProperty(this.elementRef.nativeElement, 'contentEditable', value); } public constructor(renderer: Renderer2, elementRef: ElementRef) { diff --git a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts index 3ebda415ff..5b6d434034 100644 --- a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts +++ b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts @@ -103,8 +103,8 @@ describe('Nimble anchor', () => { }); it('has expected defaults for contentEditable', () => { - expect(directive.contentEditable).toBeFalse(); - expect(nativeElement.contenteditable).toBeFalse(); + expect(directive.contentEditable).toEqual('inherit'); + expect(nativeElement.contentEditable).toEqual('inherit'); }); }); @@ -121,7 +121,7 @@ describe('Nimble anchor', () => { type="${type1}" appearance="prominent" underline-hidden - contenteditable + contenteditable="true" > ` @@ -192,8 +192,8 @@ describe('Nimble anchor', () => { }); it('will use template string values for contentEditable', () => { - expect(directive.contentEditable).toBeTrue(); - expect(nativeElement.contenteditable).toBeTrue(); + expect(directive.contentEditable).toEqual('true'); + expect(nativeElement.contentEditable).toEqual('true'); }); }); @@ -227,7 +227,7 @@ describe('Nimble anchor', () => { public type = type1; public appearance: AnchorAppearance = appearance1; public underlineHidden = true; - public contentEditable = true; + public contentEditable = 'true'; } let fixture: ComponentFixture; @@ -345,14 +345,14 @@ describe('Nimble anchor', () => { }); it('can be configured with property binding for contentEditable', () => { - expect(directive.contentEditable).toBeTrue(); - expect(nativeElement.contenteditable).toBeTrue(); + expect(directive.contentEditable).toEqual('true'); + expect(nativeElement.contentEditable).toEqual('true'); - fixture.componentInstance.contentEditable = false; + fixture.componentInstance.contentEditable = 'false'; fixture.detectChanges(); - expect(directive.contentEditable).toBeFalse(); - expect(nativeElement.contenteditable).toBeFalse(); + expect(directive.contentEditable).toEqual('false'); + expect(nativeElement.contentEditable).toEqual('false'); }); }); @@ -386,7 +386,7 @@ describe('Nimble anchor', () => { public type = type1; public appearance: AnchorAppearance = appearance1; public underlineHidden = true; - public contentEditable = true; + public contentEditable = 'true'; } let fixture: ComponentFixture; @@ -504,14 +504,14 @@ describe('Nimble anchor', () => { }); it('can be configured with attribute binding for contentEditable', () => { - expect(directive.contentEditable).toBeTrue(); - expect(nativeElement.contenteditable).toBeTrue(); + expect(directive.contentEditable).toEqual('true'); + expect(nativeElement.contentEditable).toEqual('true'); - fixture.componentInstance.contentEditable = false; + fixture.componentInstance.contentEditable = 'false'; fixture.detectChanges(); - expect(directive.contentEditable).toBeFalse(); - expect(nativeElement.contenteditable).toBeFalse(); + expect(directive.contentEditable).toEqual('false'); + expect(nativeElement.contentEditable).toEqual('false'); }); }); }); diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index 3a98ae1fc2..fb6fa1061c 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -39,13 +39,14 @@ export class Anchor extends AnchorBase { * @public * @remarks * HTML Attribute: contenteditable + * See https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable * * Ideally, proper support for contenteditable should come from FAST. * I have filed bug https://github.com/microsoft/fast/issues/6870 to them. * If/when it is fixed, we can remove this workaround. */ - @attr({ mode: 'boolean' }) - public contenteditable = false; + @attr({ attribute: 'contenteditable' }) + public override contentEditable = 'inherit'; } // FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here. diff --git a/packages/nimble-components/src/anchor/template.ts b/packages/nimble-components/src/anchor/template.ts index 698e3aee59..3b13c56e69 100644 --- a/packages/nimble-components/src/anchor/template.ts +++ b/packages/nimble-components/src/anchor/template.ts @@ -11,7 +11,7 @@ ViewTemplate, AnchorOptions > = (_context, definition) => html`
> { return fixture(html``); @@ -92,7 +95,7 @@ describe('Anchor', () => { } }); - describe('inner anchor isContentEditable', () => { + describe('contenteditable behavior', () => { let innerAnchor: HTMLAnchorElement; beforeEach(async () => { @@ -100,13 +103,63 @@ describe('Anchor', () => { innerAnchor = element.shadowRoot!.querySelector('a')!; }); - it('is false by default', () => { + it('has property value "inherit" and inner anchor isContentEditable is false by default', () => { + expect(element.contentEditable).toEqual('inherit'); expect(innerAnchor.isContentEditable).toBeFalse(); }); - it('is true when contenteditable set on host element', async () => { - element.setAttribute('contenteditable', ''); - await waitForUpdatesAsync(); + it('has property value "inherit" by default before connecting', () => { + const anchor = document.createElement(anchorTag); + expect(anchor.contentEditable).toEqual('inherit'); + }); + + const interestingValues = [ + { name: '', expected: true }, + { name: 'true', expected: true }, + { name: 'false', expected: false }, + { name: 'plaintext-only', expected: true }, + { name: 'inherit', expected: false }, + { name: 'badvalue', expected: false } + ] as const; + + parameterizeNamedList(interestingValues, (spec, name, value) => { + spec( + `inner anchor isContentEditable is ${value.expected.toString()} when attribute set to "${name}"`, + async () => { + element.setAttribute('contenteditable', name); + await waitForUpdatesAsync(); + expect(innerAnchor.isContentEditable).toEqual( + value.expected + ); + } + ); + }); + + parameterizeNamedList(interestingValues, (spec, name, value) => { + spec( + `inner anchor isContentEditable is ${value.expected.toString()} when property set to "${name}"`, + async () => { + element.contentEditable = name; + await waitForUpdatesAsync(); + expect(innerAnchor.isContentEditable).toEqual( + value.expected + ); + } + ); + }); + }); + + describe('with contenteditable without value', () => { + async function setupWithContenteditable(): Promise> { + return fixture( + html`` + ); + } + + it('acts like value is "true"', async () => { + ({ element, connect, disconnect } = await setupWithContenteditable()); + await connect(); + const innerAnchor = element.shadowRoot!.querySelector('a')!; expect(innerAnchor.isContentEditable).toBeTrue(); }); }); diff --git a/packages/nimble-components/src/anchor/tests/anchor.stories.ts b/packages/nimble-components/src/anchor/tests/anchor.stories.ts index acd979812d..8834db5351 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.stories.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.stories.ts @@ -15,7 +15,7 @@ interface AnchorArgs { label: string; href: string; underlineHidden: boolean; - contenteditable: boolean; + contenteditable: string; appearance: keyof typeof AnchorAppearance; } @@ -40,7 +40,7 @@ const metadata: Meta = { Click on the <${anchorTag} href=${x => (x.href !== '' ? x.href : null)} ?underline-hidden=${x => x.underlineHidden} - ?contenteditable=${x => x.contenteditable} + contenteditable=${x => x.contenteditable} appearance=${x => x.appearance} >${x => x.label} to navigate. `), @@ -60,15 +60,17 @@ const metadata: Meta = { 'Set to `prominent` to make the anchor appear in a different color than normal text.' }, contenteditable: { + options: ['false', 'true'], + control: { type: 'radio' }, description: - 'Set this when the anchor is within an editable region (i.e. element/hierarchy with [contenteditable](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable)). Whereas native elements can automatically adapt their behavior when within a `contenteditable` element, the `nimble-anchor` requires this attribute be explicitly set. When set, the anchor cannot be focused or operated.' + 'Set this to the string "true" (or set the attribute without any value) when the anchor is within an editable region (i.e. element/hierarchy with [contenteditable](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable)). Whereas native elements inherit their `contenteditable` value by default, the `nimble-anchor` requires this attribute be explicitly set. When set, the anchor cannot be focused or operated.' } }, args: { label: 'link', href: 'https://nimble.ni.dev', underlineHidden: false, - contenteditable: false, + contenteditable: 'false', appearance: 'default' } }; From cd9443a49001ff84895352875a47e01b7af0c398 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 12:30:24 -0600 Subject: [PATCH 08/13] Skip plaintext-only test on Firefox --- .../src/anchor/tests/anchor.spec.ts | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/nimble-components/src/anchor/tests/anchor.spec.ts b/packages/nimble-components/src/anchor/tests/anchor.spec.ts index a0f57a0101..1aa1003244 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.spec.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.spec.ts @@ -95,7 +95,7 @@ describe('Anchor', () => { } }); - describe('contenteditable behavior', () => { + fdescribe('contenteditable behavior', () => { let innerAnchor: HTMLAnchorElement; beforeEach(async () => { @@ -114,17 +114,19 @@ describe('Anchor', () => { }); const interestingValues = [ - { name: '', expected: true }, - { name: 'true', expected: true }, - { name: 'false', expected: false }, - { name: 'plaintext-only', expected: true }, - { name: 'inherit', expected: false }, - { name: 'badvalue', expected: false } + { name: '', expected: true, skipTag: '' }, + { name: 'true', expected: true, skipTag: '' }, + { name: 'false', expected: false, skipTag: '' }, + { name: 'plaintext-only', expected: true, skipTag: '#SkipFirefox' }, + { name: 'inherit', expected: false, skipTag: '' }, + { name: 'badvalue', expected: false, skipTag: '' } ] as const; parameterizeNamedList(interestingValues, (spec, name, value) => { spec( - `inner anchor isContentEditable is ${value.expected.toString()} when attribute set to "${name}"`, + `inner anchor isContentEditable is ${value.expected.toString()} when attribute set to "${name}" ${ + value.skipTag + }`, async () => { element.setAttribute('contenteditable', name); await waitForUpdatesAsync(); @@ -137,7 +139,9 @@ describe('Anchor', () => { parameterizeNamedList(interestingValues, (spec, name, value) => { spec( - `inner anchor isContentEditable is ${value.expected.toString()} when property set to "${name}"`, + `inner anchor isContentEditable is ${value.expected.toString()} when property set to "${name}" ${ + value.skipTag + }`, async () => { element.contentEditable = name; await waitForUpdatesAsync(); From d4f96c881d4c21c40eaf395ce3f686e6d278f366 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 12:51:49 -0600 Subject: [PATCH 09/13] Remove fdescribe --- packages/nimble-components/src/anchor/tests/anchor.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nimble-components/src/anchor/tests/anchor.spec.ts b/packages/nimble-components/src/anchor/tests/anchor.spec.ts index 1aa1003244..3f11bef6b1 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.spec.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.spec.ts @@ -95,7 +95,7 @@ describe('Anchor', () => { } }); - fdescribe('contenteditable behavior', () => { + describe('contenteditable behavior', () => { let innerAnchor: HTMLAnchorElement; beforeEach(async () => { From 0a60564844cee5912637b6cf5ce27082939ef718 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 15:20:27 -0600 Subject: [PATCH 10/13] Leave contentEditable uninitialized --- .../anchor/tests/nimble-anchor.directive.spec.ts | 4 ++-- packages/nimble-components/src/anchor/index.ts | 2 +- .../nimble-components/src/anchor/tests/anchor.spec.ts | 9 ++------- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts index 5b6d434034..5b98299f43 100644 --- a/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts +++ b/angular-workspace/projects/ni/nimble-angular/src/directives/anchor/tests/nimble-anchor.directive.spec.ts @@ -103,8 +103,8 @@ describe('Nimble anchor', () => { }); it('has expected defaults for contentEditable', () => { - expect(directive.contentEditable).toEqual('inherit'); - expect(nativeElement.contentEditable).toEqual('inherit'); + expect(directive.contentEditable).toBeUndefined(); + expect(nativeElement.contentEditable).toBeUndefined(); }); }); diff --git a/packages/nimble-components/src/anchor/index.ts b/packages/nimble-components/src/anchor/index.ts index fb6fa1061c..82c05f85e5 100644 --- a/packages/nimble-components/src/anchor/index.ts +++ b/packages/nimble-components/src/anchor/index.ts @@ -46,7 +46,7 @@ export class Anchor extends AnchorBase { * If/when it is fixed, we can remove this workaround. */ @attr({ attribute: 'contenteditable' }) - public override contentEditable = 'inherit'; + public override contentEditable!: string; } // FoundationAnchor already applies the StartEnd mixin, so we don't need to do it here. diff --git a/packages/nimble-components/src/anchor/tests/anchor.spec.ts b/packages/nimble-components/src/anchor/tests/anchor.spec.ts index 3f11bef6b1..cac1096879 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.spec.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.spec.ts @@ -103,16 +103,11 @@ describe('Anchor', () => { innerAnchor = element.shadowRoot!.querySelector('a')!; }); - it('has property value "inherit" and inner anchor isContentEditable is false by default', () => { - expect(element.contentEditable).toEqual('inherit'); + it('has undefined property value and inner anchor isContentEditable is false by default', () => { + expect(element.contentEditable).toBeUndefined(); expect(innerAnchor.isContentEditable).toBeFalse(); }); - it('has property value "inherit" by default before connecting', () => { - const anchor = document.createElement(anchorTag); - expect(anchor.contentEditable).toEqual('inherit'); - }); - const interestingValues = [ { name: '', expected: true, skipTag: '' }, { name: 'true', expected: true, skipTag: '' }, From 60d3fd746e4da5a1dd56128e4754d2f985dbffca Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 17:24:59 -0600 Subject: [PATCH 11/13] Switch Blazor property to string --- .../NimbleBlazor/Components/NimbleAnchor.razor.cs | 2 +- .../NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs index 8faf679f1b..1c89bee5aa 100644 --- a/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs +++ b/packages/nimble-blazor/NimbleBlazor/Components/NimbleAnchor.razor.cs @@ -14,7 +14,7 @@ public partial class NimbleAnchor : NimbleAnchorBase /// Whether the anchor should behave like it is in an editable region. /// [Parameter] - public bool? ContentEditable { get; set; } + public string? ContentEditable { get; set; } /// /// The appearance of the anchor. diff --git a/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs b/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs index a350f36c29..76c04176fd 100644 --- a/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs +++ b/packages/nimble-blazor/Tests/NimbleBlazor.Tests/Unit/Components/NimbleAnchorTests.cs @@ -44,9 +44,9 @@ public void AnchorAppearance_AttributeIsSet(AnchorAppearance value, string expec [Fact] public void AnchorContentEditable_AttributeIsSet() { - var anchor = RenderWithPropertySet(x => x.ContentEditable, true); + var anchor = RenderWithPropertySet(x => x.ContentEditable, "true"); - Assert.Contains("contenteditable", anchor.Markup); + Assert.Contains("contenteditable=\"true\"", anchor.Markup); } [Fact] From b27093a20f153160fbec1d1f8c71c871a25d3db8 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 7 Dec 2023 17:51:16 -0600 Subject: [PATCH 12/13] Feedback --- packages/nimble-components/src/anchor/template.ts | 6 +++++- .../nimble-components/src/anchor/tests/anchor.stories.ts | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/nimble-components/src/anchor/template.ts b/packages/nimble-components/src/anchor/template.ts index 3b13c56e69..13cd273f22 100644 --- a/packages/nimble-components/src/anchor/template.ts +++ b/packages/nimble-components/src/anchor/template.ts @@ -9,7 +9,11 @@ import type { Anchor } from '.'; export const template: FoundationElementTemplate< ViewTemplate, AnchorOptions -> = (_context, definition) => html`
= (_context, definition) => html`${ + /* top-container div is necessary because setting contenteditable directly on the native anchor instead + leaves it focusable, unlike the behavior you get when the anchor is _within_ a contenteditable element. + */ '' +}
= { - Click on the <${anchorTag} + x.contenteditable}>Click on the <${anchorTag} href=${x => (x.href !== '' ? x.href : null)} ?underline-hidden=${x => x.underlineHidden} contenteditable=${x => x.contenteditable} From 740ce3587fb4185ece920c3dd5fd87a54b14ec9c Mon Sep 17 00:00:00 2001 From: m-akinc <7282195+m-akinc@users.noreply.github.com> Date: Mon, 11 Dec 2023 10:30:18 -0600 Subject: [PATCH 13/13] Update packages/nimble-components/src/anchor/tests/anchor.stories.ts Co-authored-by: Milan Raj --- packages/nimble-components/src/anchor/tests/anchor.stories.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nimble-components/src/anchor/tests/anchor.stories.ts b/packages/nimble-components/src/anchor/tests/anchor.stories.ts index a24758720e..fae5ba6b63 100644 --- a/packages/nimble-components/src/anchor/tests/anchor.stories.ts +++ b/packages/nimble-components/src/anchor/tests/anchor.stories.ts @@ -64,7 +64,7 @@ const metadata: Meta = { options: ['false', 'true'], control: { type: 'radio' }, description: - 'Set this to the string "true" (or set the attribute without any value) when the anchor is within an editable region (i.e. element/hierarchy with [contenteditable](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable)). Whereas native elements inherit their `contenteditable` value by default, the `nimble-anchor` requires this attribute be explicitly set. When set, the anchor cannot be focused or operated.' + 'Set this to the string "true" (or set the attribute without any value) when the anchor is within an editable region (i.e. element/hierarchy with [contenteditable](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable)). Whereas native elements inherit their `contenteditable` value by default, the `nimble-anchor` requires this attribute be explicitly set.' } }, args: {