text in table cells not painted when visibility goes from collapsed to visible

RESOLVED FIXED

Status

()

RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

15 years ago
a the row visibility toggles from collapsed to visible the text is not redrawn.

fantasai, do you have an idea where that happens
(Assignee)

Comment 1

15 years ago
Created attachment 147434 [details]
testcase
(Assignee)

Comment 2

15 years ago
Created attachment 147435 [details]
the same toggling the column and not the row
(Assignee)

Comment 3

15 years ago
this became visible after my checkin for bug 77019
(Assignee)

Comment 4

15 years ago
Created attachment 147486 [details] [diff] [review]
patch
(Assignee)

Comment 5

15 years ago
Comment on attachment 147486 [details] [diff] [review]
patch

I need the change in the argument to  AdjustForCollapsingCols and
AdjustForCollapsingRows for the next step, hooking up the overflow handling
Attachment #147486 - Flags: superreview?(dbaron)
Attachment #147486 - Flags: review?(dbaron)
(Assignee)

Comment 6

15 years ago
taking, I believe the patch fixes all issues in attachment 147353 [details] from bug 77019
Assignee: nobody → bernd_mozilla
Comment on attachment 147486 [details] [diff] [review]
patch

>+        while (cellFrame) {
>+          const nsStyleDisplay* cellDisplay = cellFrame->GetStyleDisplay();
>+          if (NS_STYLE_DISPLAY_TABLE_CELL == cellDisplay->mDisplay) {
>+            nsTableCellFrame* cFrame = (nsTableCellFrame*)cellFrame;
>+            nsPoint offset;
>+            cFrame->GetCollapseOffset(aPresContext, offset);
>+            if (offset.y)
>+              cFrame->SetCollapseOffsetY(aPresContext, 0);

Why not just cut everything from "nsPoint offset" to "if (offset.y)" and change
nsTableCellFrame::SetCollapseOffset{X,Y} to pass |aOffsetX != 0| instead of
|PR_TRUE| for the last argument?
Comment on attachment 147486 [details] [diff] [review]
patch

>+              cFrame->SetCollapseOffsetY(aPresContext, 0);

Also, why is this 0 and not -aYGroupOffset?

Comment 9

15 years ago
Created attachment 147842 [details]
Global testcase
(Assignee)

Comment 10

15 years ago
please notice that the global testcase invokes the border collapse code and the
bc border painting is broken.
(Assignee)

Comment 11

15 years ago
Created attachment 147916 [details] [diff] [review]
revised patch

Thanks David for that nifty code in SetCollapseOffset.

>Also, why is this 0 and not -aYGroupOffset?

Because the uncollapsed position is 0 see
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.cpp#421
ff

The idea is that the cell content of rowspanning cells is moved up. The cells
are moved relative to the row. So if a row is not collapsed we dont need to
move relative to the row.

The comment in nsTableCellFrame.cpp leaves however a big question mark how
correct the whole approach is. 

I dont read from CSS2.1:

The 'visibility' property takes the value 'collapse' for row, row group,
column, and column group elements. This value causes the entire row or column
to be removed from the display, and the space normally taken up by the row or
column to be made available for other content. Contents of spanned rows and
columns that intersect the collapsed column or row are clipped. The suppression
of the row or column, however, does not otherwise affect the layout of the
table. This allows dynamic effects to remove table rows or columns without
forcing a re-layout of the table in order to account for the potential change
in column constraints.

that cells that originate in a row and are rowspanning should be displayed if
the originating row is collapsed. I see only that cells that span into that row
should be shortened.

The whole code is pretty old and did previously only get executed on the
initial reflow. It lacks currently any overflow reporting. I would prefer to go
with that reset to 0 now and fix or even better toremove that shift buisiness
when reporting the correct overflow areas.
(Assignee)

Updated

15 years ago
Attachment #147486 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #147916 - Flags: superreview?(dbaron)
Attachment #147916 - Flags: review?(dbaron)

Comment 12

15 years ago
Created attachment 147933 [details]
Global testcase with both border-collapse models
Attachment #147842 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
fix checked in. I filed bug 242998 for the overflow area and bug 242997 for the
broder collapse issue.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Attachment #147486 - Flags: superreview?(dbaron)
Attachment #147486 - Flags: review?(dbaron)
You need to log in before you can comment on or make changes to this bug.