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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
XML
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: bz)

Tracking

({regression})

Trunk
mozilla1.9alpha8
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
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).
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.
Assignee: nobody → xml
Component: Style System (CSS) → XML
QA Contact: style-system → ashshbhatt

Updated

11 years ago
Summary: Flash of unstyled content on literarymoose on first load/shift->reload → Flash of unstyled content on XML pages on first load (or shift-reload)
Created attachment 264213 [details] [diff] [review]
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?
Flags: blocking1.9?
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.
Not a blocker, but wanted for sure.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted1.9]
Created attachment 267304 [details] [diff] [review]
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.
Attachment #264213 - Attachment is obsolete: true
Attachment #267304 - Flags: superreview?(jonas)
Attachment #267304 - Flags: review?(jonas)
Assignee: xml → bzbarsky
Priority: -- → P2
Summary: Flash of unstyled content on XML pages on first load (or shift-reload) → [FIX]Flash of unstyled content on XML pages on first load (or shift-reload)
Target Milestone: --- → mozilla1.9alpha6
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
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
Attachment #267304 - Flags: superreview?(jonas)
Attachment #267304 - Flags: superreview+
Attachment #267304 - Flags: review?(jonas)
Attachment #267304 - Flags: review+
Created attachment 271789 [details] [diff] [review]
Updated to that comment and merged to tip
Attachment #267304 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9+
Whiteboard: [wanted1.9]
You need to log in before you can comment on or make changes to this bug.