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

Don't break subpixel AA when cursor is at the end of longest line #16595

Merged
merged 2 commits into from Jan 20, 2018

Conversation

Projects
None yet
2 participants
@as-cii
Member

as-cii commented Jan 19, 2018

Description of the Change

With the Electron upgrade, something changed in the way characters are rendered/measured, and that was causing subpixel anti-aliasing to stop working when cursors were at the end of the longest line.

before

Every cursor has a width that is calculated in the following way:

  • If there's a character after the cursor, the width corresponds to the width of such character.
  • Otherwise, the width equals to the "base character width" measured on a dummy line.

In the latter case, even if we were setting the width of the content container to account for the width of such cursor, some rounding problem was causing the cursor to be able to escape the container and thus break subpixel anti-aliasing.

With this pull request, instead of rounding the value we assign to the container width, we will always ceil it. This ensures that cursors are always strictly contained within the parent element and resolves the subpixel anti-aliasing issue.

after

Alternate Designs

None.

Benefits

Fixes a regression in the subpixel anti-aliasing rendering code path that was causing text to look blurry when cursor was at the end of the longest line.

Possible Drawbacks

None.

Verification Process

See previously attached GIFs.

Applicable Issues

It doesn't seem like this problem has been reported anywhere. /cc: @Ben3eeE @ungb @rsese in case you are aware of any recent report about this. If so, feel free to edit the body of this pull request to automatically close any relevant issue once this pull request gets merged.

/cc: @nathansobo

Don't break subpixel AA when cursor is at the end of longest line
With the Electron upgrade, something changed in the way characters are
rendered/measured, and that was causing subpixel anti-aliasing to stop
working when cursors were at the end of the longest line.

Every cursor has a width that is calculated in the following way:

* If there's a character after the cursor, the width corresponds to
width of such character.
* Otherwise, the width equals to the "base character width" measured on
a dummy line.

In the latter case, even if we were setting the width of the content
container to account for the width of such cursor, some rounding problem
was causing the cursor to be able to escape the container and thus break
subpixel anti-aliasing.

With this commit, instead of rounding the value we assign to the
container width, we will always ceil it. This ensures that cursors are
always strictly contained within the parent element and resolves the
subpixel anti-aliasing issue.

@as-cii as-cii self-assigned this Jan 19, 2018

@as-cii as-cii merged commit ef96cc7 into master Jan 20, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@as-cii as-cii deleted the as-fix-subpixel-aa branch Jan 20, 2018

@50Wliu

This comment has been minimized.

Member

50Wliu commented Jan 21, 2018

@as-cii Unfortunately I am still running into this issue even on 1.25.0-dev-ef96cc770, safe mode. Perhaps the 150% display scaling on Windows 10 has something to do with it.

subpixel-aa-loss-longest-line

@as-cii

This comment has been minimized.

Member

as-cii commented Feb 5, 2018

Yeah that's strange. I wonder if we should round up to the nearest physical sub pixel instead of simply using Math.ceil.

Can you test replacing the call to Math.ceil in this diff with ceilToPhysicalPixelBoundary and report what you see? Thanks!

@50Wliu

This comment has been minimized.

Member

50Wliu commented Feb 5, 2018

@as-cii My intent was to file an actual issue about this a few days ago but when I went to re-reproduce just to be safe I found I couldn't. If it happens again I'll test the change you suggested.

@50Wliu 50Wliu referenced this pull request Mar 6, 2018

Closed

Loss of subpixel AA on soft-wrapped line #16889

1 of 1 task complete

@jasonrudolph jasonrudolph referenced this pull request May 8, 2018

Merged

Upgrade to Electron 2.0.0 #17273

60 of 60 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment