Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Jul 13, 2021

1- Marks subtree of colorfilter (css filter attribute with svg filter) to prevent canvas use due to webkit bug that skips applying filter to canvas element in subtree. Same fix as prior ShaderMask failure in webkit.
2- Moves svg element before the filter interior div since Safari sometimes doesn't apply filter if it is defined after the element that contains css attribute.
3- WebKit fails to resolve filter: url("#fcf") when svg is referenced inside shadowdom. This PR moves to a svg resource model that adds/removes filters to a root level host node to get around webkit failure.

Fixes: flutter/flutter#85733

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@ferhatb ferhatb added the platform-web Code specifically for the web engine label Jul 13, 2021
@ferhatb ferhatb requested a review from yjbanov July 13, 2021 17:25
@google-cla google-cla bot added the cla: yes label Jul 13, 2021
@ferhatb ferhatb changed the title [web] Fix webkit ColorFilter.mode [web] Fix webkit ColorFilter.mode for webkit Jul 13, 2021
final ui.ColorFilter filter;
html.Element? _filterElement;
bool containerVisible = true;
static int activeColorFilterCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a static, can we have a PrerollContext class that's passed down to the preroll method recursively? This class could have a (non-static) field activeColorFilterCount.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

++activeColorFilterCount;
super.preroll();
--activeColorFilterCount;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example with PrerollContext:

void preroll(PrerollContext context) {
  context.activeColorFilterCount++;
  super.preroll(context);
  context.activeColorFilterCount--;
}

We can later add other things to the context, such as transform and clip stack, and avoid doing parent look-ups that we do today in a few places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/// Add an element as a global resource to be referenced by CSS.
///
/// This call create a global resource host element on demand and either
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "This method creates"

html.Element? _sceneHostElement;

/// A child element of body outside the shadowroot that hosts
/// global resources such svg filters and clip paths when using webkit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to code in addResource this element hosts resources on all browsers, not only when using webkit, and it not always a child of the body element.

_resourcesHost = html.DivElement()
..style.visibility = 'hidden';
if (isWebKit) {
final html.Node bodyNode = html.document.body!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it easier for us to support multiple Flutter views in the future, can we add this element to flt-glass-pane instead of body?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Web][Safari][html]: Fix shadowDom preventing svg url resolution. Fix BlendMode.srcIn color effect is not rendered correctly in safari browser

2 participants