380028 - [FIX]Flash of unstyled content on XML pages on first load (or shift-reload)

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

[Martijn Wargers (dead)]

I tested this with a clean profile.
When first loading or shift->reloading that site, I'm seeing a flash of unstyled content.
This regressed between 2007-04-20 and 2007-04-21, a regression from bug 84582 in that case.

I could try and come up with a testcase if wanted, I guess.

Comment 1

[philippe (part-time)]

I've seen this on long xhtml documents (text only) with linked stylesheets. 
If I load the stylesheet as <?xml-stylesheet href="./style.css media="screen"?> there are no problems (tested on my local dev server only).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, the XML content sink basically starts layout as soon as it opens the root... which means that blocking layout start on stylesheets doesn't work for <link>, of course.

Let me think about what we can do here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that doesn't quite work

With this patch, we still end up showing stuff.  The problem is that by the time the reflow event from the StartLayout fires (which is before the stylesheet is loaded), we've got a fair amount of stuff in the DOM.  Then we do that reflow (and show that stuff).  And then we proceed to notify a few times when we close tags that are at the current notification level...

Maybe we could put the StartLayout call in HandleStartElement in the XML sink off on a a timer?  That wouldn't necessarily help if a <script> comes before a <link>, however...  Though perhaps we should just reset the timer if we're waiting on a script too?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Couldn't we make a special "hack" for xhtml such that if we open an <html:head> we stall calling StartLayout until it's been closed. That should work pretty fine together with StartLayout off an event since that way we'll most likely always hit the start <html:head> before StartLayout is called.

Comment 5

[Johnny Stenback (:jst)]

Not a blocker, but wanted for sure.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch

As above, but also skips calling MaybeStartLayout when the root is opened and any time a tag is opened inside an <html:head>.

These should in fact be enough to fix the bug, so I could take out the notification changes if you prefer.  But they generally seem like the right thing to do to me...  My only worry is the complexity.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 267304 [details] [diff] [review]
Updated patch

>Index: content/base/src/nsContentSink.cpp
> nsContentSink::StyleSheetLoaded(nsICSSStyleSheet* aSheet,
>                                 PRBool aWasAlternate,
>                                 nsresult aStatus)
> {
>   if (!aWasAlternate) {
>     NS_ASSERTION(mPendingSheetCount > 0, "How'd that happen?");
>     --mPendingSheetCount;
> 
>-    if (mPendingSheetCount == 0 && mDeferredLayoutStart) {
>-      // We might not have really started layout, since this sheet was still
>-      // loading.  Do it now.  Probably doesn't matter whether we do this
>-      // before or after we unblock scripts, but before feels saner.  Note that
>-      // if mDeferredLayoutStart is true, that means any subclass StartLayout()
>-      // stuff that needs to happen has already happened, so we don't need to
>-      // worry about it.
>-      StartLayout(PR_FALSE);
>+    if (mPendingSheetCount == 0 &&
>+        (mDeferredLayoutStart || mDeferredFlushTags)) {
>+      if (mDeferredLayoutStart) {
>+        // We might not have really started layout, since this sheet was still
>+        // loading.  Do it now.  Probably doesn't matter whether we do this
>+        // before or after we unblock scripts, but before feels saner.  Note
>+        // that if mDeferredLayoutStart is true, that means any subclass
>+        // StartLayout() stuff that needs to happen has already happened, so we
>+        // don't need to worry about it.
>+        StartLayout(PR_FALSE);
>+      } else if (mDeferredFlushTags) {
>+        FlushTags();
>+      }

Seems like it'd be good to deal with mDeferredLayoutStart and mDeferredFlushTags both being set. Probably flushing first?

Looks good otherwise. r/sr=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to that comment and merged to tip

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.