895096 - border-collapsed table borders have inconsistent widths when device pixel scale is not 1 (zoom)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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".)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

At some zoom levels, border widths are inconsistent.

Comment 4

[Masaya Iseki[:isk](UTC+9)]

Attachment: Change CSS pixels into device pixels - WIP.patch

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

Comment 5

[Masaya Iseki[:isk](UTC+9)]

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.

Comment 6

[Ruvim Pinka]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 9

[Roman]

Attachment: Testcase including 0.5px

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%.

Comment 11

[Astley Chen (inactive)]

Are you still working on this?

Comment 12

[Masaya Iseki[:isk](UTC+9)]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[Ya-Chieh Wu (inactive)]

I am working on this.

Comment 17

[Ya-Chieh Wu (inactive)]

Attachment: wip

rewrite the patch to fix into the current codebase.

Comment 18

[Ya-Chieh Wu (inactive)]

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

Comment 19

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1379038-Change-CSS-pixels-to-device-pixels-to-be.patch

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

Comment 20

[Ya-Chieh Wu (inactive)]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 23

[Twi]

https://bugzilla.mozilla.org/show_bug.cgi?id=410959
https://bugzilla.mozilla.org/show_bug.cgi?id=895096
I believe these two bugs are duplicates of each other.

Comment 24

[Ya-Chieh Wu (inactive)]

(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.

Comment 25

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-895096-Round-border-collapsed-table-borders-to-d.patch

Comment 26

[Ya-Chieh Wu (inactive)]

Attachment: Bug-895096-Part-I-Round-border-collapsed-table-borde.patch

Comment 27

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-895096-Part-I-Round-border-collapsed-table-borde.patch

update to fit into the newest codebase.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 29

[Ya-Chieh Wu (inactive)]

(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.

Comment 30

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-895096-Part-I-Round-border-collapsed-table-borde.patch

Comment 31

[Ya-Chieh Wu (inactive)]

Attachment: Part-II-merge-the-identical-parameters.patch

Comment 32

[Ya-Chieh Wu (inactive)]

Attachment: Part-III-clean-up-twipsPerPixel-to-oneDev.patch

Comment 33

[Ya-Chieh Wu (inactive)]

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

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 36

[Ya-Chieh Wu (inactive)]

Attachment: Part I : Round-border-collapsed-table-borde.patch

Comment 37

[Ya-Chieh Wu (inactive)]

Attachment: Part-II-merge-the-identical-parameters.patch

Comment 38

[Ya-Chieh Wu (inactive)]

Attachment: Part-III-clean-up-twipsPerPixel-to-oneDev.patch

Comment 39

[Ya-Chieh Wu (inactive)]

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
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

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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