Brian Smith pointed out to me a page he was working on which showed the following bug: when device pixel scale is not 1.0 (which is now the default on many Windows sytems), border-collapse borders are uneven widths. In general, this problem was fixed by bug 287624, which rounds all border widths to the nearest device pixel, which avoids subpixel layout leading to a border width that isn't an integral number of device pixels having the two sides of the border snap to pixel boundaries differently at different subpixel positions of the border. However, the table border-collapsing code takes these device-pixel-rounded border widths and then rounds them to *CSS* pixels, which is wrong. See the macros BC_BORDER_TOP_HALF_COORD etc., which use nsPresContext::AppUnitsPerCSSPixel as input. (There might be some other related code that needs to be cleaned up as well.) These macros should be uniformly using the device pixels to app units conversion, which would lead to the border widths being consistent. (And we should avoid the variable name "p2t", which is ancient; we should probably use "d2a" for "device pixels to app units".)

At some zoom levels, border widths are inconsistent.

This patch has still bug. The outer line width of the table(left, right, top, bottom) is not equal the inner line width of the table in this patch. The outer line is rendered at |DrawSolidBorderSegment|[1]. (poly[3].x - poly[2].x) is constant in any device pixel scale but the outer line width and the inner line width are different in some device pixel scale (they are same in some device pixel scale) I don't know why it happens. Would you please tell how to fix it? [1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3393

Sorry I have insufficient. poly[3].x - poly[2].x = const is in vertical, so poly[3].x - poly[2].x means the line width.

Is this bug duplicate of the Bug #410959? See also the similar issue: https://stackoverflow.com/questions/15612824/border-width-showing-differently-for-different-cells-in-firefox

(In reply to Ruvim Pinka from comment #6) > Is this bug duplicate of the Bug #410959? Probably, yes, but let's keep it separate for now.

Comment on attachment 8395405 [details] [diff] [review] Change CSS pixels into device pixels - WIP.patch Sorry for missing this email earlier. (In reply to Masaya Iseki[:isk](UTC+9) from comment #4) > This patch has still bug. > > The outer line width of the table(left, right, top, bottom) is not equal the > inner line width of the table in this patch. > The outer line is rendered at |DrawSolidBorderSegment|[1]. (poly[3].x - > poly[2].x) is constant in any device pixel scale but the outer line width > and the inner line width are different in some device pixel scale (they are > same in some device pixel scale) > I don't know why it happens. > Would you please tell how to fix it? I don't understand what bug you're saying the patch has. If the total border width is an odd number of device pixels, we expect the half drawn by the cell on the left or top side to be one device pixel wider than the half drawn by the bottom or right side. (That said, I didn't look at the patch closely, and it's certainly easy to get some of this scaling off. Presumably this is converting the widths in the BCPropData so that they're always device pixels rather than CSS pixels? It might help to use our new unit-based types to help enforce correct conversions, although they're not especially well documented and also not yet used much in layout code, so it would probably be a bit of work to get started on that.)

Adding a testcase which includes disappearing borders from bug 410959: zoom in to more than 200% and the lines in the second table will most likely disappear completely. Thought it's worth having this specifically included. Chrome omits 0.5px lines completely at low zooms. IE11 renders them at 100%, and fades the lines when zooming below 50% or so. Observe also the second table splitting into two parts at zoom levels below 20%.

Are you still working on this?

Sorry, now I don't have a time to work on this task.

The reason we round border widths is this: When we're drawing, we handle sub-pixel positions by snapping *edges* to the nearest pixel boundary. However, for borders, it's important that the border have the same visible width (in device pixels) no matter what its subpixel position is. So we round borders (in the style system) to an integer number of device pixels. This ensures that both sides of the border get snapped the same way when we're drawing, since they're an integer number of device pixels apart. Dividing up border-collapse borders is somewhat similar. We want to have the half that's in each cell be snapped consistently, so we should ensure each half is an integer number of device pixels, just like we ensure that for the border as a whole. Thus rounding the borders to CSS pixel rather than device pixels is wrong. The halves should be rounded to device pixels.

The code referenced in comment 14 is just a copy of what was originally written in comment 4, which describes a problem with it.

I am working on this.

rewrite the patch to fix into the current codebase.

I think I need to fix some code in nsCSSRendering::DrawTableBorderSegment.

thx for explaining the context in Comment 13. Could you help to review it?

try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c16e1eb9fac15f14f447aa31ca16303be06ed61&selectedJob=126257452

Comment on attachment 8901643 [details] [diff] [review] 0001-Bug-1379038-Change-CSS-pixels-to-device-pixels-to-be.patch >Bug 1379038 - Change CSS pixels to device pixels to be consistent for table borders. r=dbaron "Change" sounds like it could mean "convert" in this context, so instead I'd suggest: Round border-collapsed table borders to device pixels rather than CSS pixels, as for other borders, and store them (as BCPixelSize) as device pixels rather than CSS pixels. >- nscoord twipsPerPixel = NSIntPixelsToAppUnits(1, aAppUnitsPerCSSPixel); >+ nscoord d2aPerPixel = NSIntPixelsToAppUnits(1, aAppUnitsPerDevPixel); Call this variable oneDevPixel instead. >+ ? RoundFloatToPixel(((float)dashLength) / 2.0f, d2aPerPixel) In this call, use aAppUnitsPerDevPixel rather than oneDevPixel. (They're really the same, just different types.) Same in the calls to GetDashInfo, and DrawSolidBorderSegment, and DrawDashedSegment, and the other calls to RoundFloatToPixel. Please don't reformat code like this: >- DrawDashedSegment(aDrawTarget, rect, dashLength, aBorderColor, >- aAppUnitsPerDevPixel, twipsPerPixel, horizontal); >+ DrawDashedSegment(aDrawTarget, >+ rect, >+ dashLength, >+ aBorderColor, >+ aAppUnitsPerDevPixel, >+ d2aPerPixel, >+ horizontal); Leave the parameters so that you have as many as possible as fit within 78 or 79 characters (or just leave things as they are if they weren't quite like that). Also don't reformat code like this: >- DrawSolidBorderSegment(aDrawTarget, rect, aBorderColor, >- aAppUnitsPerDevPixel, twipsPerPixel); >+ DrawSolidBorderSegment( >+ aDrawTarget, rect, aBorderColor, aAppUnitsPerDevPixel, d2aPerPixel); Please add a comment, where the type is defined, explaining that BCPixelSize is in device pixels. >- nscoord offset = CalcVerCornerOffset(ownerSide, cornerSubWidth, >- maxInlineSegBSize, true, >- bStartBevel); >- >- mBStartBevelOffset = bStartBevel ? >- nsPresContext::CSSPixelsToAppUnits(maxInlineSegBSize): 0; >+ nscoord offset = CalcVerCornerOffset(ownerSide, >+ cornerSubWidth, >+ maxInlineSegBSize, >+ true, >+ bStartBevel, >+ aIter.mTable->PresContext()); >+ >+ mBStartBevelOffset = >+ bStartBevel >+ ? aIter.mTable->PresContext()->DevPixelsToAppUnits(maxInlineSegBSize) >+ : 0; Put the nsPresContext* in a local variable rather than calling aIter.mTable->PresContext() twice. Also don't reformat so much. >- LogicalRect segRect(aIter.mTableWM, >- mOffsetI - nsPresContext::CSSPixelsToAppUnits(largeHalf), >- mOffsetB, >- nsPresContext::CSSPixelsToAppUnits(mWidth), mLength); >- nscoord bEndBevelOffset = (mIsBEndBevel) ? >- nsPresContext::CSSPixelsToAppUnits(mBEndInlineSegBSize) : 0; >+ LogicalRect segRect( >+ aIter.mTableWM, >+ mOffsetI - aIter.mTable->PresContext()->DevPixelsToAppUnits(largeHalf), >+ mOffsetB, >+ aIter.mTable->PresContext()->DevPixelsToAppUnits(mWidth), >+ mLength); >+ nscoord bEndBevelOffset = >+ (mIsBEndBevel) >+ ? aIter.mTable->PresContext()->DevPixelsToAppUnits(mBEndInlineSegBSize) >+ : 0; Marking as review- because I didn't review carefully enough, because there were too many reformatting changes that made the changes hard to find. Could you also explain what you did to make sure that: * you fixed all of the callers that create BCPixelSize types to use device pixels rather than CSS pixels * you did the same for all callers that consume the types As a followup, there are a number of methods in nsCSSRendering that have an aTwipsPerPixel parameter that should now be called aAppUnitsPerDevPixel. (Some of them also have an existing aAppUnitsPerDevPixel parameter.) Please write an additional patch to clean that up (and merge the identical parameters). (In DrawDashedSegment it's even usused already.) Could you also write a separate followup patch to make these methods on nsTableRowFrame: nscoord GetBStartBCBorderWidth() const { return mBStartBorderWidth; } nscoord GetBEndBCBorderWidth() const { return mBEndBorderWidth; } and any similar methods (like the two on nsTableColFrame) return BCPixelSize rather than nscoord.

Er, after the last chunk of quoted code, I meant to say the same thing as for the next-to-last chunk of quoted code.

(In reply to David Baron :dbaron: ⌚️UTC-7 (away September 4-8) from comment #21) > Comment on attachment 8901643 [details] [diff] [review] > 0001-Bug-1379038-Change-CSS-pixels-to-device-pixels-to-be.patch > > >Bug 1379038 - Change CSS pixels to device pixels to be consistent for table borders. r=dbaron > > "Change" sounds like it could mean "convert" in this context, so instead I'd > suggest: > > Round border-collapsed table borders to device pixels rather than CSS > pixels, as for other borders, and store them (as BCPixelSize) as device > pixels rather than CSS pixels. ok. > >- nscoord twipsPerPixel = NSIntPixelsToAppUnits(1, aAppUnitsPerCSSPixel); > >+ nscoord d2aPerPixel = NSIntPixelsToAppUnits(1, aAppUnitsPerDevPixel); > > Call this variable oneDevPixel instead. ok. > >+ ? RoundFloatToPixel(((float)dashLength) / 2.0f, d2aPerPixel) > > In this call, use aAppUnitsPerDevPixel rather than oneDevPixel. (They're > really the same, just different types.) > > Same in the calls to GetDashInfo, and DrawSolidBorderSegment, and > DrawDashedSegment, and the other calls to RoundFloatToPixel. I don't really get this since |RoundFloatToPixel| consume |RoundFloatToPixel(float, nscoord,)|. So I still use oneDevPixel in my patch. > > Please don't reformat code like this: > > >- DrawDashedSegment(aDrawTarget, rect, dashLength, aBorderColor, > >- aAppUnitsPerDevPixel, twipsPerPixel, horizontal); > >+ DrawDashedSegment(aDrawTarget, > >+ rect, > >+ dashLength, > >+ aBorderColor, > >+ aAppUnitsPerDevPixel, > >+ d2aPerPixel, > >+ horizontal); > ok. > > Please add a comment, where the type is defined, explaining that > BCPixelSize is in device pixels. ok. > Put the nsPresContext* in a local variable rather than calling > aIter.mTable->PresContext() twice. Also don't reformat so much. > ok. > Could you also explain what you did to make sure that: > * you fixed all of the callers that create BCPixelSize types to use device > pixels rather than CSS pixels > * you did the same for all callers that consume the types So what I did was I searched all *CSSPixels* under layout/tables and looked into them to think how should I start. And then I realized that when all callers who want to use the border infomation, they call |GetBCBorderWidth| or |GetContinuousBCBorderWidth|... The callers won't get |BCPixelSize| directly. Instead, they get border width in appunit/*device Pixel in appunit*/. Thus, I think I only need to clear up all the functions which use to save and get border width. So I go through all the *CSSPixels* keyword under layout/tables and change them to *DevPixels*. There are only two *CSSPixel* that I leave them there which are http://searchfox.org/mozilla-central/source/layout/tables/nsTableCellFrame.cpp#329 and http://searchfox.org/mozilla-central/source/layout/tables/nsTableCellFrame.cpp#344 which I don't think I need to do anything for them. try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf54b28e54bc3bddef58e88e84fab0be8a00302&selectedJob=128235940 And for the followup patch, I will handle them. many thanks.

update to fit into the newest codebase.

(In reply to Ya-Chieh Wu from comment #24) > (In reply to David Baron :dbaron: ⌚️UTC-7 (away September 4-8) from comment #21) > > >+ ? RoundFloatToPixel(((float)dashLength) / 2.0f, d2aPerPixel) > > > > In this call, use aAppUnitsPerDevPixel rather than oneDevPixel. (They're > > really the same, just different types.) > > > > Same in the calls to GetDashInfo, and DrawSolidBorderSegment, and > > DrawDashedSegment, and the other calls to RoundFloatToPixel. > > I don't really get this since |RoundFloatToPixel| consume > |RoundFloatToPixel(float, nscoord,)|. > So I still use oneDevPixel in my patch. So aAppUnitsPerDevPixel and oneDevPixel are the same *number*. But aAppUnitsPerDevPixel is described as a conversion factor, whereas oneDevPixel is a length. I believe the thing that these functions take as an argument is a conversion factor, so they should use aAppUnitsPerDevPixel.

(In reply to David Baron :dbaron: ⌚️UTC-4 (away September 4-8) from comment #28) > So aAppUnitsPerDevPixel and oneDevPixel are the same *number*. But > aAppUnitsPerDevPixel is described as a conversion factor, whereas > oneDevPixel is a length. I believe the thing that these functions take as > an argument is a conversion factor, so they should use aAppUnitsPerDevPixel. I see. thanks for explaining this.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c712c1c59c3dc2e7d6939ddec4dc3fc5d88d800

Comment on attachment 8904829 [details] [diff] [review] 0001-Bug-895096-Part-I-Round-border-collapsed-table-borde.patch >Bug 895096 - Part I: Round border-collapsed table borders to device pixels rather than CSS pixels, as for other borders, and store them (as BCPixelSize) as device pixels rather than CSS pixels. Please fix the occurrence of two spaces in "border-collapsed table borders" to just one space. In CalcVerCornerOffset and CalcHorCornerOffset, please add the new aPresContext argument at the start rather than the end, since that's the normal order. In nsTableRowFrame::GetBCBorderWidth: >+ nsPresContext* presContext = this->PresContext(); don't use "this->". r=dbaron with that

Comment on attachment 8904832 [details] [diff] [review] Part-III-clean-up-twipsPerPixel-to-oneDev.patch The things that were previously aTwipsPerPixel should now be aOneDevPixel, not oneDevPixel, per the convention for naming function arguments. r=dbaron with that fixed

wait the result of try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c6d4a77fd4bc85dc00f571d5b9e850beddb8afa

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6a0d528af1 Part 1: Round border-collapsed table borders to device pixels rather than CSS pixels, as for other borders, and store them (as BCPixelSize) as device pixels rather than CSS pixels. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/78e432e75482 Part 2: Merge the identical parameters. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/405957d41476 Part 3: Clean up twipsPerPixel to oneDevPixel. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/5d6a0d528af1 https://hg.mozilla.org/mozilla-central/rev/78e432e75482 https://hg.mozilla.org/mozilla-central/rev/405957d41476