Changeset 181720 in webkit


Ignore:
Timestamp:
Mar 18, 2015 7:15:00 PM (9 years ago)
Author:
commit-queue@webkit.org
Message:

Switching between two SVG images with no intrinsic sizes causes them to get the default SVG size instead of the container size.
​https://bugs.webkit.org/show_bug.cgi?id=142805.

Patch by Said Abou-Hallawa <​sabouhallawa@apple.com> on 2015-03-18
Reviewed by Darin Adler.
Source/WebCore:

The bug happens due to wrong logic in RenderImage::imageDimensionsChanged().
This function decides to setNeedsLayout() if the intrinsic size of the image
changes. If the size does not change, it only repaints the image rectangle.
When switching the src of the an image between two SVG images and both of
them have no intrinsic size, we do not updateInnerContentRect() and this
means an SVGImageForContainer is not going to be created for this image.
When the image is drawn, it is drawn directly from the SVGImage. And this
means the drawing has to be scaled by container_size / SVG_default_intrinsic_size

After figuring out that I need to updateInnerContentRect() to fix this bug,
I found out Blink has already changed this code to do the same thing. But
they also did more clean-up in this function. Here is the link
​https://codereview.chromium.org/114323004. I think their change seems correct
although they did not say what exactly they were trying to fix.

The plan for repaintOrMarkForLayout(), which is the new name of this function,
is the following:

-- setNeedLayout() if the intrinsic size changes and it affects the size

of the image.

-- updateInnerContentRect() if the intrinsic size did not change but the

image has exiting layout.

-- repaint the image rectangle if layout is not needed.

This change also removes the call to computeLogicalWidthInRegion(), which is
almost running a layout for the image. This call figures out whether the image
needs to setNeedsLayout(). This call is unnecessary; the image needs to run a
layout if the intrinsic size has changed and it affects the size of the image.

Test: svg/as-image/svg-no-intrinsic-size-switching.html

  • rendering/RenderImage.cpp:

(WebCore::RenderImage::styleDidChange): Change the function call.
(WebCore::RenderImage::imageChanged): Rename local variable and change the
function call.

(WebCore::RenderImage::updateIntrinsicSizeIfNeeded): Simplify this function.
Call setIntrinsicSize() with the new size unless the image is in error state.

(WebCore::RenderImage::repaintOrMarkForLayout): This a better name for this
function since it is called even if the intrinsic size was not changed.
(WebCore::RenderImage::imageDimensionsChanged): Deleted.

  • rendering/RenderImage.h: Rename imageDimensionsChanged() and change the

updateIntrinsicSizeIfNeeded() to return void.

  • rendering/svg/RenderSVGForeignObject.cpp:

(WebCore::RenderSVGForeignObject::paint): Code cleanup. This function can
only handle the paint phases PaintPhaseForeground and PaintPhaseSelection.
Use this information to simplify the logic and order of painting there.

LayoutTests:

  • svg/as-image/svg-no-intrinsic-size-switching-expected.html: Added.
  • svg/as-image/svg-no-intrinsic-size-switching.html: Added.

Ensure that switching the source of an <img> element between two SVG images,
which have no intrinsic sizes, gets the image the size of the container and
not the default SVG intrinsic size which is 300x150 pixels.

Location:
trunk
Files:
2 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r181712 r181720  
     12015-03-18  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Switching between two SVG images with no intrinsic sizes causes them to get the default SVG size instead of the container size.
     4        https://bugs.webkit.org/show_bug.cgi?id=142805.
     5
     6        Reviewed by Darin Adler.
     7
     8        * svg/as-image/svg-no-intrinsic-size-switching-expected.html: Added.
     9        * svg/as-image/svg-no-intrinsic-size-switching.html: Added.
     10        Ensure that switching the source of an <img> element between two SVG images,
     11        which have no intrinsic sizes, gets the image the size of the container and
     12        not the default SVG intrinsic size which is 300x150 pixels.
     13
    1142015-03-18  Alexey Proskuryakov  <ap@apple.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r181713 r181720  
     12015-03-18  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Switching between two SVG images with no intrinsic sizes causes them to get the default SVG size instead of the container size.
     4        https://bugs.webkit.org/show_bug.cgi?id=142805.
     5
     6        Reviewed by Darin Adler.
     7       
     8        The bug happens due to wrong logic in RenderImage::imageDimensionsChanged().
     9        This function decides to setNeedsLayout() if the intrinsic size of the image
     10        changes. If the size does not change, it only repaints the image rectangle.
     11        When switching the src of the an image between two SVG images and both of
     12        them have no intrinsic size, we do not updateInnerContentRect() and this
     13        means an SVGImageForContainer is not going to be created for this image.
     14        When the image is drawn, it is drawn directly from the SVGImage. And this
     15        means the drawing has to be scaled by container_size / SVG_default_intrinsic_size
     16       
     17        After figuring out that I need to updateInnerContentRect() to fix this bug,
     18        I found out Blink has already changed this code to do the same thing. But
     19        they also did more clean-up in this function. Here is the link
     20        https://codereview.chromium.org/114323004. I think their change seems correct
     21        although they did not say what exactly they were trying to fix.
     22       
     23        The plan for repaintOrMarkForLayout(), which is the new name of this function,
     24        is the following:
     25            -- setNeedLayout() if the intrinsic size changes and it affects the size
     26               of the image.
     27            -- updateInnerContentRect() if the intrinsic size did not change but the
     28               image has exiting layout.
     29            -- repaint the image rectangle if layout is not needed.
     30           
     31        This change also removes the call to computeLogicalWidthInRegion(), which is
     32        almost running a layout for the image. This call figures out whether the image
     33        needs to setNeedsLayout(). This call is unnecessary; the image needs to run a
     34        layout if the intrinsic size has changed and it affects the size of the image.
     35                   
     36        Test: svg/as-image/svg-no-intrinsic-size-switching.html
     37
     38        * rendering/RenderImage.cpp:
     39        (WebCore::RenderImage::styleDidChange): Change the function call.
     40        (WebCore::RenderImage::imageChanged): Rename local variable and change the
     41        function call.
     42       
     43        (WebCore::RenderImage::updateIntrinsicSizeIfNeeded): Simplify this function.
     44        Call setIntrinsicSize() with the new size unless the image is in error state.
     45       
     46        (WebCore::RenderImage::repaintOrMarkForLayout): This a better name for this
     47        function since it is called even if the intrinsic size was not changed.
     48        (WebCore::RenderImage::imageDimensionsChanged): Deleted.
     49       
     50        * rendering/RenderImage.h: Rename imageDimensionsChanged() and change the
     51        updateIntrinsicSizeIfNeeded() to return void.
     52       
     53        * rendering/svg/RenderSVGForeignObject.cpp:
     54        (WebCore::RenderSVGForeignObject::paint): Code cleanup. This function can
     55        only handle the paint phases PaintPhaseForeground and PaintPhaseSelection.
     56        Use this information to simplify the logic and order of painting there.
     57
    1582015-03-18  Jeremy Jones  <jeremyj@apple.com>
    259

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

    r181505 r181720  
    181181// Sets the image height and width to fit the alt text.  Returns true if the
    182182// image size changed.
    183 bool RenderImage::setImageSizeForAltText(CachedImage* newImage /* = 0 */)
     183ImageSizeChangeType RenderImage::setImageSizeForAltText(CachedImage* newImage /* = 0 */)
    184184{
    185185    IntSize imageSize;
    … …  
    199199
    200200    if (imageSize == intrinsicSize())
    201         return false;
     201        return ImageSizeChangeNone;
    202202
    203203    setIntrinsicSize(imageSize);
    204     return true;
     204    return ImageSizeChangeForAltText;
    205205}
    206206
    … …  
    210210    if (m_needsToSetSizeForAltText) {
    211211        if (!m_altText.isEmpty() && setImageSizeForAltText(imageResource().cachedImage()))
    212             imageDimensionsChanged(true /* imageSizeChanged */);
     212            repaintOrMarkForLayout(ImageSizeChangeForAltText);
    213213        m_needsToSetSizeForAltText = false;
    214214    }
    215215#if ENABLE(CSS_IMAGE_ORIENTATION)
    216216    if (diff == StyleDifferenceLayout && oldStyle->imageOrientation() != style().imageOrientation())
    217         return imageDimensionsChanged(true /* imageSizeChanged */);
     217        return repaintOrMarkForLayout(ImageSizeChangeForAltText);
    218218#endif
    219219
    … …  
    223223            || oldStyle->imageResolutionSnap() != style().imageResolutionSnap()
    224224            || oldStyle->imageResolutionSource() != style().imageResolutionSource()))
    225         imageDimensionsChanged(true /* imageSizeChanged */);
     225        repaintOrMarkForLayout(ImageSizeChangeForAltText;
    226226#endif
    227227}
    … …  
    247247    }
    248248
    249     bool imageSizeChanged = false;
     249    ImageSizeChangeType imageSizeChange = ImageSizeChangeNone;
    250250
    251251    // Set image dimensions, taking into account the size of the alt text.
    … …  
    259259            return;
    260260        }
    261         imageSizeChanged = setImageSizeForAltText(imageResource().cachedImage());
    262     }
    263 
    264     imageDimensionsChanged(imageSizeChanged, rect);
    265 }
    266 
    267 bool RenderImage::updateIntrinsicSizeIfNeeded(const LayoutSize& newSize, bool imageSizeChanged)
    268 {
    269     if (newSize == intrinsicSize() && !imageSizeChanged)
    270         return false;
    271     if (imageResource().errorOccurred())
    272         return imageSizeChanged;
     261        imageSizeChange = setImageSizeForAltText(imageResource().cachedImage());
     262    }
     263
     264    repaintOrMarkForLayout(imageSizeChange, rect);
     265}
     266
     267void RenderImage::updateIntrinsicSizeIfNeeded(const LayoutSize& newSize)
     268{
     269    if (imageResource().errorOccurred() || !m_imageResource->hasImage())
     270        return;
    273271    setIntrinsicSize(newSize);
    274     return true;
    275272}
    276273
    … …  
    283280}
    284281
    285 void RenderImage::imageDimensionsChanged(bool imageSizeChanged, const IntRect* rect)
     282void RenderImage::repaintOrMarkForLayout(ImageSizeChangeType imageSizeChange, const IntRect* rect)
    286283{
    287284#if ENABLE(CSS_IMAGE_RESOLUTION)
    … …  
    291288    if (scale <= 0)
    292289        scale = 1;
    293     bool intrinsicSizeChanged = updateIntrinsicSizeIfNeeded(imageResource().intrinsicSize(style().effectiveZoom() / scale), imageSizeChanged);
     290    LayoutSize newIntrinsicSize = imageResource().intrinsicSize(style().effectiveZoom() / scale);
    294291#else
    295     bool intrinsicSizeChanged = updateIntrinsicSizeIfNeeded(imageResource().intrinsicSize(style().effectiveZoom()), imageSizeChanged);
    296 #endif
     292    LayoutSize newIntrinsicSize = imageResource().intrinsicSize(style().effectiveZoom());
     293#endif
     294    LayoutSize oldIntrinsicSize = intrinsicSize();
     295
     296    updateIntrinsicSizeIfNeeded(newIntrinsicSize);
    297297
    298298    // In the case of generated image content using :before/:after/content, we might not be
    … …  
    303303        return;
    304304
    305     bool shouldRepaint = true;
    306     if (intrinsicSizeChanged) {
    307         if (!preferredLogicalWidthsDirty())
    308             setPreferredLogicalWidthsDirty(true);
    309 
    310         bool hasOverrideSize = hasOverrideHeight() || hasOverrideWidth();
    311         if (!hasOverrideSize && !imageSizeChanged) {
    312             LogicalExtentComputedValues computedValues;
    313             computeLogicalWidthInRegion(computedValues);
    314             LayoutUnit newWidth = computedValues.m_extent;
    315             computeLogicalHeight(height(), 0, computedValues);
    316             LayoutUnit newHeight = computedValues.m_extent;
    317 
    318             imageSizeChanged = width() != newWidth || height() != newHeight;
    319         }
    320 
    321         // FIXME: We only need to recompute the containing block's preferred size
    322         // if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing).
    323         // There's no easy way to detect that shrink-to-fit is needed, always force a layout.
    324         bool containingBlockNeedsToRecomputePreferredSize =
    325             style().logicalWidth().isPercent()
    326             || style().logicalMaxWidth().isPercent()
    327             || style().logicalMinWidth().isPercent();
    328 
    329         bool layoutSizeDependsOnIntrinsicSize = style().aspectRatioType() == AspectRatioFromIntrinsic;
    330 
    331         if (imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize) {
    332             shouldRepaint = false;
    333             if (!selfNeedsLayout())
    334                 setNeedsLayout();
    335         }
    336 
    337         if (everHadLayout() && !selfNeedsLayout()) {
    338             // The inner content rectangle is calculated during layout, but may need an update now
    339             // (unless the box has already been scheduled for layout). In order to calculate it, we
    340             // may need values from the containing block, though, so make sure that we're not too
    341             // early. It may be that layout hasn't even taken place once yet.
    342 
    343             // FIXME: we should not have to trigger another call to setContainerSizeForRenderer()
    344             // from here, since it's already being done during layout.
    345             updateInnerContentRect();
    346         }
    347     }
    348 
    349     if (shouldRepaint) {
    350         LayoutRect repaintRect;
    351         if (rect) {
    352             // The image changed rect is in source image coordinates (pre-zooming),
    353             // so map from the bounds of the image to the contentsBox.
    354             repaintRect = enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), contentBoxRect()));
    355             // Guard against too-large changed rects.
    356             repaintRect.intersect(contentBoxRect());
    357         } else
    358             repaintRect = contentBoxRect();
     305    bool imageSourceHasChangedSize = oldIntrinsicSize != newIntrinsicSize || imageSizeChange != ImageSizeChangeNone;
     306
     307    if (imageSourceHasChangedSize)
     308        setPreferredLogicalWidthsDirty(true);
     309
     310    // If the actual area occupied by the image has changed and it is not constrained by style then a layout is required.
     311    bool imageSizeIsConstrained = style().logicalWidth().isSpecified() && style().logicalHeight().isSpecified();
     312    bool needsLayout = !imageSizeIsConstrained && imageSourceHasChangedSize;
     313
     314    // FIXME: We only need to recompute the containing block's preferred size
     315    // if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing).
     316    // There's no easy way to detect that shrink-to-fit is needed, always force a layout.
     317    bool containingBlockNeedsToRecomputePreferredSize =
     318        style().logicalWidth().isPercent()
     319        || style().logicalMaxWidth().isPercent()
     320        || style().logicalMinWidth().isPercent();
     321
     322    bool layoutSizeDependsOnIntrinsicSize = style().aspectRatioType() == AspectRatioFromIntrinsic;
     323
     324    if (needsLayout || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize) {
     325        setNeedsLayout();
     326        return;
     327    }
     328
     329    if (everHadLayout() && !selfNeedsLayout()) {
     330        // The inner content rectangle is calculated during layout, but may need an update now
     331        // (unless the box has already been scheduled for layout). In order to calculate it, we
     332        // may need values from the containing block, though, so make sure that we're not too
     333        // early. It may be that layout hasn't even taken place once yet.
     334
     335        // FIXME: we should not have to trigger another call to setContainerSizeForRenderer()
     336        // from here, since it's already being done during layout.
     337        updateInnerContentRect();
     338    }
     339
     340    LayoutRect repaintRect = contentBoxRect();
     341    if (rect) {
     342        // The image changed rect is in source image coordinates (pre-zooming),
     343        // so map from the bounds of the image to the contentsBox.
     344        repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), repaintRect)));
     345    }
    359346       
    360         repaintRectangle(repaintRect);
    361 
    362         // Tell any potential compositing layers that the image needs updating.
    363         contentChanged(ImageChanged);
    364     }
     347    repaintRectangle(repaintRect);
     348
     349    // Tell any potential compositing layers that the image needs updating.
     350    contentChanged(ImageChanged);
    365351}
    366352

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

    r181412 r181720  
    3434class HTMLMapElement;
    3535
     36enum ImageSizeChangeType {
     37    ImageSizeChangeNone,
     38    ImageSizeChangeForAltText
     39};
     40
    3641class RenderImage : public RenderReplaced {
    3742public:
    … …  
    4449    CachedImage* cachedImage() const { return imageResource().cachedImage(); }
    4550
    46     bool setImageSizeForAltText(CachedImage* newImage = 0);
     51    ImageSizeChangeType setImageSizeForAltText(CachedImage* newImage = nullptr);
    4752
    4853    void updateAltText();
    … …  
    7580    virtual void styleDidChange(StyleDifference, const RenderStyle*) override final;
    7681
    77     virtual void imageChanged(WrappedImagePtr, const IntRect* = 0) override;
     82    virtual void imageChanged(WrappedImagePtr, const IntRect* = nullptr) override;
    7883
    7984    void paintIntoRect(GraphicsContext*, const FloatRect&);
    … …  
    108113
    109114    IntSize imageSizeForError(CachedImage*) const;
    110     void imageDimensionsChanged(bool imageSizeChanged, const IntRect* = 0);
    111     bool updateIntrinsicSizeIfNeeded(const LayoutSize&, bool imageSizeChanged);
     115    void repaintOrMarkForLayout(ImageSizeChangeType, const IntRect* = nullptr);
     116    void updateIntrinsicSizeIfNeeded(const LayoutSize&);
    112117    // Update the size of the image to be rendered. Object-fit may cause this to be different from the CSS box's content rect.
    113118    void updateInnerContentRect();

  • trunk/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp

    r179982 r181720  
    5454void RenderSVGForeignObject::paint(PaintInfo& paintInfo, const LayoutPoint&)
    5555{
    56     if (paintInfo.context->paintingDisabled()
    57         || (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseSelection))
     56    if (paintInfo.context->paintingDisabled())
     57        return;
     58
     59    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseSelection)
    5860        return;
    5961
    … …  
    6668
    6769    SVGRenderingContext renderingContext;
    68     bool continueRendering = true;
    6970    if (paintInfo.phase == PaintPhaseForeground) {
    7071        renderingContext.prepareToRenderSVGContent(*this, childPaintInfo);
    71         continueRendering = renderingContext.isRenderingPrepared();
     72        if (!renderingContext.isRenderingPrepared())
     73            return;
    7274    }
    7375
    74     if (continueRendering) {
    75         // Paint all phases of FO elements atomically, as though the FO element established its
    76         // own stacking context.
    77         bool preservePhase = paintInfo.phase == PaintPhaseSelection || paintInfo.phase == PaintPhaseTextClip;
    78         LayoutPoint childPoint = IntPoint();
    79         childPaintInfo.phase = preservePhase ? paintInfo.phase : PaintPhaseBlockBackground;
    80         RenderBlock::paint(childPaintInfo, IntPoint());
    81         if (!preservePhase) {
    82             childPaintInfo.phase = PaintPhaseChildBlockBackgrounds;
    83             RenderBlock::paint(childPaintInfo, childPoint);
    84             childPaintInfo.phase = PaintPhaseFloat;
    85             RenderBlock::paint(childPaintInfo, childPoint);
    86             childPaintInfo.phase = PaintPhaseForeground;
    87             RenderBlock::paint(childPaintInfo, childPoint);
    88             childPaintInfo.phase = PaintPhaseOutline;
    89             RenderBlock::paint(childPaintInfo, childPoint);
    90         }
     76    LayoutPoint childPoint = IntPoint();
     77    if (paintInfo.phase == PaintPhaseSelection) {
     78        RenderBlock::paint(childPaintInfo, childPoint);
     79        return;
    9180    }
     81
     82    // Paint all phases of FO elements atomically, as though the FO element established its
     83    // own stacking context.
     84    childPaintInfo.phase = PaintPhaseBlockBackground;
     85    RenderBlock::paint(childPaintInfo, childPoint);
     86    childPaintInfo.phase = PaintPhaseChildBlockBackgrounds;
     87    RenderBlock::paint(childPaintInfo, childPoint);
     88    childPaintInfo.phase = PaintPhaseFloat;
     89    RenderBlock::paint(childPaintInfo, childPoint);
     90    childPaintInfo.phase = PaintPhaseForeground;
     91    RenderBlock::paint(childPaintInfo, childPoint);
     92    childPaintInfo.phase = PaintPhaseOutline;
     93    RenderBlock::paint(childPaintInfo, childPoint);
    9294}
    9395
Note: See TracChangeset for help on using the changeset viewer.