feat(declarations): add popover attributes to JSX declarations #5064

tanner-reits · 2023-11-13T16:01:30Z

What is the current behavior?

Stencil's JSX declarations do not support popover or associated popover attributes

Closes: #4745

What is the new behavior?

Adds the popover attribute to the base HTMLAttributes
Adds popoverTargetAction, popoverTargetElement, and popoverTarget to the declarations for button and input

Does this introduce a breaking change?

Yes
No

Testing

Automated testing

Can't run automated tests because popover is not supported in all browsers we run e2e tests against via BrowserStack

Manual testing

Installed a build of this branch in a started project and was able to build a demo using popovers and triggers.

Other information

popover is not yet supported in all browsers. It will be up to the developer to decide when they can leverage these APIs

github-actions · 2023-11-13T16:03:08Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1368 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	23
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	22
src/compiler/prerender/prerender-main.ts	22
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	18
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	404
TS2322	384
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2416	6
TS2344	5
TS2493	3
TS2488	2
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

tanner-reits

Noting one thing

tanner-reits · 2023-11-13T16:48:13Z

src/declarations/stencil-public-runtime.ts

@@ -1364,6 +1374,7 @@ export namespace JSXBase {
    tabIndex?: number;
    tabindex?: number | string;
    title?: string;
+    popover?: 'auto' | 'manual';


So, this deviates a bit from the reproduction case in the linked issue and the HTML spec for the global popover attribute. According to the spec, the popover attribute will default to "auto" if you do something like this:

<div popover>...</div>

But, if we update our declaration here to be:

popover?: true | 'auto' | 'manual';

and write the same HTML, we actually end up getting the behavior for the "manual" value because it does not reflect the attribute correctly.

Not sure if this is something we've encountered before or can get around, but wanted to call it out since with this definition you'll be forced to explicitly set "auto" or "manual".

should we maybe leave a jsx comment here to that effect?

+1 for a comment here.

Looking at lib.dom.ts, it gets typed as:

popover: string | null;

I assume they use string here rather than 'auto' | 'manual' to:

allow for additional values to be added to the spec

adhere to this part of the spec: "The popover attribute's invalid value default is the manual state..."

In the case of null, I think that's to handle the latter part of the same sentence in the spec (emphasis mine):

The popover attribute's invalid value default is the manual state and its missing value default is the no popover state.

Thoughts on aligning this with lib.dom.ts' typings?

We can do that. Thoughts on typing it as 'auto' | 'manual' | string | null? I know that's different from the spec a bit, but offers a bit of intellisense for devs. But, I also get if we don't wanna do that in case they remove one of those values. Any preference?

I think that's fine - I doubt one of those will be removed, so the only thing I wanna watch out for is making sure string there doesn't widen "over" the auto | manual (basically ensure that the intellisense works for those value).

string did widen over the other types so I just ended up matching the lib.dom.ts types.

rwaskiewicz · 2023-11-14T13:31:08Z

src/declarations/stencil-public-runtime.ts

@@ -1364,6 +1374,7 @@ export namespace JSXBase {
    tabIndex?: number;
    tabindex?: number | string;
    title?: string;
+    popover?: 'auto' | 'manual';


+1 for a comment here.

Looking at lib.dom.ts, it gets typed as:

popover: string | null;

I assume they use string here rather than 'auto' | 'manual' to:

allow for additional values to be added to the spec

adhere to this part of the spec: "The popover attribute's invalid value default is the manual state..."

In the case of null, I think that's to handle the latter part of the same sentence in the spec (emphasis mine):

The popover attribute's invalid value default is the manual state and its missing value default is the no popover state.

Thoughts on aligning this with lib.dom.ts' typings?

feat(declarations): add popover attributes

3ece649

tanner-reits commented Nov 13, 2023

View reviewed changes

tanner-reits force-pushed the treits/feat/add-popover-support branch from febbe1e to 3ece649 Compare November 13, 2023 17:00

tanner-reits marked this pull request as ready for review November 13, 2023 17:37

tanner-reits requested a review from a team as a code owner November 13, 2023 17:37

tanner-reits requested review from rwaskiewicz and alicewriteswrongs November 13, 2023 17:37

rwaskiewicz suggested changes Nov 14, 2023

View reviewed changes

align types with lib.dom.ts types

68c7696

tanner-reits requested a review from rwaskiewicz November 15, 2023 15:58

add comment

c2a9dc6

rwaskiewicz approved these changes Nov 15, 2023

View reviewed changes

alicewriteswrongs approved these changes Nov 15, 2023

View reviewed changes

tanner-reits added this pull request to the merge queue Nov 15, 2023

Merged via the queue into main with commit f73aa14 Nov 15, 2023
120 checks passed

tanner-reits deleted the treits/feat/add-popover-support branch November 15, 2023 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(declarations): add popover attributes to JSX declarations #5064

feat(declarations): add popover attributes to JSX declarations #5064

tanner-reits commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

tanner-reits left a comment

tanner-reits Nov 13, 2023

alicewriteswrongs Nov 13, 2023

rwaskiewicz Nov 14, 2023

tanner-reits Nov 14, 2023

rwaskiewicz Nov 15, 2023

tanner-reits Nov 15, 2023

rwaskiewicz Nov 14, 2023

feat(declarations): add popover attributes to JSX declarations #5064

feat(declarations): add popover attributes to JSX declarations #5064

Conversation

tanner-reits commented Nov 13, 2023 • edited Loading

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Nov 13, 2023 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

tanner-reits left a comment

Choose a reason for hiding this comment

tanner-reits Nov 13, 2023

Choose a reason for hiding this comment

alicewriteswrongs Nov 13, 2023

Choose a reason for hiding this comment

rwaskiewicz Nov 14, 2023

Choose a reason for hiding this comment

tanner-reits Nov 14, 2023

Choose a reason for hiding this comment

rwaskiewicz Nov 15, 2023

Choose a reason for hiding this comment

tanner-reits Nov 15, 2023

Choose a reason for hiding this comment

rwaskiewicz Nov 14, 2023

Choose a reason for hiding this comment

tanner-reits commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

`--strictNullChecks` error report