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

Rendering fix (subpixel -> old one) #1083

Closed
wants to merge 1 commit into from
Closed

Conversation

bews
Copy link
Contributor

@bews bews commented Apr 24, 2017

Was discussed in #1071

@AlMcKinlay
Copy link
Member

So, I have no idea what this code was even meant to do in the first place. @thelounge/maintainers anyone?

@xPaw
Copy link
Member

xPaw commented Apr 25, 2017

It's a trick to force certain elements to be rendered on the GPU.

@AlMcKinlay
Copy link
Member

Alright, is there a reason we are currently doing that?

@astorije
Copy link
Member

I believe it was added for opening/closing channel list and user list, but why that was deferred to the GPU, that I have no idea...
Maybe the animation wasn't super smooth on mobiles?

@astorije
Copy link
Member

Also, just to be clear, you reported this as a regression from Shout, but this was introduced 3 years ago by erming himself, so I'm not sure how The Lounge regressed from it :)

@xPaw
Copy link
Member

xPaw commented Apr 25, 2017

Opening the sidebar uses translate3d, probably yes, to force GPU before it starts. Forcing GPU layer is also helpful for scroll performance, I was debugging some perf stuff in devtools and doing such a trick improved things. I cant provide more details right now as I am on mobile.

What i want to know, why does translateZ of 0 does different font rendering?

@astorije
Copy link
Member

Note that it's not the first time that someone reports that the text in the user list is not as clean as the text of the chat window. Maybe some GPUs do not render things correctly and make simplifications instead?

@bews
Copy link
Contributor Author

bews commented Apr 25, 2017

Also, just to be clear, you reported this as a regression from Shout, but this was introduced 3 years ago by erming himself, so I'm not sure how The Lounge regressed from it :)

Just checked myself, you're right. But Shout doesn't use subpixel rendering with this. Guess transform: translateZ(0); should be combined with something else make this change, but I couldn't find it. So the easiest way to fix this is to remove those lines.

@astorije astorije requested a review from xPaw April 27, 2017 23:55
@astorije
Copy link
Member

@xPaw, @YaManicKill, what do we want to do with this?

@xPaw
Copy link
Member

xPaw commented Apr 29, 2017

So this PR removes animation when opening or closing the channel list (with hamburger button).

@bews Can you test if adding will-change: transform fixes your issue? http://stackoverflow.com/a/32744881/2200891

@bews
Copy link
Contributor Author

bews commented Apr 29, 2017

So this PR removes animation when opening or closing the channel list (with hamburger button).

No, it doesn't. What browser do you use? I can't reproduce it here.

Can you test if adding will-change: transform fixes your issue?

Yes, replacing

-webkit-transform: none !important;
transform: none !important;

with

will-change:transform;

fixes rending too. Should we use this way?

@@ -787,8 +787,6 @@ kbd {
right: 0;
width: 180px;
transition: all .4s;
-webkit-transform: translateZ(0);
transform: translateZ(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really strange, but sidebar rendering was fixed by one of the recent commits, so it's not necessary to remove these lines anymore. Magic! Although there is a chance that this problem will appear again in future.

@astorije
Copy link
Member

It's really strange, but sidebar rendering was fixed by one of the recent commits, so it's not necessary to remove these lines anymore.

Should we close this PR (for now at least) then?

@bews
Copy link
Contributor Author

bews commented Apr 30, 2017

Should we close this PR (for now at least) then?

No, chat block is still broken, so at least other 2 lines should be removed.

-webkit-transform: none !important;
transform: none !important;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ones.

@astorije
Copy link
Member

astorije commented May 1, 2017

According to this link given by @xPaw in #1120, translateZ(0); was used to improve performance in Safari and Safari Mobile.

Also, I'm thinking #1120 will supersede this, won't it, @xPaw? If so we should close for now.

@xPaw xPaw closed this May 1, 2017
astorije pushed a commit that referenced this pull request Jun 10, 2017
@xPaw xPaw added this to the 2.3.2 milestone Jun 20, 2017
eliemichel pushed a commit to eliemichel/lounge that referenced this pull request Aug 31, 2017
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@xPaw xPaw removed their request for review November 3, 2018 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants