Fixes T7680. Make it so the listen behavior can be initialized multiple times from the server by having the behavior only update a few static data variables on subsequent initializations.
Details
- Reviewers
epriestley - Maniphest Tasks
- T7680: Quicksand does not update `pageObjects` correctly
- Commits
- Restricted Diffusion Commit
rPd8ab5f594c7f: Quicksand - make page object notifications work properly with quicksand
visited TX with user A and left a comment with user B and got the "reload" and "TX updated" bubbles.
Reloaded and navigated to /maniphest/ with user A and had user B leave another comment on TX - no "reload" bubble and correct "TX updated" bubble.
Navigated to TX again with user A and had user B leave a comment and got the "reload" and "TX updated" bubbles.
visited TX with user A and left a comment with user B and got the "reload" and "TX updated" bubbles. navigated away with user A and the "reload" bubble was automagically closed.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
We have several pieces of page metadata to update after a navigation event:
- pageObjects (T7680)
- Page title (T7744)
- Notification/Message counts (not specifically filed anywhere, I think)
- Help menu items (T7724)
- DarkConsole (T7700)
- Maybe Hi-Sec? (T7064)
- Global file upload mask (T7685)
Offhand, I can come up with three ways to do these updates:
- Reinitialize the behaviors, like this diff, and have the behavior do "initialize-or-update".
- Make the behaviors listen for a "teardown" / navigate-away event. This might be a good fit for turning off the global file upload mask, for instance.
- Explicitly make Quicksand do these updates in its response handler.
Although (1) and (2) might be good fits for some of these, I believe they won't work with the "back" button unless we do a lot of extra work. Specifically, when you press "back", we do this:
- We remove the current page's content from the document.
- We dump the old page's content back into the document.
- We do not re-run behaviors.
So I'd expect this change to fail in this way:
- You load page "A", which sets pageObjects = [A] via behavior initialization.
- You navigate to page "B", which sets pageObjects = [B] via behavior update.
- You press "Back", returning to page "A".
- You're now on page "A", but pageObjects = [B], which is incorrect.
Of the methods above, only (3) gives us an easy way to handle the back button. It also seems like the most reasonable "natural" approach for some of the options, like the help menu, page title, and maybe notification/message counts (which need logic to use the "most recent" values, not the values when the page was loaded) and probably Hi-Sec and maybe others. I think we should start with that and see if we can just do everything with that. If not, we can look at changing Quicksand to have a more formal initialize/teardown process for behaviors.
To do (3), we'd do something like:
- In PhabricatorStandardPageView->renderForQuicksand(), generate all the other data we need (pageObjects, title, etc.) and send it back with the response.
- In JX.Quicksand._onresponse(), change _content to store the whole response instead of just storing the content property.
- In JX.Quicksand._draw(), after swapping page content, go through the list of state changes we need to make and explicitly update them.
- Probably this means "fire an event which stuff can listen to", e.g. this.invoke('navigate', old, new) where old and new are page states.
- Handlers can listen for this and propagate state changes into the document.
- For example, the pageObjects handler would do something like JX.Leader.removeSubscriptions(old.pageObjects); JX.Leader.addSubscriptions(new.pageObjects);.
- Generating old for the initial page might be slightly tricky? Not sure. We may have to build it separately and ship it down on the initial load as part of Quicksand initialization.
This might not be general enough for everything, but I think it's an easier place to start than trying to get behaviors to work with the back button -- at least, without a major overhaul to how behaviors work.