fix!: remove checks for IE and EdgeHTML in core #6336
Conversation
rachel-fenichel
added
component: browser
breaking change
core/field.ts
Outdated
| @@ -797,7 +797,7 @@ export abstract class Field implements IASTNodeLocationSvg, | |||
| scaledWidth += 1 * scale; | |||
| scaledHeight += 1 * scale; | |||
| } else { | |||
| if (!userAgent.EDGE && !userAgent.IE) { | |||
| if (!userAgent.EDGE) { | |||
There was a problem hiding this comment.
Not really related to this PR but I don't understand how this set of if/else squares with the comments above it. If Gecko and Edge are in the same behavior group why are they handled differently here?
I also don't understand why the bounding box behavior from the browser comes into play - I don't see where that's called in the calculation of getHeightWidth but I didn't finish tracing the call all the way down the stack.
so no action needed but if you are familiar with this bit of code i would love to learn more about it :)
There was a problem hiding this comment.
I looked into this more and concluded that we can remove the checks for Edge entirely, and that it makes sense to do so in the same PR since multiple of the checks are for both IE and Edge.
However, I need to load it in Firefox to check that it works after my changes, and I found that Firefox is broken.
|
Also, I don't think this should be a chore. We don't have a strict definition, but generally I think chores shouldn't affect source code. In particular I think chores are left out of the release notes by some tools, or chores should be things we are comfortable leaving out of the release notes as they wouldn't affect external users. So I would probably call this a fix even though it's not really a bug fix, but it's the closest thing. Or our docs say we can use the "deprecate" tag even though I'm not sure anyone has, and it only sort of fits this scenario. |
rachel-fenichel
changed the title
core/field.ts
Outdated
| scaledWidth = bBox.width * scale; | ||
| scaledHeight = bBox.height * scale; | ||
| scaledWidth = (bBox.width + 1) * scale; | ||
| scaledHeight = (bBox.height +1) * scale; |
There was a problem hiding this comment.
this is the line causing the clang error
| // Useragent for JavaFX: | ||
| // Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.44 | ||
| // (KHTML, like Gecko) JavaFX/8.0 Safari/537.44 | ||
| isJavaFx = has('JavaFX'); | ||
| isChrome = (has('Chrome') || has('CriOS')) && !isEdge; | ||
| isChrome = (has('Chrome') || has('CriOS')); |
There was a problem hiding this comment.
do we use these for anything? maybe we should fully get rid of the ones we don't need at all
maribethb
added
PR: fix
rachel-fenichel
force-pushed
the
remove_ie_support
branch
from
9e7c5a2
to
9deef2c
Compare
|
Updated to fix Firefox by returning to Fully removing all of the user agent types we don't use sounds good as a followup PR, and matches with the goal of getting API changes out of the way this quarter. |
The basics
npm run formatandnpm run lintThe details
Resolves
Part of #6325
Proposed Changes
userAgent.IEuserAgent.IEBreaking change because of the removed export.
Behavior Before Change
Assorted checks for IE in our code.
Behavior After Change
No special behaviour for IE--but note that there are still special cases for Edge.
Reason for Changes
No more IE support.
Test Coverage
Built and ran tests.
Additional Information
isGeckostill uses whether IE has been found insideuseragent.jsisGecko = has('Gecko') && !isWebKit && !isIe && !isEdge;We should consider removing all exports from
useragent.jsthat we aren't already using. User agent sniffing is highly not-recommended, and we can get rid of excess exports. It's technically a breaking change, but I think we want to do it in this release.