1104354 - nsHTMLCanvasFrame should override nsIFrame::GetIntrinsicSize

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, nsHTMLCanvasFrame has a GetIntrinsicRatio() implementation, but not a GetIntrinsicSize() implementation.  (So, it presumably just inherits the trivial one from nsFrame.cpp.)

It should have a GetIntrinsicSize() implementation.

Comment 1

[Daniel Holbert [:dholbert]]

Note that this bug means we're technically returning the wrong thing from IsPercentageAware, for <canvas> frames with explicit height/width attributes (which establish their intrinsic size).


In the code quoted below, we currently take the "return true" path, believing that our <canvas> has an intrinsic ratio but lacks an intrinsic size:

> 651 IsPercentageAware(const nsIFrame* aFrame)
[...]
> 702     // Per CSS 2.1, section 10.3.2:
> 703     //   If 'height' and 'width' both have computed values of 'auto' and
> 704     //   the element has an intrinsic ratio but no intrinsic height or
> 705     //   width and the containing block's width does not itself depend
> 706     //   on the replaced element's width, then the used value of 'width'
> 707     //   is calculated from the constraint equation used for
> 708     //   block-level, non-replaced elements in normal flow. 
> 709     nsIFrame *f = const_cast<nsIFrame*>(aFrame);
> 710     if (f->GetIntrinsicRatio() != nsSize(0, 0) &&
> 711         // Some percents are treated like 'auto', so check != coord
> 712         pos->mHeight.GetUnit() != eStyleUnit_Coord) {
> 713       const IntrinsicSize &intrinsicSize = f->GetIntrinsicSize();
> 714       if (intrinsicSize.width.GetUnit() == eStyleUnit_None &&
> 715           intrinsicSize.height.GetUnit() == eStyleUnit_None) {
> 716         return true;
> 717       }
> 718     }
> 719   }
> 720 
> 721   return false;
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp?rev=e299958f1fd5&mark=710-715#702

(I'm not sure if this actually matters in practice.)

My more direct reason for filing this bug is that I need an IntrinsicSize for the canvas in bug 1103184.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This adds static helper-functions to convert the (CSS-pixel-valued) "GetCanvasSize" output into (a) an intrinsic size, and (b) an intrinsic ratio.

(These static helper-functions are just an implementation detail, for now, but they'll be useful to avoid repeated GetCanvasSize calls in bug 1103184.)

Comment 3

[Daniel Holbert [:dholbert]]

Try run (including both this bug's patch & bug 1103184's patch):
 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2a79c7928f68

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8528633 [details] [diff] [review]
fix v1

>layout/generic/nsHTMLCanvasFrame.cpp
>+/* Helper for our nsIFrame::GetIntrinsicSize() impl. Takes the result of
>+ * "GetCanvasSize()" as a parameter, which may help help avoid redundant
>+ * indirect calls to GetCanvasSize().

typo: help help

>+ * @arg aCanvasSizeInPx The canvas's size in CSS pixels, as returned
>+ *                      by GetCanvasSize().

I don't think I've seen @arg before, perhaps you meant @param?

>+  IntrinsicSize intrinsicSize;
>+  if (aCanvasSizeInPx.width > 0) {
>+    intrinsicSize.width.SetCoordValue(
>+      nsPresContext::CSSPixelsToAppUnits(aCanvasSizeInPx.width));
>+  }

IntrinsicSize::width is a nsStyleCoord and its default ctor initializes
mUnit to eStyleUnit_Null so if anyone does .width.GetCoordValue() on
the result returned here it will assert if the aCanvasSizeInPx.width
was negative:
  inline nscoord nsStyleCoord::GetCoordValue() const
  {
    NS_ASSERTION((mUnit == eStyleUnit_Coord), "not a coord value");

Actually, now that I look at the spec:
https://html.spec.whatwg.org/multipage/scripting.html#the-canvas-element
I see that width/height are unsigned so if we implement that we can
remove these checks and assert that they are non-negative in
GetCanvasSize() and/or HTMLCanvasElement::GetWidthHeight().

>+IntrinsicSizeFromCanvasSize(const nsIntSize& aCanvasSizeInPx)

Same typos in the comment for this function.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #4)
> IntrinsicSize::width is a nsStyleCoord and its default ctor initializes
> mUnit to eStyleUnit_Null

Except, IntrinsicSize's own no-arg constructor explicitly initializes both of its members to eStyleUnit_None:
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h?rev=66f1f54e19bc#361
 
> so if anyone does .width.GetCoordValue() on
> the result returned here it will assert if the aCanvasSizeInPx.width
> was negative:

No one should be doing that anyway, because they should be checking the unit before they call GetCoordValue().

> Actually, now that I look at the spec:
> https://html.spec.whatwg.org/multipage/scripting.html#the-canvas-element
> I see that width/height are unsigned so if we implement that we can
> remove these checks and assert that they are non-negative in
> GetCanvasSize() and/or HTMLCanvasElement::GetWidthHeight().

Do you mind if I punt on that to a followup? I'm wanting to keep this pretty limited in scope, as a helper for bug 1103184.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> No one should be doing that anyway, because they should be checking the unit
> before they call GetCoordValue().

(Oh, sorry -- ignore this part. I thought the bit I was responding to was a continuation of the "unit might be null" train of thought, but it wasn't. :))

Comment 7

[Daniel Holbert [:dholbert]]

(Actually, yeah -- I'll go ahead & add the checks you suggested.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Fixed comment typos & added assertions to verify that HTMLCanvasFrame gives us a non-negative size (and changed code to remove "> 0" checks).

Note that an intrinsic size of 0 is allowable; at least, in nsImageFrame, we explicitly set the intrinsic size (cached in a member-var) to be 0-by-0 in several error-handling places.  So, I'm not filtering 0 values out here anymore. (though my previous patch did, with "> 0" checks)

Comment 9

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Except, IntrinsicSize's own no-arg constructor explicitly initializes both
> of its members to eStyleUnit_None:

Ah, I missed those.  (I kind of wonder why we do that though...)

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8529132 [details] [diff] [review]
fix v2

This looks good.  Thanks!

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #9) 
> Ah, I missed those.  (I kind of wonder why we do that though...)

(I think because IntrinsicSize's members are supposed to have only one of two units: _None or _Coord. If we initialized them to have a third unit by default, it'd be a little broken. And _None (for "no intrinsic size") is a reasonable default, until an explicit size has been set.)

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the review!

Comment 13

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4772b662ccef

(Before landing, I added #includes for mozilla/Assertions.h to both files where I added MOZ_ASSERTs, to make sure they're recognized & don't implicitly depend on some other #include-chain / unified-build configuration.)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4772b662ccef