Remove internal option layout.autosize='initial' (Fixes #537) #577

n-riesco · 2016-05-27T19:46:43Z

Moved initial call to plotAutoSize into Plots.supplyDefaults(gd).
Replaced { autosize: 'initial' } with the flag
gd._fullLayout._initialAutoSizeIsDone.
{ autosize: false } the values of width and height undefined in
gd.layout will be autosized only once.
{ autosize: true } only autosizes the values of width and height
undefined in gd.layout.

Fixes #537

* Commit 5df675a (fix demo/outside legend bug and null data autoscale bug) introduced a guard in plotAutoSize to avoid calling layoutStyles while autosize is set to 'initial'. * Commit ee974d9 (autosizing in shareplots, autosize aspect ratio restrictions and ...) removed the call to layoutStyles but forgot to remove the guard. * This commit removes the guard. * Checked that all the jasmine and image tests still pass.

* Moved initial call to `plotAutoSize` into `Plots.supplyDefaults(gd)`. * Replaced `{ autosize: 'initial' }` with the flag `gd._fullLayout._initialAutoSizeIsDone`. * `{ autosize: false }` the values of width and height undefined in `gd.layout` will be autosized only once. * `{ autosize: true }` only autosizes the values of width and height undefined in `gd.layout`. Fixes plotly#537

* Previous image didn't honour the width and height set in the layout.

* Added test to check `Plotly.newPlot` respects `layout.width` and `layout.height`.

etpinard · 2016-05-30T16:16:55Z

src/plots/plots.js

+        newWidth,
+        newHeight;
+
+    if(typeof gd.emit === 'function') gd.emit('plotly_autosize');


why do we need this?

i.e. that if(typeof gd.emit === 'function')

Otherwise I get errors in the console (perhaps some test are mocking gd with something other than a DOM element?). I will come back to you on this tomorrow.

Good call. Plots.supplyDefaults should work with plain objects.

Could we make this check more robust? I'd vote for using Lib.isPlotDiv.

etpinard · 2016-05-30T16:22:09Z

test/jasmine/tests/plot_api_test.js

+
+            Plotly.plot(gd, data).then(function() {
+                Plotly.newPlot(gd, data, { height: 50 }).then(function() {
+                    expect(gd._fullLayout.height).toBe(50);


we should also check to svg attributes too to make sure that it updated properly.

etpinard · 2016-05-30T16:32:12Z

@n-riesco looks like you're on the right path.

Replaced { autosize: 'initial' } with the flag gd._fullLayout._initialAutoSizeIsDone

Yep. Great idea. 👍

{ autosize: false } the values of width and height undefined in gd.layout will be autosized only once.

Hmm. Is this the current behavior? Shouldn't { autosize: false } lead to layout.width: 700 and layout.height: 400 as per their defaults?

{ autosize: true } only autosizes the values of width and height undefined in gd.layout

Great.

Unfortunately, the plotly.js auto-size routine has very poor test coverage; I'm a little concerned with merging this PR. @n-riesco could make sure that all code paths in plotAutoSize are tested under Plotly.plot and Plotly.relayout?

n-riesco · 2016-05-30T18:31:54Z

On 30/05/16 17:32, Étienne Tétreault-Pinard wrote:

@n-riesco https://github.com/n-riesco looks like you're on the right path.
{ autosize: false } the values of width and height undefined in gd.layout will be autosized only once.

Hmm. Is this the current behavior? Shouldn't |{ autosize: false }| lead to |layout.width: 700| and |layout.height: 400| as per their defaults?

The current behaviour is that if layout.width or layout.height are undefined, then layout.autosize is set to 'initial'.
When layout.autosize is set to 'initial' (and assuming config.fillFrame is false), plotAutosize gets the layout width and height using window.getComputedStyle(gd). Only if this fails, then the default width and height values are used.

Unfortunately, the plotly.js auto-size has very poor test coverage; I'm a little concerned with the PR. @n-riesco https://github.com/n-riesco could make sure that all code paths in |plotAutoSize| https://github.com/plotly/plotly.js/pull/577/files#diff-ad4f76ccd6044ed16514297078e13b84R783 are tested under |Plotly.plot| and |Plotly.relayout|.

OK. I'll add more tests.

alexcjohnson · 2016-05-30T21:49:24Z

@n-riesco this is great, that's definitely where the autosize code belongs.

My only question (which goes along with @etpinard 's concern about test coverage - there are tons of cases to consider) is whether there are cases we'd like autosize to coerce to true - ie if width or height is missing? For example if someone is using element size to determine plot size, but has not explicitly set autosize: true - then something outside the plot (window resize or some event on their page) causes the element size to change, I would expect a simple redraw or relayout to resize the plot too.

This then begs the question (that we probably don't need to delve into now, might be a rabbit hole, but something to keep in mind for later) of whether we actually gain anything with the initial / redraw distinction. Either I'm missing something or it used to work differently, but I don't see that plotAutoSize does anything time-consuming, so what's to stop us from always running it if there's one or more missing dimensions or if autosize is explicitly true? There will be places (relayout?) where we need to track whether anything actually changed to decide how much to do next, but we already do that anyway.

n-riesco · 2016-05-31T09:43:11Z

Below are the tests I'm planning to add:

autosize:false, fillFrame:true
autosize:false, fillFrame:false and frameMargins
autosize:false, fillFrame:false and no frameMargins
autosize:true, fillFrame:true
autosize:true, fillFrame:false and frameMargins
autosize:true, fillFrame:false and no frameMargins

For each test, I'll check fullLayout width and height after plot and after relayout.

These tests should cover the main execution paths in plotAutoSize.

Additional tests (from comments):

check SVG properties in test 'should respect layout.width and layout.height'
test supplyDefaults(gd) can be called with mocked gd.

etpinard · 2016-05-31T16:40:56Z

@n-riesco your plans sounds good!

* Fix bug in `plotAutoSize`, triggered when `autosize` and `frameMagins` are both enabled and `gd` is a plain object. * Ensure `autosize` doesn't set values of width and height smaller than the minimum defined in the corresponding layout attribute.

* Do not do the initial autosize if both config.autosizable and layout.autosize are false.

* Fix bug introduced in the commit for respecting `config.autosizable`.

n-riesco · 2016-06-01T15:45:45Z

While writing the tests for this PR, I noticed that config accepts the attribute autosizable. In master, this attribute is only honoured by plotPolar. Its meaning in plotPolar is:

if config.autosizable: false and layout.autosize: false, then no initial autosize is carried out (if needed, the default values of width and height are used);
if config.autosizable: true, then an initial autosize is carried out (regardless of layout.autosize).

I've updated this PR so that autosizable is always honoured.

n-riesco · 2016-06-01T15:48:16Z

The PR is ready for review again.

etpinard · 2016-06-06T15:49:02Z

src/plots/plots.js

+            factor = 1 - 2 * frameMargins;
+
+        var gdBB;
+        try {


I think we should avoid using try-catch as much as possible - because they are very slow.

Here too, I'd vote for using Lib.isPlotDiv. But, I'm open to other (possible more strict) suggestions.

How about?

var gdBB = fullLayout._container && fullLayout._container.node ? fullLayout._container.node().getBoundingClientRect() : { width: fullLayout.width, height: fullLayout.height };

* Initialise `layout.width` and `layout.height` if `autosize` is false.

n-riesco · 2016-06-07T11:55:56Z

@etpinard About autosizable, I think it should be kept in Plotly v1, and removed in Plotly v2. The reason why I think it should be removed is that I don't think autosizable should be a global setting (different plots should be able to define different values of autosizable).

For Plotly v2, I would suggest to combine autosize and autosizable into a single setting. How about something like this?

{
    autosize: {
        valType: 'enumerated',
        role: 'info',
        values: ['never', 'initial', 'always'],
        description: [
            'Determines how a layout width or height',
            'that has been left undefined by the user',
            'is set.',

            'If *never*, the default values of width and height will be used.',
            'If *initial*, an undefined layout width or height',
            'will be initialized on the first call to plot.'
            'If *always*, an undefined layout width or height',
            'will be set on the first call to plot and subsequent calls to relayout.'
        ].join(' ')
    }
}

n-riesco · 2016-06-07T11:56:45Z

PR ready for review again.

etpinard · 2016-06-07T13:59:30Z

src/plots/plots.js

-        } catch(err) {
-            gdBB = {
+        var gdBB = fullLayout._container && fullLayout._container.node ?
+            fullLayout._container.node().getBoundingClientRect() : {


etpinard · 2016-06-07T14:09:52Z

Thanks @n-riesco . you're almost there.

As mentioned previously, this PR was set uncover a lot of un-tested code. Commits 172d81d and 862578e were no exceptions.

Would you mind a few test cases (one per commit is fine) for the two commits above? To lock their behavior until v2.0.0.

n-riesco · 2016-06-07T17:47:19Z

I've added tests for the commits you suggested and this uncovered an issue with 862578e and another with Lib.isPlotDiv. Both are now fixed and tested.

The PR is ready for review again.

etpinard · 2016-06-09T15:33:37Z

Great. I tested your branch and I couldn't find any edge cases that you might have missed. 🎉

This for sure wins the PR of the week 🏆

I'll make two minor comments on the line diff (nothing major) and then it'll be time to 💃 .

etpinard · 2016-06-09T15:34:35Z

src/plots/layout_attributes.js

@@ -47,11 +47,16 @@ module.exports = {
    autosize: {
        valType: 'enumerated',
        role: 'info',
-        // TODO: better handling of 'initial'
-        values: [true, false, 'initial'],
+        values: [false, true],


we should change the valType to 'boolean' and remove the values field.

etpinard · 2016-06-10T22:17:42Z

test/jasmine/tests/plots_test.js

@@ -326,7 +383,7 @@ describe('Test Plots', function() {
        beforeEach(function(done) {
            gd = createGraphDiv();

-            Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
+            Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], { autosize: true })


@n-riesco , @chriddyp and I are testing the mapbox branch (which is rebased on top of the latest master) in the plot.ly workspace at the moment and we some issues.

Looks like this PR here is backward incompatible and the line ⏫ is a symptom.

More clearly,

Plotly.plot(gd, data);

should go through the plotAutoSize routine on the initial Plotly.plot call.

I believe changing the autosize attribute dflt to true would solve this. What are your thoughts on that?

@etpinard An alternative that would be fully backwards-compatible is that Plot.resize(gd) deletes gd.layout.width and gd.layout.height and calls Plotly.relayout(gd, { autosize: true });. I'll prepare a PR so that you can check that it'd fix the issue in the mapbox branch.

etpinard · 2016-06-13T17:22:28Z

As per-slack conversation with @n-riesco , I'm reverting this PR and will spend more time testing in plot.ly's workspace before the next minor release - to make sure we handle all edge cases in a backward-compatible way.

n-riesco added 5 commits May 27, 2016 20:21

autosize: update baseline image ternary_simple

f9140b0

* Previous image didn't honour the width and height set in the layout.

test: make toimage_test.js stricter

206888b

test: add test for issue plotly#537

9c2a0d9

* Added test to check `Plotly.newPlot` respects `layout.width` and `layout.height`.

etpinard reviewed May 30, 2016
View reviewed changes

etpinard added bug something broken status: in progress labels May 30, 2016

etpinard reviewed May 30, 2016
View reviewed changes

n-riesco added 8 commits June 1, 2016 09:40

test: autosize sets size of main SVG

53d4b90

test: supplyDefaults accepts plain objects

61aa240

test: autosize, fillFrame and frameMargins

7815efe

autosize: respect config.autosizable

7e95d93

* Do not do the initial autosize if both config.autosizable and layout.autosize are false.

test: config.autosizable

370ad8b

autosize: supplyDefaults accepts plain objects

46d7af5

* Fix bug introduced in the commit for respecting `config.autosizable`.

test: account for default autosizable being false

94f450a

etpinard reviewed Jun 6, 2016
View reviewed changes

n-riesco added 6 commits June 7, 2016 10:37

Merge branch 'master' into remove-autosize-initial

b286469

autosize: use Lib.isPlotDiv(gd)

49bb2cf

autosize: expand description of config.autosizable

0c316ba

autosize: revert change of variable name

3ed69b1

autosize: fix condition to call sanitizeMargins

862578e

autosize: restore backwards-compatibility with v1

172d81d

* Initialise `layout.width` and `layout.height` if `autosize` is false.

etpinard reviewed Jun 7, 2016
View reviewed changes

n-riesco added 6 commits June 7, 2016 16:32

test: autosize is backwards-compatible

42a3ed0

test: supplyDefaults calls sanitizeMargins once

de69b3d

lib: ensure isPlotDiv accepts plain objects

ada5323

test: Lib.isPlotLib accepts plain objects

bc1a970

autosize: use Lib.isPlotDiv

6021c43

autosize: ensure sanitizeMargins is called once

e822ff7

etpinard added status: reviewable and removed status: in progress labels Jun 9, 2016

etpinard reviewed Jun 9, 2016
View reviewed changes

autosize: change attribute valType to boolean

fbc01c3

etpinard merged commit 25a8d9c into plotly:master Jun 9, 2016

n-riesco deleted the remove-autosize-initial branch June 10, 2016 08:04

etpinard reviewed Jun 10, 2016
View reviewed changes

This was referenced Jun 13, 2016

Revert "Remove internal option layout.autosize='initial' (Fixes #537)" #633

Merged

Refactor plot autosize #635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove internal option layout.autosize='initial' (Fixes #537) #577

Remove internal option layout.autosize='initial' (Fixes #537) #577

n-riesco commented May 27, 2016

etpinard May 30, 2016

etpinard May 30, 2016

n-riesco May 30, 2016

etpinard Jun 6, 2016

etpinard May 30, 2016

etpinard commented May 30, 2016 •

edited

Loading

n-riesco commented May 30, 2016

alexcjohnson commented May 30, 2016

n-riesco commented May 31, 2016 •

edited

Loading

etpinard commented May 31, 2016

n-riesco commented Jun 1, 2016

n-riesco commented Jun 1, 2016

etpinard Jun 6, 2016

n-riesco Jun 7, 2016

n-riesco commented Jun 7, 2016

n-riesco commented Jun 7, 2016

etpinard Jun 7, 2016

etpinard commented Jun 7, 2016

n-riesco commented Jun 7, 2016

etpinard commented Jun 9, 2016

etpinard Jun 9, 2016

etpinard Jun 10, 2016

n-riesco Jun 12, 2016

n-riesco Jun 12, 2016

etpinard commented Jun 13, 2016

Remove internal option layout.autosize='initial' (Fixes #537) #577

Remove internal option layout.autosize='initial' (Fixes #537) #577

Conversation

n-riesco commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented May 30, 2016 • edited Loading

n-riesco commented May 30, 2016

alexcjohnson commented May 30, 2016

n-riesco commented May 31, 2016 • edited Loading

etpinard commented May 31, 2016

n-riesco commented Jun 1, 2016

n-riesco commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Jun 7, 2016

n-riesco commented Jun 7, 2016

Choose a reason for hiding this comment

etpinard commented Jun 7, 2016

n-riesco commented Jun 7, 2016

etpinard commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 13, 2016

etpinard commented May 30, 2016 •

edited

Loading

n-riesco commented May 31, 2016 •

edited

Loading