Skip to content

Native DataTransfer#files should not be accessed when not needed or not relevant#13371

Merged
niegowski merged 4 commits intomasterfrom
ck/13366
Feb 3, 2023
Merged

Native DataTransfer#files should not be accessed when not needed or not relevant#13371
niegowski merged 4 commits intomasterfrom
ck/13366

Conversation

@niegowski
Copy link
Copy Markdown
Contributor

Suggested merge commit message (convention)

Fix (clipboard, engine): Dragging images in the editor should not lag in Firefox. Closes #13366.


Additional information

# Conflicts:
#	packages/ckeditor5-engine/src/view/datatransfer.ts
const evtData: ClipboardEventData = {
dataTransfer: new DataTransfer( 'clipboardData' in domEvent ? domEvent.clipboardData! : domEvent.dataTransfer! )
dataTransfer: new DataTransfer(
'clipboardData' in domEvent ? domEvent.clipboardData! : domEvent.dataTransfer!,
Copy link
Copy Markdown
Member

@Reinmar Reinmar Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extract these expressions. They made it hard for me to parse this code.


constructor( nativeDataTransfer: DomDataTransfer ) {
this.files = getFiles( nativeDataTransfer );
constructor( nativeDataTransfer: DomDataTransfer, options: { cacheFiles?: boolean } = {} ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the new option need to be documented?

Or it's blocked by the issue about porting API docs to TSDoc?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not blocked.

But, IMO, if the parameter name makes its purpose obvious, do we really need docs every time?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second thought. It's not obvious, the cashing happens anyway. It should be called eagerlyGetFiles or lazyGetFiles, cacheItNow or something in this direction.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that while the name says what it does. It does not say anything about why you may need that. And that's what the documentation should cover in most cases too.

@niegowski
Copy link
Copy Markdown
Contributor Author

@arkflpc can we merge this PR?

@niegowski niegowski requested a review from arkflpc February 3, 2023 12:52
@niegowski niegowski merged commit 04c7d47 into master Feb 3, 2023
@niegowski niegowski deleted the ck/13366 branch February 3, 2023 13:29
pomek pushed a commit that referenced this pull request Feb 6, 2023
Fix (clipboard, engine): Dragging images in the editor should not lag in Firefox. Closes #13366.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image drag & drop extremely sluggish in Firefox 109

3 participants