Changeset 128346 in webkit


Ignore:
Timestamp:
Sep 12, 2012 12:17:03 PM (12 years ago)
Author:
leviw@chromium.org
Message:

Inline repainting can be off-by-one with sub-pixel enabled
​https://bugs.webkit.org/show_bug.cgi?id=95882

Reviewed by Eric Seidel.

Source/WebCore:

With sub-pixel layout enabled, context accumulated from containing renderers is used to properly
pixel snap. Selection repaint rects are stored outside the affected renderers, and are handed to
the embedder without the context to correctly determine the snapped values. This can result in
repaint areas smaller than what's needed.

This could be fixed three ways:

  • by changing selection repaint rects to an IntRect and pixel snapping the values before storing them outside the render tree. This is the narrowest solution.
  • by adapting layout to store pixel snapping hints along with the size/location of the renderer. This is the best possible solution, as it would help solve a lot of pixel snapping issues and reduce complexity of handling. I eventually intend on implementing this.
  • by reverting repaintUsingContainer to IntRects and using pixel snapping when possible, and enclosingIntRect where we lack context.

This implements the last solution, as it's the least invasive, but can potentially fix numerous
sub-pixel repaint issues.

Test: fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html

  • rendering/RenderBlockLineLayout.cpp:

(WebCore::RenderBlock::layoutRunsAndFloats):

  • rendering/RenderLayer.cpp:

(WebCore::RenderLayer::updateLayerPositions):
(WebCore::RenderLayer::scrollTo):
(WebCore::RenderLayer::repaintIncludingNonCompositingDescendants):

  • rendering/RenderObject.cpp:

(WebCore::RenderObject::repaintUsingContainer):
(WebCore::RenderObject::repaint):
(WebCore::RenderObject::repaintRectangle):
(WebCore::RenderObject::repaintAfterLayoutIfNeeded):

  • rendering/RenderObject.h:

(RenderObject):

  • rendering/RenderSelectionInfo.h:

(WebCore::RenderSelectionInfo::repaint):
(WebCore::RenderBlockSelectionInfo::repaint):

LayoutTests:

Test that we properly repaint selection gaps inside tables with sub-pixel offsets.

  • fast/sub-pixel/selection/selection-rect-in-sub-pixel-table-expected.txt: Added.
  • fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html: Added.
  • platform/chromium-mac/fast/sub-pixel/selection/selection-rect-in-sub-pixel-table-expected.png: Added.
  • platform/chromium/TestExpectations:
  • platform/mac-lion/Skipped:
  • platform/mac-snowleopard/Skipped:
  • platform/mac-wk2/Skipped:
  • platform/mac/Skipped:
  • platform/qt-4.8/Skipped:
  • platform/qt/Skipped:
  • platform/win-wk2/Skipped:
  • platform/win-xp/Skipped:
  • platform/win/Skipped:
  • platform/wincairo/Skipped:
  • platform/wk2/Skipped:
Location:
trunk
Files:
3 added
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r128342 r128346  
     12012-09-12  Levi Weintraub  <leviw@chromium.org>
     2
     3        Inline repainting can be off-by-one with sub-pixel enabled
     4        https://bugs.webkit.org/show_bug.cgi?id=95882
     5
     6        Reviewed by Eric Seidel.
     7
     8        Test that we properly repaint selection gaps inside tables with sub-pixel offsets.
     9
     10        * fast/sub-pixel/selection/selection-rect-in-sub-pixel-table-expected.txt: Added.
     11        * fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html: Added.
     12        * platform/chromium-mac/fast/sub-pixel/selection/selection-rect-in-sub-pixel-table-expected.png: Added.
     13        * platform/chromium/TestExpectations:
     14        * platform/mac-lion/Skipped:
     15        * platform/mac-snowleopard/Skipped:
     16        * platform/mac-wk2/Skipped:
     17        * platform/mac/Skipped:
     18        * platform/qt-4.8/Skipped:
     19        * platform/qt/Skipped:
     20        * platform/win-wk2/Skipped:
     21        * platform/win-xp/Skipped:
     22        * platform/win/Skipped:
     23        * platform/wincairo/Skipped:
     24        * platform/wk2/Skipped:
     25
    1262012-09-12  Zan Dobersek  <zandobersek@gmail.com>
    227

  • trunk/LayoutTests/platform/chromium/TestExpectations

    r128314 r128346  
    34283428BUGWK94256 DEBUG : fast/block/inline-children-root-linebox-crash.html = PASS CRASH
    34293429
     3430// Need baselines after bots cycle
     3431BUGLEVIW LINUX WIN SNOWLEOPARD : fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html = PASS IMAGE IMAGE+TEXT MISSING
     3432
    34303433// Following tests need baselines on Win and Linux
    34313434BUGWK94492 WIN LINUX : css3/filters/custom/custom-filter-color-matrix.html = IMAGE

  • trunk/LayoutTests/platform/mac-lion/Skipped

    r127933 r128346  
    119119fast/sub-pixel/table-rows-no-gaps.html
    120120fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     121fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    121122
    122123# This media test always failed on Lion

  • trunk/LayoutTests/platform/mac-snowleopard/Skipped

    r127933 r128346  
    213213fast/sub-pixel/table-rows-no-gaps.html
    214214fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     215fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    215216
    216217# Frame::findString does nothing on pages that prevent selection

  • trunk/LayoutTests/platform/mac-wk2/Skipped

    r127933 r128346  
    217217fast/sub-pixel/table-rows-no-gaps.html
    218218fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     219fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    219220
    220221# fast/events/pagehide-timeout.html, pageshow-pagehide-on-back-cached-with-frames.html, and

  • trunk/LayoutTests/platform/mac/Skipped

    r128072 r128346  
    825825fast/sub-pixel/table-rows-no-gaps.html
    826826fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     827fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    827828
    828829# No CORS support for media elements is implemented yet.

  • trunk/LayoutTests/platform/qt-4.8/Skipped

    r127933 r128346  
    106106fast/sub-pixel/table-rows-no-gaps.html
    107107fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     108fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    108109
    109110# SVG Fonts are only supported when using QRawFont, which is not

  • trunk/LayoutTests/platform/qt/Skipped

    r128306 r128346  
    308308fast/sub-pixel/table-rows-no-gaps.html
    309309fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     310fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    310311
    311312# CSS Regions support not yet enabled. http://webkit.org/b/57312

  • trunk/LayoutTests/platform/win-wk2/Skipped

    r127933 r128346  
    950950fast/sub-pixel/table-rows-no-gaps.html
    951951fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     952fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    952953
    953954# HiDPI tests require test infrastructure enhancements

  • trunk/LayoutTests/platform/win-xp/Skipped

    r127933 r128346  
    4949fast/sub-pixel/table-rows-no-gaps.html
    5050fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     51fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    5152
    5253# REGRESSION (r83928 or before): Some tests failing assertions in MarkStack::internalAppend / MarkStack::drain

  • trunk/LayoutTests/platform/win/Skipped

    r128129 r128346  
    17461746fast/sub-pixel/table-rows-no-gaps.html
    17471747fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     1748fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    17481749
    17491750# No CORS support for media elements is implemented yet.

  • trunk/LayoutTests/platform/wincairo/Skipped

    r127933 r128346  
    21352135fast/sub-pixel/table-rows-no-gaps.html
    21362136fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     2137fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    21372138
    21382139#Battery Status API is not implemented.

  • trunk/LayoutTests/platform/wk2/Skipped

    r128338 r128346  
    9898fast/sub-pixel/table-rows-no-gaps.html
    9999fast/sub-pixel/sub-pixel-accumulates-to-layers.html
     100fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
    100101
    101102# [WK2][WTR] svg/animations/animate-text-nested-transforms.html fails

  • trunk/Source/WebCore/ChangeLog

    r128345 r128346  
     12012-09-12  Levi Weintraub  <leviw@chromium.org>
     2
     3        Inline repainting can be off-by-one with sub-pixel enabled
     4        https://bugs.webkit.org/show_bug.cgi?id=95882
     5
     6        Reviewed by Eric Seidel.
     7
     8        With sub-pixel layout enabled, context accumulated from containing renderers is used to properly
     9        pixel snap. Selection repaint rects are stored outside the affected renderers, and are handed to
     10        the embedder without the context to correctly determine the snapped values. This can result in
     11        repaint areas smaller than what's needed.
     12
     13        This could be fixed three ways:
     14        - by changing selection repaint rects to an IntRect and pixel snapping the values before storing
     15          them outside the render tree. This is the narrowest solution.
     16        - by adapting layout to store pixel snapping hints along with the size/location of the renderer.
     17          This is the best possible solution, as it would help solve a lot of pixel snapping issues and
     18          reduce complexity of handling. I eventually intend on implementing this.
     19        - by reverting repaintUsingContainer to IntRects and using pixel snapping when possible, and
     20          enclosingIntRect where we lack context.
     21
     22        This implements the last solution, as it's the least invasive, but can potentially fix numerous
     23        sub-pixel repaint issues.
     24
     25        Test: fast/sub-pixel/selection/selection-rect-in-sub-pixel-table.html
     26
     27        * rendering/RenderBlockLineLayout.cpp:
     28        (WebCore::RenderBlock::layoutRunsAndFloats):
     29        * rendering/RenderLayer.cpp:
     30        (WebCore::RenderLayer::updateLayerPositions):
     31        (WebCore::RenderLayer::scrollTo):
     32        (WebCore::RenderLayer::repaintIncludingNonCompositingDescendants):
     33        * rendering/RenderObject.cpp:
     34        (WebCore::RenderObject::repaintUsingContainer):
     35        (WebCore::RenderObject::repaint):
     36        (WebCore::RenderObject::repaintRectangle):
     37        (WebCore::RenderObject::repaintAfterLayoutIfNeeded):
     38        * rendering/RenderObject.h:
     39        (RenderObject):
     40        * rendering/RenderSelectionInfo.h:
     41        (WebCore::RenderSelectionInfo::repaint):
     42        (WebCore::RenderBlockSelectionInfo::repaint):
     43
    1442012-09-12  Michael Saboff  <msaboff@apple.com>
    245

  • trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

    r128083 r128346  
    12331233            // before layout started.  Luckily the layer has cached the repaint rect for its original
    12341234            // position and size, and so we can use that to make a repaint happen now.
    1235             repaintUsingContainer(containerForRepaint(), layer()->repaintRect());
     1235            repaintUsingContainer(containerForRepaint(), pixelSnappedIntRect(layer()->repaintRect()));
    12361236        }
    12371237    }

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r128134 r128346  
    401401            if (view && !view->printing()) {
    402402                if (m_repaintStatus & NeedsFullRepaint) {
    403                     renderer()->repaintUsingContainer(repaintContainer, oldRepaintRect);
     403                    renderer()->repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldRepaintRect));
    404404                    if (m_repaintRect != oldRepaintRect)
    405                         renderer()->repaintUsingContainer(repaintContainer, m_repaintRect);
     405                        renderer()->repaintUsingContainer(repaintContainer, pixelSnappedIntRect(m_repaintRect));
    406406                } else if (shouldRepaintAfterLayout())
    407407                    renderer()->repaintAfterLayoutIfNeeded(repaintContainer, oldRepaintRect, oldOutlineBox, &m_repaintRect, &m_outlineBox);
    … …  
    17341734    // Just schedule a full repaint of our object.
    17351735    if (view && !usesCompositedScrolling())
    1736         renderer()->repaintUsingContainer(repaintContainer, m_repaintRect);
     1736        renderer()->repaintUsingContainer(repaintContainer, pixelSnappedIntRect(m_repaintRect));
    17371737
    17381738    // Schedule the scroll DOM event.
    … …  
    48024802void RenderLayer::repaintIncludingNonCompositingDescendants(RenderBoxModelObject* repaintContainer)
    48034803{
    4804     renderer()->repaintUsingContainer(repaintContainer, renderer()->clippedOverflowRectForRepaint(repaintContainer));
     4804    renderer()->repaintUsingContainer(repaintContainer, pixelSnappedIntRect(renderer()->clippedOverflowRectForRepaint(repaintContainer)));
    48054805
    48064806    for (RenderLayer* curr = firstChild(); curr; curr = curr->nextSibling()) {

  • trunk/Source/WebCore/rendering/RenderObject.cpp

    r127318 r128346  
    12781278}
    12791279
    1280 void RenderObject::repaintUsingContainer(RenderBoxModelObject* repaintContainer, const LayoutRect& r, bool immediate) const
     1280void RenderObject::repaintUsingContainer(RenderBoxModelObject* repaintContainer, const IntRect& r, bool immediate) const
    12811281{
    12821282    if (!repaintContainer) {
    … …  
    13051305            LayoutRect repaintRectangle = r;
    13061306            if (viewHasCompositedLayer &&  v->layer()->transform())
    1307                 repaintRectangle = v->layer()->transform()->mapRect(r);
     1307                repaintRectangle = enclosingIntRect(v->layer()->transform()->mapRect(r));
    13081308            v->repaintViewRectangle(repaintRectangle, immediate);
    13091309            return;
    … …  
    13321332
    13331333    RenderBoxModelObject* repaintContainer = containerForRepaint();
    1334     repaintUsingContainer(repaintContainer ? repaintContainer : view, clippedOverflowRectForRepaint(repaintContainer), immediate);
     1334    repaintUsingContainer(repaintContainer ? repaintContainer : view, pixelSnappedIntRect(clippedOverflowRectForRepaint(repaintContainer)), immediate);
    13351335}
    13361336
    … …  
    13531353    RenderBoxModelObject* repaintContainer = containerForRepaint();
    13541354    computeRectForRepaint(repaintContainer, dirtyRect);
    1355     repaintUsingContainer(repaintContainer ? repaintContainer : view, dirtyRect, immediate);
     1355    repaintUsingContainer(repaintContainer ? repaintContainer : view, pixelSnappedIntRect(dirtyRect), immediate);
    13561356}
    13571357
    … …  
    13881388
    13891389    if (fullRepaint) {
    1390         repaintUsingContainer(repaintContainer, oldBounds);
     1390        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldBounds));
    13911391        if (newBounds != oldBounds)
    1392             repaintUsingContainer(repaintContainer, newBounds);
     1392            repaintUsingContainer(repaintContainer, pixelSnappedIntRect(newBounds));
    13931393        return true;
    13941394    }
    … …  
    13991399    LayoutUnit deltaLeft = newBounds.x() - oldBounds.x();
    14001400    if (deltaLeft > 0)
    1401         repaintUsingContainer(repaintContainer, LayoutRect(oldBounds.x(), oldBounds.y(), deltaLeft, oldBounds.height()));
     1401        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldBounds.x(), oldBounds.y(), deltaLeft, oldBounds.height()));
    14021402    else if (deltaLeft < 0)
    1403         repaintUsingContainer(repaintContainer, LayoutRect(newBounds.x(), newBounds.y(), -deltaLeft, newBounds.height()));
     1403        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(newBounds.x(), newBounds.y(), -deltaLeft, newBounds.height()));
    14041404
    14051405    LayoutUnit deltaRight = newBounds.maxX() - oldBounds.maxX();
    14061406    if (deltaRight > 0)
    1407         repaintUsingContainer(repaintContainer, LayoutRect(oldBounds.maxX(), newBounds.y(), deltaRight, newBounds.height()));
     1407        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldBounds.maxX(), newBounds.y(), deltaRight, newBounds.height()));
    14081408    else if (deltaRight < 0)
    1409         repaintUsingContainer(repaintContainer, LayoutRect(newBounds.maxX(), oldBounds.y(), -deltaRight, oldBounds.height()));
     1409        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(newBounds.maxX(), oldBounds.y(), -deltaRight, oldBounds.height()));
    14101410
    14111411    LayoutUnit deltaTop = newBounds.y() - oldBounds.y();
    14121412    if (deltaTop > 0)
    1413         repaintUsingContainer(repaintContainer, LayoutRect(oldBounds.x(), oldBounds.y(), oldBounds.width(), deltaTop));
     1413        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldBounds.x(), oldBounds.y(), oldBounds.width(), deltaTop));
    14141414    else if (deltaTop < 0)
    1415         repaintUsingContainer(repaintContainer, LayoutRect(newBounds.x(), newBounds.y(), newBounds.width(), -deltaTop));
     1415        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(newBounds.x(), newBounds.y(), newBounds.width(), -deltaTop));
    14161416
    14171417    LayoutUnit deltaBottom = newBounds.maxY() - oldBounds.maxY();
    14181418    if (deltaBottom > 0)
    1419         repaintUsingContainer(repaintContainer, LayoutRect(newBounds.x(), oldBounds.maxY(), newBounds.width(), deltaBottom));
     1419        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(newBounds.x(), oldBounds.maxY(), newBounds.width(), deltaBottom));
    14201420    else if (deltaBottom < 0)
    1421         repaintUsingContainer(repaintContainer, LayoutRect(oldBounds.x(), newBounds.maxY(), oldBounds.width(), -deltaBottom));
     1421        repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldBounds.x(), newBounds.maxY(), oldBounds.width(), -deltaBottom));
    14221422
    14231423    if (newOutlineBox == oldOutlineBox)
    … …  
    14441444        if (rightRect.x() < right) {
    14451445            rightRect.setWidth(min(rightRect.width(), right - rightRect.x()));
    1446             repaintUsingContainer(repaintContainer, rightRect);
     1446            repaintUsingContainer(repaintContainer, pixelSnappedIntRect(rightRect));
    14471447        }
    14481448    }
    … …  
    14631463        if (bottomRect.y() < bottom) {
    14641464            bottomRect.setHeight(min(bottomRect.height(), bottom - bottomRect.y()));
    1465             repaintUsingContainer(repaintContainer, bottomRect);
     1465            repaintUsingContainer(repaintContainer, pixelSnappedIntRect(bottomRect));
    14661466        }
    14671467    }

  • trunk/Source/WebCore/rendering/RenderObject.h

    r127681 r128346  
    757757    // Actually do the repaint of rect r for this object which has been computed in the coordinate space
    758758    // of repaintContainer. If repaintContainer is 0, repaint via the view.
    759     void repaintUsingContainer(RenderBoxModelObject* repaintContainer, const LayoutRect&, bool immediate = false) const;
     759    void repaintUsingContainer(RenderBoxModelObject* repaintContainer, const IntRect&, bool immediate = false) const;
    760760   
    761761    // Repaint the entire object.  Called when, e.g., the color of a border changes, or when a border

  • trunk/Source/WebCore/rendering/RenderSelectionInfo.h

    r112248 r128346  
    6969    void repaint()
    7070    {
    71         m_object->repaintUsingContainer(m_repaintContainer, m_rect);
     71        m_object->repaintUsingContainer(m_repaintContainer, enclosingIntRect(m_rect));
    7272    }
    7373
    … …  
    9090    void repaint()
    9191    {
    92         m_object->repaintUsingContainer(m_repaintContainer, m_rects);
     92        m_object->repaintUsingContainer(m_repaintContainer, enclosingIntRect(m_rects));
    9393    }
    9494   
Note: See TracChangeset for help on using the changeset viewer.