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.

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

fix: RichTextBox LinkClicked event for "friendly named" hyperlinks #1745

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Sep 3, 2019

Fixes #1631
Relates to #1139

Proposed changes

The newer RichEdit controls (v4.1+) in specific cases had troubles detecting "friendly named" hyperlinks as the Text property was reportig less characters than the rage of the actual hyperlink.

Customer Impact

  • "friendly named" hyperlinks raise LinkClicked events in RichEdit v4.1+.

Regression?

  • Yes

Risk

  • Low

Test methodology

  • manual

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

The newer RichEdit controls (v4.1+) in specific cases had troubles
detecting "friendly named" hyperlinks as the `Text` property was
reportig less characters than the rage of the actual hyperlink.

Fixes dotnet#1631
Relates to dotnet#1139
@RussKie RussKie requested a review from a team as a code owner September 3, 2019 14:19
@RussKie
Copy link
Member Author

RussKie commented Sep 3, 2019

The original code in .NET Framework was presumably put in place in response to "VSWhidbey 504502" bug, but neither me nor @Olina-Zhang were able to track it down.
@merriemcgaw @Tanya-Solyanik any ideas where we could find it?

@RussKie RussKie added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 3, 2019
@RussKie RussKie self-assigned this Sep 3, 2019
@RussKie RussKie added this to the 3.0-GA milestone Sep 3, 2019
@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #1745 into release/3.0 will decrease coverage by 0.05415%.
The diff coverage is 0%.

@@                  Coverage Diff                  @@
##           release/3.0       #1745         +/-   ##
=====================================================
- Coverage     25.97551%   25.92135%   -0.05416%     
=====================================================
  Files              804         804                 
  Lines           267706      267706                 
  Branches         37950       37950                 
=====================================================
- Hits             69538       69393        -145     
- Misses          193242      193381        +139     
- Partials          4926        4932          +6
Flag Coverage Δ
#Debug 25.92135% <0%> (-0.05416%) ⬇️
#production 25.92135% <0%> (-0.05416%) ⬇️
#test 100% <ø> (ø) ⬆️


if (c.cpMax > Text.Length || c.cpMax - c.cpMin <= 0)
Debug.Assert((c.cpMax - c.cpMin) > 0, "CHARRANGE was null or negative - can't do it!");
if (c.cpMax - c.cpMin <= 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the fix?

Copy link
Contributor

@weltkante weltkante Sep 3, 2019

Choose a reason for hiding this comment

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

Yes, it removes a workaround which was causing problems. its not known what it was working around (some comment refers to a bug id which couldn't be tracked down yet)

a long explanation can be found on stackoverflow

The point is that Text.Length can be the wrong value to compare against in presence of "friendly name links", a feature added by upgrading the RTF control. The workaround existed before the control upgrade but was never updated. Since its not know what its working around removing it makes the most sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so if there is a friendly name link, then Text.Length will be longer than is technically allowed (greater than cpMax), but since it is still visually allowed, this appears as a bug.

It seems odd to me that we would ever have opted to make the string empty is this circumstance. Truncation would be more appropriate, but anyways... fix looks good.

Is there any reason we would want to maintain this behavior? I cannot think of anything off the top of my head.

@RussKie RussKie added waiting-approval and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Sep 4, 2019
@RussKie
Copy link
Member Author

RussKie commented Sep 4, 2019

Signed off by the CTI

@zsd4yr
Copy link
Contributor

zsd4yr commented Sep 4, 2019

approved by tactics

@RussKie RussKie merged commit 4ad4e26 into dotnet:release/3.0 Sep 4, 2019
@RussKie RussKie deleted the dev/igveliko/1631_RichTextBox_LinkClicked branch September 4, 2019 20:26
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants