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 dock button rendering issue #14998

Merged
merged 1 commit into from Aug 29, 2017

Conversation

Projects
None yet
5 participants
@simurai
Member

simurai commented Jul 12, 2017

Description of the Change

This promotes the .atom-dock-toggle-button-inner to a GPU layer.

Alternate Designs

  • There would be other ways like transform: translateZ(0) or backface-visibility: hidden, but will-change is more official.
  • We could also promote the parent element .atom-dock-toggle-button, but since the "inner" element is the one that actually gets transformed, it seems more appropriate.

Why Should This Be In Core?

It's where the dock styles live.

Benefits

Fixes this rendering issue: #14915

Possible Drawbacks

Forcing layers could have some side effects elsewhere.

Applicable Issues

Fixes #14915

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Jul 24, 2017

Contributor

@HotelCalifornia if you are able to reproduce the issue consistently can you try testing this by adding the following to your stylesheet?

.atom-dock-toggle-button-inner {
  will-change: transform;
}
Contributor

Arcanemagus commented Jul 24, 2017

@HotelCalifornia if you are able to reproduce the issue consistently can you try testing this by adding the following to your stylesheet?

.atom-dock-toggle-button-inner {
  will-change: transform;
}
@HotelCalifornia

This comment has been minimized.

Show comment
Hide comment
@HotelCalifornia

HotelCalifornia Jul 24, 2017

My repro: close and reopen Atom manually, black bars appear

before adding stylesheet rule before
after adding stylesheet rule after

Note that the edge of the dock is a little messed up (i.e. has some artifacts that I don't believe were there before), but there are no black bars anymore (persists through restarts).

It should be also noted that the thin, black line to the right of the gray one is there because my screenshot bled over to the other monitor. The artifacts I'm referring to are the thin, gray line to the right of the image, as well as whatever is going on at the top of the minimap.

HotelCalifornia commented Jul 24, 2017

My repro: close and reopen Atom manually, black bars appear

before adding stylesheet rule before
after adding stylesheet rule after

Note that the edge of the dock is a little messed up (i.e. has some artifacts that I don't believe were there before), but there are no black bars anymore (persists through restarts).

It should be also noted that the thin, black line to the right of the gray one is there because my screenshot bled over to the other monitor. The artifacts I'm referring to are the thin, gray line to the right of the image, as well as whatever is going on at the top of the minimap.

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Aug 28, 2017

Contributor

Besides confirming that this works for me, is there any other testing I can help on for this?

Contributor

Arcanemagus commented Aug 28, 2017

Besides confirming that this works for me, is there any other testing I can help on for this?

@Ben3eeE Ben3eeE requested a review from ungb Aug 28, 2017

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Aug 28, 2017

Contributor

@Arcanemagus thanks for verifying! I've tested this as well and looks good to me. I was able to see the issue before and fixed afterwards.

@simurai regarding your comment:

Forcing layers could have some side effects elsewhere.

It seems this css style is only on the class .atom-dock-toggle-button-inner can you go into more details about your concerns if there's anything we should test or look out for? If you're good I think we should merge this in.

Contributor

ungb commented Aug 28, 2017

@Arcanemagus thanks for verifying! I've tested this as well and looks good to me. I was able to see the issue before and fixed afterwards.

@simurai regarding your comment:

Forcing layers could have some side effects elsewhere.

It seems this css style is only on the class .atom-dock-toggle-button-inner can you go into more details about your concerns if there's anything we should test or look out for? If you're good I think we should merge this in.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 29, 2017

Member

Forcing layers could have some side effects elsewhere.

Well, it looks like currently there is a big layer that gets removed, once will-change: transform; is added. I guess because it's not needed when promoting only the dock toggle button. Not sure if that has an impact on the editor underneath? Could also be a good thing.

layers

Ok, let's merge and keep it in mind. Also, /cc @as-cii @nathansobo FYI

Member

simurai commented Aug 29, 2017

Forcing layers could have some side effects elsewhere.

Well, it looks like currently there is a big layer that gets removed, once will-change: transform; is added. I guess because it's not needed when promoting only the dock toggle button. Not sure if that has an impact on the editor underneath? Could also be a good thing.

layers

Ok, let's merge and keep it in mind. Also, /cc @as-cii @nathansobo FYI

@simurai simurai merged commit b7a1443 into master Aug 29, 2017

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

@simurai simurai deleted the sm-dock-button-rendering branch Aug 29, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 29, 2017

Contributor

❤️

Contributor

nathansobo commented Aug 29, 2017

❤️

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