392318 - [FIX][mozilla24.com] loading mozilla24.com displays debug alert

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: screen shot of the alert dialog

When I load mozilla24.com, an alert pops up with the message:

jCarousel: No width/height set for items.  This will cause an infinite loop.  Aborting...

That looks like a debug alert and not something that a regular user should see when browsing to the page.

Comment 1

[u217243]

Is the alert still shown?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> Is the alert still shown?

Yes, I still see it.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Now when I go to the site, I get redirected to http://www.mozilla24.com/en-US/ , which doesn't display the dialog.  But if I click on the "Japanese" link in the top-right corner of the page, then I get redirected to http://www.mozilla24.com/ja , which does still display the dialog.

Comment 4

[u217243]

I set width/height in CSS and it should work, but the alert is still shown on GranParadiso or Minefield. No alert on Firefox 2.

I removed the alert from script but the carousel effect doesn't work.

Conclusion: The jCarousel library seems not to be compatible with Trunk/Fx3 :-(

Comment 5

[Martijn Wargers (dead)]

This regressed on trunk between 2007-04-20 and 2007-04-21:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-20+03&maxdate=2007-04-21+06&cvsroot=%2Fcvsroot
Somehow caused by the patch for bug 84582?

Comment 6

[Martijn Wargers (dead)]

This is caused by the patch for bug 8458, when inlining the relevant style, the bug doesn't show anymore.
Also moving the stylesheet links before the script tags makes the bug go away.
I'm still working on getting a minimized testcase.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

Uh...  This page is assuming that DOMContentLoaded will fire after all styles are loaded?  That assumption is false on trunk, of course.  It's also false in Opera, last I checked.

We could block DOMContentLoaded on pending sheet loads easily enough.  Should we?  Is this pattern common enough that we'd want to account for it?  For example, is this jCarousel thing common enough?

In the name of compat this seems like a good thing to not change, but I really dislike having to introduce what are effectively bugs to work around a clearly incorrect page (it should be using onload, not DOMContentLoaded).

Comment 9

[Martijn Wargers (dead)]

(In reply to comment #8)
> Uh...  This page is assuming that DOMContentLoaded will fire after all styles
> are loaded?  That assumption is false on trunk, of course.  It's also false in
> Opera, last I checked.

The testcase and url work as intended in Opera9.20.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Sure, but see <http://lists.w3.org/Archives/Public/public-webapi/2007Jul/0014.html>.  In this case it happens to work in Opera for some reason, but it's not guaranteed in general, apparently.

Comment 11

[Martijn Wargers (dead)]

Attachment: tweaked Opera's testcase

This is Opera's testcase, but now with the external <link> after the <script>.
Opera9.20 first shows fail and then after the stylesheet has been loaded, it shows pass. So it seems like Opera is firing 2 DOMContentLoaded events in this case.

Comment 12

[Bob Clary [:bc] (inactive)]

bz, I can do a scan for the libraries for jquery and jcarousel if you can wait 2-3 days for the answer.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Bob, it's OK, actually.  We have another problem here, and fixing it should just help this bug.  Martijn pointed out that having the size be 0x0 in DOMContentLoaded is wrong...

The issue is that I listened to Jonas in bug 84582 comment 43, second paragraph.  :) The reason we needed layout before EndLoad() is because EndLoad() drops the reference to the parser, which means that after that point the document can't call FlushPendingNotifications() on the sink.. and that hence layout flushes don't trigger an InitialReflow() after that point.  That's what happens here: we get to DOMContentLoaded with an already-null mParser but without having called InitialReflow.  I wish I'd documented that code better when I initially wrote it.

So either DOMContentLoaded needs to happen after all sheets are loaded, or we need to start layout before firing DOMContentLoaded, or we need to make it so that flushing during DOMContentLoaded can start layout.  Jonas, thoughts?  Doing the last one of these seems best to me, to be honest.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Did the original site change?  I can't reproduce the problem there anymore...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for option 3

This makes us not give 0x0 on Martijn's testcase, though we don't give the numbers set in the sheet, of course.

Is this enough to fix the site?  I can't reproduce the bug at all with the site....  Any help with that would be much appreciated.

Comment 16

[Martijn Wargers (dead)]

Koshei removed the alert that happened. You should see 3 images in the carousel widget, however, as you can see here at the bottom of this image:
http://martijn.martijn.googlepages.com/carousel.jpg
If you can see that, and the carousel thing works as expected, then I guess this would be fixed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  That part works.  Or at least works as well as it did in 1.8, which is "not very"... but that looks like a jCarousel issue.  Thanks for clarifying!

Comment 18

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

Wanted-1.9 rather than blocking1.9+ largely because I'm skeptical about catering to sites that do non-content things with DOMContentLoaded.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fix and mochitest checked in.