This is sort of a follup on the discussion in bug 173051 comment 17 through bug 173051 comment 23. I think we should round CSS border widths to the nearest device pixel in the style system so that a given border width always rounds to the same size. The idea here is that we're solving this problem only for borders and not for anything else -- since: 1) for things like widths we want to ensure that 4 * 25% is 100%, not a few pixels less, and 2) we don't want consistency of heights for things handled by (1), since that creates gaps like in bug 173051. roc points out that we should be rounding to device pixels, not CSS pixels. This might be a little hard for printers given our current code.

(And, FWIW, it was only my original misunderstanding of the patch in bug 173051 that made me think it would worsen this situation -- I think it has no effect.)

*** Bug 295781 has been marked as a duplicate of this bug. ***

what about anti-aliasing here ? (thinking of cairo)

You want to avoid antialiasing here because it will produce poor-looking output.

Not only is this a problem with length-based sizes (e.g. pt, pc, in, cm), but it is also a problem with relative sizes (e.g. em, %). I just sorta noticed this when I was trying to convert a stylesheet to use all relative sizes, but I can't without getting odd renderring differences of how big .1em or .15em is. Just to add, Konqueror does this, so maybe you can take a gander if you need advice (doubtful).

*** Bug 321310 has been marked as a duplicate of this bug. ***

This is a proposal patch. I think that we should round the border width and the outline width to device unit. And the outline inside edge should be out of border edge. This patch bases on outside at drawing the border, and it bases on inside at drawing the border. By this patch, the padding or the content width has a rounding problem. But I think that it is not more important than this bug.

Comment on attachment 249631 [details] [diff] [review] Patch rv1.0 No, the point of this bug was to do the rounding in the style system so that we take that rounding into account during layout.

(In nsRuleNode, most likely.)

(In reply to comment #10) > (In nsRuleNode, most likely.) > Thank you for your review and the hint. I'll retry.

This patch doesn't break the computed value. But the layout code always get actual values for the device.

The previous patch breaks the printing.

Comment on attachment 249677 [details] [diff] [review] Patch rv2.1 This patch has a crash bug...

No, I want us to change the computed value.

(In reply to comment #15) > No, I want us to change the computed value. > Wow, really? http://www.w3.org/TR/CSS21/cascade.html#actual-value > 6.1.4 Actual values > > A used value is in principle the value used for rendering, but a user agent may not be able to make use of the value in a given environment. For example, a user agent may only be able to render borders with integer pixel widths and may therefore have to approximate the computed width, or the user agent may be forced to use only black and white shades instead of full colour. The actual value is the used value after any approximations have been applied. I understand that the computed value is not rounded value by device...

I think you're right -- probably it is better the way you had it, although it is more complicated. But there shouldn't need to be any nsCSSRendering changes -- only changes to nsComputedDOMStyle and nsRuleNode/nsStyleStruct -- are those what broke printing?

(In reply to comment #17) > are those what broke printing? Yes. We should not |IntScaledPixelsToTwips| for rounding the borders in nsCSSRendering.

(In reply to comment #18) > We should not |IntScaledPixelsToTwips| Oops, I wanted to say as "we should not use |IntScaledPixelsToTwips|...

The first change in nsCSSRendering still looks substantive -- like it would break printing. (Or does it fix something? If so, why did nobody notice?)

A few other issues: * GetComputedBorderWidth won't work correctly if a border has been rounded down to zero. * You don't have an equivalent change for the computed values of outline-width * The macro ROUND_TO_ACTUAL_LENGTH doesn't explain itself very well; also since it's in a header file it should probably have an NS_ at the start, so I'd call it NS_ROUND_BORDER_TO_PIXELS.

(In reply to comment #21) > A few other issues: > > * GetComputedBorderWidth won't work correctly if a border has been rounded down > to zero. The border is not rounded to zero. #define ROUND_TO_ACTUAL_LENGTH(l,tpp) \ ((l) == 0) ? 0 : PR_MAX((tpp), (l) / (tpp) * (tpp)) > * You don't have an equivalent change for the computed values of outline-width The computed value of outline-width is computed from specified value directly in nsComputedDOMStyle.cpp. > * The macro ROUND_TO_ACTUAL_LENGTH doesn't explain itself very well; also since > it's in a header file it should probably have an NS_ at the start, so I'd call > it NS_ROUND_BORDER_TO_PIXELS. > O.K. (In reply to comment #20) > The first change in nsCSSRendering still looks substantive -- like it would > break printing. (Or does it fix something? If so, why did nobody notice?) The other points only be cleaned up.

(In reply to comment #22) > The border is not rounded to zero. > #define ROUND_TO_ACTUAL_LENGTH(l,tpp) \ > ((l) == 0) ? 0 : PR_MAX((tpp), (l) / (tpp) * (tpp)) That deserves at least a comment, since it's a pretty major assumption that we could easily change in the future. But actually, we should probably get it right now. Looking at other browsers: * WinIE rounds to nearest, except rounds up to 1px for anything > 0 * Konqueror/Linux does the same as WinIE * Opera/Linux rounds to nearest * Safari rounds down So rounding up doesn't seem very compatible. I would suggest following either WinIE+Konqueror or Opera, probably the former. But even with that you need to document that GetComputedBorderWidth works correctly only thanks to your choice of rounding behavior.

Er, oh, I didn't read your code carefully enough -- you are doing that.

Er, no, you're not -- you're rounding down, except rounding up to 1. You should round to nearest, except rounding anything greater than zero up to one -- and probably have a comment explaining that.

Oops, I didn't read after comment 23, sorry.

> #define NS_ROUND_BORDER_TO_PIXELS(l,tpp) \ > ((l) == 0) ? 0 : PR_MAX((tpp), ((l) + 0.5) / (tpp) * (tpp)) Ah... Should it be |((l) <= 0) ? 0 :...| ?

Oops, the previous patch is wrong, sorry.

(In reply to comment #29) > > #define NS_ROUND_BORDER_TO_PIXELS(l,tpp) \ > > ((l) == 0) ? 0 : PR_MAX((tpp), ((l) + 0.5) / (tpp) * (tpp)) > > Ah... Should it be |((l) <= 0) ? 0 :...| ? The parser ensures that it is never less than zero: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsCSSParser.cpp&rev=3.333&mark=4461-4466,4676-4678#4348 (and hopefully the content code does the same for any HTML things that map to border). That said, it doesn't hurt to check again.

I tested the patch rv2.5. The result looks like same as IE.

Comment on attachment 249696 [details] [diff] [review] Patch rv2.5 >Index: layout/base/nsCSSRendering.cpp > /* Get our conversion values */ >- nscoord twipsPerPixel = aPresContext->IntScaledPixelsToTwips(1); >+ nscoord twipsPerPixel = NSIntPixelsToTwips(1, aPresContext->PixelsToTwips()); This change is substantive. Is it breaking something or fixing something? (It ought to either break or fix printing on Windows -- although maybe with cairo we do that stuff differently now.) >Index: layout/generic/nsImageFrame.cpp >+ nsRecessedBorder(nscoord aBorderWidth, nsPresContext* aContext) >+ : nsStyleBorder(aContext) Please use aPresContext rather than aContext. >Index: layout/style/nsStyleStruct.h >+// The boder width will be rounded to nearest value. But if the width is lesser >+// than one device pixel and the specified value is greater than zero, it will >+// be one device pixel. >+#define NS_ROUND_BORDER_TO_PIXELS(l,tpp) \ >+ ((l) == 0) ? 0 : PR_MAX((tpp), ((l) + ((tpp) / 2)) / (tpp) * (tpp)) I'd change this comment to: Border widths are rounded to the nearest integer number of pixels, but values between zero and one device pixels are always rounded up to one device pixel. (The minimum to correct the English would be s/boder/border/ and s/lesser/less/, but the use of future tense and of the singular are awkward in this context.) >- // Get the computed border width for a particular side, in twips. Note that >+ // Get the actual border width for a particular side, in twips. Note that > // this is zero if and only if there is no border to be painted for this > // side. That is, this value takes into account the border style. >+ // Get the computed border width for a particular side, in twips. Note that >+ // this is zero if and only if there is no border to be painted for this >+ // side. That is, this value takes into account the border style. You should mention that this is true because of the NS_ROUND_BORDER_TO_PIXELS macro, so somebody searching for that macro will find it. >- // mComputedBorder holds the CSS2.1 computed border-width values. In >+ // mActualBorder holds the CSS2.1 actual border-width values. In > // particular, these widths take into account the border-style for the >- // relevant side. >+ // relevant side. And the values are rounded by the size of device pixcel. s/. And/ and/ s/by the size of/to the nearest/ > // mBorder holds the nscoord values for the border widths as they would be if > // all the border-style values were visible (not hidden or none). This > // member exists solely so that when we create structs using the copy > // constructor during style resolution the new structs will know what the > // specified values of the border were in case they have more specific rules > // setting the border style. Note that this isn't quite the CSS specified > // value, since this has had the enumerated border widths converted to > // lengths, and all lengths converted to twips. But it's not quite the >- // computed value either; mComputedBorder is that. >+ // computed value either. If you need to get computed value, you can get by >+ // GetComputedBorderWidth method. I'd just remove the "; mComputed border is that" and not add anything >+ // Note that they are specified values, you can get the actual values by >+ // GetOutlineWidth and GetOutlineOffset. You cannot get the computed values >+ // directly. s/they are/these are/ s/, you/. You/ s/by/using/ or s/by/with/ s/. You/. You/ (two spaces) >- aOffset = mOutlineOffset.GetCoordValue(); >+ aOffset = NS_ROUND_BORDER_TO_PIXELS(mOutlineOffset.GetCoordValue(), >+ mTwipsPerPixel); Given that this macro uses the value multiple times, you should save mOutlineOffset.GetCoordValue() in a variable and then use the variable inside the macro. >+ // This value is actual value, so it's rounded by the device pixel. s/is/is the/ s/by the/to the nearest/ You'll have r+sr=dbaron if you explain the very first change (see my first comment above).

>>Index: layout/base/nsCSSRendering.cpp >> /* Get our conversion values */ >>- nscoord twipsPerPixel = aPresContext->IntScaledPixelsToTwips(1); >>+ nscoord twipsPerPixel = NSIntPixelsToTwips(1, aPresContext->PixelsToTwips()); > > This change is substantive. Is it breaking something or fixing something? (It > ought to either break or fix printing on Windows -- although maybe with cairo > we do that stuff differently now.) If |IntScaledPixelsToTwips| is used at printing, the double/groove/ridge style borders will be solid line when the border width is less than the 'virtual' CSS pixel. By this patch, the very thin border can be the styled line if it has enough width for printing.

Comment on attachment 249758 [details] [diff] [review] Patch rv2.5.1 OK, I see. That pixelsToTwips is used for rounding, and it makes that one consistent with the others. >- // mComputedBorder holds the CSS2.1 computed border-width values. In >+ // mActualBorder holds the CSS2.1 actual border-width values. In > // particular, these widths take into account the border-style for the >- // relevant side. >+ // relevant side and the values are rounded to the nearest device pixcel. s/pixcel/pixel/ r+sr=dbaron

Thank you dbaron, for your long review! I have a worry. I think that we need same rounding process for margin, padding, width, height, and other properties that has length value. How do you think it?

checked-in, thank you.

We *might* want it for margin and padding; we don't want it for the others. We use sub-pixel coordinates to allow things like 2 * 50% to reliably both: * cover all of the parent * not wrap (That said, the way we do it today doesn't work perfectly for things like 4 * 25%.)

I see... But it sounds like difficult things...

http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSRendering.cpp#3522 warning: unused variable ‘p2t’

(In reply to comment #40) > http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSRendering.cpp#3522 > warning: unused variable ‘p2t’ > Thank you, that is fixed by the patch of bug 365336.