403519 - Cell widths don't add up to table width with colspan and marquee

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Uri Bernstein]

Attachment: testcase

In the URL, there is a white space to the right of the gray area with a blue headline and a marquee. That white space shouldn't be there.

Attached is the smallest testcase I could make - with two table rows, and a cell with colspan="2" containing a marquee. Note the white empty area on the right.

This regressed between 2007-09-24 and 2007-09-25, so I'm guessing bug 367673.

First reported by Nathaniel (נתנאל) on the mozilla.org.il forum.

Comment 1

[Daniel Holbert [:dholbert]]

The testcase triggers these assertions:

###!!! ASSERTION: didn't subtract all that we added: 'totalSPref == 0 && totalSMin == 0 && totalSNonPctPref == 0 && nonPctCount == 0 && minOutsidePref == 0 && minWithinPref == 0 && (info.prefCoord == 0 || info.prefCoord == nscoord_MAX) && (info.prefPercent == 0.0f || !spanHasNonPct)', file /scratch/work/builds/trunk.07-11-12.08-21/mozilla/layout/tables/BasicTableLayoutStrategy.cpp, line 531

###!!! ASSERTION: nscoord addition will reach or pass nscoord_MAX: '(PRInt64)a + (PRInt64)b < (PRInt64)nscoord_MAX', file ../../dist/include/gfx/nsCoord.h, line 153

###!!! ASSERTION: didn't subtract all that we added: 'space == 0 && ((l2t == FLEX_PCT_LARGE) ? (-0.001f < basis.f && basis.f < 0.001f) : (basis.c == 0 || basis.c == nscoord_MAX))', file /scratch/work/builds/trunk.07-11-12.08-21/mozilla/layout/tables/BasicTableLayoutStrategy.cpp, line 1013

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: (whoops -- testcase for different bug, accidentally posted here.)

> (See
> http://people.mozilla.com/~dholbert/tests/403511/box.html for pretty colored
> boxes)

Attaching that testcase here.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 288357 [details]
(whoops -- testcase for different bug, accidentally posted here.)

D'oh.... Wrong bug. Sorry.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch fixes the visible bug and the "didn't subtract all that we added" asserts.

Testcase still triggers lots of these:
###!!! ASSERTION: Doing nscoord subtraction with values > nscoord_MAX: 'a < nscoord_MAX && b < nscoord_MAX', file ../../dist/include/gfx/nsCoord.h, line 208

###!!! ASSERTION: nscoord subtraction will reach or pass nscoord_MAX: '(PRInt64)a - (PRInt64)b < (PRInt64)nscoord_MAX', file ../../dist/include/gfx/nsCoord.h, line 210

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch v2

This patch fixes those last few nscoord_MAX assertions.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 288372 [details] [diff] [review]
patch v2

Summary of patch:  Support cells w/ prefWidth=nscoord_MAX when computing column intrinsic widths.

Previously, we did normal arithmetic on info.prefCoord here.  But it's possible for info.prefCoord to be nscoord_MAX (e.g. using a marquee), so we need to use the NSCoordSaturating Add/Subtract functions.

After this patch, all operations on info.prefCoord in ComputeColumnIntrinsicWidths are safe.

Comment 7

[Daniel Holbert [:dholbert]]

One clarification -- the last chunk of the patch only indirectly involves info.prefCoord being nscoord_MAX.  The danger was that allocatedPref could be set to nscoord_MAX here:
>                 nscoord allocatedPref = 
>                     (info.prefCoord == nscoord_MAX ? 
>                      nscoord_MAX : 

and then we do (bad!) arithmetic on it here:
>-                nscoord spanPref = scolFrame->GetPrefCoord() + allocatedPref;

Comment 8

[Daniel Holbert [:dholbert]]

Checked in.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: reftest

Comment 10

[Daniel Holbert [:dholbert]]

Reftest checked in.

Comment 11

[Martijn Wargers (dead)]

Attachment: changed reftest to not use marquee

I think this reftest is incorrect, compare:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/403519-1.html
http://lxr.mozilla.org/seamonkey/source/layout/reftests/bugs/403519-1-ref.html
with branch builds and with IE.
I guess ideally, IE's behavior is wanted, but unfortunately, there is a big chance that won't be possible with the way marquee has to work currently.

I kind of doubt this is an accurate reftest (I have no idea how the width and margin are supposed to work in this case), but at least it doesn't use a marquee.

Comment 12

[Martijn Wargers (dead)]

dholbert and I just agreed the changed reftest I made should be added to the list, instead of replacing this one.
The problem with the reftest that was already was checked in, is that it's causing a failure with my current patch for bug 407016.
So if that patch would get in the tree, I'll just disable dholbert reftest for the time being.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 293730 [details] [diff] [review]
changed reftest to not use marquee

r=dholbert, with adding it as second reftest instead of replacing first one.

Comment 14

[Martijn Wargers (dead)]

Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.288; previous revision: 1.287
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/403519-2.html,v
done
Checking in 403519-2.html;
/cvsroot/mozilla/layout/reftests/bugs/403519-2.html,v  <--  403519-2.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/403519-2-ref.html,v
done
Checking in 403519-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/403519-2-ref.html,v  <--  403519-2-ref.htm
l
initial revision: 1.1
done

I checked that reftest in as an extra reftest.
(Like I said, I also doubt about the assumptions this reftest makes about how certain things should look like)

Comment 15

[Martijn Wargers (dead)]

I had to disable the reftest for this bug, now that I've checked in the patch for bug 407016, because it failed on MacOSX Darwin 8.8.4 qm-xserve01 dep unit test.

The latest patch for bug 407016 was succeeding for me on windows with the first reftest for this bug. I guess it might be a '1px difference' issue on the Mac.

But like we already discussed before, the first reftest from this bug is incorrect, imho.

Comment 16

[Martijn Wargers (dead)]

Attachment: reftest images showing failure on the mac

Yes, it's a pixel problem, the 'b' character is moved 1px between test and reference.

Comment 17

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

Could there be another reftest testing the same table issues but without a marquee?

Comment 18

[Martijn Wargers (dead)]

David, I added the changed reftest in comment 11 as an extra reftest, so there is a reftest still active that checks for this bug.
But I'm not really sure how correct my reftest is, either (in the sense, if Firefox should layout the reftest like that).

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> Created an attachment (id=296131) [details]
> reftest images showing failure on the mac
> 
> Yes, it's a pixel problem, the 'b' character is moved 1px between test and
> reference.

FWIW, I GDB'd through the now-disabled reftest a little, and found these sub-pixel rendering differences in the first row's cells:

Table Width:
  - Testcase:  x=0      width=52860
  - Reference: x=0      width=52860

First Cell:
  - Testcase:  x=0      width=51658
  - Reference: x=0      width=51660

Second Cell Width:
  - Testcase:  x=51778  width=722
  - Reference: x=51780  width=720

So, it's due to a 2 app-unit difference.  Our width-distribution code must be treating the two cases slightly differently.  This probably qualifies as a bug, though it's pretty minor.

I haven't yet investigated why this problem isn't showing up in the second reftest. (It may actually happen there as well, but still not trigger the 1px character shift in that case due to width differences between the two reftests.)