Draw nothing when any mask-image refer to an unresolvable URL

RESOLVED DUPLICATE of bug 1258510

Status

()

Core
Layout
RESOLVED DUPLICATE of bug 1258510
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Hint from mstange,
https://bugzilla.mozilla.org/show_bug.cgi?id=1275450#c10

Go back to this bug after finish bug 1272859.
(Assignee)

Updated

2 years ago
Summary: Draw nothing when all mask-image refer to an unresolvable URL → Draw nothing when any mask-image refer to an unresolvable URL
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Updated

2 years ago
Attachment #8759098 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8759114 - Flags: review?(mstange)
Attachment #8759115 - Flags: review?(mstange)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment on attachment 8759114 [details]
Bug 1276834 - Part 1. Change return type of GenerateMaskSurface.

https://reviewboard.mozilla.org/r/57174/#review56580
Attachment #8759114 - Flags: review?(mstange) → review+
Comment on attachment 8759115 [details]
Bug 1276834 - Part 2. Draw nothing when any mask-image refer to an unresolvable URL

https://reviewboard.mozilla.org/r/57176/#review56582

::: layout/svg/nsSVGIntegrationUtils.cpp:525
(Diff revision 3)
>  
> +static bool
> +ShouldCreateMaskSurface(const nsIFrame* aFrame,
> +                        const nsStyleSVGReset* aSVGReset,
> +                        const nsTArray<nsSVGMaskFrame *>& maskFrames,
> +                        bool& aHasUnresovlableMaskLayer)

typo (should be aHasUnresolvableMaskLayer)

::: layout/svg/nsSVGIntegrationUtils.cpp:657
(Diff revision 3)
>  
>    gfxMatrix cssPxToDevPxMatrix = GetCSSPxToDevPxMatrix(frame);
>  
>    const nsStyleSVGReset *svgReset = firstFrame->StyleSVGReset();
>    nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames();
> -  // For a HTML doc:
> +  bool shouldCreateMaskSurface = false, hasUnresovlableMaskLayer = false;

Separate lines please. Also same typo in unresolvable.

::: layout/svg/nsSVGIntegrationUtils.cpp:658
(Diff revision 3)
>    gfxMatrix cssPxToDevPxMatrix = GetCSSPxToDevPxMatrix(frame);
>  
>    const nsStyleSVGReset *svgReset = firstFrame->StyleSVGReset();
>    nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames();
> -  // For a HTML doc:
> -  //   According to css-masking spec, always create a mask surface when we
> +  bool shouldCreateMaskSurface = false, hasUnresovlableMaskLayer = false;
> +  shouldCreateMaskSurface = ShouldCreateMaskSurface(frame, svgReset,

Actually, just declare this variable in this line. If that causes the line to exceed the line length limit, just break after the equals sign.

::: layout/svg/nsSVGIntegrationUtils.cpp:670
(Diff revision 3)
> +    gfxContextAutoSaveRestore saver(&context);
> +
> +    context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
> +    context.Paint();

How is this different from just returning?
Attachment #8759115 - Flags: review?(mstange)
(Assignee)

Comment 9

2 years ago
https://reviewboard.mozilla.org/r/57176/#review56582

> How is this different from just returning?

Then it looks like no mask effect at all, you can still see whole background image, which is not we want.
Comment hidden (spam)
Comment hidden (spam)

Updated

2 years ago
Status: NEW → ASSIGNED
Depends on: 1251161
Sorry, I still don't really understand. I could try out the patch but I'd rather ask for an explanation one more time.

Is this patch just an optimization, so that we don't create an unnecessary surface? Or does it change behavior? I'm assuming the former, otherwise I'd expect tests to be added.

And I still don't understand why these lines would have any effect:

    context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
    context.Paint();

Are we not using operator OVER here?
(Assignee)

Comment 13

2 years ago
(In reply to Markus Stange [:mstange] from comment #12)
> Sorry, I still don't really understand. I could try out the patch but I'd
> rather ask for an explanation one more time.
> 
> Is this patch just an optimization, so that we don't create an unnecessary
> surface? Or does it change behavior? I'm assuming the former, otherwise I'd
> expect tests to be added.
Yes, the former
> And I still don't understand why these lines would have any effect:
> 
>     context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
>     context.Paint();
> 
> Are we not using operator OVER here?
Yes, it need to be OVER. I should call SetOp(OVER) before Paint(). In my test, it works fine since default CompOp is OVER.
(Assignee)

Comment 14

2 years ago
And we already have a test case to verify this scenario, which is mask-image-4a.html
You still haven't answered the question. If the operator is OVER, then how is painting "nothing" on top of the existing contents different from skipping those two lines?
(Assignee)

Comment 16

2 years ago
(In reply to Markus Stange [:mstange] from comment #15)
> You still haven't answered the question. If the operator is OVER, then how
> is painting "nothing" on top of the existing contents different from
> skipping those two lines?
I read nsSVGIntegrationUtils::PaintFramesWithEffects again and know I make a mistake here.
context is empty unless we paint on it. I used to think that "context" contains content of background-image, which is wrong. I am rewriting this patch
Comment hidden (spam)
Comment hidden (spam)
Comment on attachment 8759115 [details]
Bug 1276834 - Part 2. Draw nothing when any mask-image refer to an unresolvable URL

https://reviewboard.mozilla.org/r/57176/#review60808

::: layout/svg/nsSVGIntegrationUtils.cpp:760
(Diff revision 5)
>                                          maskFrames, offsetToUserSpace,
>                                          maskTransform);
> -      if (!maskSurface) {
> -        // Entire surface is clipped out.
> +      if (!maskSurface || hasUnresolvableMaskLayer) {
> +        // Entire surface is clipped out or we have non-resolvable masks.
> +        // Check hasUnresolvableMaskLayer after calling GenerateMaskSurface
> +        // to ensure we get invalidated for loads of the mask.

How does this work? Why are we not getting invalidated for loads of the mask if we don't call GenerateMaskSurface?
Attachment #8759115 - Flags: review?(mstange)
(Assignee)

Comment 20

2 years ago
Patch in Bug 1258510 already did what we need here. 
Early return without push any mask:
https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp?q=PaintFramesWithEffects&redirect_type=direct#584
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1258510
You need to log in before you can comment on or make changes to this bug.