Improve performance and avoid DOM update issues with CollectionView sorting.

Review Request #11666 — Created June 20, 2021 and submitted

Information

Review Board
release-4.0.x

Reviewers

RB.CollectionView made an attempt to rebuild the list of views
whenever there was a sort event. There were some performance and DOM
update issues that came from this, and during the investigation, it
turned out that sort handling was often run unnecessarily.

collection.fetch(), by default, would prepare a list of models, add
them to the collection, sort them silently (avoiding any event
triggers), and then eventually trigger some events (including sort).
By the time RB.CollectionView saw the sort event, the models were
actually in the correct order, yet it would go through and rebuild the
views and re-add to the DOM.

This would have been harmless, but it actually broke <option selected>
children. On initial render, an <option selected> would normally
appear selected in the browser, but the sort would then happen
immediately after that render, and browsers wouldn't preserve the sort
order (tested with Chrome and Firefox).

This manifested as a bug in the New Review Request page's Branches
drop-down, where the last item would be selected despite the default
branch being marked as selected. It wasn't an issue in Review Board 3,
due to Backbone 1.0 only triggering this sort event if we were
actually intentionally sorting (the version we use in Review Board 4,
1.3.3, along with the latest version, 4.0, trigger it under more
conditions, including when populating an empty collection for the first
time without specifying reset: true as an option).

To avoid this, we're now comparing the order of stored views with the
new order. If it differs, we re-build the order of views and re-add to
the DOM. Otherwise, we skip the entire operation.

That not only fixes the <option selected> issue, but it also improves
performance.

There's one more performance improvement in this change, which is the
manner by which views are sorted and removed. We were looping through
the list of views, trying to find one with the proper model, and while
there was logic to reduce the search space, a more efficient approach is
just to keep a map of model IDs to views and use that for all lookup.

Combined, this change improves any views using RB.CollectionView and
avoids browser quirks.

All unit tests passed.

Tested the New Review Request page with a Mercurial repository. Saw that
the correct default branch was selected in the Branches list, instead of
the last branch in the list. Verified this on Chrome and Firefox.

Summary ID
Improve performance and avoid DOM update issues with CollectionView sorting.
`RB.CollectionView` made an attempt to rebuild the list of views whenever there was a `sort` event. There were some performance and DOM update issues that came from this, and during the investigation, it turned out that sort handling was often run unnecessarily. `collection.fetch()`, by default, would prepare a list of models, add them to the collection, sort them silently (avoiding any event triggers), and then eventually trigger some events (including `sort`). By the time `RB.CollectionView` saw the `sort` event, the models were actually in the correct order, yet it would go through and rebuild the views and re-add to the DOM. This would have been harmless, but it actually broke `<option selected>` children. On initial render, an `<option selected>` would normally appear selected in the browser, but the sort would then happen immediately after that render, and browsers wouldn't preserve the sort order (tested with Chrome and Firefox). This manifested as a bug in the New Review Request page's Branches drop-down, where the last item would be selected despite the default branch being marked as `selected`. It wasn't an issue in Review Board 3, due to Backbone 1.0 only triggering this `sort` event if we were actually intentionally sorting (the version we use in Review Board 4, 1.3.3, along with the latest version, 4.0, trigger it under more conditions, including when populating an empty collection for the first time without specifying `reset: true` as an option). To avoid this, we're now comparing the order of stored views with the new order. If it differs, we re-build the order of views and re-add to the DOM. Otherwise, we skip the entire operation. That not only fixes the `<option selected>` issue, but it also improves performance. There's one more performance improvement in this change, which is the manner by which views are sorted and removed. We were looping through the list of views, trying to find one with the proper model, and while there was logic to reduce the search space, a more efficient approach is just to keep a map of model IDs to views and use that for all lookup. Combined, this change improves any views using `RB.CollectionView` and avoids browser quirks.
503231b66032469afc447f52967e5a2ee789f335
Description From Last Updated

Should we also do delete this._viewsByModelID[item.cid]?

daviddavid
david
  1. 
      
  2. reviewboard/static/rb/js/views/collectionView.es6.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Should we also do delete this._viewsByModelID[item.cid]?

  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (5d91be3)