312770 - Table too wide because of marquee

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

At the url, the tables surrounding the marquees have become wider since the fix
for bug 277208. That should not have happened.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

This happens because for some reason the offsetWidth of a display:table div is
calculated including the margins it has, I think that is an incorrect thing to
do.

Comment 3

[Martijn Wargers (dead)]

Testcase2 should give approximately/precisely the same value for the two alerts,
not something completely different.

Comment 4

[Martijn Wargers (dead)]

So I guess the offsetWidth gets calculated here:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#579
and for some reason nsTableOuterFrame.cpp is used as frame or something like that?

Comment 5

[Bernd]

Martijn, tables have a very bizarre frame structure:

the outer table frame
  the caption 
  the inner table frame

The inner table frame is the frame that corresponds to the table tag. The outer
table frame is a place holder that inherits a couple of style properties from
the the table frame (http://lxr.mozilla.org/seamonkey/source/layout/style/ua.css#60)
However if you look fromt the parent frame point of view it sees first the outer
table frame. Currently the outer table frame includes the margin around the
inner table frame and caption and adjusts the margins for the different
caption-side scenarios.

Comment 6

[Bernd]

>sees first the outer table frame 

with a 0 margin, so it probably reports the outer width which includes the table
margin as the offset width. I think Boris did file a bug about this but I can't
find it now.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase3

Sorry, my previous analysis was wrong (although it still is a bug, but it is
not what is causing this bug).

This bug happens because of this line:
http://lxr.mozilla.org/seamonkey/source/layout/style/xbl-marquee/xbl-marquee.xml#231

this.innerDiv.style.padding = "0px";
Here, the padding is changed and that seems to trigger an incremental reflow
bug, which you can see in the testcase3 that I attached.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch1

Well, this fixes it for the marquees on the url, but for particular pages with
the same layout and where there is a script which sets marquee.innerHTML to
some other text dynamically, this bug could still be seen.
The border-collapse: collapse; rule is necessary, because the marquees were
also a bit too high on the page, and I think this also fixes the remaining
issue in bug 269257.
The xbl-marquee.css print rule is for printing. Alhough IE6 doesn't wrap on
print preview, I think it's more print-friendly to wrap.

Comment 9

[Martijn Wargers (dead)]

The patch also fixes the issue mentioned in bug 276729, comment 8.

Comment 10

[Bernd]

>border-collapse: collapse;
means asking for trouble

Comment 11

[Martijn Wargers (dead)]

(In reply to comment #10)
> >border-collapse: collapse;
> means asking for trouble
What trouble?
What should I use instead? border:0px?

Comment 12

[Boris Zbarsky [:bzbarsky]]

border-collapse is not a happy camper in Gecko, unfortunately.  :(  There are
lots of bugs, esp with incremental stuff.

Martijn, I'm not sure I follow what the patch is doing...

Comment 13

[Bernd]

Martijn, border collapse, short BC, is 160k of nearly assembly code masceraded
as C++ inside nsTableFrame.cpp. See http://en.wikipedia.org/wiki/Katorga when
people work on this code. It needs to be rewritten. If you can acchieve the same
without border-collapse then do it. 

Comment 14

[Martijn Wargers (dead)]

Attachment: testcase4

Well, I need the border-collapse: collapse rule for this testcase. I get the
following results in various browsers:
Browser 			      offsetwidth offsetheight
IE6:				      261	  19
Mozilla1.7			      261	  20
patched trunk without border-collapse 261	  24
patched trunk with border-collapse    261	  20
Adding a border:0; rule doesn't make any difference, so it seems I can only use
border-collapse:collapse to correct this.
I can't really imagine it would cause many problems in this case, but if you
say so. On the other hand, this isn't really a big deal and seems only to
happen when the marquee is inside a table.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch2

This is the same as patch1, but without the border-collapse: collapse rule.
You decide which one is preferred.

> Martijn, I'm not sure I follow what the patch is doing...
Instead of adding the style rules dynamically with javascript, the style rules
are directly applied. That way, I don't trigger this incremental reflow bug
(which is I guess, part of bug 47710).
I've tested this on the majority of testcases in bugzilla and some urls, no
regressions that I know of.
Maybe you know somebody else who can review this? Doron?

Ultimately, I think returning to xul:hbox is the best solution, because then
the white-space:nowrap hack is not necessary. But currently that would regress
bug 277208, which is a worse issue than the issues that the use of
display:table hasve introduced.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Can you get away with border-spacing:0 instead of using border-collapse?  Or
does that do nothing here?

And yes, if doron can review I could sr...

Comment 17

[Martijn Wargers (dead)]

Attachment: patch3

(In reply to comment #16)
> Can you get away with border-spacing:0 instead of using border-collapse?  Or
> does that do nothing here?
Oops, I forgot about that one. Yes, that works.

Comment 18

[Doron Rosenberg (IBM)]

   } /* This hack is needed until bug 119078 gets fixed */
- 
+
+  marquee > * > * > *{
+    white-space: normal !important;
+  }
 }

Please explain why that is needed.  Are you trying to style the <html:div
anonid="innerDiv">?  If so, you should give it a class and style it using a
classname rather.

Comment 19

[Martijn Wargers (dead)]

Attachment: patch4

You're right. But I can use the [anonid="innerDiv"] attribute selector, instead
of adding a classname.

Comment 20

[Boris Zbarsky [:bzbarsky]]

For what it's worth, class selectors are a good bit faster than attribute
selectors...

Comment 21

[Doron Rosenberg (IBM)]

Comment on attachment 200214 [details] [diff] [review]
patch4

Please use a class selector

Comment 22

[Martijn Wargers (dead)]

Attachment: patch5

Ok, so like this?

Comment 23

[Doron Rosenberg (IBM)]

Comment on attachment 200226 [details] [diff] [review]
patch5

>Index: layout/style/xbl-marquee/xbl-marquee.css
>===================================================================
>RCS file: /cvsroot/mozilla/layout/style/xbl-marquee/xbl-marquee.css,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 xbl-marquee.css
>--- layout/style/xbl-marquee/xbl-marquee.css	18 Jul 2005 21:15:42 -0000	1.1
>+++ layout/style/xbl-marquee/xbl-marquee.css	20 Oct 2005 16:42:25 -0000
>@@ -45,10 +45,13 @@ marquee[height*="7"], marquee[height*="8
> 
> /* PRINT ONLY rules follow */
> @media print {
> 
>   marquee > * > * { 
>     margin: 0 !important; 
>     padding: 0 !important;
>   } /* This hack is needed until bug 119078 gets fixed */
>- 
>+
>+  marquee .innerDiv {
>+    white-space: normal !important;
>+  }
> }

You probably should just do:

div.innerDiv {

since the stylesheet is included only by the marquee anyways I believe.

Comment 24

[Martijn Wargers (dead)]

(In reply to comment #23)
> You probably should just do:
> 
> div.innerDiv {
> 
> since the stylesheet is included only by the marquee anyways I believe.
Yes, you're right, but somehow, I'm scared of that.

Comment 25

[Doron Rosenberg (IBM)]

(In reply to comment #24)
> (In reply to comment #23)
> > You probably should just do:
> > 
> > div.innerDiv {
> > 
> > since the stylesheet is included only by the marquee anyways I believe.
> Yes, you're right, but somehow, I'm scared of that.
> 
> 
> 

Should be fine.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200226 [details] [diff] [review]
patch5

Yeah, please use a straight class selector.  That's actually faster than using
a descendant like that.

sr=bzbarsky with that.

Comment 27

[Martijn Wargers (dead)]

Attachment: patch6 for checkin

Ok, this uses a straight class selector.
Ready for checkin.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Please make sure we have a bug filed on the actual underlying incremental reflow issue!

Comment 29

[Martijn Wargers (dead)]

Ok, I filed bug 313520 about that.