Bug 12440 – repaints inconsistent or draw incorrect with fixed position elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kevin M. Dean]

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.

[Kevin M. Dean]

Created attachment 12706 [details]
incorrect footer placement screenshot

This is how it renders when the page is refreshed.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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.

[David Kilzer (:ddkilzer)]

This looks like it may be a duplicate of Bug 11109.  Are you using a WebKit nightly newer than r19149 to test with?

[David Kilzer (:ddkilzer)]

(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).

[Kevin M. Dean]

(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.

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.

[Kevin M. Dean]

(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

[Kevin M. Dean]

(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.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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

Created attachment 12712 [details]
Reduction

I think this is also the key to some of the "leftover scrollbar" bugs.

[Kevin M. Dean]

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.

[Kevin M. Dean]

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

(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>

[Kevin M. Dean]

(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.

[Mark Rowe (bdash)]

<rdar://problem/4960266>

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.

(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.

[Alexey Proskuryakov]

The original page has changed - and the reduction works identically in shipping Safari and TOT for me.

[Dave Hyatt]

Removing regression keyword and lowering priority.  This occurs in Safari 2 also.

[Kevin M. Dean]

Well, it's been 16 months since the Reduction was created. Any progress?

[Dave Hyatt]

Created attachment 29340 [details]
Patch

[Darin Adler]

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?

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 29344 [details]
Better patch that doesn't turn off the optimization.

[Dave Hyatt]

Created attachment 29345 [details]
Patch

[Darin Adler]

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

[Adam Roben (:aroben)]

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

[Dave Hyatt]

Fixed in r42334.

[Pam Greene (IRC:pamg)]

Created attachment 29392 [details]
Chromium build fix

[Dimitri Glazkov (Google)]

Comment on attachment 29392 [details]
Chromium build fix

Great!

[Pam Greene (IRC:pamg)]

Landed build fix in r42392.