Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Smoother transition when newtab image loads slowly #6590

Closed

Conversation

josiah-keller
Copy link
Contributor

@josiah-keller josiah-keller commented Jan 10, 2017

Fixes #5309

When opening a new tab, the background image can load slowly, especially if it's not cached, giving a rough appearance (especially for new users). Previously, the entire newtab page contents were faded in on page load, which doesn't help if the image takes awhile to load - you can still see the image progressively rendering as it loads.

Now, the image is hidden until it loads, while the rest of the newtab page is displayed immediately with no fade for immediate interaction. Once the image loads, it fades in.

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

  1. Clear browser cache
  2. Open a new tab
  3. Note that UI elements are interactive right away, while image fades in once it loads.

@luixxiul
Copy link
Contributor

@luixxiul luixxiul added this to the 0.13.1 milestone Jan 12, 2017
@luixxiul luixxiul changed the base branch from master to 0.13.1-branch January 14, 2017 14:57
@bsclifton bsclifton force-pushed the 0.13.1-branch branch 6 times, most recently from bd9183d to 2789018 Compare January 23, 2017 17:20
@bsclifton bsclifton force-pushed the 0.13.1-branch branch 3 times, most recently from 65c7753 to 238cfd3 Compare January 25, 2017 19:19
@bsclifton bsclifton modified the milestones: 0.13.2, 0.13.1 Jan 26, 2017
@bsclifton bsclifton closed this Jan 26, 2017
@bsclifton bsclifton changed the base branch from 0.13.1-branch to master January 26, 2017 21:17
@bsclifton bsclifton reopened this Jan 26, 2017
@bsclifton
Copy link
Member

@josiah-keller sorry it's taken me so long to get to this ☹️ Would you mind giving this a rebase? I think we'll have cycles to pull it in soon 😄

@josiah-keller
Copy link
Contributor Author

@bsclifton Is there any point in that? Won't the newtab page be changing to deal with the memory issue anyway? Seems more logical to close this to me.

@bsclifton
Copy link
Member

@josiah-keller you're right- the image for the new tab page is currently disabled (as of tonite). We will be re-enabling it as soon as we fix the memory issue (which should be soon- next few weeks). We can just hold off until then- I think this is a great change though 😄

@josiah-keller
Copy link
Contributor Author

@bsclifton I noticed #6965 was put in the 0.13.3 milestone so I went ahead and did a rebase.

@luixxiul luixxiul modified the milestones: 0.13.3, 0.13.4 Feb 7, 2017
@bsclifton
Copy link
Member

Will be ready for re-review after #7160 is accepted

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Hey there 😄 Sorry it's taken so long for me to dig in and review this ☹️

I spent a good amount of time with this PR and the changes are sometimes noticeable, but other times the flash of white still hits you

I wanted to ask (if you're still up for it) if you'd be able to try an approach where (if images are set to true) the body is black (instead of white). And only after the image finishes loading, it would start the animation transition to fade in. This is similar to the behavior I've seen on sites such as Google Hangouts:
https://hangouts.google.com/

Let me know what you think 😄

@bsclifton bsclifton modified the milestones: 0.13.6, 0.13.5 Feb 16, 2017
@josiah-keller
Copy link
Contributor Author

josiah-keller commented Feb 16, 2017

@bsclifton I've noticed that too, and in those cases the correct class for the animation still gets applied. I assumed it was just bad animation performance on my machine. I'm fine with doing whatever changes you think are best, but won't this just give a flash of black instead? Or am I misunderstanding you?

@bsclifton
Copy link
Member

bsclifton commented Feb 17, 2017

@josiah-keller I think if you give it a shot, you'll notice the black (fading into lighter colors) seems to be a lot nicer. I tried a reloading a bunch on the https://hangouts.google.com URL and it is very easy on the eyes

Going forward, now that backgrounds can be disabled, we can likely update the code to make the default color configurable. At the moment, it's hardcoded to white (and I'm proposing hardcoding to black).

Here's an an open issue relating to that (in case you're interested): #7159

@josiah-keller
Copy link
Contributor Author

@bsclifton Got around to changing the background, and it does indeed look better.

@bsclifton
Copy link
Member

bsclifton commented Feb 23, 2017

@josiah-keller! The black fade in (which looks great!) makes the blinking even more obvious. I did some more digging in and here's what I've found.

The white flash is something present because of the webview. This tag is inserted by Brave when a new tab is opened which ends up creating a webcontents Chromium object. This is what seems to have a default color of white.

The issue has been reported in Electron with electron/electron#5175. We're likely going to need a Muon fix for this to look correct ☹️

@bsclifton
Copy link
Member

bsclifton commented Feb 23, 2017

Given the show stopper documented above (needing to have a Muon fix), I'm going to close this PR. Thanks for digging in @josiah-keller - I'll make sure to share the findings above in the original issue

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

Successfully merging this pull request may close these issues.

3 participants