Closed Bug 1219985 Opened 9 years ago Closed 6 years ago

canvas 2d context {alpha: false} attribute is reset when canvas size changed

Categories

(Core :: Graphics: Canvas2D, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: yury, Assigned: mstange)

References

Details

(Keywords: testcase, Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

Attached file Test case (obsolete) —
If the canvas's size changed (width and/or height property), the context created with getContext('2d', {alpha: false}); becomes transparent, e.g.

var ctx = canvas.getContext('2d', {alpha: false});
canvas.width = 200;
// ctx is transparent now

It's expected (?) that context stays transparent.
Attachment #8680945 - Attachment is obsolete: true
(In reply to Yury Delendik (:yury) from comment #1)
> Created attachment 8680958 [details]
> Smaller test case (shall be green rect on black)

Is green-on-black the expected result or the bug result? I'm seeing green-on-red.
Flags: needinfo?(ydelendik)
Keywords: testcase
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #2)
> (In reply to Yury Delendik (:yury) from comment #1)
> > Created attachment 8680958 [details]
> > Smaller test case (shall be green rect on black)
> 

> Is green-on-black the expected result or the bug result? I'm seeing
> green-on-red.

Expected result green-on-black. Though I cannot find correct behavior in the spec that changing canvas size shall reset its alpha property.


(Sorry, "It's expected (?) that context stays transparent." shall be corrected to "It's expected (?) that context stays opaque.")

I checked Chrome's behavior and it paints green-on-white. I guess there is another interpretation of the `{alpha: true}`.

So two points at https://html.spec.whatwg.org/multipage/scripting.html we might find interesting:

"When the user agent is to set bitmap dimensions to width and height, it must run the following steps:
  ...
  3. Resize the output bitmap to the new width and height and clear it to fully transparent black.
  ..."

and

"... When a CanvasRenderingContext2D object has its alpha flag set to false, then its alpha channel must be fixed to 1.0 (fully opaque) for all pixels, and attempts to change the alpha component of any pixel must be silently ignored."

So it's hard to tell what takes a precedence here.
Flags: needinfo?(ydelendik)
Thanks Yury, I'm not sure what the correct behaviour is here. I leave that up to the GFX development team.
Whiteboard: [gfx-noted]
The expected result is green on black. The spec is clear about that. Resizing a canvas should not reset the alpha property, and any wording that refers to "transparent black" is practically "opaque black" when referring to a cavnas context with "alpha=false".

The bug is due to a clash with the deprecated moz-opaque HTML attribute, which was standardized as the "alpha" option.

In dom/canvas/CanvasRenderingContextHelper.cpp's UpdateContext, this is called:

  currentContext->SetIsOpaque(GetOpaqueAttr());

It sets the opaqueness based on the existence of the moz-opaque HTML attribute, regardless of the context's current alpha setting.
After this value is set, SetContextOptions() is called which should re-set the alpha option, but that doesn't happen after a resize, as it's called from HTMLCanvasElement::AfterMaybeChangeAttr, rather than GetContext(), so it has no options.

One solution is to finally remove moz-opaque.
Another is to only consult moz-opaque when UpdateContext is called via GetContext. But this will break code that dynamically removes the moz-opaque attribute from elements and expects the canvas to apply the changes on resize, without calling getContext().
Flags: webcompat?
Flags: needinfo?(mstange)
The best solution that keeps the current behavior is to ignore moz-opaque if alpha is set to false, but that would require more code changes (specifically to support a deprecated feature.)
(In reply to Ori Avtalion (salty-horse) from comment #5)
> The expected result is green on black. The spec is clear about that.
> Resizing a canvas should not reset the alpha property, and any wording that
> refers to "transparent black" is practically "opaque black" when referring
> to a canvas context with "alpha=false".

I don't know that the spec is clear, it is more silent on the issue, but, yes, I agree that all that is in the spec would point to this conclusion.  That the alpha property survives intact after the resize, as the intent doesn't seem to be to count it in the drawing state.  In other words, if alpha was to be reset when resize happens, it would suggest that it is part of the draw state and that save/restore should deal with the alpha as well.

I'd still rather have it explicitly called out in the spec.  Rik, thoughts?
Flags: needinfo?(cabanier)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Thanks for the investigation, Ori.

(In reply to Ori Avtalion (salty-horse) from comment #6)
> The best solution that keeps the current behavior is to ignore moz-opaque if
> alpha is set to false, but that would require more code changes
> (specifically to support a deprecated feature.)

This is the approach that I've implemented.

I would like us to remove support for the moz-opaque attribute, but we should do that separately, going the regular deprecation route: Send "intent to remove / unship" emails, maybe add console warnings, update docs, etc.
Comment on attachment 8969778 [details]
Bug 1219985 - The canvas rendering context 2d should be opaque if either the moz-opaque attribute is set or if it has been initialized with alpha:false.

https://reviewboard.mozilla.org/r/238602/#review244810

::: dom/canvas/CanvasRenderingContext2D.h:770
(Diff revision 1)
>  
>    // This is true when the canvas is valid, but of zero size, this requires
>    // specific behavior on some operations.
>    bool mZero;
>  
> +  bool mOpaqueAttrValue;

This needs a comment about why we have both.
Attachment #8969778 - Flags: review?(jmuizelaar) → review+
Removing needinfo?(cabanier).
I've set the webcompat? flag, if anyone wants to make a decision on it.
Flags: needinfo?(cabanier)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/1292058fb724
The canvas rendering context 2d should be opaque if either the moz-opaque attribute is set or if it has been initialized with alpha:false. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/1292058fb724
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.