Support border collapse in bidi context

[kygoh]

Attempt to pass the following tests in WeasyPrint test suites:

• http://test.weasyprint.org/suite-css21/chapter17/section18/test1/
• http://test.weasyprint.org/suite-css21/chapter17/section18/test2/
• http://test.weasyprint.org/suite-css21/chapter17/section18/test3/
• http://test.weasyprint.org/suite-css21/chapter17/section18/test4/

[kygoh]

The WeasyPrint test states that:

The two tables should be identical:

The 2 rendered tables in each of the following 2 tests are not exactly identical:

• test_border_collapse_bidi_border_top
• test_border_collapse_bidi_border_bottom

There is a 1 pixel difference in the cell background and horizontal border depending on the drawing direction. If necessary it can be fixed by reversing the drawing direction for rtl:

diff --git a/weasyprint/draw.py b/weasyprint/draw.py
index 6f678358..9f38ee23 100644
--- a/weasyprint/draw.py
+++ b/weasyprint/draw.py
@@ -826,7 +826,11 @@ def draw_table(stream, table):
         draw_background(stream, row_group.background)
         for row in row_group.children:
             draw_background(stream, row.background)
-            for cell in row.children:
+            if table.style['direction'] == 'rtl':
+                cells = reversed(row.children)
+            else:
+                cells = row.children
+            for cell in cells:
                 if (table.style['border_collapse'] == 'collapse' or
                         cell.style['empty_cells'] == 'show' or
                         not cell.empty):
@@ -951,11 +955,15 @@ def draw_collapsed_borders(stream, table):
             score, style, width, color, 'top',
             (pos_x1, pos_y, pos_x2 - pos_x1, 0)))
 
-    for x in range(grid_width):
+    if table.style['direction'] == 'rtl':
+        grid_width_range = range(grid_width - 1, -1, -1)
+    else:
+        grid_width_range = range(grid_width)
+    for x in grid_width_range:
         add_horizontal(x, 0)
     for y in range(grid_height):
         add_vertical(0, y)
-        for x in range(grid_width):
+        for x in grid_width_range:
             add_vertical(x + 1, y)
             add_horizontal(x, y + 1)

[liZe]

Thanks for the pull request.

This one is really complicated! There’s at least one big problem with your PR: box position can’t depend on the direction, coordinates are always relative to the left and the top of the layout. It means that, with rtl direction, the left position has to be set on the box, even if its layout depends on its right side and the right side of its parent.

Maybe the best way to handle this correctly is to handle CSS Logical first, so that we can use left/right and start/end correctly. And maybe it would require to support CSS Writing Modes first.

That’s definitely a looooot of work! 😱

If you find a way to handle rtl border collapse without changing *_box_x functions, we’ll merge this PR with pleasure. But you’ve been warned: it may be "a bit" complex 😄.

[liZe]

(Note that I’ve also tried to fix #2055, but not pushed my code yet. I’ll clean my code and push it soon.)

[liZe]

I’ve open #2058 so that you can see what I’ve managed to get. It’s far from perfect, and as explained above I’m not sure that we can easily get a "clean" solution.

(I should have pushed this code earlier.)

Feel free to "steal" some of my code if you want!

[kygoh]

If you find a way to handle rtl border collapse without changing *_box_x functions

Currently changing content_box_x to:

WeasyPrint/weasyprint/formatting_structure/boxes.py

Lines 172 to 179 in 2b0930c

	def content_box_x(self):
	"""Absolute horizontal position of the content box."""
	if self.style['direction'] == 'rtl':
	return self.position_x + self.margin_right + self.padding_right + \
	self.border_right_width
	else:
	return self.position_x + self.margin_left + self.padding_left + \
	self.border_left_width

appears to "solve" #2055 and has not failed any test which gives the impression that there are no problems with the PR.

Appreciate if you can point out available test cases that fail it or some reference so that relevant test cases can be written.

[liZe]

Appreciate if you can point out available test cases that fail it or some reference so that relevant test cases can be written.

Here’s an example:

<div style="float: left; border-right: 100px solid black; direction: rtl">abc</div>

x is by definition the distance from the left, it doesn’t depend on the text direction. Believe me, that’s a really bad idea to change this. 😄

[kygoh]

If you find a way to handle rtl border collapse without changing *_box_x functions

Didn't change *_box_x functions but handled rtl for TableCellBox in avoid_collisions as special case.

[kygoh]

Commit 8ef14df also added test case based on #2058 (comment)

[liZe]

Thanks for your work on this.

Could you please avoid to change draw.py and boxes.py? What we want is to fix the layout. Changing what x means is a workaround, and adding some code in draw is a workaround to fix the workaround. I’m afraid to maintain this. 😄

I can help with that if you prefer!

[kygoh]

adding some code in draw is a workaround to fix the workaround

I would disagree that the code added to draw is a workaround but rather a deliberate attempt to always draw from left to right when boxes are arranged from right to left.

When leftmost cell is drawn first, the rightmost cell will "overlap" the leftmost cell if the position_x and/or width is not absolute numbers. Likewise, when rightmost cell is drawn first, the leftmost cell will "overlap" the rightmost cell.

This is not noticed in pdf as I think it's rendered in "vector" but when rendered as pixel, the one pixel difference, bounded in red, can be seen when zoom in:

1. The column_positions and column_widths values are similar but in reverse order:
   a. [3.5, 28.78125] and [25.28125, 86.015625] for top table
   b. [28.78125, 3.5] and [86.015625, 25.28125] for bottom table
2. The top table is drawn from left-to-right, the yellow background (from right cell) overlaps the aqua background.
3. The bottom table is drawn from right-to-left, the aqua background (from left cell) overlaps the yellow background.

[liZe]

This is not noticed in pdf as I think it's rendered in "vector" but when rendered as pixel, the one pixel difference, bounded in red, can be seen when zoom in:

Oh, OK, I understand now. Fighting against rasterization problems is a well-known problem in WeasyPrint. 😄

For tests, we usually build our cases in a way that positions and sizes are are integers. That’s sometimes a bit frustrating, but we prefer to have complexity in the way tests are built than in the library’s code.

For real-life documents, we usually don’t care about rasterization artifacts. The different problems we can get depend on the rendering library, OS, screen size, zoom level, and other crazy reasons (including hardware, true story 😁). So, we just can’t handle this correctly and reliably. We tried hard in the past, and it’s often caused more harm than we thought…

If you think that your code is ready to review, I’ll take care of cleaning tests!

[kygoh]

If you think that your code is ready to review

After putting in more thoughts, I would like to appeal to you to reconsider commit bb14490 or something along that line instead of the latest commit. The latest commit is merely replicating this logic:

WeasyPrint/weasyprint/formatting_structure/boxes.py

Lines 57 to 64 in bb14490

	def content_box_x(self):
	"""Absolute horizontal position of the content box."""
	if self.style['direction'] == 'rtl':
	return self.position_x + self.margin_right + self.padding_right + \
	self.border_right_width
	else:
	return self.position_x + self.margin_left + self.padding_left + \
	self.border_left_width

to layout/table.py:

WeasyPrint/weasyprint/layout/table.py

Lines 35 to 57 in a7f355c

	if table.style['direction'] == 'rtl':
	content_box_x = table.position_x + table.margin_right + \
	table.padding_right + table.border_right_width
	else:
	content_box_x = table.content_box_x()
	rows_left_x = content_box_x + border_spacing_x
	if table.style['direction'] == 'ltr':
	position_x = content_box_x
	rows_x = position_x + border_spacing_x
	for width in column_widths:
	position_x += border_spacing_x
	column_positions.append(position_x)
	position_x += width
	rows_width = position_x - rows_x
	else:
	position_x = content_box_x + table.width
	rows_x = position_x - border_spacing_x
	for width in column_widths:
	position_x -= border_spacing_x
	position_x -= width
	column_positions.append(position_x)
	rows_width = rows_x - position_x

and layout/float.py:

WeasyPrint/weasyprint/layout/float.py

Lines 158 to 168 in a7f355c

	rtl = containing_block.style['direction'] == 'rtl'
	if isinstance(containing_block, boxes.TableCellBox) and rtl:
	cb = containing_block
	cb_x = cb.position_x + cb.margin_right + cb.padding_right + \
	cb.border_right_width
	max_left_bound = cb_x
	max_right_bound = cb_x + containing_block.width
	else:
	max_left_bound = containing_block.content_box_x()
	max_right_bound = \
	containing_block.content_box_x() + containing_block.width

Although you had made it clear to avoid changing *_box_x functions, I feel putting the logic in content_box_x conveys the intention more clearly as opposed to repeating the logic in layouts.

I was naive to assume this logic applies to all boxes until you pointed out #2056 (comment). Thus, I tried a mixin approach to make it clear that only relevant boxes (TableBox and TableCellBox) handle rtl. If there are other boxes that need to handle rtl moving forward, then just inherit BidiAware.