Changing canvas height immediately after page load does not relayout canvas

Created attachment 275373 [details]
Repro

Load the attached test.html (or go to http://www.colorizephoto.com/safaribug/test.html).  This html contains a canvas and some javascript to increase the height of that canvas.

Expected:
The blue canvas is very tall ~1000px tall.

Actual:
The blue canvas is only ~150px tall.


This repros on Mac Safari 9.1, Safari Technical Preview 9.1.1, WebKit Nightly 9.1 (11601.5.17.1) and on Safari on iOS 9.3.1.

Note, this is very similar to Chrome bug 598212 (https://bugs.chromium.org/p/chromium/issues/detail?id=598212), but does not have the exact same repro.

Note that resizing the browser causes the canvas to become the correct size.

[Simon Fraser (smfr)]

Sounds a bit like bug 152556.

[Antoine Quint]

This is not a duplicate of https://bugs.webkit.org/show_bug.cgi?id=152556, that bug was specific to iOS and WebGL, while this affects a <canvas> element and OS X.

[Antoine Quint]

The test case works fine in Firefox but is incorrect in both Safari and Chrome.

[Radar WebKit Bug Importer]

<rdar://problem/25824682>

[Simon Fraser (smfr)]

This seems to be more like a combination of flexbox and canvas resizing. Does a test case that doesn't use flexbox show the bug?

[Simon Fraser (smfr)]

In the test case, we hit RenderHTMLCanvas::canvasSizeChanged() under the attribute setting, but return early because the size appears to not change, so never do the setNeedsLayout() that should happen in this case.

[David]

I believe this is not a bug, but the expected & correct behavior.

See these articles for details:

http://webglfundamentals.org/webgl/lessons/webgl-resizing-the-canvas.html
https://www.khronos.org/webgl/wiki/HandlingHighDPI
http://www.html5rocks.com/en/tutorials/canvas/hidpi/

You can get what you want in your test code by changing the line:

canvas.height = 1000;

to:

canvas.style.height = "1000px";

[Simon Fraser (smfr)]

I think it is a bug, because it works in contexts other than flexbox.

[David]

Sorry, I might be wrong because there's no WebGL involved, I wasn't paying close attention, and I'm not sure about the differences between 2d and 3d canvas behavior. For the 3d context, the width attribute and CSS width style are two different things. Using the style attribute does "fix" the issue in this case, and I reproduced the problem and fix in Chrome Version 50.0.2661.86 (64-bit).

[David]

(In reply to comment #9)
> I think it is a bug, because it works in contexts other than flexbox.

I'm guessing it's confusing because people have tried to make it easy and convenient to set both the resolution and layout size at the same time using the width and height attributes.

But when you let flexbox handle the layout, then setting the width or height attributes should only affect the resolution, because flexbox has constrained the layout size. It makes sense that setting width doesn't do what you expect when flexbox is involved, and does when flexbox isn't.

If you draw on the canvas after setting the width or height attributes, you will see that the backing store resolution is changing.

Try this, for example, and try changing the width & height.

  setTimeout(function() {
    canvas.height = 200;
    canvas.width = 2000;
    var c=document.getElementById("canvas");
    var ctx=c.getContext("2d");
    ctx.rect(20,20,150,100);
    ctx.stroke();
  }, 0);

So, I still don't think this is a bug. Use the CSS style attributes to change the layout of a canvas.

It definitely seems like a bug. The default min-height for items is 'auto' which means it should size to the height of the item. Changing the height in the script (not in the setTimeout call) or resizing the window to force a layout shows flexbox does the take min-height into account correctly in those cases.  Something about this scenario results in layout not running even though a property that affects layout changed.

[David]

The canvas element's "width" and "height" attributes don't always affect the layout. Sometimes they're not allowed to affect layout, like when the size is set in CSS. The HTML attributes are only guaranteed to change resolution.

The only guaranteed way to change the layout size of a canvas is to use style/CSS width & height, not the HTML attributes. And when the CSS size is specified, that overrides the HTML size attributes. I think that's what you've bumped into here.

I just wanted to help explain, I have no bearing on this report, and I hope I'm not doing more damage than good by jumping in here.

The exact circumstances HTML size attributes can and should affect layout are not clear to me, but it's important to know that the answer is not always. By using flexbox, you've added a style constraint that may conflict with the HTML attributes, and if the conflict can't be resolved, the style/CSS attributes are supposed to win. I think that's what happens in your test case. flexbox is allowed to set it's layout before you update the canvas height, so the flexbox uses the canvas' default of 150px. The style size is now effectively set, so subsequent changes to the HTML height attribute might only update the bitmap resolution and not layout.

The links I posted above are relevant - this issue applies to both 2d and 3d canvases - if you want to understand more about this issue. You can also check out the spec for some explanation: https://html.spec.whatwg.org/#attr-canvas-width

"A canvas element can be sized arbitrarily by a style sheet, its bitmap is then subject to the 'object-fit' CSS property."

"Whenever the width and height content attributes are set, removed, changed, or redundantly set to the value they already have, if the canvas context mode is 2d, the user agent must set bitmap dimensions to the numeric values of the width and height content attributes."

[Antoine Quint]

Per the spec, all replaced content, which <canvas> is considered to be, have their width and height attributes affect their layout values should there not be any CSS properties set to control them. So in this case, there is definitely a bug since width and height are "auto". RenderImage::repaintOrMarkForLayout() does what we want here, we may need to bring this up to RenderReplaced.

[Antoine Quint]

Created attachment 280857 [details]
Patch to see if the code from RenderImage doesn't break anything

[WebKit Commit Bot]

Attachment 280857 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 280946 [details]
Patch

[Antoine Quint]

Created attachment 280948 [details]
Patch

[zalan]

Comment on attachment 280948 [details]
Patch

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

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:103
> +    if (parent())
> +        setNeedsLayoutIfNeededAfterIntrinsicSizeChange();

I think the preferred way to write this:
if (!parent())
  return;
setNeedsLayoutIfNeededAfterIntrinsicSizeChange()

> Source/WebCore/rendering/RenderImage.cpp:300
> +        setNeedsLayoutIfNeededAfterIntrinsicSizeChange();
> +        if (needsLayout())

I believe this is behavior change. What if needsLayout bit is already set when we call into setNeedsLayoutIfNeededAfterIntrinsicSizeChange() like due to position change or some other property that requires layout (other than size change). In such cases we early return here while on trunk we might fall through.

[Antoine Quint]

Created attachment 280955 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280955 [details]
Patch

Clearing flags on attachment: 280955

Committed r201889: <http://trac.webkit.org/changeset/201889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.