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.

Sign up for GitHub

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

Jump to bottom

ARIAMixin properties should be nullable? #1119

Closed
h-joo opened this issue Aug 25, 2021 · 7 comments
Closed

ARIAMixin properties should be nullable? #1119

h-joo opened this issue Aug 25, 2021 · 7 comments

Comments

@h-joo
Copy link

h-joo commented Aug 25, 2021

In the ARIAMixin definition of TS 4.4-rc, the aria* properties are non-nullable, but according to the spec draft for AOM aria reflection with the AriaAttributes mixin, it seems to be nullable. Should these types need to be reconsidered to be nullable?
@orta
Copy link
Contributor

orta commented Aug 25, 2021 •
edited
Loading

Linking to microsoft/TypeScript#45571

It looks like this is defined upstream as not being nullable: https://github.com/w3c/webref/blob/master/ed/idlnames/ARIAMixin.idl - which does seem to match that spec https://www.w3.org/TR/wai-aria-1.2/#AriaAttributes

I think I agree that this is probably something we want to be classed as optional properties though, not quite decided. What does it look like to hit an error with this?

@h-joo
Copy link
Author

h-joo commented Aug 25, 2021 •
edited
Loading

We used to have Element-like interface implementation which used to have their own ariaLabel/ariaExpanded properties. The type of the two properties is string|undefined which now have problem on assignment to Element type as Element starts to extend from ARIAMixin as well with having the properties as a string type.

Sample error message:

TS2345: Argument of type 'X' is not assignable to parameter of type 'Element'.
  Types of property 'ariaExpanded' are incompatible.

@saschanaz
Copy link
Contributor

So, per the spec, assigning undefined is wrong and it will be converted to a string "undefined".

@aomarks aomarks mentioned this issue Sep 3, 2021
Closed
copybara-service bot pushed a commit to material-components/material-web that referenced this issue Sep 8, 2021
@dfreedm @copybara-github
(chore) Fix Typescript 4.4 compilation issues with aria properties
91c0bd2 
Typescript 4.4 defines aria properties on Element prototype to be non-nullable. This matches the spec language in https://www.w3.org/TR/wai-aria-1.2/#AriaAttributes, but Chrome and Safari as of when these properties were added to MWC allowed the property to be set with `null` to clear the attribute.

There should be some ongoing work to align the spec and browsers in behavior, but that is out of scope for this project, unblocking Typescript 4.4 is of higher priority.

Related microsoft/TypeScript-DOM-lib-generator#1119
Fixes #2676

PiperOrigin-RevId: 395526088
@copybara-service copybara-service bot mentioned this issue Sep 8, 2021
Merged
copybara-service bot pushed a commit to material-components/material-web that referenced this issue Sep 8, 2021
@dfreedm @copybara-github
(chore) Fix Typescript 4.4 compilation issues with aria properties
de3fab3 
Typescript 4.4 defines aria properties on Element prototype to be non-nullable. This matches the spec language in https://www.w3.org/TR/wai-aria-1.2/#AriaAttributes, but Chrome and Safari as of when these properties were added to MWC allowed the property to be set with `null` to clear the attribute.

There should be some ongoing work to align the spec and browsers in behavior, but that is out of scope for this project, unblocking Typescript 4.4 is of higher priority.

Related microsoft/TypeScript-DOM-lib-generator#1119
Fixes #2676

PiperOrigin-RevId: 395526088
copybara-service bot pushed a commit to material-components/material-web that referenced this issue Sep 8, 2021
@dfreedm @copybara-github
(chore) Fix Typescript 4.4 compilation issues with aria properties
17d51f7 
Typescript 4.4 defines aria properties on Element prototype to be non-nullable. This matches the spec language in https://www.w3.org/TR/wai-aria-1.2/#AriaAttributes, but Chrome and Safari as of when these properties were added to MWC allowed the property to be set with `null` to clear the attribute.

There should be some ongoing work to align the spec and browsers in behavior, but that is out of scope for this project, unblocking Typescript 4.4 is of higher priority.

Related microsoft/TypeScript-DOM-lib-generator#1119
Fixes #2676

PiperOrigin-RevId: 395581595
@radium-v radium-v mentioned this issue Sep 16, 2021
Closed
@jnurthen
Copy link

Note w3c/aria#1611 makes them nullable again.

@saschanaz
Copy link
Contributor

Thanks, in that case it will be updated by webref within a few days.

This was referenced Oct 27, 2021
Closed
Closed
@saschanaz
Copy link
Contributor

Fixed by #1200

@github-actions close

@github-actions
Copy link
Contributor

Closing because @saschanaz is one of the code-owners of this repository.

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 15, 2022
@peter50216
WebUI: Update TypeScript compiler to 4.5.5
f875565 
There are several major issues when upgrading:

* Generated file for mojo bindings contains reference to MojoHandle and
  mojo.internal.MojomType. Add stub for those types to bindings.d.ts.

* DOM types changes: Some DOM types that are not available on all
  browsers are removed from TypeScript lib.d.ts, and some types are
  added.
  * Some redundant type definitions from
    ash/webui/camera_app_ui/resources/js/externs/types.d.ts are removed.
  * Other properties used are added to
    //tools/typescript/definitions/pending.d.ts, and added to
    definitions for gn that use those types.
  * DataTransferItem.webkitGetAsEntry() type returns possibly null, so
    null assertion is added.
  * ariaLabel for HTMLElement is a string and never null|undefined in
    TypeScript 4.5 type definitions. This is an issue for TypeScript
    lib.dom.d.ts [1], since the spec says string|null. This can be fixed
    by using the lib replacement available in TypeScript 4.5, and
    reference to the @types/web instead of using the built-in ones,
    which contains a newer lib.dom.d.ts that already fix this issue [2],
    and will be done in a later CL.
    For now, the conflicting definitions are removed.
  * Several @types packages are updated to the latest version for the
    DOM changes, this includes:
    * @types/dom-mediacapture-record: 1.0.10 -> 1.0.11
    * @types/w3c-css-typed-object-model-level-1: 20180410.0.3 -> 20180410.0.4
    * @types/webrtc: 0.0.29 -> 0.0.31

* TypeScript produces class property directly [3] when targeting ESNext,
  which is not supported by polymer-css-build. Fixed by targeting ES2021
  instead.

* chrome/browser/resources/settings/search_settings.ts has subclass
  re-declare protected member as different types and cause compile
  error. Fix by using declare as suggested by compiler.

[1]: microsoft/TypeScript-DOM-lib-generator#1119
[2]: https://www.npmjs.com/package/@types/web
[3]: https://github.com/tc39/proposal-class-fields

Bug: b:213292127, 1285794
Test: compile chrome

Change-Id: I8e09dfc0ac9f8ba0679d2ea1eb2a2f36dba425e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3380108
Reviewed-by: Shik Chen <shik@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Pi-Hsun Shih <pihsun@chromium.org>
Cr-Commit-Position: refs/heads/main@{#971074}
@h-joo h-joo mentioned this issue Feb 16, 2022
Open
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@peter50216 @copybara-github
WebUI: Update TypeScript compiler to 4.5.5
bfeffde 
There are several major issues when upgrading:

* Generated file for mojo bindings contains reference to MojoHandle and
  mojo.internal.MojomType. Add stub for those types to bindings.d.ts.

* DOM types changes: Some DOM types that are not available on all
  browsers are removed from TypeScript lib.d.ts, and some types are
  added.
  * Some redundant type definitions from
    ash/webui/camera_app_ui/resources/js/externs/types.d.ts are removed.
  * Other properties used are added to
    //tools/typescript/definitions/pending.d.ts, and added to
    definitions for gn that use those types.
  * DataTransferItem.webkitGetAsEntry() type returns possibly null, so
    null assertion is added.
  * ariaLabel for HTMLElement is a string and never null|undefined in
    TypeScript 4.5 type definitions. This is an issue for TypeScript
    lib.dom.d.ts [1], since the spec says string|null. This can be fixed
    by using the lib replacement available in TypeScript 4.5, and
    reference to the @types/web instead of using the built-in ones,
    which contains a newer lib.dom.d.ts that already fix this issue [2],
    and will be done in a later CL.
    For now, the conflicting definitions are removed.
  * Several @types packages are updated to the latest version for the
    DOM changes, this includes:
    * @types/dom-mediacapture-record: 1.0.10 -> 1.0.11
    * @types/w3c-css-typed-object-model-level-1: 20180410.0.3 -> 20180410.0.4
    * @types/webrtc: 0.0.29 -> 0.0.31

* TypeScript produces class property directly [3] when targeting ESNext,
  which is not supported by polymer-css-build. Fixed by targeting ES2021
  instead.

* chrome/browser/resources/settings/search_settings.ts has subclass
  re-declare protected member as different types and cause compile
  error. Fix by using declare as suggested by compiler.

[1]: microsoft/TypeScript-DOM-lib-generator#1119
[2]: https://www.npmjs.com/package/@types/web
[3]: https://github.com/tc39/proposal-class-fields

Bug: b:213292127, 1285794
Test: compile chrome

Change-Id: I8e09dfc0ac9f8ba0679d2ea1eb2a2f36dba425e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3380108
Reviewed-by: Shik Chen <shik@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Pi-Hsun Shih <pihsun@chromium.org>
Cr-Commit-Position: refs/heads/main@{#971074}
NOKEYCHECK=True
GitOrigin-RevId: f875565b3bbb7895d1f8ac3e7a252ab1611be58a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@orta @jnurthen @saschanaz @h-joo