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(clip): add an extra space to the clip-path width to prevent unexpected clip. close #19051 #19056

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

RexSkz
Copy link
Contributor

@RexSkz RexSkz commented Aug 30, 2023

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Add an extra space to the clip-path width. This will prevent the unexpected clip.

Fixed issues

Details

Before: What was the problem?

As the example https://codepen.io/rexskz/pen/WNLweyw shows, the rightmost position of the line that has cap: 'round' is clipped incorrectly, while the leftmost part is okay.

image

This issue only happens on the device with devicePixelRatio >= 3, and the chart canvas must have a special width.

After: How does it behave after the fixing?

The rightmost position of the line should not be clipped.

image

This is caused by a round-off error.

Let's see if the rect is:

x = 80.8;      // float
y = 100;
width = 500.4; // float
height = 100;
// x + width = 581.2

Browsers will render the clip-path using rect but handle the value as integers, so the original code converts x and width to integers:

x = Math.floor(x);         // 80
width = Math.round(width); // 500
// x + width = 580

The result is 1.2 less, meaning the graph's rightmost 1.2px will be clipped.

To make sure the clip-path wraps the whole graph, the x should keep floored, but the width should be ceiled (not rounded), and if x is floored, width should be add an extra 1px.

P.S. During the visual test, I found that some cases may also be clipped incorrectly before, and will be corrected in this PR, for example:

image image

Document Info

One of the following should be checked.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Aug 30, 2023

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

src/chart/helper/createClipPathFromCoordSys.ts Outdated Show resolved Hide resolved
src/chart/helper/createClipPathFromCoordSys.ts Outdated Show resolved Hide resolved
@RexSkz
Copy link
Contributor Author

RexSkz commented Aug 30, 2023

@Ovilia Done; please take a look and notify me if there are other suggestions :)

@RexSkz
Copy link
Contributor Author

RexSkz commented Aug 31, 2023

How about triggering the workflow? I'll quickly fix the raised issue if there is one :)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19056@a57202d

@Ovilia
Copy link
Contributor

Ovilia commented Sep 15, 2023

Hi, I checked the demo with 5.4.3 and it seems not to be clipped. Did I miss anything here?

@RexSkz
Copy link
Contributor Author

RexSkz commented Sep 15, 2023

Hi, I checked the demo with 5.4.3 and it seems not to be clipped. Did I miss anything here?

Slightly resize the page to a specific width (e.g. 1346px ~ 1348px in my Mac) and you'll see the clipping issue at the right:

image

The correct behaviour should be:

image

@RexSkz
Copy link
Contributor Author

RexSkz commented Sep 15, 2023

Oh, it seems that using Math.ceil is enough, no need to consider DPR... (The line width is 50px but the error is 1px, it must be a round-off error only.)

I'll change my code and see if it works well.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 15, 2023
@RexSkz RexSkz changed the title fix(clip): calculate with dpr when extending clip path. close #19051 fix(clip): add an extra space to the clip-path width to prevent unexpected clip. close #19051 Sep 15, 2023
@RexSkz
Copy link
Contributor Author

RexSkz commented Sep 15, 2023

Hi @Ovilia, I've made changes to my code and the content of this PR. Please have a look, it should be a lot simpler than the previous version~

@Ovilia Ovilia added this to the 5.5.0 milestone Sep 19, 2023
Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Please don't use force push in your next PR because we'd like to record every commit changes. Don't worry if there are too many commits. The reviewer will use squash push when necessary.

@Ovilia Ovilia merged commit fa231d3 into apache:master Sep 19, 2023
2 checks passed
@echarts-bot
Copy link

echarts-bot bot commented Sep 19, 2023

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants