New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support border collapse in bidi context #2056
base: main
Are you sure you want to change the base?
Conversation
The WeasyPrint test states that:
The 2 rendered tables in each of the following 2 tests are not exactly identical:
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) |
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 |
(Note that I’ve also tried to fix #2055, but not pushed my code yet. I’ll clean my code and push it soon.) |
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! |
Currently changing WeasyPrint/weasyprint/formatting_structure/boxes.py Lines 172 to 179 in 2b0930c
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. |
Here’s an example: <div style="float: left; border-right: 100px solid black; direction: rtl">abc</div>
|
Didn't change |
To ensure tables are identical at pixel level regardless of direction
Commit 8ef14df also added test case based on #2058 (comment) |
Thanks for your work on this. Could you please avoid to change I can help with that if you prefer! |
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! |
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
to WeasyPrint/weasyprint/layout/table.py Lines 35 to 57 in a7f355c
and WeasyPrint/weasyprint/layout/float.py Lines 158 to 168 in a7f355c
Although you had made it clear to avoid changing 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 ( |
Attempt to pass the following tests in WeasyPrint test suites: