363144 - frames are misplaced (reflow branch regression)

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Oleg Romashin (:romaxa)]

After landing reflow branch patch
http://www.mtv.de/home/index_eplus3.php
Left borders of the frames are move to right. Right borders of the frames are
overlaps by the green backgrounds.

See screen shots in attachments.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Screenshot before reflow branch patch

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: After reflow brunch commit

Comment 3

[Boris Zbarsky [:bzbarsky]]

It looks like those cells are ending up with a different width now (97 instead of 72 px over here).  The cell has width="72".

Would be nice to have a testcase.

Chances are, bug 363146 is the same thing.

Comment 4

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

This works for me now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I bet bug 363329 fixed this... (and bug 363146, probably).

Comment 6

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

Actually, no, it doesn't work for me now -- I was looking at the wrong part of the page.

Comment 7

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

Not fixed by bug 363329 either.  A testcase would be useful.

Comment 8

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

...nor is it fixed by bug 363874.

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase

Comment 10

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

That testcase is displayed much closer to the way other browsers display it after the patches to bug 363329 and bug 363874 -- other browsers that display the MTV page correctly differ on the testcase, and we're within the bounds of those differences.  So I don't think it reflects the problem that remains after those patches.

Comment 11

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

... but it seems like those patches broke some previously-functioning behavior, which could be related.

Comment 12

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

I think this will be fixed by the correct fix to bug 363874.

Comment 13

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

Attachment: patch

I initially tried the approach in bug 363874 comment 7, but it turns out it doesn't match what WinIE does -- what WinIE does is similar to this code, except with a discontinuity that no other browsers have (it uses entirely different ratios depending on whether there's any pref width to distribute) -- and discontinuities are bad since they mean that small changes in something like font size can cause major changes in layout.  This does something more like what we used to do, although it doesn't add the behavior in bug 363329 comment 31, which we used to do and which Konqueror also does.

This also fixes the mistake in bug 363329 comment 34.

Comment 14

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

The above patch is a diff against a tree that already has the patches for bug 363329 and bug 365173.

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 249872 [details] [diff] [review]
patch

Works ok

Comment 16

[Bernd]

Comment on attachment 249872 [details] [diff] [review]
patch

This snippet would benefit from a comment what is the concept behind the change.

+            nscoord minWithinPref =
+                PR_MIN(info.minCoord, totalSPref - totalSMin);
+            NS_ASSERTION(minWithinPref >= 0, "neither value can be negative");
+            info.minCoord -= minWithinPref;

// Distribute any min that fits under the pref width
+                // within the difference between min and pref width.

seems to terse


What I understand is:

totalSPref - totalSMin is the excess of accumulated prefwidth over accumulated minwidth. If the cell has also min width to distribute there are additional constraints how the min width should be distributed.
r+ if this understanding is correct

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 249872 [details] [diff] [review]
patch

>+++ BasicTableLayoutStrategy.cpp	2006-12-28 12:49:04.000000000 -0500
>+            nscoord totalSPref = 0, totalSMin = 0, totalSNonPctPref = 0;

Maybe add some docs about what the variables mean?

>+            nscoord minWithinPref =

I'd like to see this documented too.  The naming is not exactly transparent...

I'd like to see those comments before I can sr.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 249872 [details] [diff] [review]
patch

sr- pending those comments.

Comment 19

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

Attachment: patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 251317 [details] [diff] [review]
patch

Awesome.  Thanks for the documentation!

sr=bzbarsky

Comment 21

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

Checked in to trunk.