372365 - Clientwidth returns something differently now for tables

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

This was mentioned at Hendrix feedback:
http://groups.google.com/group/mozilla.feedback/browse_thread/thread/1149af0cac4ea0d9/f0f1b01485de0d1e#f0f1b01485de0d1e

This regressed between 2006-01-25 and 2006-01-26, so probably a regression from the frame display lists patch.

See also testcase, you should see 3 squares inside the gray area.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 2

[Mats Palmgren (inactive)]

Attachment: Screenshot, IE7

Comment 3

[Mats Palmgren (inactive)]

Attachment: Screenshot, Firefox 2.0.0.2

Comment 4

[Mats Palmgren (inactive)]

Attachment: Screenshot, Opera 9.02

Comment 5

[Mats Palmgren (inactive)]

Attachment: Screenshot, Webkit nightly

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The problem is that bug 317375 removed
*|*::-moz-table-outer {
...
-  border: inherit;

so that GetUsedBorder() is zero for the outer table frame.
layout/style/nsComputedDOMStyle.cpp was updated:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/style/nsComputedDOMStyle.cpp&rev=1.166&root=/cvsroot&mark=2482-2489#2477
but we need to do it for other DOM stuff that calculates dimensions too.

With this patch we get the same result as Firefox 2.0.0.2 for the testcase.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do you need to modify GetClientAreaSize? Isn't the frame already corrected by GetScrollInfo?

   nsIFrame *frame = presShell->GetPrimaryFrameFor(this);
   if (!frame) {
     return;
   }
 
+  if (frame->GetType() == nsGkAtoms::tableOuterFrame) {
+    frame = frame->GetFirstChild(nsnull);
+  }

How about moving this code to a shared static helper, say GetStyledFrameFor? You'd call it from GetScrollInfo and GetOffsetRect. Hmm, you could also share GetCurrentDoc, GetShellAt and FlushPendingNotifications calls in there.

Otherwise OK.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Nits fixed.  I found that there is a base class method that does the
flush-then-get-frame thing. Additionally, I made GetScrollInfo() always
initialize its out params and improved the documentation a bit.

There are a few code sharing improvements also in bug 111207, in case
you want to look at those at the same time... ;-)

Comment 9

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-03-15 13:43 PST.

-> FIXED

Comment 10

[Bill Gianopoulos [:WG9s]]

This may have caused bug 375003.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So how come tests were not checked in with the patch?  What's the plan for getting them in, Mats?  Is something blocking them?

Also, see bug 383898.

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #11)
> So how come tests were not checked in with the patch?  What's the plan for
> getting them in, Mats?  Is something blocking them?

I will add mochitests on bug 375003 that covers this bug and more.

> Also, see bug 383898.

Fixed by the patch on bug 375003.  Sorry for the regression.