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

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: dbaron, Assigned: ywu, Mentored)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(5 attachments, 9 obsolete attachments)

444 bytes, text/html
Details
868 bytes, text/html
Details
49.46 KB, patch
ywu
: review+
Details | Diff | Splinter Review
12.41 KB, patch
ywu
: review+
Details | Diff | Splinter Review
21.49 KB, patch
ywu
: review+
Details | Diff | Splinter Review
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".)
Posted file testcase
At some zoom levels, border widths are inconsistent.
Duplicate of this bug: 891322
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
Duplicate of this bug: 946527
Summary: border-collapsed table borders have inconsistent widths when device pixel scale is not 1 → border-collapsed table borders have inconsistent widths when device pixel scale is not 1 (zoom)
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
Assignee: nobody → iseki.m.aa
Attachment #8395405 - Flags: feedback?(dbaron)
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.
(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.
Blocks: 410959
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.)
Attachment #8395405 - Flags: feedback?(dbaron) → feedback-

Comment 9

5 years ago
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%.
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]

Updated

4 years ago
Duplicate of this bug: 1201114
Priority: -- → P3
Are you still working on this?
Flags: needinfo?(iseki.m.aa)
Sorry, now I don't have a time to work on this task.
Flags: needinfo?(iseki.m.aa)
Assignee: iseki.m.aa → nobody
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.
(Assignee)

Comment 16

2 years ago
I am working on this.
Assignee: nobody → ywu
(Assignee)

Comment 17

2 years ago
Posted patch wip (obsolete) — Splinter Review
rewrite the patch to fix into the current codebase.
Attachment #8395405 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
I think I need to fix some code in nsCSSRendering::DrawTableBorderSegment.
(Assignee)

Updated

2 years ago
Blocks: 1379038
(Assignee)

Comment 19

2 years ago
thx for explaining the context in Comment 13. Could you help to review it?
Attachment #8901100 - Attachment is obsolete: true
Attachment #8901643 - Flags: review?(dbaron)
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.
Attachment #8901643 - Flags: review?(dbaron) → review-
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 hidden (me-too)
(Assignee)

Comment 24

2 years ago
(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.
(Assignee)

Comment 25

2 years ago
Attachment #8901643 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Attachment #8904409 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
update to fit into the newest codebase.
Attachment #8904474 - Attachment is obsolete: true
(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.
(Assignee)

Comment 29

2 years ago
(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.
(Assignee)

Comment 30

2 years ago
Attachment #8904479 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1397119
(Assignee)

Updated

2 years ago
Attachment #8904829 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8904830 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8904832 - Flags: review?(dbaron)
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
Attachment #8904829 - Flags: review?(dbaron) → review+
Attachment #8904830 - Flags: review?(dbaron) → review+
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
Attachment #8904832 - Flags: review?(dbaron) → review+
(Assignee)

Comment 36

2 years ago
Attachment #8904829 - Attachment is obsolete: true
Attachment #8907389 - Flags: review+
(Assignee)

Comment 37

2 years ago
Attachment #8904830 - Attachment is obsolete: true
Attachment #8907390 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8904832 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 40

2 years ago
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
Keywords: checkin-needed
Depends on: 1485179
Depends on: 1508921
You need to log in before you can comment on or make changes to this bug.