Bug Tracker

Ticket #11415 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

Silently ignore negative CSS values where they are illegal

Reported by: SineSwiper Owned by: SineSwiper
Priority: low Milestone: 1.7.2
Component: effects Version: 1.7.1
Keywords: Cc:
Blocking: Blocked by:

Description

Currently, there is a browser normalization problem with the way negative values are treated between IE and Firefox. Firefox will silently ignore negative CSS values where they are not appropriate. IE will error out.

While "allowing" an illegal value for a property may seem something less to do with jQuery and more to do with the JS programmer, these kind of problems can occur in a single frame of animation, and go unnoticed until a check in IE reveals the problem. One such example is the  jQuery UI bug #8142, which uses an easing of 'easeInOutElastic' to inadvertently cause a negative height problem.

There have been some fixed bugs in the past to take care of these piecemeal and has been a source of dupe bugs, such as #3881, #4413, #5051, #5588, #10294, #10582. However, the css(), prop(), and animate() should go ahead and take care of all of the potential cases for this problem and eliminate all of them at the same time.

Based on my research, the following CSS properties cannot contain negative values, according to the W3C CSS2.1 standard (and certain CSS3 modules):

  • border-spacing
  • padding-top, padding-right, padding-bottom, padding-left, and padding
  • border-top-width, border-right-width, border-bottom-width, border-left-width, and border-width
  • border-top, border-right, border-bottom, border-left, and border (if specifying a width)
  • font-size and font (if specifying a size)
  • width and height
  • min-width, max-width, min-height, and max-height
  • line-height
  • outline-width and outline (if specifying a width)
  • widows

CSS3:

  • column-gap and column-rule-width
  • device-width, device-height, color, color-index, monochrome
  • marquee-play-count
  • background-size, border-image-slice, border-image-width, border-image-outset, box-shadow (only the blur radius)

Change History

comment:1 Changed 3 years ago by dmethvin

  • Owner set to SineSwiper
  • Status changed from new to pending

As a thought exercise, if it were Chrome or Firefox that was picky would you want this behavior? Is IE wrong about the error or is the code wrong in a possibly-benign way and other browsers ignored it?

With .css("border", "1px 1px -1px 4px"), are we supposed to parse the shorthand property and look for negative numbers? A more horrifying fix IMO would be to wrap it all in a try/catch and ignore any hint of an error coming out of the property assignment.

I think our goals for cross-browser consistency should cover valid inputs, not invalid ones. Invalid inputs are errors and should be fixed.

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

comment:2 Changed 3 years ago by SineSwiper

  • Status changed from pending to new

The fact that it's a browser inconsistency does add weight to the problem. However, even if Chrome/Firefox did the same thing, we'd still want to make sure that the automations of animations and the like are as painless as possible. Part of jQuery's magic is being able to do stuff to even zero elements and not giving you an "Object required" error. The same should apply to this problem.

A string like .css("border", "1px 1px -1px 4px") would be unlikely to be used in repeating patterns, and it would be more likely that "border-left-width" would be changed. We can ignore the harder-to-catch and less-likely-to-be-invalid shorthand properties: border, border-TRBL, padding, border-width, font, outline, box-shadow.

Instead, a simple one-liner (with a complex RE) can catch the others. Something like:

if (/^(border-(spacing|-(top|right|bottom|left)(-width)?|image-(slice|width|outset))|padding-(top|right|bottom|left)|font|(font|background)-size|((min|max|device)-)?(width|height)|(outline|column-rule)-width|line-height|widows|column-gap|color(-index)?|monochrome|marquee-play-count)$/.test(prop) && /^\s*-/.test(val)) return $obj;

Heck, even the other ones aren't difficult to parse. I already included above the ones that feature the negative illegal on the front end.

else if (/^(border(-(top|right|bottom|left))?|padding|outline)$/.test(prop) && /(^|\s+)-\d+/.test(val)) return $obj;

That covers all of them except for box-shadow, which likely isn't going to be used, anyway.

comment:3 Changed 3 years ago by scott.gonzalez

We should handle the properties from fxAttrs, and leave the rest to another plugin or the end user.

The implementation would be really small. We're already guarding against this for width and height, so we can just change:

jQuery.each([ "width", "height" ], function( i, prop ) {

to:

jQuery.each( fxAttrs.concat.apply( [], fxAttrs ), function( i, prop ) {

comment:4 Changed 3 years ago by Dave Methvin

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

Fix #11415: Stop non-negative prop undershoot on animation.

This doesn't fix *all* of them (see the ticket for a supposedly complete list) but these were already handy so it was relatively cheap to fix them. If you need others fixed, add a custom step function as was done here. Thanks @scott_gonzalez!

Changeset: 56426261f0ee97d6d2f903d08b137a6f5f2c0916

comment:5 Changed 3 years ago by dmethvin

  • Priority changed from undecided to low
  • Type changed from bug to enhancement
  • Component changed from unfiled to effects
  • Milestone changed from None to 1.7.2

comment:6 Changed 3 years ago by anonymous

Good, good. Thanks for your help on this from both JQ/UI sides.

Note: See TracTickets for help on using tickets.