Closed Bug 363874 Opened 14 years ago Closed 14 years ago

Weird center behaviour on www.nu.nl

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha3

People

(Reporter: ria.klaassen, Assigned: dbaron)

References

()

Details

(Keywords: regression, Whiteboard: [patch])

Attachments

(5 files, 3 obsolete files)

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.
Comparison branch (on top) and trunk (bottom).
Attached file 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.
Depends on: 363329
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.
Attached patch patch (obsolete) — Splinter Review
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.)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #249639 - Flags: superreview?(bzbarsky)
Attachment #249639 - Flags: review?(bernd_mozilla)
Component: Layout → Layout: Tables
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Whiteboard: [patch]
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.
Attachment #249639 - Flags: superreview?(bzbarsky)
Attachment #249639 - Flags: review?(bernd_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.)
Flags: blocking1.9?
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.
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.
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).
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
Actually, reopening; just because IE does this doesn't mean we have to emulate it.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attachment #249639 - Attachment is obsolete: true
I notified the webmaster in any case.
(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. 

Probably the site owners don't read e-mails. I got no reply. And the form on the site doesn't send.
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #258610 - Attachment is obsolete: true
Attachment #258620 - Flags: superreview?(roc)
Attachment #258620 - Flags: review?(bernd_mozilla)
Attached patch patchSplinter Review
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).
Attachment #258620 - Attachment is obsolete: true
Attachment #258623 - Flags: superreview?(roc)
Attachment #258623 - Flags: review?(bernd_mozilla)
Attachment #258620 - Flags: superreview?(roc)
Attachment #258620 - Flags: review?(bernd_mozilla)
Status: REOPENED → ASSIGNED
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha3
I will do the review over the weekend
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.
Attachment #258623 - Flags: review?(bernd_mozilla) → review+
Attachment #258623 - Flags: superreview?(roc) → superreview+
(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.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 374819
Depends on: 374934
Depends on: 375058
Depends on: 377040
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.