363874 - Weird center behaviour on www.nu.nl

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:
- Go to http://www.nu.nl/ and scroll down to the bottom
- Increase the font-size with ctrl+scroll wheel 

Result: the middle and right column will be torn apart and the line with links on the bottom are not centered and not wrapped (at least not centered equally with balkje.gif). The TD follows the width of the unwrapped text on the bottom when it resizes. And the width will follow an even weirder pattern when the font-size is decreased.

In branch the TD is smaller and has always the same width and the div on the bottom and the image "balkje.gif" are correctly centered.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot of the problem

Comparison branch (on top) and trunk (bottom).

Comment 2

[Martijn Wargers (dead)]

Attachment: Partly stripped down

I've partly stripped down the site in question, but I think this could very well be similar to bug 363329, because when I remove the colspan="2", everything shows normal again.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Thanks, I did some efforts to make a testcase myself but halfway I realized that there was a big chance that it was a dupe of something because I also saw it on some other sites and I wouldn't have been the only one who noticed it.

Comment 4

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

Attachment: patch

I noticed this mistake a few days ago, and it fixes this bug.  (Note that I tested in a tree that also had the patch to bug 363329, which reindents this code, but this patch was generated in a tree without that patch.)

This mistake was causing and pref-width distributed from a spanning cell to a column that contains a cell with a specified width to be treated as though it were a specified width rather than as a pref-width.  (Specified widths on spanning cells, are, based on my memory of my testing, treated like pref widths.)

Comment 5

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

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

So this isn't right either.  We actually do need to distinguish between specified and non-specified widths for spanning cells.

Comment 6

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

Or maybe the old code was even correct, given existing behavior handling:

<table border>
  <tr><td colspan="2">aoeui aoeui aoeui aoeui aoeui aoeui aoeui</td></tr>
  <tr><td width="75">x 75</td><td width="75">xxxxx 75</td></tr>
</table>

(The table ends up at the width of the top cell.)

Comment 7

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

I think the old code there is correct -- what needs to happen to fix this bug is that when we distribute the width of a spanning cell across its columns we need to distribute min width first, account for the effects of that min distributing bumping up pref width (in terms of the amount of pref width to distribute and what ratios to use for the distribution), and then distribute the pref width.

Comment 8

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

Then again, that business of distributing min first and recomputing for pref is something that only WinIE does and no other browsers have done, and it has some rather messy consequences.

Comment 9

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

So, actually, after writing some testcases and looking at behavior in IE, this bug seems like it's WONTFIX / INVALID.  If I make the font size bigger in WinIE (which requires Tools -> Internet Options -> General -> Appearance -> Accessibility -> Ignore font sizes specified on Web pages, I see the same problem that I see in Mozilla on Linux (where my default font is a bit wider for a given size).

Comment 10

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

Marking invalid.

If the page wants to fix this, they can specify a width on the table cell at the bottom that has colspan="2" so that the text inside that cell wraps.

Comment 11

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

Actually, reopening; just because IE does this doesn't mean we have to emulate it.

Comment 12

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

Attachment: testcase

Comment 13

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

Attachment: reference rendering for testcase

Comment 14

[Ria Klaassen (not reading all bugmail)]

I notified the webmaster in any case.

Comment 15

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #9)
> So, actually, after writing some testcases and looking at behavior in IE, this
> bug seems like it's WONTFIX / INVALID.  If I make the font size bigger in WinIE
> (which requires Tools -> Internet Options -> General -> Appearance ->
> Accessibility -> Ignore font sizes specified on Web pages, I see the same
> problem that I see in Mozilla on Linux (where my default font is a bit wider
> for a given size).
>
Yes, I see this too. Firefox displayed the site a lot better than Internet Explorer and I took it for granted because I was used to it.
I considered it as a bug (because it looks less good with an increased font-size) but maybe it isn't. It's just a personal opinion. 

Comment 16

[Ria Klaassen (not reading all bugmail)]

Probably the site owners don't read e-mails. I got no reply. And the form on the site doesn't send.

Comment 17

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

Attachment: patch

This fixes the bug.  I haven't written extensive tests for it yet; I think I want to wait on that until I'm confident the algorithm is Web-compatible.  (I'll incorporate the test above, though.)

Reftests pass.

I converted storage of the specified width from a boolean to a coordinate.  If I keep that change, I want to convert PrefPercent to SpecPercent as a followup patch.  However, given the way I wrote the column distribution code, I think it will actually be simpler to undo that change and go back to using a boolean.

Comment 18

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

Attachment: patch

This is the patch I mentioned when attaching the previous one; I think this way is a good bit simpler, despite my initial expectation that the other way would be.

This patch does the following:
 * cleans up the member accessors in nsTableColFrame.h by consolidating a few operations that are always done together in order to simplify them, and removing some then-unneded getters (and changes BasicTableLayoutStrategy to match)
 * adds an mSpanHasSpecifiedCoord to nsTableColFrame and changes the accumulation code (that is, the code that transfers the Span values into the main values, which is now moved into a method on nsTableColFrame) to accumulate it
 * fixes a |hasSpecifiedWidth = PR_TRUE| for handling of 'max-width' in GetWidthInfo that I'm pretty convinced is bogus (the patch even contains a reftest for the cases where it doesn't make sense in case anyone is tempted to do the same thing in the future).  (Note that we still don't handle 'max-width' correctly, but this at least makes us do less rather than do silly things.)
 * changes the meaning of the Span* members of nsTableColFrame to be total widths rather than additions to the current width (which is necessary for the following item)
 * changes the code to distribute the widths of spanning cells to pay attention to whether the cell has a specified width.  If it does, then we do all the calculations based on the value that would contribute to the preferred width once a specified width is introduced.

I've seen a bunch of other bugs that are regressions from bug 300030 that I think are duplicates of this one; I expect this patch will fix them too, although I haven't hunted around for them yet.

Comment 19

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

Attachment: patch

Pretty much the same as the previous patch, but remove an unused macro from the first approach, and add some more comments to nsTableColFrame (replacing the ones I removed, and more).

Comment 20

[Bernd]

I will do the review over the weekend

Comment 21

[Bernd]

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

r+

I am not sure at two places:

+  void AddSpanCoords(nscoord aSpanMinCoord, nscoord aSpanPrefCoord,
+                     PRBool aSpanHasSpecifiedCoord) {
+    NS_ASSERTION(aSpanMinCoord <= aSpanPrefCoord,
+                 "intrinsic widths out of order");
+
+    if (aSpanHasSpecifiedCoord && !mSpanHasSpecifiedCoord) {
+      mSpanPrefCoord = mSpanMinCoord;

shouldn't that take also into account  the newly set aSpanMinCoord which influences later mSpanMinCoord
 
+      mSpanHasSpecifiedCoord = PR_TRUE;
+    }
+    if (!aSpanHasSpecifiedCoord && mSpanHasSpecifiedCoord) {
+      aSpanPrefCoord = aSpanMinCoord; // NOTE: modifying argument
+    }

what happens if mSpanMinCoord is larger than aSpanMinCoord?

If you could explain what happens in those two scenarios it would be good.

Comment 22

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

(In reply to comment #21)
> shouldn't that take also into account  the newly set aSpanMinCoord which
> influences later mSpanMinCoord

It doesn't need to, since the new aSpanPrefCoord is >= the aSpanMinCoord, and we'll take that into account.

> +      mSpanHasSpecifiedCoord = PR_TRUE;
> +    }
> +    if (!aSpanHasSpecifiedCoord && mSpanHasSpecifiedCoord) {
> +      aSpanPrefCoord = aSpanMinCoord; // NOTE: modifying argument
> +    }
> 
> what happens if mSpanMinCoord is larger than aSpanMinCoord?

I'm not sure why that's an interesting case.  Nothing should happen, since the current mSpanMinCoord is already large enough.

> If you could explain what happens in those two scenarios it would be good.

I'll add some more comments to AddCoords, and point to them from AddSpanCoords:
   * Note that the implementation of this functions is a bit tricky
   * since mPrefCoord means different things depending on
   * whether mHasSpecifiedCoord is true (and likewise for aPrefCoord and
   * aHasSpecifiedCoord).  If mHasSpecifiedCoord is false, then
   * all widths added had aHasSpecifiedCoord false and mPrefCoord is the
   * largest of the pref widths.  But if mHasSpecifiedCoord is true,
   * then mPrefCoord is the largest of (1) the pref widths for cells
   * with aHasSpecifiedCoord true and (2) the min widths for cells with
   * aHasSpecifiedCoord false.

Comment 23

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

Checked in to trunk.

Comment 24

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/bugs&command=DIFF_FRAMESET&file=reftest.list&rev1=1.25&rev2=1.26&root=/cvsroot