Closed Bug 1103000 Opened 9 years ago Closed 9 years ago

Regression: min-height no longer has the unitless length quirk

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
32 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: estellnb, Assigned: bzbarsky)

References

(
URL
)

Details

Attachments

(1 file)

Update the unitless length quick whitelist to match spec changes
10.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) KHTML/4.13.3 (like Gecko) Konqueror/4.13



Actual results:

Regression: the css property min-height can no more be set via Javascript and style.minHeight though an object with the respective name still exists when listing all objects of a div. 
example: see the Javascript code of http://www.elstel.org/index.html.en - The link collection at the left beginning with Rescue the Rainforest should move down to the bottom of the page.
It still works with Konqueror.
The page is doing:

    AktionenContent.style.minHeight = height(Artikel) - height(AktionenFooter); 

This is not a valid CSS min-height value, because it doesn't include the unit.  The correct way to do this would be:

    AktionenContent.style.minHeight = (height(Artikel) - height(AktionenFooter)) + "px";

The page _is_ in quirks mode, but https://quirks.spec.whatwg.org/#the-unitless-length-quirk doesn't apply to the min-height property.

That said, why does it not apply there?  Does IE not do it?  Because WebKit/Blink sure do, and Gecko used to until bug 774122 was fixed.
Blocks: 774122
Status: UNCONFIRMED → NEW
Component: General → CSS Parsing and Computation
Ever confirmed: true
Flags: needinfo?(zcorpan)
Summary: Regression: style.minHeight no more known by Javascript → Regression: min-height no longer has the unitless length quirk
Thanks a lot for your quick and precise answer!

The page was in deed never sufficiently tested with IE (newer versions no more run with wine). However elder versions of Firefox seemed not to mind the missing 'px' addition. I will have to scan my whole page now for errors like this.

By the way is there some means in order to convert other units like em or ex to px by Javascript?
> However elder versions of Firefox seemed not to mind the missing 'px' addition.

Right, because they did the unitless length quirk for pretty much all properties.

> By the way is there some means in order to convert other units like em or ex to px by
> Javascript?

Sort of.  You can create a div with the relevant dimensions and then measure its size...
(In reply to Please do not ask for reviews for a bit [:bz] from comment #2)
> That said, why does it not apply there?  Does IE not do it?  Because
> WebKit/Blink sure do, and Gecko used to until bug 774122 was fixed.

Because in the research done back then, min/max-width/height without unit was pretty rare. Not zero but on the same order as typo-ed properties. It was a mostly arbitrary cutoff. The applicable goal here is "Where possible, limit quirks to a fixed set of legacy features so they don't propagate into new features." It would be easy to add more legacy longhand properties to the list though.

I think Blink/WebKit don't have a whitelist here. I can file bugs.
Flags: needinfo?(zcorpan)
Well, the first question then is whether we should just (re)add the quirk for min/max-height/widt.
Flags: needinfo?(dbaron)
I'd be in favor of just readding the quirk for min/max-height/width.
Flags: needinfo?(dbaron)
Actually, I take that back.

One of my guidelines about web compatibility bugs is to mostly ignore them when they're reported by the page author, and only consider them an actual compatibility problem when they're reported by the user of the page.  We haven't met that threshold here.  Or are there other bugs reported on this?
No, this is the only report so far.
There was bug 844649 also.

Apart from min/max-width/height you have a reports for background-position (bug 875153) and text-indent (bug 844641). Earlier bottom was not in the list but it was added before shipping 17 I think (bug 776591).

Here is the number of occurrences of this quirk for various properties when I researched this in 2012 (dataset dotnetdotcom's web200904, 600,000 pages, not including external stylesheets):

width:              46530
font-size:          35296
height:             31132
padding:            18278
padding-left:       16898
margin-bottom:      11898
margin-left:        11565
margin-top:         11393
top:                10483
left:                9219
padding-right:       9159
margin-right:        8283
padding-top:         8168
border-width:        5602
margin:              5331
padding-bottom:      5279
size:                5022
border:              4664
border-bottom-width: 3505
border-top-width:    2438
border-left-width:   2384
border-right-width:  1891
border-bottom:       1725
right:               1226
border-right:        1100
border-left:          887
letter-spacing:       874
border-top:           842
word-spacing:         381
text-indent:          378
fp-font-size:         362
max-width:            285
spacing:              243
min-width:            219
background-position:  135
bottom:               101
max-height:            95
position:              80
end:                   68
start:                 49
vertical-align:        47
min-height:            38
border-spacing:         7

Looking at this it seems quite possible that other browsers would have more trouble with the various border shorthand properties than with min-height.
But did we support the quirk in those border shorthands prior to bug 774122?  We generally tried not to do so in shorthands, but I guess some were (and are) missing nsAutoParseCompoundProperty.
No, but other browsers do. (Blink just dropped hashless hex color quirk for border shorthands: https://code.google.com/p/chromium/issues/detail?id=391343 )
I did the same research on a newer dataset (http://webdevdata.org data set 2013-09-01 102,000 pages, again not including external stylesheets.)

"line-height": 9380
"width": 8103
"height": 4079
"font-size": 2752
"padding": 1636
"margin-top": 1616
"size": 1616
"margin": 1452
"top": 1191
"margin-left": 1158
"margin-bottom": 704
"padding-left": 703
"left": 578
"border-width": 482
"position": 451
"border": 424
"padding-top": 300
"padding-bottom": 248
"border-bottom-width": 222
"padding-right": 214
"start": 206
"height ": 168
"end": 162
"border-right": 160
"size ": 131
"margin-right": 124
"right": 108
"width ": 106
"start ": 106
"border-top-width": 101
"border-bottom": 99
"border-left-width": 98
"letter-spacing": 87
"border-right-width": 83
"width       ": 74
"bottom": 60
"margin\t\t": 48
"border-left": 48
"padding ": 47
"text-indent": 45
"word-spacing": 44
"max-width": 34
"top ": 32
"bottom ": 32
"min-width": 27
"size  ": 26
"line-height ": 25
"columns": 22
"fp-font-size": 21
"stroke-width": 20
"position ": 17
"moz-tab-size": 16
"spacing": 16
"padding-left ": 16
"min-height": 16
"border-top": 16
"height\t": 14
"columns ": 14
"width   ": 13
"max-height": 10
"margin ": 10
"left ": 10
"moz-box-shadow": 8
"padding-right ": 7
"height\t\t": 7
"border ": 6
"spacing ": 6
"bottom-width": 6
"height\t\t\t": 5
"width              ": 5
"first-position": 5
"border-image-width": 5
"pos-top": 4
"right ": 4
"gin-top": 4
"indent": 4
"width  ": 4
"height       ": 4
"font-size ": 4
"padding-top ": 4
"top-width": 3
"width \t": 3
"webkit-box-shadow": 3
"background-position": 3
"right-margin": 3
"margin-left ": 2
"padding-bottom ": 2
"mso-tstyle-colband-size": 2
"shadow": 2
"moz-columns": 2
"zoom-width": 2
"text-size": 2
"height        ": 2
"min-device-width": 2
"ms-columns": 2
"max-width ": 2
"homepage-middle-left": 2
"webkit-columns": 2
"mso-tstyle-rowband-size": 2
"o-columns": 2
"end ": 2
"tab-size": 1
"top\t\t\t\t": 1
"width\t\t": 1
"clip": 1
"width\t\t\t": 1
"height  ": 1
"columns       ": 1
"border-spacing": 1
"width          ": 1
"font-width": 1
"text-shadow": 1
"border-top ": 1
"box-shadow": 1
"height              ": 1
"margin-right ": 1
"height    ": 1
"ine-height": 1
"height          ": 1
"margin-top ": 1
"background-size": 1
"x-mii-uncompressed-size": 1
"z-position": 1
"font-height": 1
"nline-height": 1
"align": 1
"border-width ": 1
"request-start": 1
"border\t\t": 1
"x-end": 1
"size     ": 1
"padding   ": 1
"x-start": 1

(I forgot to trim trailing whitespace.)
I've checked httparchive also (300k pages - http://bigqueri.es/t/analyzing-html-css-and-javascript-response-bodies/442 ), this time looking only at external stylesheets where the top-level page is in quirks mode, for a selection of properties. The count is number of top-level pages where there is a match for the given property followed by a unitless number.

width: 517
height: 411
font-size: 276
text-indent: 14
min-height: 10
background-position: 9
max-width: 8
min-width: 8
border-spacing: 6
max-height: 1
vertical-align: 1

I haven't looked at usage of compound properties like 'border' in this dataset.

So the remaining non-compound properties have low usage (~0.02% of pages in this dataset). We can stick to the current list or add all remaining properties (the ones mentioned in this comment) to the list. The current list has been implemented but not yet landed in Blink (https://codereview.chromium.org/764703002 ). Any opinions?
I think the main reason it might make sense to add the quirk for min/max-width/height is that it's weird to support it for width/height but not those.

However given that data I'd probably be leaving the list of quirky properties as-is.
I think it would be a good idea to add at least min/max-width/height, since it did cause a regression serious enough for someone to report.

AFAICT the remaining properties from comment #14 are text-indent, background-position, border-spacing and vertical-align.

If those are added, how many more remain until all CSS 2.1 properties taking lengths are included? If it's only a few more maybe that would make sense.

Lacking an objective metric based on a bigger dataset, maybe add min/max-width/height and any other properties for which there are bug reports in any browser trying to introduce this whitelist?
Thanks. I agree that it seems good to add min/max-width/height, but I think if we do we should race to the bottom and add all remaining non-compound CSS 2.1 properties and then call it a day. Possibly excluding outline-width since that and outline-color (which is the only excluded non-compound CSS 2.1 property in the hashless whitelist) hasn't shown up at all in the data.

As for bug reports, see comment 10.
OK, I'd be happy with at a minimum everything that's had bug reports, but if there are aesthetic or risk-aversion reasons to go further I don't mind. Getting the whitelist in order (hopefully for good) before shipping in Blink would be nice.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8545362 - Flags: review?(dbaron) → review+
And for the record, the try run was https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b4965650d17 using "-b d -p linux64 -u all -t none" but apparently we do not run web platform tests in debug builds.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc4bf08ee66 to fix those.
You need to log in before you can comment on or make changes to this bug.