Bug 12440 - repaints inconsistent or draw incorrect with fixed position elements
Summary: repaints inconsistent or draw incorrect with fixed position elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL: https://rhubarb3.securesites.net/secu...
Keywords: HasReduction, InRadar
Depends on:
Blocks: 175235
  Show dependency treegraph
 
Reported: 2007-01-27 06:17 PST by Kevin M. Dean
Modified: 2017-10-12 10:31 PDT (History)
2 users (show)

See Also:


Attachments
incorrect footer placement screenshot (46.62 KB, image/jpeg)
2007-01-27 06:24 PST, Kevin M. Dean
no flags Details
When incorrect footer is repainted screenshot (83.28 KB, image/jpeg)
2007-01-27 06:26 PST, Kevin M. Dean
no flags Details
Resize corner glitch (5.68 KB, image/jpeg)
2007-01-27 07:52 PST, Kevin M. Dean
no flags Details
Reduction (494 bytes, text/html)
2007-01-27 09:27 PST, mitz
no flags Details
Patch (537.44 KB, patch)
2009-04-08 12:03 PDT, Dave Hyatt
hyatt: review-
Details | Formatted Diff | Diff
Better patch that doesn't turn off the optimization. (59.29 KB, patch)
2009-04-08 14:27 PDT, Dave Hyatt
aroben: review+
Details | Formatted Diff | Diff
Patch (180.20 KB, patch)
2009-04-08 14:41 PDT, Dave Hyatt
darin: review+
Details | Formatted Diff | Diff
Chromium build fix (1.23 KB, patch)
2009-04-10 09:52 PDT, Pam Greene (IRC:pamg)
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin M. Dean 2007-01-27 06:17:32 PST
Here's a weird one that works fine in Safari, Firefox and IE.

After loading the above page (which takes a while because Safari always seems to load a lot slower when https, why's that?), scroll down to a red link that says "IF YOU WILL BE ACCOMPANIED BY A GUEST CLICK HERE.".

Click that link and normally it will reveal below it "Guest Information". Now here's where it's inconsistent. Usually the first time I load it clicking the link changes the link text but the "Guest Information" doesn't appear until you scroll the screen, or if you click the link again before scrolling the screen, the bottom of the page will redraw incorrectly. Once you've scrolled the page, it works from that point forward.

Refreshing the page sometimes brings the problem back but not always. I think it requires refreshing the page twice which probably means it needs to refresh the linked javascript file and/or other linked assets to cause the problem again.

Just noticed another issue. After refreshing it seems to also incorrectly place my bottom footer section which is fixed positioned.
Comment 1 Kevin M. Dean 2007-01-27 06:24:24 PST
Created attachment 12706 [details]
incorrect footer placement screenshot

This is how it renders when the page is refreshed.
Comment 2 Kevin M. Dean 2007-01-27 06:26:44 PST
Created attachment 12707 [details]
When incorrect footer is repainted screenshot

This is what it looks like when you click the link again before scrolling the page and the footer is repainted into it's correct position, but leaves parts of the incorrect position in place.
Comment 3 Kevin M. Dean 2007-01-27 06:35:18 PST
It appears the the part about the incorrect positioning of the footer on refresh is not a regression, but the rest still is. Refreshing twice in Safari does make the footer appear out of place, but as soon as the link is clicking the page repaints and fixes itself.
Comment 4 Kevin M. Dean 2007-01-27 06:41:52 PST
Well, it seems that things are more being caused by the out of position fixed footer than the javascript link I initially thought.

If you use the site options  at the bottom to disable the footer the problem goes away. So, there appears to be some issue with placing fixed position divs and how the screen gets repainted when content is updated.
Comment 5 Kevin M. Dean 2007-01-27 07:11:28 PST
Plot thickens... with the floating footer back on. It appears that the footer is only incorrectly positioned when it's drawing over top of the form fields.

http://test.nationalsalute.com/volunteers/volunteers2.php

At this link, you should be able to refresh without issue as long as the window is tall enough to place the footer below the 3 fields. Then resize your window so the footer covers part of the fields and refresh. It should then start incorrectly placing the footer.
Comment 6 Kevin M. Dean 2007-01-27 07:16:45 PST
Man, I keep having to revise this. It's not the form fields specifically, but it's any other content that's appearing on the page above the footer.
Comment 7 David Kilzer (:ddkilzer) 2007-01-27 07:29:58 PST
This looks like it may be a duplicate of Bug 11109.  Are you using a WebKit nightly newer than r19149 to test with?

Comment 8 David Kilzer (:ddkilzer) 2007-01-27 07:34:08 PST
(In reply to comment #5)
> Plot thickens... with the floating footer back on. It appears that the footer
> is only incorrectly positioned when it's drawing over top of the form fields.
> 
> http://test.nationalsalute.com/volunteers/volunteers2.php
> 
> At this link, you should be able to refresh without issue as long as the window
> is tall enough to place the footer below the 3 fields. Then resize your window
> so the footer covers part of the fields and refresh. It should then start
> incorrectly placing the footer.

I can't reproduce with a locally-built debug build of WebKit r19183 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).

Comment 9 Kevin M. Dean 2007-01-27 07:41:26 PST
(In reply to comment #7)
> This looks like it may be a duplicate of Bug 11109.  Are you using a WebKit
> nightly newer than r19149 to test with?
> 

I'm using r19186, but as it's appearing the core problem of the issue is broken in Safari also, I don't think it's the same issue.

I've put up another test page with different linked files that form the page with the main change being moving both of my fixed divs (header and footer) to the bottom of the code which resolves the out-of position redraw issue and the clicking the link update issue.

http://test.nationalsalute.com/ship_tours/tours3.php

Normally I code them in order - fixed header, non-fixed content, fixed footer. This way if they use the site options to change their styles to not fixed they render in the correct positions. Now I have it coded - non-fixed content, fixed header, fixed footer. With this change they won't appear in the correct positions when disabling the fixed css unless I do some if/thens based off their settings via php, which is doable, but it shouldn't have to. A fixed position div no matter where it's coded should not be affected by the other non-fixed code.

Comment 10 mitz 2007-01-27 07:43:13 PST
Looks like sometimes the fixed position is computed during the "horizontal and vertical scrollbars present" layout pass and not updated when the horizontal scrollbar is finally removed.
Comment 11 Kevin M. Dean 2007-01-27 07:45:39 PST
(In reply to comment #8)

> I can't reproduce with a locally-built debug build of WebKit r19183 with Safari
> 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).
> 

Sometimes it takes a couple of refreshes to trigger a reload of all the content. Cached content seems to work better.

Also, I'm having the problem with r19186 and I noticed it with r19136 and since it happens in Safari too, I'm assuming all previous builds have the issue.

10.4.8 (8L127) PPC
Comment 12 Kevin M. Dean 2007-01-27 07:47:40 PST
(In reply to comment #10)
> Looks like sometimes the fixed position is computed during the "horizontal and
> vertical scrollbars present" layout pass and not updated when the horizontal
> scrollbar is finally removed.
> 

That would explain the height difference. When the div is out of position, it's move up 15 px, which seems to be the same height as the added horz. scrollbar when it is there.
Comment 13 Kevin M. Dean 2007-01-27 07:52:49 PST
Created attachment 12709 [details]
Resize corner glitch

Just discovered an unrelated glitch, but tied to refreshing the page. When I have the window resized so there's a horz scroll bar the bottom right resize area. where the empty square appears between the horz and vert scrolls is actually drawing screen content. If you position it right you can even see the line between my light blue content area and the dark blue page background.
Comment 14 Kevin M. Dean 2007-01-27 08:53:12 PST
Alright, every corner I turn reveals more information. I think I've found the absolute final cause of this from the page side.

So without moving my fixed header div to the end of the file like I mentioned before, I found what inside the header was causing the issue. The logo in the upper left is a flash piece implemented inline via:

<SCRIPT src="/js/flash_logo.js" type="text/javascript"></SCRIPT>

When I remove that script tag all problems go away and the screen be refreshed:

http://test.nationalsalute.com/ship_tours/tours4nologo.php

It's not the code itself since I can completely blank out the flash_logo.js and it will still cause the footer to glitch or I can place the entire script inline without the external js and things will work fine. 

http://test.nationalsalute.com/ship_tours/tours5inline.php

It seems to be just the external linking to files itself that's of issue. This would explain why it takes 2 refreshes to trigger, I find Safari doesn't refresh the external linked files like css and js until at least the 2nd refresh.

This may also be related to a previous bug I posted: http://bugs.webkit.org/show_bug.cgi?id=12392


Comment 15 mitz 2007-01-27 09:27:57 PST
Created attachment 12712 [details]
Reduction

I think this is also the key to some of the "leftover scrollbar" bugs.
Comment 16 Kevin M. Dean 2007-01-27 10:47:31 PST
I've reduced down the code from my site as far as I can with some interesting combinations of results.

http://test.nationalsalute.com/volunteers/volunteers6.php

Seems to require the following:

another script linked in the head

The fixed div header contains a table or the header is itself a fixed table.

The table has a set width and at least 2 table datas have a set width. The first td contains the script tag and the 2nd td contains an img.

The img tag doesn't have to take up the full width of the 2nd td, but it has to  be an image. Removing it or replacing it with a non-breaking space does not cause the footer to break. Also  for some reason if I remove the picture background from the table, the problem also goes away.

Also, if refreshing doesn't work, try resizing the window a little and then refresh. It seems to trigger it quicker.

It's almost as if space is being reserved by the various diva and table widths as it should, but when the javscript is embedded it thinks it might need more horz space, trigger the reserving of space at the bottom for the scrollbar, but when it renders it doesn't need the space.

Comment 17 Kevin M. Dean 2007-01-27 12:11:38 PST
Since I still needed to fix this for current Safari so it's not broken for people using the current version, I've made the regular site code print the script to the page instead of linking, so this link should now be fine:

http://test.nationalsalute.com/ship_tours/tours.php

I've cleaned up and change some links, so here's the latest active links from the site since a few of the ones previously listed will no longer work.

The version with the flash javascript linked:
http://test.nationalsalute.com/ship_tours/tours7linked.php

The version with the flash javascript link completely removed:
http://test.nationalsalute.com/ship_tours/tours4nologo.php

The reduced site code:
http://test.nationalsalute.com/volunteers/volunteers6.php

Comment 18 mitz 2007-01-27 12:22:52 PST
(In reply to comment #17)
> Since I still needed to fix this for current Safari so it's not broken for
> people using the current version, I've made the regular site code print the
> script to the page instead of linking, so this link should now be fine:
> 
> http://test.nationalsalute.com/ship_tours/tours.php

I believe a more robust workaround for you would be to force a relayout further down in your document. Try adding this right before the </body> tag:
<script type="text/javascript">
document.body.offsetTop; // Force a relayout to work around a Safari layout issue.
</script>

Comment 19 Kevin M. Dean 2007-01-27 12:49:21 PST
(In reply to comment #18)
> (In reply to comment #17)
> > Since I still needed to fix this for current Safari so it's not broken for
> > people using the current version, I've made the regular site code print the
> > script to the page instead of linking, so this link should now be fine:
> > 
> > http://test.nationalsalute.com/ship_tours/tours.php
> 
> I believe a more robust workaround for you would be to force a relayout further
> down in your document. Try adding this right before the </body> tag:
> <script type="text/javascript">
> document.body.offsetTop; // Force a relayout to work around a Safari layout
> issue.
> </script>

I tried it with:

 http://test.nationalsalute.com/ship_tours/tours7linked.php

... and it doesn't seem to have any effect. Refreshing still causes the footer to jump up. I'll just stick with the non-linked script for now since it appears to do the job.
Comment 20 Mark Rowe (bdash) 2007-01-28 19:10:56 PST
<rdar://problem/4960266>
Comment 21 mitz 2007-02-07 08:31:21 PST
The problem seems to be that -[WebDynamicScrollBarsView updateScrollers] does not relayout the document one last time after adding/removing scrollbars. The relayout is at the beginning of the for loop instead of at the end. Looking at the history of that method, I found out that initially that relayout was meant to layout if needed in the resize case. Later on the "scrollbars changed" condition was added and the relayout was made unconditional, but I think that should have been added at the end of the loop, to ensure that the last layout reflects the final scrollbar status.

I made the change locally and it fixed this bug, but it had also affected many other cases (involving objects whose position depends on the viewport dimensions) and not always in a good way.

I don't think two passes are really sufficient for all cases, and in fact I have been able to come up with cases that do not behave consistently (i.e. for the same document and window size, sometimes scrollbars will be present and sometimes they will not). While there are "looping" cases (where a scrollbar is needed but once you add it it's no longer needed), just giving up on them (after two iterations) is not always correct. It would be good to have a specification of the expected behavior for all cases and then move forward with implementing it. I think it could be done without hurting performance in the common case.
Comment 22 mitz 2007-02-07 08:43:17 PST
(In reply to comment #21)
> [...] I found out that initially that relayout was meant
> to layout if needed in the resize case.

I meant the live resize case.
Comment 23 Alexey Proskuryakov 2007-05-06 04:43:31 PDT
The original page has changed - and the reduction works identically in shipping Safari and TOT for me.
Comment 24 Dave Hyatt 2007-05-10 16:52:47 PDT
Removing regression keyword and lowering priority.  This occurs in Safari 2 also.

Comment 25 Kevin M. Dean 2008-05-31 18:45:21 PDT
Well, it's been 16 months since the Reduction was created. Any progress?
Comment 26 Dave Hyatt 2009-04-08 12:03:37 PDT
Created attachment 29340 [details]
Patch
Comment 27 Darin Adler 2009-04-08 13:13:50 PDT
Comment on attachment 29340 [details]
Patch

>  #if USE(ACCELERATED_COMPOSITING)
> +    if (view()) {
> +        bool layoutPending = view()->layoutPending() || renderer()->needsLayout();
> +        // If we didn't update compositing layers because of layout(), we need to do so here.
> +        if (!layoutPending)
> +            view()->updateCompositingLayers();
>      }
> +#endif

The comment above no longer makes sense, because there's no call to layout(). Long term will updating of compositing layers move to use the same timer as layout?

> -    // Now do our painting/layout, but only if we aren't in a subframe or if we're in a subframe
> -    // that has been sized already.  Otherwise, our view size would be incorrect, so doing any 
> -    // layout/painting now would be pointless.
> +    // We used to force a synchronous display and flush here.  This really isn't
> +    // necessary and can in fact be actively harmful if pages are loading at a rate of > 60fps
> +    // (if your platform is syncing flushes and limiting them to 60fps).
> +    m_overMinimumLayoutThreshold = true;
>      if (!ownerElement() || (ownerElement()->renderer() && !ownerElement()->renderer()->needsLayout())) {
>          updateRendering();
>          
>          // Always do a layout after loading if needed.
>          if (view() && renderer() && (!renderer()->firstChild() || renderer()->needsLayout()))
>              view()->layout();
> -            
> -        // Paint immediately after the document is ready.  We do this to ensure that any timers set by the
> -        // onload don't have a chance to fire before we would have painted.  To avoid over-flushing we only
> -        // worry about this for the top-level document.  For platforms that use native widgets for ScrollViews, this
> -        // call does nothing (Mac, wx).
> -        // FIXME: This causes a timing issue with the dispatchDidFinishLoad delegate callback on Mac, so think
> -        // before enabling it, even if Mac becomes viewless later.
> -        // See <rdar://problem/5092361>
> -        if (view() && !ownerElement())
> -            view()->hostWindow()->paint();
>      }

Why are we still doing a layout here?

>                  // Set the initial vMode to AlwaysOn if we're auto.
>                  if (vMode == ScrollbarAuto)
> -                    setVerticalScrollbarMode(ScrollbarAlwaysOn); // This causes a vertical scrollbar to appear.
> +                    setVerticalScrollbarMode(ScrollbarAlwaysOff); // This causes a vertical scrollbar to appear.

Both of the comments above are now wrong.

The patch otherwise looks good. How carefully have you studied the layout test result changes?
Comment 28 Dave Hyatt 2009-04-08 13:27:41 PDT
Comment on attachment 29340 [details]
Patch

Argh, thanks for noticing that I had turned vertical scrollbars off Darin.  Minusing the patch because I had the vertical scrollbar optimization turned off, and I forgot I had turned it off. Sigh.  This changes everything, so will need to completely retest.
Comment 29 Dave Hyatt 2009-04-08 14:27:21 PDT
Created attachment 29344 [details]
Better patch that doesn't turn off the optimization.
Comment 30 Dave Hyatt 2009-04-08 14:41:08 PDT
Created attachment 29345 [details]
Patch
Comment 31 Darin Adler 2009-04-08 15:05:04 PDT
Comment on attachment 29345 [details]
Patch

>  #if USE(ACCELERATED_COMPOSITING)
> +    if (view()) {
> +        bool layoutPending = view()->layoutPending() || renderer()->needsLayout();
> +        // If we didn't update compositing layers because of layout(), we need to do so here.
> +        if (!layoutPending)
> +            view()->updateCompositingLayers();
>      }
> +#endif

This comment no longer makes sense, because there's no call to layout() any more.

Otherwise, r=me
Comment 32 Adam Roben (:aroben) 2009-04-08 15:05:30 PDT
Comment on attachment 29344 [details]
Better patch that doesn't turn off the optimization.

> +        if (sendContentResizedNotification && m_inUpdateScrollbarsPass < 2) {

I think you should store "2" in a named constant.

> +        if (suppressScrollers) {
> +            [[self verticalScroller] setNeedsDisplay: NO];
> +            [[self horizontalScroller] setNeedsDisplay: NO];
>          }

I think we don't normally leave spaces after colons in ObjC method calls.

r=me
Comment 33 Dave Hyatt 2009-04-08 15:34:16 PDT
Fixed in r42334.

Comment 34 Pam Greene (IRC:pamg) 2009-04-10 09:52:00 PDT
Created attachment 29392 [details]
Chromium build fix
Comment 35 Dimitri Glazkov (Google) 2009-04-10 10:10:04 PDT
Comment on attachment 29392 [details]
Chromium build fix

Great!
Comment 36 Pam Greene (IRC:pamg) 2009-04-10 10:14:33 PDT
Landed build fix in r42392.