New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.width(value) sets incorrect width value if block has border and value specified in ems and box-sizing:border-box #1712

Closed
mgol opened this Issue Oct 16, 2014 · 3 comments

Comments

Projects
None yet
4 participants
@mgol
Member

mgol commented Oct 16, 2014

Originally reported by tde: http://bugs.jquery.com/ticket/14038

API documentation specifies: "Note that .width("value") sets the content width of the box regardless of the value of the CSS box-sizing property."

There are two key factors leading to this bug

"box-sizing:border-box"
"border:1px solid black"
width specified in ems
Code that reproduces this bug

<html>
    <head>
        <style>
            #container {width:100%;}
            #box {border:1px solid black; margin:0 auto; box-sizing:border-box;}
        </style>
        <script type="text/javascript" src="http://code.jquery.com/jquery-2.0.1.js"></script>
        <script>
            var $ = jQuery.noConflict();
            $(document).ready(function() {
                $('#box').width('10em'); // width should be 10em, but jquery set it to 12em
            });
        </script>
    </head>
    <body>
        <div id="container">
            <div id="box">
                <p>some text</p>
            </div>
        </div>
    </body>
</html>

@mgol mgol added the Bug label Oct 16, 2014

@mgol mgol added this to the 3.0.0 milestone Oct 16, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 16, 2014

Member

Original discussion:

Changed 16 months ago by tde@…

Same behavior with JQuery 2.0.2

comment:2 Changed 16 months ago by dmethvin

Hi, can you convert your example inline here to an example on jsfiddle.net?

comment:3 Changed 16 months ago by m_gol

Owner set to tde@…
Status changed from new to pending
comment:4 Changed 16 months ago by tde@…

Status changed from pending to new
test case - http://jsfiddle.net/JqmQ6/1/

comment:5 follow-up: ↓ 6 Changed 16 months ago by timmywil

Status changed from new to closed
Resolution set to notabug
Respecting border-box is outside the scope of width, due to the fact that we have specific methods that account for padding, margin, and border (innerWidth/outerWidth(true|false)/width). However, you can use .css('width') instead: http://jsfiddle.net/timmywil/JqmQ6/3/

comment:6 in reply to: ↑ 5 Changed 16 months ago by anonymous

Replying to timmywil:

Respecting border-box is outside the scope of width, due to the fact that we have specific methods that account for padding, margin, and border (innerWidth/outerWidth(true|false)/width). However, you can use .css('width') instead: http://jsfiddle.net/timmywil/JqmQ6/3/

Clearly a bug. You just provided workaround. But If I want block .with('10em) - it should be 10em, not 12em (if border is 1px) and not 14em (if border is 2px) and not 16em (if border is 3px) etc. Open your mind and reconsider this shit. Documentation clearly states - "Note that .width("value") sets the content width of the box regardless of the value of the CSS box-sizing property."

comment:7 Changed 16 months ago by timmywil

Status changed from closed to reopened
Resolution notabug deleted
Valid. The documentation you quote is the point I was making. Nevertheless, this seems to be an edge case caused by combining a non-px like em with border-box.

comment:8 Changed 16 months ago by timmywil

Owner changed from tde@… to timmywil
Priority changed from undecided to high
Status changed from reopened to assigned
Component changed from unfiled to css
Milestone changed from None to 1.11/2.1
comment:9 Changed 16 months ago by timmywil

So, afaict, there are two routes we can take.

  1. If the unit is not pixels, skip width/height augmentation completely. This is historically how we've dealt with most problems involving non-px units (with some exceptions). This also has the advantage of being a small fix.
  2. We could do something similar to the main tween and convert all necessary values to the set unit. This is more accurate, but could get very expensive as it could potentially require a loop for width, border, margin, and padding in order to unify the units for value adjustment each time width is set (imagine animating width).

Thoughts?

comment:10 Changed 16 months ago by gibson042

I'm in favor of exploring the size and performance impact of option 2, but it may take a while to do so. A stopgap in the meantime might be reasonable, but option 1 seems like overkill.

comment:11 Changed 16 months ago by timmywil

Perhaps we could ensure the units are the same before the setPositiveNumber call. If they're not, set the value as-is.

comment:12 Changed 9 months ago by Joshua Tausz

I discovered a similar case which expands the scope of this bug, using jquery 1.10.2, and Chrome 29, on Windows 7 Enterprise.

This bug occurs with other types of unit miss-match.

In the example provided with the initial report, set a width of 50%, and padding px of 10, and no border. This will result in a width of 70%, very different behavior from expected.

Setting a padding of 2em returns a width of 74%. This which would seem to indicate that 1em is translated to 12px before adding those 12px to the width as 12% each.

I was able to use Chrome's built-in debugger to trace through some of the code.

In file jqery-1.10.js, the function agumentWidthOrHeight, returns -20, to reflect that padding is set to 10px on either side, but there is no unit attached.

Specifically, this occurs on line 7201.

val -= jQuery.css(elem, "padding" + cssExpand [ i ], true, styles);

This is already part of a loop used to add the various values together, so type checking/conversion could be handled inside this existing loop, if the coder were to go with comment:9's option 2.

This value of -20 is used in function setPositiveNumber, under the value "subtract", on line 7179:

Math.max( 0, matches[ 1 ] - ( subtract || 0 )  ) + matches [ 2 ] || "px" )

I suggest using type conversion, and transmitting the type with the value reported from function agumentWidthOrHeight, so that type conversion can be done on or before line 7179.

With the additional style miss-matches, which can cause further issues, I suggest that this is no longer as much of an edge-case.

comment:13 Changed 8 months ago by dmethvin

Milestone changed from 1.11/2.1 to 1.11.1/2.1.1
comment:14 Changed 7 months ago by timmywil

Milestone changed from 1.11.1/2.1.1 to 1.12/2.2

Member

mgol commented Oct 16, 2014

Original discussion:

Changed 16 months ago by tde@…

Same behavior with JQuery 2.0.2

comment:2 Changed 16 months ago by dmethvin

Hi, can you convert your example inline here to an example on jsfiddle.net?

comment:3 Changed 16 months ago by m_gol

Owner set to tde@…
Status changed from new to pending
comment:4 Changed 16 months ago by tde@…

Status changed from pending to new
test case - http://jsfiddle.net/JqmQ6/1/

comment:5 follow-up: ↓ 6 Changed 16 months ago by timmywil

Status changed from new to closed
Resolution set to notabug
Respecting border-box is outside the scope of width, due to the fact that we have specific methods that account for padding, margin, and border (innerWidth/outerWidth(true|false)/width). However, you can use .css('width') instead: http://jsfiddle.net/timmywil/JqmQ6/3/

comment:6 in reply to: ↑ 5 Changed 16 months ago by anonymous

Replying to timmywil:

Respecting border-box is outside the scope of width, due to the fact that we have specific methods that account for padding, margin, and border (innerWidth/outerWidth(true|false)/width). However, you can use .css('width') instead: http://jsfiddle.net/timmywil/JqmQ6/3/

Clearly a bug. You just provided workaround. But If I want block .with('10em) - it should be 10em, not 12em (if border is 1px) and not 14em (if border is 2px) and not 16em (if border is 3px) etc. Open your mind and reconsider this shit. Documentation clearly states - "Note that .width("value") sets the content width of the box regardless of the value of the CSS box-sizing property."

comment:7 Changed 16 months ago by timmywil

Status changed from closed to reopened
Resolution notabug deleted
Valid. The documentation you quote is the point I was making. Nevertheless, this seems to be an edge case caused by combining a non-px like em with border-box.

comment:8 Changed 16 months ago by timmywil

Owner changed from tde@… to timmywil
Priority changed from undecided to high
Status changed from reopened to assigned
Component changed from unfiled to css
Milestone changed from None to 1.11/2.1
comment:9 Changed 16 months ago by timmywil

So, afaict, there are two routes we can take.

  1. If the unit is not pixels, skip width/height augmentation completely. This is historically how we've dealt with most problems involving non-px units (with some exceptions). This also has the advantage of being a small fix.
  2. We could do something similar to the main tween and convert all necessary values to the set unit. This is more accurate, but could get very expensive as it could potentially require a loop for width, border, margin, and padding in order to unify the units for value adjustment each time width is set (imagine animating width).

Thoughts?

comment:10 Changed 16 months ago by gibson042

I'm in favor of exploring the size and performance impact of option 2, but it may take a while to do so. A stopgap in the meantime might be reasonable, but option 1 seems like overkill.

comment:11 Changed 16 months ago by timmywil

Perhaps we could ensure the units are the same before the setPositiveNumber call. If they're not, set the value as-is.

comment:12 Changed 9 months ago by Joshua Tausz

I discovered a similar case which expands the scope of this bug, using jquery 1.10.2, and Chrome 29, on Windows 7 Enterprise.

This bug occurs with other types of unit miss-match.

In the example provided with the initial report, set a width of 50%, and padding px of 10, and no border. This will result in a width of 70%, very different behavior from expected.

Setting a padding of 2em returns a width of 74%. This which would seem to indicate that 1em is translated to 12px before adding those 12px to the width as 12% each.

I was able to use Chrome's built-in debugger to trace through some of the code.

In file jqery-1.10.js, the function agumentWidthOrHeight, returns -20, to reflect that padding is set to 10px on either side, but there is no unit attached.

Specifically, this occurs on line 7201.

val -= jQuery.css(elem, "padding" + cssExpand [ i ], true, styles);

This is already part of a loop used to add the various values together, so type checking/conversion could be handled inside this existing loop, if the coder were to go with comment:9's option 2.

This value of -20 is used in function setPositiveNumber, under the value "subtract", on line 7179:

Math.max( 0, matches[ 1 ] - ( subtract || 0 )  ) + matches [ 2 ] || "px" )

I suggest using type conversion, and transmitting the type with the value reported from function agumentWidthOrHeight, so that type conversion can be done on or before line 7179.

With the additional style miss-matches, which can cause further issues, I suggest that this is no longer as much of an edge-case.

comment:13 Changed 8 months ago by dmethvin

Milestone changed from 1.11/2.1 to 1.11.1/2.1.1
comment:14 Changed 7 months ago by timmywil

Milestone changed from 1.11.1/2.1.1 to 1.12/2.2

@Gerst20051

This comment has been minimized.

Show comment
Hide comment
@Gerst20051

Gerst20051 Oct 6, 2015

👍 just ran into this bug (is it a bug or expected behavior?) but as pointed out above setting the width using .css('width', width); works as expected.

Gerst20051 commented Oct 6, 2015

👍 just ran into this bug (is it a bug or expected behavior?) but as pointed out above setting the width using .css('width', width); works as expected.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 6, 2015

Member

@Gerst20051 We've accepted it as a bug and (currently) plan to fix it for 3.0.0. We still have a lot to do for 3.0.0 though!

Member

mgol commented Oct 6, 2015

@Gerst20051 We've accepted it as a bug and (currently) plan to fix it for 3.0.0. We still have a lot to do for 3.0.0 though!

timmywil added a commit to timmywil/jquery that referenced this issue Nov 6, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Nov 9, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Nov 9, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Nov 9, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Nov 9, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Nov 9, 2015

@timmywil timmywil closed this in 75b3cdd Nov 9, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.