Closed Bug 625561 Opened 9 years ago Closed 9 years ago

Flickering due to late application of external CSS when loaded/reloaded; inconsistent across browser windows

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: kdevel, Assigned: ttaubert)

References

()

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(2 files, 3 obsolete files)

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre

A browser window may keep a state which causes late application of external CSS. Initially the content is rendered without CSS and shortly thereafter the CSS is applied. This appears as flicker.

When you pull the tab out creating a new browser window the error vansishes. If you put the tab back oder otherwise invoke the URL in the first window and reload the error is reproduced.

Reproducible: Always

Steps to Reproduce:
1. open http://hg.mozilla.org/mozilla-central
2. etrl-e (enter panorama)
3. shrink tab group, pull tab out of tab group (-> tab group vanishes)
4. click at tab
5. reload (ctrl-r or mouse on reload) 
Actual Results:  
Page is renderered without CSS initially then CSS is applied.

Expected Results:  
Page is rendered with CSS.
Attached video screencast
reduced STR:

1. ctrl-e (-> panorama)
2. ctrl-e (back to normal view)
3. open http://hg.mozilla.org/mozilla-central
The first bad revision is:
changeset:   59796:abc78094ad64
user:        Mounir Lamouri <mounir.lamouri@gmail.com>
date:        Mon Jan 03 13:57:45 2011 +0100
summary:     Bug 569399 - autofocus sometimes does not work because the element has no frame yet. r=hsivonen a=blocking-betaN
Product: 	Core
Component: 	HTML: Parser 

According to Mike Beltzner's remark in Bug 609396 I would like to set the "Component" accordingly but I can't: Theres no option named "HTML: Parser".
Component: General → HTML: Parser
Product: Firefox → Core
Version: unspecified → Trunk
There was no such option in the original drop-down element ...
Repro'd with Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre ID:20110114030359

Panorama seems to be an easy Way to trigger this Bug, but I've seen this myself "in the Wild" without using it as well.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression
OS: Linux → All
This has fixed the problem:

author	Sean Dunn <seanedunn@yahoo.com>
	Tue Nov 16 16:35:00 2010 -0600 (at Tue Nov 16 16:35:00 2010 -0600)
changeset 60722	99bae22d8c8c
parent 60721	f2091ca7dc39
pushlog:	99bae22d8c8c
Bug 609685 - "Having opened panorama once in a window affects all rendering in that window, even in tabs that are opened later" [f=ian r=ian a=blocking]
QA Contact: general → parser
Hardware: x86_64 → All
Indeed.  If panorama requests layout even when we don't have all of our style... we have to do layout.  Even though we don't have all the styles yet.

Good thing they stopped doing that.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 609685
The issue is back again. Just checking with nightly build.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
2011-02-03-09-mozilla-central is okay, but the error reappears with

The first bad revision is:
changeset:   61867:54da8f75df7f
user:        Tim Taubert <tim.taubert@gmx.de>
date:        Thu Feb 03 02:53:13 2011 +0100
summary:     Bug 594958 - Tab thumbnails sometimes have gray (now black) bars at the bottom [r=ian, a=blocking2.0:final+]

The issue can be reproduced with each of patch v4, v5, v6 and of course with the patch for checkin. This is remarkable since until a few hours ago I have been using a version of 2011-01-31 (ba3fe7ee56b9 + v5/v6 of his that patch) which does not show the issue.
Over to Tim, as this is blamed on his recent changes.
Component: HTML: Parser → TabCandy
Product: Core → Firefox
QA Contact: parser → tabcandy
Alternatively one may back out

changeset: 61680:d94a56d78123
user: Patrick Walton <pwalton@mozilla.com>
date: Mon Jan 31 16:41:05 2011 -0800
summary: Bug 624931 - Use -moz-transform for Panorama zoom. r=iangilman a=sdwilsh

These changes are conflicting with those of 61867:54da8f75df7f.
This started happening again because the code from bug 594958 is flushing layout on the content window.  It's still passing DO_NO_FLUSH to the drawWindow call, so I assume it doesn't _mean_ to flush.  But _calculateClippingRect requests the scroll* properties of the body, which triggers a layout flush of course.

(I'm also not sure where the magic "25" adjustment to the width comes from, but that's a separate issue.)
(In reply to comment #13)
> This started happening again because the code from bug 594958 is flushing
> layout on the content window.  It's still passing DO_NO_FLUSH to the drawWindow
> call, so I assume it doesn't _mean_ to flush.  But _calculateClippingRect
> requests the scroll* properties of the body, which triggers a layout flush of
> course.

Ok that's bad. I wasn't aware of this bug when I "fixed" bug 594958.

> (I'm also not sure where the magic "25" adjustment to the width comes from, but
> that's a separate issue.)

That's the vertical scrollbar. Loosing some pixels on right does not hurt that much so that's a bit easier than getting the documentElement and its clientWidth/Height (we also don't want the documentElement - we want the window size as that is what the user sees).
> That's the vertical scrollbar.

Nothing guarantees that 25 is larger than the vertical scrollbar width, for what it's worth (and in fact on Windows with wide scrollbars I'll bet money it's not).

For the rest, what's the code trying to measure?  That is, why do you need the |if (body)| codepath at all, instead of just using the window's innerWidth/innerHeight?
This seems worse than the issue fixed by bug 594958, so if this can't be sorted out quickly, that patch should probably be backed out.
Assignee: nobody → tim.taubert
Blocks: 594958
(In reply to comment #15)
> Nothing guarantees that 25 is larger than the vertical scrollbar width, for
> what it's worth (and in fact on Windows with wide scrollbars I'll bet money
> it's not).

Mhh true :/ We should correctly calculate the scrollbar width and subtract that from the window's inner width.

> For the rest, what's the code trying to measure?  That is, why do you need the
> |if (body)| codepath at all, instead of just using the window's
> innerWidth/innerHeight?

Often the body's scroll size is greater than the viewport. So we want the window size at the minimum. If the page is greater than that we want the body's scroll size.
(In reply to comment #13)
> This started happening again because the code from bug 594958 is flushing
> layout on the content window.  It's still passing DO_NO_FLUSH to the drawWindow
> call, so I assume it doesn't _mean_ to flush.  But _calculateClippingRect
> requests the scroll* properties of the body, which triggers a layout flush of
> course.

So, is there any other way to determine a page's bounds? Or do we need to wait until the page reaches some state ("load")?
> We should correctly calculate the scrollbar width

Or just ask Gecko for it, as a followup.

> is there any other way to determine a page's bounds?

Could add one in windowutils.  Doesn't look like there is one now, offhand.  roc?
Attached patch patch v1 (obsolete) — Splinter Review
Seems there's an easy fix for this. I'll file a follow-up for the scrollbar width issue.
Attachment #509767 - Flags: review?(ian)
(In reply to comment #20)
> Created attachment 509767 [details] [diff] [review]
> patch v1
> 
> Seems there's an easy fix for this. I'll file a follow-up for the scrollbar
> width issue.

So it's okay to flush layout if the page has loaded?
It won't cause the FOUC issue this bug is about.

It might impact performance, depending on how it interleaves with the refresh driver and with page scripts.
(In reply to comment #22)
> It might impact performance, depending on how it interleaves with the refresh
> driver and with page scripts.

Ok, sorry I didn't know this could have so much side-effects. So we need another follow up bug for getting page bounds without impacting on the content window?
Comment on attachment 509767 [details] [diff] [review]
patch v1

>   _update: function TabItems__update(tab) {
>     try {
>       if (this._pauseUpdateForTest)
>         return;
> 
>       Utils.assertThrow(tab, "tab");
> 
>+      // If the tab hasn't loaded completely, skip this update.
>+      if (tab.linkedBrowser.contentDocument.readyState != 'complete')
>+        return;

It's interesting that _update gets called here at all. Seems like this should be prevented.
Apart from that, I think we need a patch that makes this code not touch document.body, given comment 22.
Attachment #509767 - Flags: review?(ian) → review-
(In reply to comment #23)
> Ok, sorry I didn't know this could have so much side-effects. So we need
> another follow up bug for getting page bounds without impacting on the content
> window?

Please no other followup. If it's an easy fix, it can just be done here. If it's not an easy fix, we need to revert the patch from bug 594958.
patch v1 breaks the smooth opening of new tabs wich have been created in panorama.
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #25)
> Please no other followup. If it's an easy fix, it can just be done here. If
> it's not an easy fix, we need to revert the patch from bug 594958.

Ok, there seems no easy way of retrieving the actual page size for now. There are many other issues with tab thumbnail generation and we will punt them all after fx4.

I reverted the patch and removed the whole part that accesses the document's body (so no FOUC and no performance impact). We're having pretty good results anyway. I filed bug 631593 and added a comment to replace the 25px static scrollbar width. That's better than removing it completely as that should be ok on most systems.
Attachment #509767 - Attachment is obsolete: true
Attachment #509830 - Flags: review?(ian)
v2 looks good!
Attached patch patch v3 (test corrected) (obsolete) — Splinter Review
Corrected/adapted test for bug 594958.
Attachment #509830 - Attachment is obsolete: true
Attachment #509851 - Flags: review?(ian)
Attachment #509830 - Flags: review?(ian)
(In reply to comment #24)
> It's interesting that _update gets called here at all. Seems like this should
> be prevented.

This might be handled in a separate bug, but do you know what's going on here yet? Why is the _update called so aggressively during page load when panorama isn't open?
(In reply to comment #30)
> This might be handled in a separate bug, but do you know what's going on here
> yet? Why is the _update called so aggressively during page load when panorama
> isn't open?

This is done because we listen on TabAttrModified. TabItems._update() handles the thumbnail, favicon, url (stored internally) and label - so any of them could have been modified.

We should probably split that up and handle thumbnail updates separately. This could be done together with bug 598435 that suggests to use MozAfterPaint.
Using TabAttrModified to update tab thumbnails /while panorama isn't open/ sounds way too aggressive and wasteful to me (MozAfterPaint seems even worse).
Note that this was actually discussed somewhat in bug 609685 comment 2.  That's when the DO_NOT_FLUSH was added to the drawWindow call.
(In reply to comment #32)
> Using TabAttrModified to update tab thumbnails /while panorama isn't open/
> sounds way too aggressive and wasteful to me (MozAfterPaint seems even worse).

We receive the events when the user is not in the Panorama UI, but we simply set dirty flags; it's not till you reenter the Panorama UI that thumbnails are actually updated. If, for some reason, we are actually  updating thumbnails while not in the Panorama UI, that's definitely a problem.
(In reply to comment #34)
> (In reply to comment #32)
> > Using TabAttrModified to update tab thumbnails /while panorama isn't open/
> > sounds way too aggressive and wasteful to me (MozAfterPaint seems even worse).
> 
> We receive the events when the user is not in the Panorama UI, but we simply
> set dirty flags; it's not till you reenter the Panorama UI that thumbnails are
> actually updated. If, for some reason, we are actually  updating thumbnails
> while not in the Panorama UI, that's definitely a problem.

This bug seems to indicate that this doesn't work as expected.
(In reply to comment #35)
> > We receive the events when the user is not in the Panorama UI, but we simply
> > set dirty flags; it's not till you reenter the Panorama UI that thumbnails are
> > actually updated. If, for some reason, we are actually  updating thumbnails
> > while not in the Panorama UI, that's definitely a problem.
> 
> This bug seems to indicate that this doesn't work as expected.

Indeed. Filed bug 631662 to investigate.
Comment on attachment 509851 [details] [diff] [review]
patch v3 (test corrected)

Looks good.
Attachment #509851 - Flags: review?(ian) → review+
Attachment #509851 - Flags: approval2.0?
blocking2.0: ? → final+
Whiteboard: [softblocker]
Attachment #509851 - Flags: approval2.0?
Attachment #509851 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/20ffcaad9700
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Verified on 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Blocks: 631530
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.