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 vanishing list on firefox #8690

Closed
wants to merge 3 commits into from
Closed

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented May 28, 2020

So, this resolves the issue of disappearing components on firefox when scrolling. but I dislike removing this functionality for chrome that doesn't have a problem with it. How difficult would it be to add the browser we are building for as a classname to the HTML element during the build step? We could then enable or disable rules based on browser

Additional details:
There is interplay between position: sticky and z-index. If I change z-index to 0 less of the components disappear when scrolling, if I change it to -1 everything is great but the tabs are gone so :P

@brad-decker brad-decker requested a review from a team as a code owner May 28, 2020 19:33
@brad-decker brad-decker marked this pull request as draft May 28, 2020 19:33
@brad-decker brad-decker requested a review from whymarrh May 28, 2020 19:33
@Gudahtt
Copy link
Member

Gudahtt commented May 28, 2020

Any idea what the underlying cause is here? e.g. maybe a Firefox bug?

@metamaskbot
Copy link
Collaborator

Builds ready [3443c69]
Page Load Metrics (648 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint308438115
domContentLoaded34482064611053
load34682264811053
domInteractive34482064611053

@brad-decker
Copy link
Contributor Author

I don't even know, it doesn't have issues when in full screen, either... 🤕

Pretty sure there's a better solution -- at least I hope there is. I'll keep digging.

@Gudahtt
Copy link
Member

Gudahtt commented May 28, 2020

How difficult would it be to add the browser we are building for as a classname to the HTML element during the build step? We could then enable or disable rules based on browser

This is certainly a thing we could do. I think it'd be pretty easy - we already inject things via envify. It's wouldn't be the worst solution. This feature is nice, but it's not critical.

@brad-decker
Copy link
Contributor Author

@Gudahtt and @whymarrh this PR now adds static asset replacement feature to the build process, wherein each .HTML file has a class="browser-classname" added to the HTML element. Then, during static asset copying...instead of just copying them over we read the file, replace that browser-classname with chrome-browser or firefox-browser or whatever.. now we have the ability to override styling based on browser with no (little) runtime cost

@metamaskbot
Copy link
Collaborator

Builds ready [9301715]
Page Load Metrics (634 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint309242178
domContentLoaded4638116327033
load4658136347033
domInteractive4628116327033

@brad-decker brad-decker marked this pull request as ready for review May 28, 2020 22:06
@brad-decker brad-decker requested a review from kumavis as a code owner May 28, 2020 22:06
@whymarrh
Copy link
Contributor

Well this escalated quickly.

@brad-decker
Copy link
Contributor Author

It really got out of hand fast.

@whymarrh
Copy link
Contributor

This isn't a bad solution, but I am curious whether or not there's a way for us to fix this in CSS.

@whymarrh
Copy link
Contributor

I think we might be able to avoid all of this using the translateZ(0) hack.

In ui/app/components/ui/tabs/index.scss:

 @import 'tab/index';
 
 .tabs {
+  // Just for Firefox — https://github.com/MetaMask/metamask-extension/pull/8690
+  -moz-transform: translateZ(0);
+
   &__list {
     display: flex;
     justify-content: flex-start;

@brad-decker
Copy link
Contributor Author

@whymarrh very interesting. that does fix the problem! Way better fix. Do you want to do the honors or should I close this and submit a new pr with that one liner?

@whymarrh
Copy link
Contributor

I can open one up, sure.

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.

4 participants