Bug 103519

The following patch shows that the element definitely shakes: --- a/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp +++ b/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp @@ -441,6 +441,9 @@ void TextureMapperLayer::flushCompositingStateSelf(GraphicsLayerTextureMapper* g m_transform.setSize(m_state.size); m_transform.setFlattening(!m_state.preserves3D); m_transform.setChildrenTransform(m_state.childrenTransform); + + if (m_fixedToViewport) + printf("pos %f\n", m_transform.combined().m42()); } pos 316.438544 pos 317.167813 pos 316.897082 pos 316.626351 pos 316.355620 pos 316.626351 pos 316.626351 pos 316.355620 pos 317.084889 pos 316.814159 pos 316.543428 pos 316.272697 pos 317.001966 pos 316.731235 pos 316.460504 pos 317.189773 pos 316.648311 pos 316.377580 pos 317.106849

Chromium shake the overlay element, too. I think layout causes this shaking. However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp. I think it is related to something like TiledBackingStore::mapToContents.

(In reply to comment #2) > Chromium shake the overlay element, too. > I think layout causes this shaking. > > However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp. > > I think it is related to something like TiledBackingStore::mapToContents. Yeah, it seems to map things into non discrete positions making it blurry.

(In reply to comment #3) > (In reply to comment #2) > > Chromium shake the overlay element, too. > > I think layout causes this shaking. > > > > However, Coordinated Graphics has another problem. When shaking element, the fixed layer is blurry. Chromium always renders crisp. > > > > I think it is related to something like TiledBackingStore::mapToContents. > > Yeah, it seems to map things into non discrete positions making it blurry. Do you think what is the scope of this bug? We need to separate two problems. 1. Shaking layout 2. Blurry fixed layer. #1 may require to fix layout routine. It seems to be hard. We can fix #2 by ourselves.

> Do you think what is the scope of this bug? > > We need to separate two problems. > 1. Shaking layout > 2. Blurry fixed layer. > > #1 may require to fix layout routine. It seems to be hard. > We can fix #2 by ourselves. We could do that, basically we need to fix both

Created attachment 177490 [details] patch Fix for shaking. Blurring is a separate issue and should be addressed separately.

(In reply to comment #6) > Created an attachment (id=177490) [details] > patch > > Fix for shaking. Blurring is a separate issue and should be addressed separately. This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug.

(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=177490) [details] [details] > > patch > > > > Fix for shaking. Blurring is a separate issue and should be addressed separately. > > This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug. This is not all of that patch. But doing this does mean that the transform used for events and painting AC is slightly different that the one for positioning the content.

(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=177490) [details] [details] > > patch > > > > Fix for shaking. Blurring is a separate issue and should be addressed separately. > > This has been changed to fix the blurry tiles when it was scrolled. If that was wrong, I think we should just rollout the patch for bug 103079 and reopen that bug. Patch has a lot of good things which I would keep, we just need to find good solution to fix blurring issue, including fixed elements

Comment on attachment 177490 [details] patch removing the flags yet, as proper fix should also solve blurring issue

Created attachment 177745 [details] Patch

Created attachment 178066 [details] Patch (work in progress) This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking)

Created attachment 178067 [details] Testcase

(In reply to comment #12) > Created an attachment (id=178066) [details] > Patch (work in progress) > > This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking) This relates to our visibleContentsRect which in WebCore doesn't 100% represent where we are (as it is roundedIntRect). We can only fix this properly if we move to scale in WebCore.

(In reply to comment #14) > (In reply to comment #12) > > Created an attachment (id=178066) [details] [details] > > Patch (work in progress) > > > > This patch shows crisp pixel aligned fixed positioned elements, but with the following test case it can be seen that it sometimes moves one pixel (shaking) > > This relates to our visibleContentsRect which in WebCore doesn't 100% represent where we are (as it is roundedIntRect). We can only fix this properly if we move to scale in WebCore. Does Qt have the same problem?

> Does Qt have the same problem? Yes. Your adjustment code hides the shaking somehow (but results in some misalignment and blurriness), so turning it off and doing debugging makes it easier to see the issue. I didn't dig into fixing this adjustment code yet. How big the issue is depends on the exact scale. At some scales it is barely notifiable.

Created attachment 178123 [details] Patch (work in progress) Fixes the temporarily position adjustment to be aligned to device units (comparing using the float version of visibleContentsRect). Fixed that the layers can be blurry then the page reaches the end of the page. one-off shaking still there.

(In reply to comment #17) > Created an attachment (id=178123) [details] > Patch (work in progress) > > Fixes the temporarily position adjustment to be aligned to device units (comparing using the float version of visibleContentsRect). > Fixed that the layers can be blurry then the page reaches the end of the page. > > one-off shaking still there. With this patch you can go to google.com, search for "hest" If you use the default EFL window size, the page will be slightly scaled down. Now hover the mouse over one of the >> signs for one element Then the fixed position element pops up and if you put the mouse over it and scroll up and down, it stays perfectly and is very crisp :-) That is quite an improvement on what we had before, even if we have the one-off issue at certain scales.

Comment on attachment 178123 [details] Patch (work in progress) View in context: https://bugs.webkit.org/attachment.cgi?id=178123&action=review great work! However, we will use pageScaleFactor and deviceScaleFactor in WebCore::Page and move GraphicsLayerCA's snapping algorithm to RenderLayerBacking or RenderLayerCompositor in the future. > Source/WebKit2/UIProcess/PageViewportController.cpp:97 > + // Unfortunately it doesn't seem to be enough, so just always allow one pixel more. I think it is because CoordinatedGraphicsLayer::computePixelAlignment replaces a layer, so the layer shifts off by 1 pixel. I think we can not reproduce this bug before this patch. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:108 > + computePixelAlignment(adjustedPosition, adjustedSize, adjustedAnchorPoint, pixelAlignmentOffset); It is better to computePixel during flushing. computing here causes redundant computing over and over again. for example, setPosition after setSize. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:863 > + FloatRect alignedBounds = roundedIntRect(scaledBounds); You're right. I think mac has bug also. I think we should use something like pixelSnappedIntRect instead of roundedIntRece and enclosingIntRect to preserve size (size change causes TiledBackingStore's work!) http://trac.webkit.org/wiki/LayoutUnit

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:108 > > + computePixelAlignment(adjustedPosition, adjustedSize, adjustedAnchorPoint, pixelAlignmentOffset); > > It is better to computePixel during flushing. > computing here causes redundant computing over and over again. for example, setPosition after setSize. Possible yes, but I didn't look into making this performant yet :-) > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:863 > > + FloatRect alignedBounds = roundedIntRect(scaledBounds); > > You're right. I think mac has bug also. > I think we should use something like pixelSnappedIntRect instead of roundedIntRece and enclosingIntRect to preserve size (size change causes TiledBackingStore's work!) > http://trac.webkit.org/wiki/LayoutUnit I am not sure, because LayoutUnits store the value (float) in an int. So if you apply scale to a rect and then remove it again, then you don't necessarily end up with the same rect, at least not if the values are not int aligned first. Also then we would need to enable sub pixel layouts which we cannot yet due to some blocker bugs (cairo fonts for one)

Created attachment 178215 [details] Patch

Comment on attachment 178215 [details] Patch Attachment 178215 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15175704

Created attachment 178220 [details] Patch

Comment on attachment 178220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178220&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:98 > + if (!isIntegral(m_effectiveScale)) { if (modf(m_effectiveScale))

Created attachment 178223 [details] Patch

Comment on attachment 178223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178223&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:599 > + // Sets the values. > + computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset); Let's rename it then to adjustGeometryForPixelAlignment

> > + // Sets the values. > > + computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset); > > Let's rename it then to adjustGeometryForPixelAlignment You find that more clear? it modifies its arguments and I am just passing them directly here. The method itself doesn't adjust anything, plus I like using the same naming as in the mac code

(In reply to comment #27) > > > + // Sets the values. > > > + computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset); > > > > Let's rename it then to adjustGeometryForPixelAlignment > > You find that more clear? it modifies its arguments and I am just passing them directly here. The method itself doesn't adjust anything, plus I like using the same naming as in the mac code Can be renamed later, if we decide to do so.

Landed in 136976