Closed Bug 243412 Opened 20 years ago Closed 10 years ago

Implement 'box-sizing' (dropping the -moz- prefix)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
relnote-firefox --- 29+

People

(Reporter: annevk, Assigned: alexhenrie24)

References

(Blocks 2 open bugs, )

Details

(Keywords: css3, dev-doc-complete, Whiteboard: [parity-IE] [parity-opera] [parity-webkit])

Attachments

(7 files, 8 obsolete files)

6.04 KB, patch
dbaron
: feedback+
Details | Diff | Splinter Review
83.00 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.44 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
40.98 KB, image/png
Details
6.20 KB, patch
dbaron
: review+
dao
: checkin+
Details | Diff | Splinter Review
144.85 KB, patch
Details | Diff | Splinter Review
1.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
CSS3 UI has reached CR. This means we can (and are encouraged to) drop the -moz-
prefix from the 'box-sizing' property.

Since quite a lot of sites rely on this property we should probably not remove
'-moz-box-sizing' but implement it like it was done with 'opacity'. (There is of
course the simple fact that lots of site authors were already using 'box-sizing'
because of Opera.)

Currently Mozilla supports 'padding-box' as a value, if we don't want to remove
that value, we should rename it to '-moz-padding-box', since it is not a valid
value according to the specification.
Attached patch patch (untested) (obsolete) — Splinter Review
Since I don't have the ability to build and that will take some time before I
do I created a patch based on the 'moz-opacity' -> 'opacity' implementation.

It might be that I missed a few things, but like I said, I wasn't able to test
this.
1)  I don't see a reason to create a DOM alias.  box-sizing is not commonly
    scripted, unlike opacity.

2)  I'm rather opposed to throwing this switch unless some testing is done to
    see how good our implementation is.  In particular, I suspect that max and
    min widths are broken (especially on replaced elements).  In fact, I don't
    see how the CSS2.1 algorigthm for min/max widths on replaced elements makes
    sense as it stands in the presence of box-sizing.  I've sent mail to
    www-style to that effect.  In any case, someone needs to start writing
    testcases.
I suspect lots of stuff is broken and I'm opposed to the property in general.  I
think we should not implement it.
Removing it altogether is fine by me.
Although I don't really like the property either, the 'calc()' proposal
discussed on www-style seems much better, /this/ is the property people are
using. I visit some fora quite often and this is what people are using to "hack
around" the box model differences.

This is also one of the solutions people are talking about and which is placed
on various webdesigner/webdeveloper resources. We can of course say they are
nuts for using this stupid property, but I don't think they will understand and
I don't think that we should break sites that are already using it.

I think this needs to be implemented (we should at least keep the
'-moz-box-sizing') and to do that we probably have to wait for W3C clarification.
I guess I'm one of the 'nuts'y people for wanting the ability to select between
box models, but the main reason why is that for the kind of web applications
that I'm building, 'content box' sizing is exactly the wrong kind of sizing I
need. I'm trying to do layouts that require 'border box' sizing. This is one
that Microsoft got right and the CSS committee got wrong.

In any case, there's no need to fight (or switch ;-)) - just give web developers
like me the choice via the 'box-sizing' property.

And *please*, if you're not going to implement this property, do *not* remove
-moz-box-sizing. Setting '* {-moz-box-sizing: border-box}' is the only way I've
been able to keep sane. My other stuff runs so well in Mozilla, but if I didn't
have border-box sizing, trying to lay out GUI would be a nightmare.

Cheers,

- Bill
Keywords: qawanted
It looks like box-sizing is dropped all together:
http://www.w3.org/TR/2002/WD-css3-box-20021024/#the-box-width
Please note that you are referring to a very out-of-date draft, CSS3 UI which
included this property as mentioned in comment 0 is a bit more recent.
Blocks: 357457
Assignee: dbaron → nobody
QA Contact: ian → style-system
Depends on: 308801
IE 8 supports "box-sizing" now without prefix (Opera anyhow since 7.0). This should increase importance.

It would be a mercy for web authors and web compatibility to get this fixed soon.
Depends on: 338554
Whiteboard: [parity-IE] [parity-opera]
My use case for this, for anyone who's interested in hearing it:

I've got a DHTML-based puzzle game. Some of its UI is the same no matter where the player is the game, while the rest is in several "screens" that get dynamically switched between. Now, for this game, different screens need to look different depending on what they need to present to the player; screens may need paddings of varying sizes and one of them doesn't need a border. But they all need to be the same size.

Ideally, I would write
.screen {
/* dimensions go here */
box-sizing: border-box;
}
and Mission Accomplished.
Sure, and right now you can write |-moz-box-sizing: border-box;|
Yes, as long as A. it doesn't get removed as per earlier discussion, and B. I repeat myself three times to ensure all major Web browsers listen to me. I'm a Web developer, not a leader of some snark-hunting expedition...
1. What QA can one help out doing to get this fixed? 

2. Shouldn't Gecko perhaps drop the nonstandard padding-box value first?
Depends on: 593964
WebKit nightlies support box-sizing without -webkit-prefix now.
Whiteboard: [parity-IE] [parity-opera] → [parity-IE] [parity-opera] [parity-webkit]
Just pointing out that, in 2011, Firefox 4 (and possibly 5-6) will be the only browser left requiring a prefix for this well supported property.
(In reply to comment #15)
> Just pointing out that, in 2011, Firefox 4 (and possibly 5-6) will be the only
> browser left requiring a prefix for this well supported property.
That's what the whiteboard is saying so it is known ;)

I think it's too late for Firefox 4 but can we get it triaged for Firefox 5 ?
Florent, the property is not in CR.  It's not well-defined how it should behave in many cases in the spec yet.  Yes, some browsers dropped prefixes prematurely, but their implementations are not compatible with each other last I checked.  So I wouldn't call this "well supported"....
And in particular, the interaction of box-sizing with replaced element sizing when max/min-width/height are used is still not defined.
Thanks Boris for this background information. I hadn't tested the different implementations beyond display:block (non-replaced) elements. And I obviously don't advise dropping prefixes prematurely.
Actually, it's been in CR for many years: css3-ui has been CR since 2004. The reason we haven't dropped prefixes is because there are some significant bugs with our implementation (see the dependencies).
Er, you're right that CSS3 UI defines box-sizing.  But it doesn't define how it should work in enough detail that it can be implemented, leaving that to CSS3 Box, as far as I can tell...

But yes, we should fix the obvious dependent bugs, anyway, then create a decent test suite for this (which CSS3 UI certainly won't have otherwise).
The relevant part is not dropping the -moz- but correcting the behavior of min-height and min-width, as for now these ignore box-sizing, while other browsers apply to min-xxx the same rules as to xxx.
Depends on: 761989
Depends on: 520992
Only Firefox still uses the prefix.

http://caniuse.com/css3-boxsizing
Depends on: 771414
Blocks: unprefix
Please drop the prefix already. You guys have dropped prefix for transitions & animations... duh.
Niloy, we dropped the prefix for those because their implementations match the spec.

The implementation of -moz-box-sizing does NOT match the spec.  See the bugs blocking this one.
i know the prefix was supposedly dropped, but it doesn't seem to be working without it. 2¢

with -moz prefix: desired result
http://jsbin.com/ezotow/6

without -moz prefix: broken
http://jsbin.com/ezotow/8
(In reply to russo from comment #27)
> i know the prefix was supposedly dropped

No. It has not been dropped yet (this bug is still open).
Apologies, thank you for the clarification
Depends on: 778413
I'll give $50 NZD to anyone who fixes this bug before the end of 2012.
Lulz!

Seriously, please fix this, this property is turning out to be a little precious gem  and I have keep writing -moz-boz-sizing in my css.
If your comment is not about pinpointing the bug or contributing a fix, it is off-topic and does not belong here. These are our guidelines for using Bugzilla. Please read them before you add another comment:
  https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Depends on: 776265
Depends on: 801116
There is one extremely important reason to drop the prefix even before fixing all issues with the current implementation - the technique of using:
* { box-sizing: border-box; }
has become popular recently (especially that some high-profile in web dev world people - as Paul Irish - has become advocating it) and if it's used for a mobile site, authors can easily forget about the prefix needed only for a browser with about 1% market share. And apart from most CSS3 features, not including a prefixed version of the property results not only in a minor degradation but in a completely broken site layout.

It would be interesting to know some stats about sites using border-box, especially on mobile. Nonetheless, this is something Mozilla should be wary about.
Blocks: 850827
(In reply to Michał Z. Gołębiowski from comment #33)
> There is one extremely important reason to drop the prefix even before
> fixing all issues with the current implementation - the technique of using:
> * { box-sizing: border-box; }
> has become popular recently (especially that some high-profile in web dev
> world people - as Paul Irish - has become advocating it) and if it's used
> for a mobile site, authors can easily forget about the prefix needed only
> for a browser with about 1% market share. And apart from most CSS3 features,
> not including a prefixed version of the property results not only in a minor
> degradation but in a completely broken site layout.
> 
> It would be interesting to know some stats about sites using border-box,
> especially on mobile. Nonetheless, this is something Mozilla should be wary
> about.

This blocks bug 850827 - Google have started using an unprefixed box-sizing on the Google Maps mobile site. I agree with this comment, it'd be really great to have this unprefixed, even if there are problems.
Just to note, desktop GMail also uses box-sizing, but only with a -webkit prefix, meaning we get spurious horizontal scrollbars in the 3-panel mode :( (I'll file a bug for evangelism)
Blocks: 852146
No longer depends on: 593964
Bug 243412 does not depend on any open bugs now. Can it be unprefixed?
needinfo'ing :jwir3 wrt comment #36, as he expressed interest in this and it'd be good to get the ball rolling :)
Flags: needinfo?(sjohnson)
(In reply to Chris Lord [:cwiiis] from comment #37)
> needinfo'ing :jwir3 wrt comment #36, as he expressed interest in this and
> it'd be good to get the ball rolling :)

Sure, I can look into this. Note that it's going to take a bit of time to determine the testing situation, and what we need to do to verify that our implementation matches spec. I will look into it in the next week or so.
Assignee: nobody → sjohnson
Flags: needinfo?(sjohnson)
Attached patch Patch v1Splinter Review
Not sure if anything else needs to be done, but this does the unprefixing itself.
Assignee: sjohnson → Ms2ger
Attachment #148310 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #734251 - Flags: review?(dbaron)
Any chance it'll get into Firefox 23? Does anything else have to be done?
> Any chance it'll get into Firefox 23?

Yes, if the review happens in the next week or so.

> Does anything else have to be done?

Well, running a nightly or better yet a try build with the patch through the box-sizing test suite (which exists, right?) would be a good start....
OK, I've cloned the mozilla-inbound repo, build the OS X 64-bit debug build, run the test using:
./mach reftest layout/reftests/box-sizing/
then I applied the patch, re-run `./mach build` and then these tests.

All of them passed.

Anything else? :)
(In reply to Boris Zbarsky (:bz) from comment #41)
> > Does anything else have to be done?
> 
> Well, running a nightly or better yet a try build with the patch through the
> box-sizing test suite (which exists, right?) would be a good start....

Looking in the csswg repo, I see no reason to believe that there is a test suite for any part of css-ui, fwiw.
> Anything else? :)

We obviously pass our own tests.

The question is whether our behavior sufficiently matches the spec to unprefix.

That requires either testing against the _spec_ test suite or verifying that our tests sufficiently match the spec and have sufficient coverage...
(In reply to :Ms2ger from comment #43)
> Looking in the csswg repo, I see no reason to believe that there is a test
> suite for any part of css-ui, fwiw.

(In reply to Boris Zbarsky (:bz) from comment #44)
> We obviously pass our own tests.
> 
> The question is whether our behavior sufficiently matches the spec to
> unprefix.
> 
> That requires either testing against the _spec_ test suite or verifying that
> our tests sufficiently match the spec and have sufficient coverage...

What Boris and Ms2ger are saying here is that there doesn't appear to be any tests in the CSS specification test suite, located here:
http://www.w3.org/Style/CSS/Test/

The css ui doesn't have any tests listed next to it, which indicates that there aren't any tests available for this spec yet. Probably as part of this bug, it might be a good idea to write some tests that should conform to the individual specification sections (see css3-multicol tests for an example). 

Take a look here for more information on writing tests, too:
http://wiki.csswg.org/test/format
So, basically, we need to:

A) Construct tests that identify what the spec requires the behavior of box-sizing to be,
B) Verify our implementation passes these tests, and
C) Submit the tests to the W3C.
Hope dev will not mind this, since this patch passes FF own test suites and no W3C tests are available currently, I think real life testing by landing this patch in early FF24 Nightly cycle (which kicked in already) will be better if any site broken issues bugs filed against this, then it should be backout from next Aurora merger, all in all Nightly is bleeding edge test branch, so landing will not hurt. Just a suggestion..
The issue is not with whether sites are broken with the patch.

The issue is whether our implementation is broken enough that unprefixing it means we screw over web developers who then can't use the property.

Again, what needs to happen is checking what parts of the behavior are not tested and testing them.  As a simple example, there are apparently absolutely no tests that test the interaction of box-sizing and the treatment of max/min-width/height on replaced elements with intrinsic sizes (read: images) that have nonzero paddings and borders.
Thanks boris for clearing up my confusion.. Keep up your work devs... I hope you don't mind my comment...
The computed height is wrong in padding|border|(margin)-box model with window.getComputedStyle(object,null).height function.
That currently doesn't follow the box model.
Current cross UA status: Chrome and Opera fits to box-model with this function.
(In reply to Robert Utasi [:hunboy] from comment #50)
> The computed height is wrong

Bug 520992? (fixed in Fx24)
(In reply to j.j. (mostly inactive in 2013, too) from comment #51)
> 
> Bug 520992? (fixed in Fx24)

Sorry for this bugspam, I've missed this codechange 1 month ago. I can confirm it works correctly on nightly with pagging|border-box model as well.
Comment on attachment 734251 [details] [diff] [review]
Patch v1

From a code perspective this patch looks fine, so r=dbaron on the code once we're ready to land it.  But clearing review request to indicate that it isn't yet ready to land.

However, before it lands, as Boris said, we could use some additional test coverage of box sizing -- in particular, for the image scaling code.  While the spec doesn't define precise wording, we should ensure that all the image scaling cases yield the same results for the same *specified content box width/height*, whether that width/height was specified directly or whether a larger width was specified and padding-box or border-box box-sizing was used.

I thought we had a bunch of tests for the image scaling code somewhere that could be modified to test this, but I can't find them.  That doesn't mean they aren't there, though.
Attachment #734251 - Flags: review?(dbaron) → feedback+
I think the test I was thinking of was https://test.csswg.org/suites/css2.1/20110323/html4/replaced-min-max-001.htm

So a good start would be converting that to reftests (which I think may already be done somewhere), then adding variants with padding, border, and margin, and then adjusting those variants for different values of box-sizing to make sure that changes that don't change the specified content-box widths/heights don't change the size of the box.
Is there any reason other than comments #3-5 why this bug can't be shipped in either 23 or 24?
There are no longer issues with this bug. There aren't many test cases. I suspect you cover over 99.9% of all cases with your implementation at least. Wouldn't it be easier to just ship it now and let people file bugs as the happen?
You already shipped the flexible box model in spite of the fact that the implementation is incomplete (IE flex-wrap:wrap). As a developer I can't stand vendor prefixes. This is the only prefixed property I use anymore (webkit included).
> Is there any reason other than comments #3-5 why this bug can't be shipped

Allen, please do read the whole bug; this is discussed at length starting in comment 44.
Believe me I have read the whole bug. I've been following this bug for 2 years at least.

I'm suggesting that any issues with the implementation are minor and it will be easier to find them by shipping now rather than trying desperately to create tests that will aim at the odd aspects of the spec. If you look at comment 45, he said that there doesn't seem to be any tests for this part of the spec.

The last couple of bugs that were blocking this were all minor. The image padding bug for example, I never encountered it and after I checked out that bug I had no problems creating a work around.

I only mentioned comments #3-5 because the very people that said they didn't want to implement this part of the spec are the same people that seem to be delaying it for looking for non-existent tests. In other words this is being delayed for political reasons rather than technical.
I think the issue is that for flexbox we knew in what ways it didn't match the spec and we agreed those were minor enough to ship.

The problem here is that we don't even know what's broken; once we do we can decide whether it's minor...
That's kind of my point. You don't know and you won't know what's broken.

If you were to ship now I can only see 3 things happening.

1) A website doesn't use box-sizing at all and shouldn't be affected
2) A website only uses non-prefixed box-sizing and is currently broken as a result (may or may not be fixed by shipping this patch)
3) A website uses the -moz- prefix in addition to being non-prefixed and either works or doesn't, but shipping this bug wouldn't change that.

A 4th scenario involves -moz- prefixing only, but that would only break if you removed the prefix rather than just not requiring it.

One commonality you'll notice in my scenarios is that anything that would be broken by shipping this is already broken anyway.

From what I've read here there are no tests and therefore this bug will continue to rot for another 10 years and you'll never know what is actually broken.

At the very least you should consider making a preference to enabling box-sizing unprefixed (personally I would rather skip that). That will at least allow us to actively search for websites that are broken under the implementation. As of right now there are websites that are broken by not shipping this patch as is, in fact you have 2 of them listed as blocked bugs. Maybe they have additional issues and maybe they don't, but you won't know until you ship this.
> 3) A website uses the -moz- prefix in addition to being non-prefixed and either works or
> doesn't, but shipping this bug wouldn't change that.

Assuming they use the same value for the two properties.

But the real issue with unprefixing broken crap is not site breakage today but causing issues for web developers tomorrow.

> there are no tests and therefore this bug will continue to rot 

No, what will happen is that someone will step up and write tests.  Worst case, the spec will need them anyway, so they will happen much sooner than 10 years from now.
Actually there are only 3 possible values. border-box, padding-box, and content-box. Firefox is the only vendor that supports padding-box. content-box is the default and is implemented automatically. That leaves border-box, so no that argument is invalid.

As a web developer myself I'm telling you right now you are causing more problems by NOT unprefixing it than the .0001% odd cases that MIGHT crop up as a result. The web developers of tomorrow are almost universally using it RIGHT NOW. Most of them develop on webkit. You are the ONLY vendor that doesn't support this. I can't tell you how many times I see webkit only prefixing (without the W3C version) in code. That's a problem in itself, but by eliminating as many vendor prefixes as possible, it minimizes the requirements to get things working in firefox. I'm also beginning to see people implement this as * { box-sizing:border-box; } (in other words firefox isn't supported) This is a MUCH bigger problem than a couple minor issues cropping up.

Why is it ok for IE (ironically I believe they were the first to support it), chrome, safari, and opera to do this a long time ago if they don't have any tests? It's obviously worked out for them. I can promise you that any odd issues that might crop up aren't worth the delay because you will fix more problems than you create.

The real problem you guys had was lack of support for min/max values as well as table support. That's been fixed for a while. One day out of the blue, the SVG bug disappeared, which was minor, as well as the image padding issue, which I had a work-around.
So, I've got some tests in my patch repository I've been working on. I should have them ready to post shortly. They might need some tweaking to get into W3C format. 

One thing I've noticed, though, is that webkit and opera don't seem to implement box-sizing for replaced elements. More specifically, the test in comment 54 that dbaron mentioned doesn't seem to be different with opera (I think I still have an old presto version) or chromium when I add padding to the images and then specify box-sizing: padding-box (or -o-box-sizing: padding-box or -webkit-box-sizing: padding-box).
(In reply to Scott Johnson (:jwir3) from comment #63)
> So, I've got some tests in my patch repository I've been working on. I
> should have them ready to post shortly. They might need some tweaking to get
> into W3C format. 
> 
> One thing I've noticed, though, is that webkit and opera don't seem to
> implement box-sizing for replaced elements. More specifically, the test in
> comment 54 that dbaron mentioned doesn't seem to be different with opera (I
> think I still have an old presto version) or chromium when I add padding to
> the images and then specify box-sizing: padding-box (or -o-box-sizing:
> padding-box or -webkit-box-sizing: padding-box).

Ah, you know, it might be just that webkit/presto don't implement padding-box...
(In reply to Scott Johnson (:jwir3) from comment #64)
 
> Ah, you know, it might be just that webkit/presto don't implement
> padding-box…

Afaict, IE 10 doesn't implement it either. If I remember correctly, 'padding-box' is a recent addition to the spec. And it is marked as 'at risk'.
Firefox appears to be the only browser that supports "padding-box". I wouldn't include it in any tests.
http://caniuse.com/#feat=css3-boxsizing
(In reply to Allen Williams from comment #66)
> Firefox appears to be the only browser that supports "padding-box". I
> wouldn't include it in any tests.
> http://caniuse.com/#feat=css3-boxsizing

We should probably have tests for these, though. Even if no browser pass the tests, it would be useful for future conformance.
(In reply to Scott Johnson (:jwir3) from comment #67)
> (In reply to Allen Williams from comment #66)
> > Firefox appears to be the only browser that supports "padding-box". I
> > wouldn't include it in any tests.
> > http://caniuse.com/#feat=css3-boxsizing
> 
> We should probably have tests for these, though. Even if no browser pass the
> tests, it would be useful for future conformance.
Or if no other browser cares to implement it, maybe it should be removed from spec and Gecko... (let's have this discussion in a different place if interested)
Attached patch box-sizing-tests (obsolete) — Splinter Review
Finished tests for box-sizing, but there seems to be a slight problem on some platforms where the p element in box-sizing-replaced-{001,002}.html (by looking at it using the inspector) in example 1 below produces a box with a slightly larger height (~4px) than the <p> element in box-sizing-replaced-{001,002}.ref.html. I'm still analyzing why this is going wrong.

The devtools inspector shows that there are no margins or padding, and if I set the line-height to be the same on both <p> elements, or turn off margins and padding globally using the * selector, it has no effect. Basically, the problem comes up on OSX platforms - there's about a 4px gap between the top line and the bottom line (e.g. in https://tbpl.mozilla.org/?tree=Try&rev=2f1c02122c4f ) that isn't there in the reference images.
Attached patch box-sizing-tests (obsolete) — Splinter Review
Initial version of w3c box-sizing-tests.

https://tbpl.mozilla.org/?tree=Try&rev=a33150f7ef61
Attachment #782863 - Attachment is obsolete: true
Attachment #783214 - Flags: review?(dbaron)
So what's needed to breath life into this bug again? Tests?
I still think if the people at mozilla didn't hate it, it would be implemented by now. Just look at the comments at the beginning of the page. I don't get the hate for it. And contrary to their comments I feel that while calc() is great, there are many cases where border-box is far superior. I use both where I feel appropriate. I just think its embarrassing that -moz-box-sizing is the only prefixed code I have left.

I may not like chrome, but I'm happy that they said they will never prefix anything else and plan to remove all prefixes. I just hope they actually kill them too. I don't ever want to see -webkit-border-radius again especially if its implemented. I wish Mozilla would adopt the no prefix policy as well.
> I wish Mozilla would adopt the no prefix policy as well.

We did, quite a while ago.
> So what's needed to breath life into this bug again? Tests?

A review from dbaron.
Comment on attachment 783214 [details] [diff] [review]
box-sizing-tests

In general, could you use "-ref" in filenames rather than ".ref"?
(Should be easy to fix by search-replace on the patch.)

The test files (but *not* the -ref files) need at least a <link
rel="help" pointing to the relevant section of the spec, which is
presumably http://www.w3.org/TR/css3-ui/#box-sizing .  A
<meta name="assert"> describing what the test is trying to test
wouldn't be a bad idea, either.

I think the box-sizing-content-box tests (not references) should use
percentages just like the box-sizing-padding-box and
box-sizing-border-box tests.  Otherwise they're just testing the initial
value of the property, which is tested well enough by
property_database.js-based tests.

box-sizing-padding-box-003.ref.xht (the reference!) probably should
have set widths/heights rather than using min-width/min-height, so
that it tests that min-width/height are working correctly a little
better.

box-sizing-replaced-* should probably mention in a comment at the
top that they're derived from replaced-min-max-001.

I don't understand why #img1 is so much simpler in your test than the
test you derived it from -- could you use the same test?

I'm also not convinced that the tests are all still testing what they
are supposed to at the new sizes.  My intent was that you'd still
produce a test that was leading to 75x75 images, but you'd increase the
numbers in the tests (which is using padding-box/border-box) to account
for the padding/border.  (Instead, you decreased the numbers in the
reference so all the images are smaller, but that means thet math that
was worked out to ensure all the combinations were tested isn't
*necessarily* valid anymore, though it might still be true.)  I think it
would be better to have 75x75 results, and increase the numbers in the
test file rather than decreasing them in the reference.  (And the
references should just be able to use width:75px and height:75px for
*all* the images rather than having min-width/max-width at all.)

I think these are good with those things fixed, but I'd like to take a
quick look at the box-sizing-replaced-* tests after those changes, so
marking as review-.  But thanks for writing these, and sorry for the
delay getting to the review.
Attachment #783214 - Flags: review?(dbaron) → review-
Oh, and if you happen to have the replaced variant for content box sizing, that would be useful to have as a reftest if we don't already.
Attached patch box-sizing-testsSplinter Review
(In reply to David Baron [:dbaron] (needinfo? me) from comment #75)
> Comment on attachment 783214 [details] [diff] [review]
> box-sizing-tests
> 
> In general, could you use "-ref" in filenames rather than ".ref"?
> (Should be easy to fix by search-replace on the patch.)

Fixed.

> The test files (but *not* the -ref files) need at least a <link
> rel="help" pointing to the relevant section of the spec, which is
> presumably http://www.w3.org/TR/css3-ui/#box-sizing .  A
> <meta name="assert"> describing what the test is trying to test
> wouldn't be a bad idea, either.

I did both of these, but you may want to review my wording for the <meta name="assert"> tags.

> I think the box-sizing-content-box tests (not references) should use
> percentages just like the box-sizing-padding-box and
> box-sizing-border-box tests.  Otherwise they're just testing the initial
> value of the property, which is tested well enough by
> property_database.js-based tests.

Done.

> box-sizing-padding-box-003.ref.xht (the reference!) probably should
> have set widths/heights rather than using min-width/min-height, so
> that it tests that min-width/height are working correctly a little
> better.

Fixed.

> box-sizing-replaced-* should probably mention in a comment at the
> top that they're derived from replaced-min-max-001.

Fixed, although, I noticed that the replaced-min-max-001 is flagged as having problems in the CSS test suite (#48 under "48 Untestable Reftests": http://wiki.csswg.org/test/reftest )

> I don't understand why #img1 is so much simpler in your test than the
> test you derived it from -- could you use the same test?

Yes, I modified this.

> I'm also not convinced that the tests are all still testing what they
> are supposed to at the new sizes.  My intent was that you'd still
> produce a test that was leading to 75x75 images, but you'd increase the
> numbers in the tests (which is using padding-box/border-box) to account
> for the padding/border.  (Instead, you decreased the numbers in the
> reference so all the images are smaller, but that means thet math that
> was worked out to ensure all the combinations were tested isn't
> *necessarily* valid anymore, though it might still be true.)  I think it
> would be better to have 75x75 results, and increase the numbers in the
> test file rather than decreasing them in the reference.  (And the
> references should just be able to use width:75px and height:75px for
> *all* the images rather than having min-width/max-width at all.)

Ok, I reworked all of the tests so that the height/width is adjusted such that it takes into account the padding/border in such a way that the image remains 75x75.

> I think these are good with those things fixed, but I'd like to take a
> quick look at the box-sizing-replaced-* tests after those changes, so
> marking as review-.  

Ok, I'm posting a new version now.

> But thanks for writing these, and sorry for the delay getting to the review.

You're welcome, and no problem regarding the delay.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #76)
> Oh, and if you happen to have the replaced variant for content box sizing,
> that would be useful to have as a reftest if we don't already.

I added another test, box-sizing-replaced-003, which tests the box-sizing: content-box attribute. This is essentially the same as the replaced-min-max-001 that was in the w3c test suite. We have it in our tree under layout/reftests/bugs/234686-*.html, but these are individual tests rather than a larger set of 10 images in one page. I'm not sure if we want to duplicate the tests, so I added the new test for your perusal. Just let me know if you want to remove it. I guess it still tests to make sure that the -moz-box-sizing: content-box value is being processed correctly.
Assignee: Ms2ger → sjohnson
Attachment #783214 - Attachment is obsolete: true
Attachment #812123 - Flags: review?(dbaron)
Comment on attachment 812123 [details] [diff] [review]
box-sizing-tests

(In reply to Scott Johnson (:jwir3) from comment #77)
> > The test files (but *not* the -ref files) need at least a <link
> > rel="help" pointing to the relevant section of the spec, which is
> > presumably http://www.w3.org/TR/css3-ui/#box-sizing .  A
> > <meta name="assert"> describing what the test is trying to test
> > wouldn't be a bad idea, either.
> 
> I did both of these, but you may want to review my wording for the <meta
> name="assert"> tags.


Instead of:

>+  <meta name="assert" content="The box-sized elements should each take up 1/2 of the horizontal space of the containing element, including their borders and padding." />

it would probably be better to write something like:

>+  <meta name="assert" content="-moz-box-sizing: border-box should make the element's (percentage) width be the distance from the left border edge to the right border edge." />

and similar for the other tests.  In other words, describe the feature
being tested rather than exactly what the test shows.


In box-sizing-replaced-001.xht, change:

  #img1 max-width from 175px to 115px
  #img2 min-height from 60px to 70px

unless I'm missing a reason they should be as they are.

same for box-sizing-replaced-002:
  #img1 max-width from 185px to 125px
  #img2 min-height from 60px to 80px

same for box-sizing-replaced-003:
  #img1 max-width from 165px to 125px
  #img2 min-height from 50px to 60px

r=dbaron with that
Attachment #812123 - Flags: review?(dbaron) → review+
I can push these two patches into the tree if they're ready to go. Can I assume the feedback+ is now an r+ if the tests land as described in Comment 78?
Flags: needinfo?(dbaron)
Assignee: sjohnson → bugs
Just a word of encouragement guys. I don't do anything particularly complicated replacing elements (reading this thread briefly it seems to be the sticking point as I understand it).

But.
I've been using:

-moz-box-sizing:border-box;
box-sizing:border-box;

Without issue, for months and months now. Love this property for various reasons e.g. for making input and select the same width in a responsive design, it is saving me plenty of time and my sanity.

Keep up the good work, I hope we can resolve the final issues and drop the prefix soon.
The only thing I want more from FireFox is flex-wrap.
Bug 930218 seems to be an obvious issue in the box-sizing implementation...
Depends on: 930218
Attached patch box-sizing-test Update (obsolete) — Splinter Review
This patch makes the changes noted in comment #78. 
It uses the exact wording you made, with just padding, border, and content words switched out depending on the test.

This patch should be applied on top of Scotts :)
Attachment #826471 - Flags: review?
Attachment #826471 - Flags: review? → review?(dbaron)
Comment on attachment 826471 [details] [diff] [review]
box-sizing-test Update

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-border-box-003.xht
>+  <meta name="assert" content="-moz-box-sizing: border-box should make the element's (percentage) width be the distance from the left border edge to the right border edge." />

border-box-003 tests both width and height

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-border-box-004.xht
>+  <meta name="assert" content="-moz-box-sizing: border-box should make the element's (percentage) width be the distance from the left border edge to the right border edge." />

border-box-004 tests min/max-width/height, and uses lengths rather than percentages

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-content-box-001.xht
>+  <meta name="assert" content="-moz-box-sizing: content-box should make the element's (percentage) width be the distance from the left content edge to the right content edge."/>

This tests both width and height.

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-content-box-002.xht
>+  <meta name="assert" content="-moz-box-sizing: content-box should make the element's (percentage) width be the distance from the left content edge to the right content edge."/>

This tests calc() with lengths and percentages, not plain percentages, and tests both width and height.

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-content-box-003.xht
>+  <meta name="assert" content="-moz-box-sizing: content-box should make the element's (percentage) width be the distance from the left content edge to the right content edge."/>

This tests both width and height.

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-padding-box-002.xht
>+  <meta name="assert" content="-moz-box-sizing: padding-box should make the element's (percentage) width be the distance from the left padding edge to the right padding edge."/>

This tests both width and height.

>diff -r c2ef3c2742f8 -r 2c04e4858e1e layout/reftests/w3c-css/submitted/ui3/box-sizing-padding-box-003.xht
>+  <meta name="assert" content="-moz-box-sizing: padding-box should make the element's (percentage) width be the distance from the left padding edge to the right padding edge."/>

This tests calc() with lengths and percentages, not plain percentages, and tests both width and height.


r=dbaron with those corrections
Attachment #826471 - Flags: review?(dbaron) → review+
Attached patch box-sizing-test Update (obsolete) — Splinter Review
> This tests calc() with lengths and percentages, not plain percentages, and tests both width and height.
So just "element's width"?

> border-box-004 tests min/max-width/height, and uses lengths rather than percentages
It would be pixels, right?

Also, shouldn't the tests use box-sizing instead of -moz-box-sizing?
Attachment #826471 - Attachment is obsolete: true
Attachment #827085 - Flags: review?(dbaron)
Comment on attachment 827085 [details] [diff] [review]
box-sizing-test Update

(In reply to Lukas Nordin from comment #86)
> Created attachment 827085 [details] [diff] [review]
> box-sizing-test Update
> 
> > This tests calc() with lengths and percentages, not plain percentages, and tests both width and height.
> So just "element's width"?

"element's (calc) width and height"

> > border-box-004 tests min/max-width/height, and uses lengths rather than percentages
> It would be pixels, right?

pixels are a type of length; the point here is that we have a length test and a percentage test, so it's better to say length

> Also, shouldn't the tests use box-sizing instead of -moz-box-sizing?

not in this patch
Attachment #827085 - Flags: review?(dbaron) → review-
> "element's (calc) width and height"
Done!

> pixels are a type of length; the point here is that we have a length test and a percentage test, so it's better to say length
Got it!
Attachment #827085 - Attachment is obsolete: true
Attachment #827301 - Flags: review?(dbaron)
Comment on attachment 827301 [details] [diff] [review]
box-sizing-test-update2.patch

r=dbaron
Attachment #827301 - Flags: review?(dbaron) → review+
Landed the tests:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3501cfab6b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1525f72e55ea

Still need to:
 (1) land the enabling
 (2) prefix-substitute the tests
 (3) push the tests to the w3c repository
Whiteboard: [parity-IE] [parity-opera] [parity-webkit] → [parity-IE] [parity-opera] [parity-webkit][leave open]
Depends on: 935850
No longer depends on: 935850
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #90)
> Still need to:
>  (1) land the enabling

What do you mean by "land the enabling"?

>  (2) prefix-substitute the tests

I assume this is a simple thing to do now that tests are written?

>  (3) push the tests to the w3c repository

Are there any technical problems concerning that?

I'd like to see box-sizing unprefixed soon so I wonder if there are any remaining technical problems for that to happen.
A basic search on GitHub:
https://github.com/search?q=box-sizing%3A+border-box&ref=cmdform&type=Code
shows that `box-sizing: border-box` is used very often and very few people add a `-moz-` prefix to the list.

Now that all the hard work has been done (thanks a lot for that!), there are no hanging critical bugs, tests have been written etc., can we get this into Firefox 29?
(In reply to Michał Gołębiowski from comment #92)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #90)
> > Still need to:
> >  (1) land the enabling
> 
> What do you mean by "land the enabling"?
> 
> >  (2) prefix-substitute the tests
> 
> I assume this is a simple thing to do now that tests are written?
> 
> >  (3) push the tests to the w3c repository
> 
> Are there any technical problems concerning that?
> 
> I'd like to see box-sizing unprefixed soon so I wonder if there are any
> remaining technical problems for that to happen.

Any comment on this?
Flags: needinfo?(dbaron)
Attached image firefox-box-sizing.png
Ridiculous.
(In reply to Michał Gołębiowski from comment #92)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #90)
> > Still need to:
> >  (1) land the enabling
> 
> What do you mean by "land the enabling"?

Write and land a patch to enable unprefixed 'box-sizing'.

> >  (2) prefix-substitute the tests
> 
> I assume this is a simple thing to do now that tests are written?

Yes.

> >  (3) push the tests to the w3c repository
> 
> Are there any technical problems concerning that?

I don't know.  At a minimum we'd probably want to move them to the layout/reftests/w3c-css/submitted/ directory.  But there might be a bunch of other issues raised by the code that checks them when they get submitted to the w3c repository.
Flags: needinfo?(dbaron)
Blocks: 946280
Attached patch Enable unprefixed 'box-sizing' (obsolete) — Splinter Review
I've rebased Ms2ger's patch to enable unprefixed 'box-sizing'. See comment #39 and comment #53.
Attachment #8361541 - Flags: review?(dbaron)
Comment on attachment 8361541 [details] [diff] [review]
Enable unprefixed 'box-sizing'

The change you removed from the original patch needs to go in nsComputedDOMStylePropertyList.h, but otherwise this looks fine.
Attachment #8361541 - Flags: review?(dbaron) → review-
Thanks for catching that. This should do the trick.
Attachment #8361541 - Attachment is obsolete: true
Attachment #8361992 - Flags: review?(dbaron)
Keywords: checkin-needed
Since you're using checkin-needed -- could you indicate which of the patches here need checkin?  Looks like there are 4 different patches attached, one with feedback+ and 3 with review+. Do the last 3 need landing?
Just attachment 8361992 [details] [diff] [review]. I'll make sure to specify next time.
Keywords: checkin-needed
So I assume the last push fixed this, and there's nothing left to land (e.g. tests)?
David Baron (comment #90) said we need to:
>  (1) land the enabling
>  (2) prefix-substitute the tests
>  (3) push the tests to the w3c repository

The last patch was step 1. This patch takes care of step 2. However, I'm not sure that I was supposed to change files in the following directories:

browser/extensions/pdfjs
layout/reftests/w3c-css/submitted
testing/xpcshell/node-http2

At any rate, it should be easy to tweak the patch as needed.
Attachment #8365750 - Flags: review?(dbaron)
(In reply to Alex Henrie from comment #105)
> Created attachment 8365750 [details] [diff] [review]
> Bug 243412. Use unprefixed box-sizing internally
> 
> David Baron (comment #90) said we need to:
> >  (1) land the enabling
> >  (2) prefix-substitute the tests
> >  (3) push the tests to the w3c repository

I realized I missed one bit:

  (1b) add a preference like those added in bug 804944

> The last patch was step 1. This patch takes care of step 2. However, I'm not
> sure that I was supposed to change files in the following directories:
> 
> browser/extensions/pdfjs
> layout/reftests/w3c-css/submitted
> testing/xpcshell/node-http2
> 
> At any rate, it should be easy to tweak the patch as needed.

The tests in layout/reftests/w3c-css/submitted were actually the primary subject of "the tests" I was talking about in part 2.

The other patches are good, but it's probably worth splitting them into multiple patches for different reviewers to look at.  In particular, I'd split them up as:

 * browser/extensions/pdfjs (r? to :bdahl)
 * rest of browser/ and toolkit/ (r? to :gavin or similar... who might ask for it to be split further)
 * mobile/android/ (r? to :mfinkle or :margaret or similar)
 * content, dom, layout, widget (r? to me)

and skip these changes, which shouldn't be touched since they're imported code:
 * testing/xpcshell/node-http2/

It's probably worth filing a separate bug for this, and marking this bug as fixed.
If you do make this a preference setting, please consider defaulting it to enabled.
The preference would be a preference for the prefixed -moz-box-sizing (not the unprefixed box-sizing), defaulting to enabled, so that authors can test their sites in advance of our dropping support for the prefixed version.  See http://dbaron.org/log/20130225-removing-prefixes
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #109)
> The preference would be a preference for the prefixed -moz-box-sizing (not
> the unprefixed box-sizing), defaulting to enabled, so that authors can test
> their sites in advance of our dropping support for the prefixed version. 
> See http://dbaron.org/log/20130225-removing-prefixes

What would be the steps for adding this?

    CSS_PROP_ALIAS(-moz-box-sizing, box_sizing, MozBoxSizing, "layout.css.prefixes.boxsizing")
and
    pref("layout.css.prefixes.boxsizing", true);
?
Maybe "layout.css.prefixes.box-sizing" for the pref name.  Other than that, comment 110 is exactly right.
Target Milestone: --- → mozilla29
Attached patch Added box-sizing preference (obsolete) — Splinter Review
Attachment #8369703 - Flags: review?(dbaron)
Attached patch Added box-sizing preference (obsolete) — Splinter Review
Well... I did a stupid, sorry about that. This one should do it!
Attachment #8369703 - Attachment is obsolete: true
Attachment #8369703 - Flags: review?(dbaron)
Attachment #8369709 - Flags: review?(dbaron)
Comment on attachment 8369709 [details] [diff] [review]
Added box-sizing preference

># HG changeset patch
># User Lukas Nordin <lukasnordin11@gmail.com>
>Bug 243412: Added box-sizing preference to control the vendor prefix; r=dbaron

Use "Add" in the commit message rather than "Added".

>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h
>--- a/layout/style/nsCSSPropList.h
>+++ b/layout/style/nsCSSPropList.h
>@@ -1384,7 +1384,7 @@
>     box_sizing,
>     BoxSizing,
>     CSS_PROPERTY_PARSE_VALUE,
>-    "",
>+    "layout.css.prefixes.box-sizing",
>     VARIANT_HK,
>     kBoxSizingKTable,
>     CSS_PROP_NO_OFFSET,

You don't want this; this puts the main property behind a pref!

r=dbaron on the other two changes
Attachment #8369709 - Flags: review?(dbaron) → review+
> Use "Add" in the commit message rather than "Added".
Ok!
> You don't want this; this puts the main property behind a pref!
Done.
Attachment #8369709 - Attachment is obsolete: true
Attachment #8369944 - Flags: review?(dbaron)
Comment on attachment 8369944 [details] [diff] [review]
Add box-sizing preference

When you get review+ with specific changes, and you make those changes, you don't need to ask for review again.

But r=dbaron again anyway.
Attachment #8369944 - Flags: review?(dbaron) → review+
Check-in needed for attachment #8369944 [details] [diff] [review]. 

(Should this request be using the checkin? flag on the attachment?)

Needs uplift as well because the original patch landed in train 29.
Keywords: checkin-needed
I landed the above patch and the prefix-substituting of the tests:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/75ac8af8c495
   https://hg.mozilla.org/integration/mozilla-inbound/rev/13f6dad08f3a

We should probably close this bug and do the substitution of our own code in other bugs (i.e., continue from attachment 836570).
Whiteboard: [parity-IE] [parity-opera] [parity-webkit][leave open] → [parity-IE] [parity-opera] [parity-webkit]
I'm not sure we really _need_ uplift for the preference thing.  Not uplifting it just means that in 29 both the prefixed and unprefixed form will be supported unconditionally, which is not fatal.  Starting with 30 people will be able to experiment with the removal of the prefixed form.  So all it'll really do is delay when we can remove the prefixed form by that one release cycle.

On the other hand, uplifting that patch won't hurt anything either; it's pretty simple...
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #118)
> We should probably close this bug and do the substitution of our own code in
> other bugs (i.e., continue from attachment 836570).

I've created bug 968029 for this purpose.
Comment on attachment 8365750 [details] [diff] [review]
Bug 243412. Use unprefixed box-sizing internally

cancelling this review request since this is now happening in bug 968029 (as described in comment 107)
Attachment #8365750 - Flags: review?(dbaron)
It's probably better not to uplift the preference, since if anyone tried it, the browser would end up somewhat broken (lacking the fixes in bug 968029).
We should probably just resolve this as FIXED, then.
Yep.

Also setting assignee to one of the many people who helped get this code in.
Assignee: bugs → alexhenrie24
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Restrict Comments: true
Keywords: qawantedverifyme
It seems this fix is already covered automatically:
https://hg.mozilla.org/mozilla-central/rev/13f6dad08f3a
Flags: in-testsuite+
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.