Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Revert "Added VisibilityHandler to handle the mounting and unmounting…#6030

Merged
darkwing merged 1 commit into
firefox-devtools:masterfrom
jasonLaster:fix-scroll-pos
Apr 20, 2018
Merged

Revert "Added VisibilityHandler to handle the mounting and unmounting…#6030
darkwing merged 1 commit into
firefox-devtools:masterfrom
jasonLaster:fix-scroll-pos

Conversation

@jasonLaster
Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster commented Apr 19, 2018

Fixes Issue: #5748

Summary of Changes

  • We no longer unmount the app when we navigate to another tool
  • This means we will naturally preserve the scroll position

Trade-offs

  • I do not believe we saw a significant performance win by unmounting the application.
  • Unmounting the app means removing codemirror's underlying dom
  • saving the scroll position on scroll is possible, but requires scrolling at the right time. There's also quite a bit of overhead in re-mounting...

… of child components of App when changing panel tabs. (firefox-devtools#5688)"

This reverts commit ea6e740.
@darkwing
Copy link
Copy Markdown
Contributor

Tested in panel and scrolling persists.

Copy link
Copy Markdown
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Tested in panel and the scroll position persisted.

@darkwing darkwing merged commit 39ac796 into firefox-devtools:master Apr 20, 2018
jasonLaster added a commit to jasonLaster/debugger.html that referenced this pull request Apr 20, 2018
… of child components of App when changing panel tabs. (firefox-devtools#5688)" (firefox-devtools#6030)

This reverts commit ea6e740.
@juliandescottes
Copy link
Copy Markdown
Member

Great that this managed to be fixed before the end of 61 Nightly!

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