Bug 156097 - Changing canvas height immediately after page load does not relayout canvas
Summary: Changing canvas height immediately after page load does not relayout canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-31 23:34 PDT by nkronlage
Modified: 2016-06-09 15:33 PDT (History)
10 users (show)

See Also:


Attachments
Repro (379 bytes, text/html)
2016-03-31 23:34 PDT, nkronlage
no flags Details
Patch to see if the code from RenderImage doesn't break anything (2.62 KB, patch)
2016-06-08 17:21 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2016-06-09 14:24 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.61 KB, patch)
2016-06-09 14:29 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (8.72 KB, patch)
2016-06-09 15:06 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nkronlage 2016-03-31 23:34:30 PDT
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.
Comment 1 nkronlage 2016-03-31 23:35:59 PDT
Note that resizing the browser causes the canvas to become the correct size.
Comment 2 Simon Fraser (smfr) 2016-03-31 23:37:32 PDT
Sounds a bit like bug 152556.
Comment 3 Antoine Quint 2016-04-20 00:55:12 PDT
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.
Comment 4 Antoine Quint 2016-04-20 00:56:13 PDT
The test case works fine in Firefox but is incorrect in both Safari and Chrome.
Comment 5 Radar WebKit Bug Importer 2016-04-20 00:57:20 PDT
<rdar://problem/25824682>
Comment 6 Simon Fraser (smfr) 2016-04-20 09:16:06 PDT
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?
Comment 7 Simon Fraser (smfr) 2016-04-20 09:55:31 PDT
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.
Comment 8 David 2016-04-26 11:21:05 PDT
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";
Comment 9 Simon Fraser (smfr) 2016-04-26 11:22:18 PDT
I think it is a bug, because it works in contexts other than flexbox.
Comment 10 David 2016-04-26 11:30:40 PDT
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).
Comment 11 David 2016-04-26 11:38:49 PDT
(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.
Comment 12 nkronlage 2016-04-27 13:58:58 PDT
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.
Comment 13 David 2016-04-28 10:37:07 PDT
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."
Comment 14 Antoine Quint 2016-06-08 17:18:50 PDT
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.
Comment 15 Antoine Quint 2016-06-08 17:21:42 PDT
Created attachment 280857 [details]
Patch to see if the code from RenderImage doesn't break anything
Comment 16 WebKit Commit Bot 2016-06-08 17:24:10 PDT
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.
Comment 17 Antoine Quint 2016-06-09 14:24:52 PDT
Created attachment 280946 [details]
Patch
Comment 18 Antoine Quint 2016-06-09 14:29:08 PDT
Created attachment 280948 [details]
Patch
Comment 19 zalan 2016-06-09 14:47:06 PDT
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.
Comment 20 Antoine Quint 2016-06-09 15:06:35 PDT
Created attachment 280955 [details]
Patch
Comment 21 WebKit Commit Bot 2016-06-09 15:33:11 PDT
Comment on attachment 280955 [details]
Patch

Clearing flags on attachment: 280955

Committed r201889: <http://trac.webkit.org/changeset/201889>
Comment 22 WebKit Commit Bot 2016-06-09 15:33:16 PDT
All reviewed patches have been landed.  Closing bug.