fix: remove some IE-specific code in dom and style #6396
Conversation
core/utils/style.ts
Outdated
| // element.style[..] is undefined for browser specific styles | ||
| // as 'filter'. | ||
| return (styles as AnyDuringMigration)[property] || | ||
| styles.getPropertyValue(property) || ''; |
There was a problem hiding this comment.
| styles.getPropertyValue(property) || ''; | |
| styles.getPropertyValue(property); |
| @@ -109,17 +106,11 @@ function getStyle(element: Element, style: string): string { | |||
| * @alias Blockly.utils.style.getComputedStyle | |||
| */ | |||
| export function getComputedStyle(element: Element, property: string): string { | |||
There was a problem hiding this comment.
Why is this exported but getStyle is not? Do we want to deprecate this and then stop exporting it in v10?
There was a problem hiding this comment.
I can't delete it because we use it in flyout_button.ts, but I can deprecate it and leave myself a note to make it @internal instead of deleting it in the next release.
There was a problem hiding this comment.
Actually, I don't like that because it means we have deprecation warnings in the browser for normal things (flyouts with buttons) for a full release.
There was a problem hiding this comment.
Can we export getStyle instead, and use that? (since it seems like the more general-purpose function anyway)
There was a problem hiding this comment.
But then we're just dropping one export to add another, which I don't like.
I'm looking into the uses of getStyle to see when one or the other gets used and whether I can simplify more.
There was a problem hiding this comment.
We ended up deleting getStyle since element.getComputedStyle returns a superset of the values on element.style, so the function would always short circuit before reaching the ||. I.e. it wasn't doing anything different from getComputedStyle.
|
Testing: before release, this should be tested in firefox in an iframe that starts hidden. |
| @@ -134,6 +106,9 @@ export function getComputedStyle(element: Element, property: string): string { | |||
| * @alias Blockly.utils.style.getCascadedStyle | |||
| */ | |||
| export function getCascadedStyle(element: Element, style: string): string { | |||
| deprecation.warn( | |||
| 'Blockly.utils.style.getCascadedStyle', 'version 9.0.0', | |||
| 'version 10.0.0'); | |||
There was a problem hiding this comment.
I like this clear approach, but I note that it doesn't makie any promises about how long the deprecated function will be available for. For example, though unlikely it's not inconceivable that a serious security bug could force a major version bump the day after 9.0 is release.
Given that:
- we don't think this is used externally, and
- we're not obliged to remove deprecated features
I think that's fine, but perhaps be careful about adopting this style more generally until we are reasonably sure we can set and stick to a schedule for major version bumps.
The basics
npm run formatandnpm run lintThe details
Resolves
Part of #6325
Proposed Changes
getCascadedStyleand mark it as deprecated.getComputedStyleby usingwindowinstead ofdocument.defaultView?.to tighten up some style checks. There's future work here to sort out theAnyDuringMigrations and figure out whether we need to accessstylein a different way.Behavior Before Change
No change in behaviour.
Behavior After Change
No change.
Reason for Changes
This code ostensibly worked in IE, but that's irrelevant since we no longer transpile to ES5.
Test Coverage
Opened the playground and clicked around in both Firefox and Chrome. Will need comprehensive testing in all browsers, along with other changes related to #6325.
Documentation
None
Additional Information
getCascadedStylewas probably not being used externally, but I'm going through the full deprecation process because of its visibility and lack of@internalmarker.