Resizable: Fix issue with border-box

[razvanphp]

This is a duplicate of PR #1451, due to lack of CLA signing success.

If you apply .resizeable() to an element which has border-box sizing and
this element has a border then the container will "shrink" by this
border size immediately as the resizing drag begins.

Unit tests all passing. Thank you @ilanc

Fixes #8932

[razvanphp]

@munkex The current PR is meant to be merged into master, so it will be available for all future builds.

Unfortunately it is not merged yet, the complete lack of interest of the maintainers for this (2 years) old bug and (3 months) old PR, made me switch do a different library and remove jquery-ui dependency completely from my project.

I recommend you to take a look here: https://github.com/taye/interact.js

[AllNamesRTaken]

@razvanphp Great fix and thanks for trying to get it in. Should be merged asap.

[jzaefferer]

Thank you for the (adapted) contribution. Could you add a unit test, that covers the described border-box case?

[jlowcs]

Is this fix going to be part of 1.12 release ?

[jzaefferer]

@jlowcs only if this PR (or a replacement) gets merged in time, which depends on someone writing a unit test, as mentioned above. Would you be interested in helping out with that?

[monsterlane]

Is this going to be merged into 1.12?

[scottgonzalez]

@monsterlane Please read the existing comments which state the requirements (we're waiting on a unit test). There is no chance of this being included in 1.12.0 at this point because we're already in the RC phase of the release process.

[monsterlane]

Interact.js is starting to look better by the minute. It's pretty sad that someone took their personal time to fix a 2y old bug in your code and someone from your team can't write a unit test for it when multiple people have said the fix would be useful to them.

[scottgonzalez]

It's pretty sad that someone took their personal time to fix a 2y old bug in your code and someone from your team can't write a unit test for it

Do you realize that you're asking someone from the team to take their personal time to write the unit test?

[monsterlane]

That's exactly what I wrote isn't it? If you're happy with the way this issue is being handled I'll join @razvanphp and start planning my migration away from your library.

[scottgonzalez]

I'm not happy that issues stay open for years. What I'm trying to explain to you is that there is no magic here. There's not some team employed by a company to write all this code. There's not some group of developers that have unlimited time to work on these projects. Your complaint is that you don't want to take your personal time to write the needed code, but that some other person should be required to spend their personal time writing the needed code.

Interestingly, what happens here is that you get angry that someone devoted enough time to fixing bugs and implementing features that they now have commit access, but they haven't devoted time to this one specific bug. I could turn this around and say that it's sad that so many people want this but none of them are willing to write a unit test for it.

[monsterlane]

I have no idea how to write a unit test and frankly almost every interaction I've had with your team has been met with disdain. Link me a guide on how to write a unit test for your library and I'll give it a shot.

[arschmitz]

@monsterlane If you felt disdain from our side i'm sorry that was certainly never our intention and i can assure you was not @scottgonzalez intention here. You must however realize your comments were very aggressive in nature when dealing with an all volunteer team.

We will be happy to help and point you in the right direction to write a test. For all our unit tests are written using QUnit https://qunitjs.com/. We also use jQuery Simulate when needed https://github.com/jquery/jquery-simulate. You can see our current tests for resizable here https://github.com/jquery/jquery-ui/tree/master/tests/unit/resizable they may be usefull for inspiration.

If you need any further help with this your best bet is probably to catch us on IRC in #jqueryui-dev

[arschmitz]

@monsterlane also if you working on the test, this PR needs to be rebased against current master it currently conflicts.

[monsterlane]

I apologize if my comments came off as aggressive. I've spoken with several of your members over Twitter and if you'd read those threads you'd understand why. I've tried to help in the past and I will give this a shot, but I'm not convinced it won't be rejected for some asinine reason as my attempts in the past were as well. That's why I didn't offer to help initially. I understand you have high standards, and you should, but it often seems if someone from your foundation doesn't do it you won't accept anything else.

Since this issue only exists when a certain CSS rule is applied to the resizable container where is the best place to apply that rule without effecting the other tests?

This change applies changes to resizing and alsoResize, are those the only set of tests I need to recreate with the additional CSS rule?

[arschmitz]

@monsterlane I would setup a new qunit module for this test and within the module definition use the beforeEach and afterEach and with in these methods use the jQuery css method to set it before and unset it after. http://api.qunitjs.com/QUnit.module/

[monsterlane]

Thanks.

[monsterlane]

When you say new module, do you mean a new file? Or what file would this go in?

Is this similar to what you're looking for? I'll include all the related ones if this is the correct format.

QUnit.test( "n box-sizing (#1563)", function( assert ) {
    assert.expect( 4 );

    var handle = ".ui-resizable-n", target = $( "#resizable1" );

    target.css( { "box-sizing": "border-box", "border": "1px solid blue" } );
    target.resizable( { handles: "all" } );

    testHelper.drag( handle, 0, -50 );
    assert.equal( target.outerHeight( ), 150, "compare height" );

    testHelper.drag( handle, 0, 50 );
    assert.equal( target.outerHeight( ), 100, "compare height" );

    assert.equal( target[ 0 ].style.left, "", "left should not be modified" );
    assert.equal( target[ 0 ].style.width, "", "width should not be modified" );
} );

[arschmitz]

@monsterlane no by module i mean the QUnit.module method i linked to. This would go in the core.js file in the resizable folder of the unit tests.

As for the code pasted parts of it look essentially right however the css call needs to be part of the module beforeEach and afterEach

[scottgonzalez]

For a single test of box-sizing, I don't think we need to add a module. The test can just modify the CSS as @monsterlane has. We have plenty of tests where there's some setup in the test itself.

[monsterlane]

@arschmitz I've added this to the bottom of the core file, is this correct? Or should I use the stacking format?

QUnit.module( "resizable: box-sizing (#1563)", {
    beforeEach: function( ) {
        var target = $( "#resizable1" );

        target.css( { "box-sizing": "border-box", "border": "1px solid blue" } );
    },
    afterEach: function( ) {
        var target = $( "#resizable1" );

        target.css( { "box-sizing": "", "border": "" } );
    }
} );

QUnit.test( "n box-sizing (#1563)", function( assert ) {
    assert.expect( 4 );

    var handle = ".ui-resizable-n", target = $( "#resizable1" ).resizable( { handles: "all" } );

    testHelper.drag( handle, 0, -50 );
    assert.equal( target.outerHeight( ), 150, "compare height" );

    testHelper.drag( handle, 0, 50 );
    assert.equal( target.outerHeight( ), 100, "compare height" );

    assert.equal( target[ 0 ].style.left, "", "left should not be modified" );
    assert.equal( target[ 0 ].style.width, "", "width should not be modified" );
} );

Which tests do you want for this? Looking at the list these are the ones I was thinking you'd want: n, s, e, w, ne, se, sw, nw, alsoResize.

Since I didn't create this PR I can't rebase it. I can send a new PR off master, when I do so how can I credit the original author of this patch?

[arschmitz]

@monsterlane That is exactly what i was talking about. Do you think we need each of the handle options since there is no change in the logic for this bit based on which handle? I think a resize one makes sense though.

At this point it would probably make sense for you to fork this PR and submit a new PR including your test so we can properly review and comment on lines. Just make sure to reference this PR in your new PR

You don't need to worry about crediting them because you wont change the author of the original commits just add your own ( this PR is infact an updated version of yet someone else's PR ) Just make sure you reference this PR in your commit message and in your description when you open the PR and your good.

[monsterlane]

@arschmitz ok I can do that. When I added the resize test it failed (width vs outerWidth) I'll need to make a few other changes before I submit an updated PR. Thanks for your help with this.

[monsterlane]

@arschmitz sorry for the delay, I'm a bit confused by this bit of code in _mouseStart:

        this.size = this._helper ? {
                width: this.helper.width(),
                height: this.helper.height()
            } : {
                width: el.width(),
                height: el.height()
            };

        this.originalSize = this._helper ? {
                width: el.outerWidth(),
                height: el.outerHeight()
            } : {
                width: el.width(),
                height: el.height()
            };

        this.sizeDiff = {
            width: el.outerWidth() - el.width(),
            height: el.outerHeight() - el.height()
        };

Why is it comparing the outerWidth to width to detect if the element has changed size?

[monsterlane]

We're moving to https://github.com/taye/interact.js as well. Our team is tired of hacking patches into jQuery UI for bugs that don't seem important to contributors of this project. I understand contributors are donating their personal time to work on this but the lack of communication is impeding our ability to move forward.

[arschmitz]

@monsterlane I apologize for missing your update here i some how just missed that you had left a new comment. Im sorry your frustrated and feel the need to move to a different lib but am still happy to help you with this patch if you are interested.

Just post back if you are and ill get you any information or help you need. If you do have a question and I don't respond in a day or so feel free to ping again to make sure i don't miss it I get hundreds of github emails a day and its easy for one to fall through the cracks sometimes.

[scottgonzalez]

Closing due to inactivity.

[razvanphp]

jQuery policy: ignore & keep PR open until it's old enough to get closed unfixed.

[monsterlane]

@razvanphp yep. How are you liking Interact? We're loving it. Instead of having to redefine core methods to change functionality like we did with jQuery UI (draggable on a zoomed element for example) we can define our own move/rotate handlers and Interact manages the gestures.

As of today we've officially removed all of jQuery UI from our develop branch. Bootstrap + plugins, Interact.js, Sortable.js, Typeahead.js, and I wrote our own version of Selectable.