Bug 109646

Summary: Constrain fixed layers to the viewport, not the document
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, benjamin, cdumez, dglazkov, eric, noam, ojan.autocc, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch bdakin: review+, webkit.review.bot: commit-queue-

Description Simon Fraser (smfr) 2013-02-12 18:20:18 PST
RenderLayerBacking::updateCompositedBounds() has code to constrain composited layer positions to the document rect. This is bad for fixed-position elements, because this constrained rect will change every time the scroll offset changes. This means that whenever layout happens (which causes us to update compositing layers), we'll recompute the fixed layer bounds, and change it if we scrolled, and that will result in jiggles.
Comment 1 Simon Fraser (smfr) 2013-02-12 18:26:19 PST
Created attachment 187984 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-12 21:36:55 PST
Comment on attachment 187984 [details]
Patch

Attachment 187984 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16522575

New failing tests:
compositing/geometry/limit-layer-bounds-fixed.html
platform/chromium/virtual/softwarecompositing/geometry/limit-layer-bounds-fixed.html
Comment 3 Benjamin Poulain 2013-02-12 22:40:42 PST
Comment on attachment 187984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187984&action=review

> LayoutTests/ChangeLog:11
> +        Constrain fixed layers to the viewport, not the document
> +        https://bugs.webkit.org/show_bug.cgi?id=109646
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        Test with a big fixed element in a compositing layer.
> +
> +        * compositing/geometry/limit-layer-bounds-fixed-expected.txt: Added.
> +        * compositing/geometry/limit-layer-bounds-fixed.html: Added.

Wouldn't it be better to test this with a ref-test?
Comment 4 Simon Fraser (smfr) 2013-02-12 22:45:48 PST
(In reply to comment #3)
> Wouldn't it be better to test this with a ref-test?

No; the visible appearance is the unchanged (unless we ref-test the layerTreeAsText output, which is weird!).
Comment 5 Benjamin Poulain 2013-02-12 23:00:20 PST
(In reply to comment #4)
> No; the visible appearance is the unchanged (unless we ref-test the layerTreeAsText output, which is weird!).

I think I fail to see how the output verify the change. Would you mind explaining a bit how the layers differed before the change?
Comment 6 Beth Dakin 2013-02-14 11:18:29 PST
Comment on attachment 187984 [details]
Patch

Looks good! You should address Benjamin's question though.
Comment 7 Simon Fraser (smfr) 2013-02-14 11:21:03 PST
(In reply to comment #5)
> (In reply to comment #4)
> > No; the visible appearance is the unchanged (unless we ref-test the layerTreeAsText output, which is weird!).
> 
> I think I fail to see how the output verify the change. Would you mind explaining a bit how the layers differed before the change?

The
  (bounds 300.00 200.00)
for the fixed layer are different (smaller) with the patch.
Comment 8 Simon Fraser (smfr) 2013-02-15 16:43:36 PST
*** Bug 108556 has been marked as a duplicate of this bug. ***
Comment 9 Simon Fraser (smfr) 2013-02-15 17:11:16 PST
http://trac.webkit.org/changeset/143073
Comment 11 Simon Fraser (smfr) 2013-02-17 11:20:13 PST
EFL might not do compositing for fixed pos. It should just get a new baseline.