Skip to content

Try fix DataGrid HTML clipboard data faulty #10477

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Try fix DataGrid HTML clipboard data faulty #10477

wants to merge 2 commits into from

Conversation

lindexi
Copy link
Member

@lindexi lindexi commented Feb 19, 2025 •
edited
Loading

Fixes #10476

Main PR

Description

The #8519 introduces behavior changed. And this pr restore the origin behavior.

The origin behavior is:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGridClipboardHelper.cs

Lines 17 to 20 in d4ffc34

{
private const string DATAGRIDVIEW_htmlPrefix = "Version:1.0\r\nStartHTML:00000097\r\nEndHTML:{0}\r\nStartFragment:00000133\r\nEndFragment:{1}\r\n";
private const string DATAGRIDVIEW_htmlStartFragment = "<HTML>\r\n<BODY>\r\n<!--StartFragment-->";
private const string DATAGRIDVIEW_htmlEndFragment = "\r\n<!--EndFragment-->\r\n</BODY>\r\n</HTML>";

But after #8519 , it is:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/DataGridClipboardHelper.cs

Lines 83 to 113 in a792086

int bytecountEndOfFragment = bytecountPrefixContext + destinationBytes.Length;
int bytecountEndOfHtml = bytecountEndOfFragment + bytecountSuffixContext;
string prefix = string.Create(CultureInfo.InvariantCulture,
$"""
Version:1.0
StartHTML:00000097
EndHTML:{bytecountEndOfHtml:00000000}
StartFragment:00000133
EndFragment:{bytecountEndOfFragment:00000000}
<HTML>
<BODY>
<!--StartFragment-->
""");
content.Insert(0, prefix);
content.Append(
"""
<!--EndFragment-->
</BODY>
</HTML>
""");

which wrong with double CR/LFs (\r\n\r\n)

Customer Impact

Fixes #10476

Regression

See #8519

Testing

Only CI.

Risk

Normal. This PR will changed the DataGrid clipboard behavior.

Cc @halgab

Microsoft Reviewers: Open in CodeFlow

@lindexi lindexi requested review from a team as code owners February 19, 2025 11:44
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Feb 19, 2025
@lindexi lindexi mentioned this pull request Feb 19, 2025
Open
@h3xds1nz
Copy link
Member

LGTM, this should be backported imho.

A unit test wouldn't hurt.

@miloush
Copy link
Contributor

miloush commented Feb 19, 2025

I don't really like the code this has been changed to, it's too fragile and might not survive git handling of new lines.

@kniessen
Copy link

Thx so much - for the ultra-quick reaction to this as well as just in general for maintaining the framework.

@lindexi
Copy link
Member Author

lindexi commented Feb 19, 2025 •
edited
Loading

@miloush What do you suggest? Thank you. How about back to origin code?

@h3xds1nz
Copy link
Member

Yeah that's a good point as raw string literals always emit whatever is in the source, and there's no way to emit specific line-endings (unfortunately, the language team has not shown any traction on a proposal to handle this).

@lindexi We could just use a syntax like

$"abcd \r\n" + 
$"xxx \r\n" + 
$"xxx \r\n";

Roslyn will lower this into one interpolated handler and it will look "decent" in the code. It will emit more AppendLiteral calls but that will perform better than original (that's just an educated guess).

But a test for the method would be enough to keep using the raw string literals here (I'm a fanboy) imho. It's an internal function by design and can be easily tested.

@lindexi
Update the prefix string.
6a1975b 
Follow @h3xds1nz 's suggestion.
@lindexi lindexi force-pushed the t/lindexi/DataGridClipboardHelper branch from 031cfea to 6a1975b Compare February 21, 2025 03:58
@halgab
Copy link
Contributor

halgab commented Jun 7, 2025

LGTM, even though I did like the added readability allowed by raw interpolated strings

@lindexi
Copy link
Member Author

lindexi commented Jun 9, 2025

I'd like it too.

@h3xds1nz
Copy link
Member

h3xds1nz commented Jun 9, 2025

@lindexi As I've said previously, why don't you just use raw-interpolated string then as previously done and write 2-3 unit tests around to verify the correct line-breaks are emitted? That way it is safe and readable.

@lindexi
Copy link
Member Author

lindexi commented Jun 10, 2025 •
edited
Loading

@h3xds1nz Thank you. I just fall back to the old code. And I do not plan to add the unit test.

And we can not control the CRLF in the raw-interpolated string, so that I do not want to use the raw-interpolated string yet.

And, why not I use the syntax like:

$"abcd \r\n" + 
$"xxx \r\n" + 
$"xxx \r\n";

Because we should pass the bytecountEndOfHtml and bytecountEndOfFragment to string.Create(CultureInfo.InvariantCulture, xx)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miloush miloush miloush approved these changes

@halgab halgab halgab approved these changes

@dipeshmsft dipeshmsft Awaiting requested review from dipeshmsft

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

@Kuldeep-MS Kuldeep-MS Awaiting requested review from Kuldeep-MS

At least 1 approving review is required to merge this pull request.

Assignees

@lindexi lindexi

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WPF DataGrid HTML clipboard data faulty
5 participants
@lindexi @h3xds1nz @miloush @kniessen @halgab