From ea25036c148e04c3d1de2ec1e13059b98200f7a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Wed, 21 Mar 2018 14:20:01 +0100 Subject: [PATCH] [FIX] web: graph rendering when in DOM Backported fixes: 1. [IMP] web: GraphView: remove hack to render in DOM - ref. 166319fa4a31c0105d786c1e8b8d0228ff45bd14 - fully backported. This commit backports the fix that removes the hack for rendering graphs (delay in a setTimeout), so that the graph is rendered when it is in the DOM. 2. [IMP] web: restore scroll position - ref. 4e204ed2233ccd36b2eb7d3e56bfa741b9cb3e14 - partially backported: only keeps `on_attach_callback` and `on_detach_callback` in the abstract_controller and in the abstract_renderer. This commit backports the use of the hooks methods `on_attach_callback` and `on_detach_callback` in abstract controller and renderer, so that the controller of any view warns the renderer when it has been attached in (or detach from) the DOM. Also, this commit adds `on_attach_callback` and `on_detach_callback` for the dashboard app, so that the graph view is correctly rendered. Fixes #24092 --- addons/board/static/src/js/dashboard.js | 26 ++++++++ addons/board/static/tests/dashboard_tests.js | 44 +++++++++++++ .../src/js/views/abstract_controller.js | 12 ++++ .../static/src/js/views/abstract_renderer.js | 8 +++ .../src/js/views/form/form_controller.js | 1 + .../src/js/views/graph/graph_renderer.js | 64 ++++++++++++++----- 6 files changed, 139 insertions(+), 16 deletions(-) diff --git a/addons/board/static/src/js/dashboard.js b/addons/board/static/src/js/dashboard.js index 558f7e53961..ee8f720a9df 100644 --- a/addons/board/static/src/js/dashboard.js +++ b/addons/board/static/src/js/dashboard.js @@ -139,6 +139,31 @@ FormRenderer.include({ this._super.apply(this, arguments); this.noContentHelp = params.noContentHelp; this.actionsDescr = {}; + this._boardSubcontrollers = []; // for board: controllers of subviews + }, + /** + * Call `on_attach_callback` for each subview + * + * @override + */ + on_attach_callback: function () { + _.each(this._boardSubcontrollers, function (controller) { + if ('on_attach_callback' in controller) { + controller.on_attach_callback(); + } + }); + }, + /** + * Call `on_detach_callback` for each subview + * + * @override + */ + on_detach_callback: function () { + _.each(this._boardSubcontrollers, function (controller) { + if ('on_detach_callback' in controller) { + controller.on_detach_callback(); + } + }); }, //-------------------------------------------------------------------------- @@ -241,6 +266,7 @@ FormRenderer.include({ hasSelectors: false, }); return view.getController(self).then(function (controller) { + self._boardSubcontrollers.push(controller); return controller.appendTo(params.$node); }); }); diff --git a/addons/board/static/tests/dashboard_tests.js b/addons/board/static/tests/dashboard_tests.js index ab0feaf2c96..4a6f25f5950 100644 --- a/addons/board/static/tests/dashboard_tests.js +++ b/addons/board/static/tests/dashboard_tests.js @@ -3,6 +3,7 @@ odoo.define('board.dashboard_tests', function (require) { var testUtils = require('web.test_utils'); var FormView = require('web.FormView'); +var ListRenderer = require('web.ListRenderer'); var createView = testUtils.createView; @@ -454,5 +455,48 @@ QUnit.test('clicking on a kanban\'s button should trigger the action', function form.destroy(); }); +QUnit.test('subviews are aware of attach in or detach from the DOM', function (assert) { + assert.expect(2); + + // patch list renderer `on_attach_callback` for the test only + testUtils.patch(ListRenderer, { + on_attach_callback: function () { + assert.step('subview on_attach_callback'); + } + }); + + var form = createView({ + View: FormView, + model: 'board', + data: this.data, + arch: '
' + + '' + + '' + + '' + + '' + + '' + + '
', + mockRPC: function (route) { + if (route === '/web/action/load') { + return $.when({ + res_model: 'partner', + views: [[4, 'list']], + }); + } + return this._super.apply(this, arguments); + }, + archs: { + 'partner,4,list': + '', + }, + }); + + assert.verifySteps(['subview on_attach_callback']); + + // restore on_attach_callback of ListRenderer + testUtils.unpatch(ListRenderer); + + form.destroy(); +}); }); diff --git a/addons/web/static/src/js/views/abstract_controller.js b/addons/web/static/src/js/views/abstract_controller.js index 3bdf8ab08ca..6a3f9a3bbaa 100644 --- a/addons/web/static/src/js/views/abstract_controller.js +++ b/addons/web/static/src/js/views/abstract_controller.js @@ -62,6 +62,18 @@ var AbstractController = Widget.extend({ } return this._super.apply(this, arguments); }, + /** + * Called each time the controller is attached into the DOM. + */ + on_attach_callback: function () { + this.renderer.on_attach_callback(); + }, + /** + * Called each time the controller is detached from the DOM. + */ + on_detach_callback: function () { + this.renderer.on_detach_callback(); + }, //-------------------------------------------------------------------------- // Public diff --git a/addons/web/static/src/js/views/abstract_renderer.js b/addons/web/static/src/js/views/abstract_renderer.js index 935fcc6643e..2394fb31a64 100644 --- a/addons/web/static/src/js/views/abstract_renderer.js +++ b/addons/web/static/src/js/views/abstract_renderer.js @@ -36,6 +36,14 @@ return Widget.extend({ this.$el.addClass(this.arch.attrs.class); return $.when(this._render(), this._super()); }, + /** + * Called each time the renderer is attached into the DOM. + */ + on_attach_callback: function () {}, + /** + * Called each time the renderer is detached from the DOM. + */ + on_detach_callback: function () {}, //-------------------------------------------------------------------------- // Public diff --git a/addons/web/static/src/js/views/form/form_controller.js b/addons/web/static/src/js/views/form/form_controller.js index 2f037a7a005..b21f9d435f8 100644 --- a/addons/web/static/src/js/views/form/form_controller.js +++ b/addons/web/static/src/js/views/form/form_controller.js @@ -99,6 +99,7 @@ var FormController = BasicController.extend({ * @todo convert to new style */ on_attach_callback: function () { + this._super.apply(this, arguments); this.autofocus(); }, /** diff --git a/addons/web/static/src/js/views/graph/graph_renderer.js b/addons/web/static/src/js/views/graph/graph_renderer.js index 74443843405..58ce4b2fc39 100644 --- a/addons/web/static/src/js/views/graph/graph_renderer.js +++ b/addons/web/static/src/js/views/graph/graph_renderer.js @@ -43,6 +43,30 @@ return AbstractRenderer.extend({ nv.utils.offWindowResize(this.to_remove); this._super(); }, + /** + * The graph view uses the nv(d3) lib to render the graph. This lib requires + * that the rendering is done directly into the DOM (so that it can correctly + * compute positions). However, the views are always rendered in fragments, + * and appended to the DOM once ready (to prevent them from flickering). We + * here use the on_attach_callback hook, called when the widget is attached + * to the DOM, to perform the rendering. This ensures that the rendering is + * always done in the DOM. + * + * @override + */ + on_attach_callback: function () { + this._super.apply(this, arguments); + this.isInDOM = true; + this._renderGraph(); + }, + /** + * @override + */ + on_detach_callback: function () { + this._super.apply(this, arguments); + this.isInDOM = false; + }, + //-------------------------------------------------------------------------- // Private //-------------------------------------------------------------------------- @@ -51,10 +75,9 @@ return AbstractRenderer.extend({ * Render the chart. * * Note that This method is synchronous, but the actual rendering is done - * asynchronously (in a setTimeout). The reason for that is that nvd3/d3 - * needs to be in the DOM to correctly render itself. So, we trick Odoo by - * returning immediately, then wait a tiny interval before actually - * displaying the data. + * asynchronously. The reason for that is that nvd3/d3 needs to be in the + * DOM to correctly render itself. So, we trick Odoo by returning + * immediately, then we render the chart when the widget is in the DOM. * * @returns {Deferred} The _super deferred is actually resolved immediately */ @@ -76,21 +99,15 @@ return AbstractRenderer.extend({ "Try to add some records, or make sure that " + "there is no active filter in the search bar."), })); - } else { - var self = this; - setTimeout(function () { - self.$el.empty(); - var chart = self['_render' + _.str.capitalize(self.state.mode) + 'Chart'](); - if (chart && chart.tooltip.chartContainer) { - self.to_remove = chart.update; - nv.utils.onWindowResize(chart.update); - chart.tooltip.chartContainer(self.el); - } - }, 0); + } else if (this.isInDOM) { + // only render the graph if the widget is already in the DOM (this + // happens typically after an update), otherwise, it will be + // rendered when the widget will be attached to the DOM (see + // 'on_attach_callback') + this._renderGraph(); } return this._super.apply(this, arguments); }, - /** * Helper function to set up data properly for the multiBarChart model in * nvd3. @@ -327,6 +344,21 @@ return AbstractRenderer.extend({ chart(svg); return chart; }, + /** + * Renders the graph according to its type. This function must be called + * when the renderer is in the DOM (for nvd3 to render the graph correctly). + * + * @private + */ + _renderGraph: function () { + this.$el.empty(); + var chart = this['_render' + _.str.capitalize(this.state.mode) + 'Chart'](); + if (chart && chart.tooltip.chartContainer) { + this.to_remove = chart.update; + nv.utils.onWindowResize(chart.update); + chart.tooltip.chartContainer(this.el); + } + }, }); });