Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested currentcolor resolution for canvas #28505

Conversation

weinig
Copy link
Contributor

@weinig weinig commented May 13, 2024

b6495ee

Support nested currentcolor resolution for canvas
https://bugs.webkit.org/show_bug.cgi?id=274120

Reviewed by Darin Adler.

Support for 'currentcolor' in canvas was previously hardcoded
to look for the explicit string, which made any nested usage,
such as in `color-mix()` broken.

To resolve this, we remove all special casing of `currentcolor`
in the canvas code (simplifying CanvasStyle considerably) and
use the CSSUnresolvedColorResolutionContext and its delegate
to resolve it at any nesting level.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/element/fill-and-stroke-styles/2d.fillStyle.colormix.currentcolor-expected.txt:
    - Update results now that we pass.

* Source/WebCore/css/color/CSSUnresolvedColorKeyword.cpp:
(WebCore::createColor):
    - Call the context's getter functions to allow lazy
      lookup of the color values.

* Source/WebCore/css/color/CSSUnresolvedColorResolutionContext.cpp:
* Source/WebCore/css/color/CSSUnresolvedColorResolutionContext.h:
    - Re-work the resolution context to contain a delegate that allows
      the client to only compute color values if they are needed.
      The client is still able to use the context without a delegate,
      and explicitly set color values before hand if they want to.
      The keyword lookup code now calls the functions on the context,
      which check for the memoized value and then call the delegate
      if available and necessary.

* Source/WebCore/html/canvas/CanvasGradient.idl:
* Source/WebCore/html/canvas/CanvasGradient.cpp:
(WebCore::CanvasGradient::addColorStop):
    - Remove special casing of `currentcolor`, instead relying on
      the elementless `parseColor()` function doing the right thing
      by specifying `Color::black` for the resolved value. This also
      now more closely follows the spec text.
    - Update IDL to pass in a ScriptExecutionContext, which is now
      needed by the elementless `parseColor()` function.

* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::setStrokeStyle):
(WebCore::CanvasRenderingContext2DBase::setFillStyle):
    - With CanvasStyle no longer having its own invalid state,
      setStrokeStyle/setFillStyle get an additional overload
      with an std::optional<CanvasStyle>, which takes the place
      of the invalid state.
    - We also remove all the special casing of `currentcolor`
      that is no longer needed.

(WebCore::CanvasRenderingContext2DBase::setShadowColor):
(WebCore::CanvasRenderingContext2DBase::setShadow):
    - We can call the standard `parseColor()` now, as it
      supports `currentcolor`.

(WebCore::toStyleVariant):
    - Use the new `CanvasStyle.visit(...)` function rather
      than checking each type one at a time.

* Source/WebCore/html/canvas/CanvasStyle.h:
    - Removes the Invalid and CurrentColor states. CurrentColor is
      handled by the Color state now. Invalid was only used briefly
      by rendering context and has been replaced by a std::optional
      result from the create functions. By removing these states,
      the switchOn statements can now have all their states handled
      without ASSERT_NOT_REACHED(). The RefPtrs have also been
      replaced by Refs, as these are known non-null.

(WebCore::CanvasStyle::visit): Added.
    - Added a helper to switch over the different internal states,
      eagerly converting to a String for color, as that is what is
      expected by the canvas rendering context code.

(WebCore::CanvasStyle::color):
    - Add check that Color is the alternative held by the variant.
      Previously, this function was only called when all the other
      types had been ruled out, but could easily have caused a crash
      if misused.

(WebCore::CanvasStyle::isCurrentColor const): Deleted.
(WebCore::CanvasStyle::overrideAlpha const): Deleted.
(WebCore::CanvasStyle::CanvasStyle): Deleted.
(WebCore::CanvasStyle::srgbaColor const): Deleted.
    - Remove a bunch of unused functions.

* Source/WebCore/html/canvas/CanvasStyle.cpp:
(WebCore::CanvasStyleColorResolutionDelegate::CanvasStyleColorResolutionDelegate):
    - Implement a simple delegate for color resolution, moving the
      existing code for resolving `currentcolor` from below.

(WebCore::parseColor):
    - Bottleneck color parsing in two parse functions, one taking
      a CanvasBase, one a ScriptExecutionContext. The one taking
      a CanvasBase further refines itself based on whether the
      CanvasBase is a canvas element, in which case, it creates
      a resolution delegate for resolving `currentcolor` based
      on the element style.

      All other canvases are treated as elementless, getting a
      hardcoded `Color::black` for `currentcolor` and utilizing
      the execution context to determine if system colors are
      supported.

(WebCore::CanvasStyle::createFromString):
(WebCore::CanvasStyle::createFromStringWithOverrideAlpha):
    - Remove special casing for `currentcolor`, calling through to the
      shared `parseColor()` function.

(WebCore::CanvasStyle::applyStrokeColor const):
(WebCore::CanvasStyle::applyFillColor const):
    - Update switchOns for Ref use and removed states.

(WebCore::isCurrentColorString): Deleted.
(WebCore::currentColor): Deleted.
(WebCore::parseColorOrCurrentColor): Deleted.
    - Remove now unused functions.

Canonical link: https://commits.webkit.org/279017@main

695d39c

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  watch
  πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@weinig weinig requested review from cdumez and rniwa as code owners May 13, 2024 22:17
@weinig weinig self-assigned this May 13, 2024
@weinig weinig added the Canvas Bugs related to the canvas element. label May 13, 2024
Source/WebCore/html/canvas/CanvasStyle.h Outdated Show resolved Hide resolved
@weinig
Copy link
Contributor Author

weinig commented May 13, 2024

This fixes support resolving nested currentcolor, but it opened at least one additional question, should light-dark() be supported?

Adding support would be really trivial, we just need set the appearance property on the CSSUnresolvedColorResolutionContext and it will just work, I am however not all that familiar with the feature, so don't know if there are any pitfalls. It might be as easy as calling Document.useDarkAppearance(nullptr), but then again, perhaps we would want to resolve a RenderStyle and pass it in? We would also probably want to make it work for OffscreenCanvas, so that would mean some way to accessing the theme from a worker (more pushing things down from Document to ScriptExecutionContext).

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 13, 2024
@weinig weinig removed the merging-blocked Applied to prevent a change from being merged label May 13, 2024
@weinig weinig force-pushed the eng/Support-nested-currentcolor-resolution-for-canvas branch from 8d77c36 to 523805c Compare May 13, 2024 23:06
@weinig weinig force-pushed the eng/Support-nested-currentcolor-resolution-for-canvas branch from 523805c to 9b41de2 Compare May 13, 2024 23:10
@weinig weinig added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label May 14, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 14, 2024
@annevk
Copy link
Contributor

annevk commented May 14, 2024

I don't think we should propagate information to a worker it might not otherwise have access to. That would have to be standardized in some fashion first.

It does make sense that light-dark() is parsed, but for now we probably have to assume that a worker is always light or always dark.

@nt1m
Copy link
Member

nt1m commented May 16, 2024

This needs rebaselining:
imported/w3c/web-platform-tests/html/canvas/offscreen/fill-and-stroke-styles/2d.fillStyle.parse.system.html

@weinig weinig removed the merging-blocked Applied to prevent a change from being merged label May 20, 2024
@weinig weinig force-pushed the eng/Support-nested-currentcolor-resolution-for-canvas branch from 9b41de2 to 695d39c Compare May 20, 2024 16:53
@cdumez cdumez added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels May 20, 2024
@JonWBedard JonWBedard added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels May 20, 2024
https://bugs.webkit.org/show_bug.cgi?id=274120

Reviewed by Darin Adler.

Support for 'currentcolor' in canvas was previously hardcoded
to look for the explicit string, which made any nested usage,
such as in `color-mix()` broken.

To resolve this, we remove all special casing of `currentcolor`
in the canvas code (simplifying CanvasStyle considerably) and
use the CSSUnresolvedColorResolutionContext and its delegate
to resolve it at any nesting level.

* LayoutTests/imported/w3c/web-platform-tests/html/canvas/element/fill-and-stroke-styles/2d.fillStyle.colormix.currentcolor-expected.txt:
    - Update results now that we pass.

* Source/WebCore/css/color/CSSUnresolvedColorKeyword.cpp:
(WebCore::createColor):
    - Call the context's getter functions to allow lazy
      lookup of the color values.

* Source/WebCore/css/color/CSSUnresolvedColorResolutionContext.cpp:
* Source/WebCore/css/color/CSSUnresolvedColorResolutionContext.h:
    - Re-work the resolution context to contain a delegate that allows
      the client to only compute color values if they are needed.
      The client is still able to use the context without a delegate,
      and explicitly set color values before hand if they want to.
      The keyword lookup code now calls the functions on the context,
      which check for the memoized value and then call the delegate
      if available and necessary.

* Source/WebCore/html/canvas/CanvasGradient.idl:
* Source/WebCore/html/canvas/CanvasGradient.cpp:
(WebCore::CanvasGradient::addColorStop):
    - Remove special casing of `currentcolor`, instead relying on
      the elementless `parseColor()` function doing the right thing
      by specifying `Color::black` for the resolved value. This also
      now more closely follows the spec text.
    - Update IDL to pass in a ScriptExecutionContext, which is now
      needed by the elementless `parseColor()` function.

* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::setStrokeStyle):
(WebCore::CanvasRenderingContext2DBase::setFillStyle):
    - With CanvasStyle no longer having its own invalid state,
      setStrokeStyle/setFillStyle get an additional overload
      with an std::optional<CanvasStyle>, which takes the place
      of the invalid state.
    - We also remove all the special casing of `currentcolor`
      that is no longer needed.

(WebCore::CanvasRenderingContext2DBase::setShadowColor):
(WebCore::CanvasRenderingContext2DBase::setShadow):
    - We can call the standard `parseColor()` now, as it
      supports `currentcolor`.

(WebCore::toStyleVariant):
    - Use the new `CanvasStyle.visit(...)` function rather
      than checking each type one at a time.

* Source/WebCore/html/canvas/CanvasStyle.h:
    - Removes the Invalid and CurrentColor states. CurrentColor is
      handled by the Color state now. Invalid was only used briefly
      by rendering context and has been replaced by a std::optional
      result from the create functions. By removing these states,
      the switchOn statements can now have all their states handled
      without ASSERT_NOT_REACHED(). The RefPtrs have also been
      replaced by Refs, as these are known non-null.

(WebCore::CanvasStyle::visit): Added.
    - Added a helper to switch over the different internal states,
      eagerly converting to a String for color, as that is what is
      expected by the canvas rendering context code.

(WebCore::CanvasStyle::color):
    - Add check that Color is the alternative held by the variant.
      Previously, this function was only called when all the other
      types had been ruled out, but could easily have caused a crash
      if misused.

(WebCore::CanvasStyle::isCurrentColor const): Deleted.
(WebCore::CanvasStyle::overrideAlpha const): Deleted.
(WebCore::CanvasStyle::CanvasStyle): Deleted.
(WebCore::CanvasStyle::srgbaColor const): Deleted.
    - Remove a bunch of unused functions.

* Source/WebCore/html/canvas/CanvasStyle.cpp:
(WebCore::CanvasStyleColorResolutionDelegate::CanvasStyleColorResolutionDelegate):
    - Implement a simple delegate for color resolution, moving the
      existing code for resolving `currentcolor` from below.

(WebCore::parseColor):
    - Bottleneck color parsing in two parse functions, one taking
      a CanvasBase, one a ScriptExecutionContext. The one taking
      a CanvasBase further refines itself based on whether the
      CanvasBase is a canvas element, in which case, it creates
      a resolution delegate for resolving `currentcolor` based
      on the element style.

      All other canvases are treated as elementless, getting a
      hardcoded `Color::black` for `currentcolor` and utilizing
      the execution context to determine if system colors are
      supported.

(WebCore::CanvasStyle::createFromString):
(WebCore::CanvasStyle::createFromStringWithOverrideAlpha):
    - Remove special casing for `currentcolor`, calling through to the
      shared `parseColor()` function.

(WebCore::CanvasStyle::applyStrokeColor const):
(WebCore::CanvasStyle::applyFillColor const):
    - Update switchOns for Ref use and removed states.

(WebCore::isCurrentColorString): Deleted.
(WebCore::currentColor): Deleted.
(WebCore::parseColorOrCurrentColor): Deleted.
    - Remove now unused functions.

Canonical link: https://commits.webkit.org/279017@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Support-nested-currentcolor-resolution-for-canvas branch from 695d39c to b6495ee Compare May 20, 2024 22:08
@webkit-commit-queue
Copy link
Collaborator

Committed 279017@main (b6495ee): https://commits.webkit.org/279017@main

Reviewed commits have been landed. Closing PR #28505 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit b6495ee into WebKit:main May 20, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Canvas Bugs related to the canvas element.
Projects
None yet
9 participants