Bug Tracker

Ticket #11004 (closed bug: fixed)

Opened 3 years ago

Last modified 2 years ago

getWH incorrectly removes padding and border width when box-sizing is border-box

Reported by: jpate Owned by: mikesherov
Priority: low Milestone: 1.8
Component: css Version: 1.7.1
Keywords: 1.8-discuss Cc:
Blocking: Blocked by:

Description (last modified by dmethvin) (diff)

tested in jquery 1.6.4, 1.7.1

tested in chrome 15.0.874.121 m win7 x64

to reproduce: create a div with the following style: box-sizing:border-box;width:50;padding:10px;margin:10px solid red

call jQuery(elem)animate({width:50;height:50})

the box will jump to the wrong starting size, then animate

test case:  http://jsfiddle.net/GUfuf/6/

patch:  https://github.com/jamie-pate/jquery/commit/72bd44aebb10a66f2d7d51f87bc398f5d6541b95#src/css.js

Change History

comment:1 Changed 3 years ago by rwaldron

  • Cc mikesherov added
  • Component changed from unfiled to css

@mikesherov can you take a look at this?

comment:3 Changed 3 years ago by jpate

Tests completed in 120276 milliseconds. 5839 tests of 5839 passed, 0 failed.

Not sure what to do about ie6/7 though, since they don't support box-sizing in a sane way (and how to detect ie 8/9 running in ie7 standards mode)

comment:4 Changed 3 years ago by timmywil

#11009 is a duplicate of this ticket.

comment:5 follow-up: ↓ 10 Changed 3 years ago by timmywil

  • Status changed from new to closed
  • Resolution set to invalid

We have 3 methods to manually account for padding, border, and margin. To retrieve a border-box width, use .outerWidth().

comment:6 Changed 3 years ago by anonymous

The problem is that animate() does not use these methods. Animations glitch out! This needs to be automatic

comment:7 Changed 3 years ago by jpate

The problem is that animate() does not use these methods. Animate uses the cssHooks for 'width' and 'height' to determine the starting size of an element.

The actual size of an element is different when box-sizing:border-box is used. the CORRECT size does not include padding or border! Therefore the starting size of the animation is wrong and it looks glitchy. Please take a look at the test case(s) I have provided.

comment:8 Changed 3 years ago by mikesherov

  • Status changed from closed to reopened
  • Resolution invalid deleted

.width() and getWH always returns content width regardless of box-sizing.

Note the warning on  http://api.jquery.com/width/

"Note that .width() will always return the content width, regardless of the value of the CSS box-sizing property" This is intended behavior for .width(). However, if animation needs to do something differently, is should probably be using .css('width').

I'll take a look.

comment:9 Changed 3 years ago by mikesherov

  • Owner set to mikesherov
  • Status changed from reopened to assigned

comment:10 in reply to: ↑ 5 Changed 3 years ago by jpate

Here is my new proposed patch, it fixes the problem by swapping out the box model when calculating the starting point of animations. Tested in ie8/9, opera 11.60, firefox 8, firefox 3.6, chrome 15.0.874.121 m, safari 5.1.2 (all on w7 x64)

 https://github.com/jamie-pate/jquery/tree/animate-swap-box-sizing-content-box

i've also added the fix to my test case at  http://moscore.com/testbed/jquery/test_animate_box_sizing.html

Last edited 3 years ago by jpate (previous) (diff)

comment:11 Changed 3 years ago by mikesherov

  • Priority changed from undecided to low
  • Milestone changed from None to 1.next

there will be a patch for css vendor prefix names shortly. At that point, I think we can figure out how to move forward on this. I'm not a fan of using swap because it's slow, but I think there is a solution here.

comment:12 follow-up: ↓ 13 Changed 3 years ago by mikesherov

css3 prefix patch:  https://github.com/jquery/jquery/pull/639

I am still going to be looking for a more comprehensive solution to this bug.

comment:13 in reply to: ↑ 12 Changed 3 years ago by jpate

Replying to mikesherov:

css3 prefix patch:  https://github.com/jquery/jquery/pull/639

I am still going to be looking for a more comprehensive solution to this bug.

is it that slow on 2 properties? :) I guess testing is in order

Last edited 3 years ago by jpate (previous) (diff)

comment:14 Changed 3 years ago by jpate

1000 iterations: $(div).stop(true).animate(size++,size++)

1.7.2pre_swap_patch 2325ms

1.7.1 1222ms

1.7.2pre_getwh_patch 1606ms

oof

Last edited 3 years ago by jpate (previous) (diff)

comment:15 Changed 3 years ago by jpate

Unfortunately swap works differently in chrome/safari vs android mobile, swapping "WebkitBoxSizing" breaks chrome/safari box sizing, but is required to fix this issue in android mobile!

frustrating

turns out this is a 'bug' in swap(), WebkitBoxSizing and BoxSizing are actually the same object in webkit, changing one causes the other to change and the new value is written to the old object

Last edited 3 years ago by jpate (previous) (diff)

comment:16 Changed 3 years ago by mikesherov

jpate, what's I'm currently thinking is that .css('width') should take into account box-sizing while leaving .width() to still return the .width() property.

All other solutions I've seen or come up with are slow hacks. Please bear with me as there are a few other tickets that need to get integrated first to make this happen.

comment:17 Changed 3 years ago by timmywil

  • Cc mikesherov removed
  • Keywords 1.8-discuss added

Please vote here on whether .css('width') and .width() should return different things based on border-box. We can safely say that .width() should never take box-sizing into account when calculating width. width/innerWidth/outerWidth/outerWidth(true) provide all of the necessary options for adding/subtracting padding/border/margin. However, if .css('width') takes box-sizing into account, it will solve some issues (for instance, when animating), but will no longer be the same as .width().

comment:18 Changed 3 years ago by timmywil

+1, The change makes sense to me, but only if we can keep it between 0-30 bytes.

comment:19 Changed 3 years ago by mikesherov

  • Description modified (diff)

+1, I'll handle this. Should actually be pretty small.

comment:20 Changed 3 years ago by dmethvin

  • Description modified (diff)

+1, I don't *want* to deal with this but I think we *have* to.

comment:21 Changed 3 years ago by jaubourg

+1

comment:22 Changed 3 years ago by rwaldron

+1, As long as it's the right thing to do

Last edited 3 years ago by rwaldron (previous) (diff)

comment:23 Changed 3 years ago by mikesherov

  • Milestone changed from 1.next to 1.8

comment:24 Changed 2 years ago by mikesherov

So, this bug in Firefox puts a wrinkle in things:  https://bugzilla.mozilla.org/show_bug.cgi?id=520992

I can either now make a support test for this behavior, or I can modify the behavior of the fix I'm doing for 10413 to also respect box-sizing for .css('width'). I'm choosing the latter for now, as it'll be smaller overall.

comment:25 Changed 2 years ago by mikesherov

I have the solution ready to go that also deals with  https://bugzilla.mozilla.org/show_bug.cgi?id=520992 I'm just now trying to find a way to make it smaller. It ended up having to be built on top of #10413.

comment:27 Changed 2 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.