css-mask works not correctly on a inline element

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(7 attachments)

(Assignee)

Description

a year ago
as title.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
The test case in Part 4 explain what goes wrong when we apply a mask onto a inline element(span/a etc..) which is broken into several lines.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8792870 - Flags: review?(mstange)
Attachment #8792871 - Flags: review?(mstange)
Attachment #8792872 - Flags: review?(mstange)
Attachment #8792873 - Flags: review?(mstange)
(Assignee)

Comment 7

a year ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #5)
> The test case in Part 4 explain what goes wrong when we apply a mask onto a
> inline element(span/a etc..) which is broken into several lines.

I reorder the patches, it should be Part 3 now.

Comment 8

a year ago
mozreview-review
Comment on attachment 8792872 [details]
Bug 1304011 - Part 5. Test case.

https://reviewboard.mozilla.org/r/79756/#review78484

::: layout/reftests/w3c-css/submitted/masking/mask-image-6-ref.html:10
(Diff revision 1)
> +    <title>CSS Masking: mask on inline element</title>
> +    <link rel="author" title="CJ Ku" href="mailto:cku@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <style type="text/css">
> +      div {
> +        margin:0px;

divs don't have any margin by default

::: layout/reftests/w3c-css/submitted/masking/mask-image-6.html:13
(Diff revision 1)
> +    <link rel="help" href="https://www.w3.org/TR/css-masking-1/#the-mask-image">
> +    <link rel="match" href="mask-image-6-ref.html">
> +    <meta name="assert" content="Test checks whether mask on inline elemnt works correctly or not.">
> +    <style type="text/css">
> +      div {
> +        margin:0px;

divs don't have any margin by default

::: layout/reftests/w3c-css/submitted/masking/mask-image-6.html:21
(Diff revision 1)
> +      }
> +      span {
> +        font-size: 100px;
> +        line-height: 100px;
> +        mask-image: url(support/transparent-100x50-blue-100x50.png);
> +        -webkit-mask-image: url(support/transparent-100x50-blue-100x50.png);

Is it a good idea to put -webkit- prefixes into web platform tests?

Comment 9

a year ago
mozreview-review
Comment on attachment 8792872 [details]
Bug 1304011 - Part 5. Test case.

https://reviewboard.mozilla.org/r/79756/#review78486
Attachment #8792872 - Flags: review?(mstange) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8792870 [details]
Bug 1304011 - Part 1. Do not merge nsDisplayMask with css mask.

https://reviewboard.mozilla.org/r/79752/#review78488

::: layout/base/nsDisplayList.cpp:6842
(Diff revision 1)
> +  // Do not merge if mFrame has mask. Continuous frames should apply mask
> +  // independent(just like nsDisplayBackgroundImage).

s/Continuous/Continuation/
s/independent/independently /
Attachment #8792870 - Flags: review?(mstange) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8792871 [details]
Bug 1304011 - Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing.

https://reviewboard.mozilla.org/r/79754/#review78494

::: layout/svg/nsSVGIntegrationUtils.cpp
(Diff revision 1)
> -  nsIFrame* firstFrame =
> -    nsLayoutUtils::FirstContinuationOrIBSplitSibling(frame);

Won't this change behavior for SVG clip-path / SVG mask? Or for filters?
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8792871 [details]
Bug 1304011 - Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing.

https://reviewboard.mozilla.org/r/79754/#review78494

> Won't this change behavior for SVG clip-path / SVG mask? Or for filters?

Arr...it's really a big problem. I will rework this patch
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

a year ago
mozreview-review
Comment on attachment 8793669 [details]
Bug 1304011 - Part 2. typedef nsSVGIntegrationUtils::PaintFramesParams to shorten code length.

https://reviewboard.mozilla.org/r/80378/#review79228
Attachment #8793669 - Flags: review?(mstange) → review+

Comment 39

a year ago
mozreview-review
Comment on attachment 8792871 [details]
Bug 1304011 - Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing.

https://reviewboard.mozilla.org/r/79754/#review79230

::: layout/svg/nsSVGIntegrationUtils.cpp:655
(Diff revision 4)
>  static void
> -SetupContextMatrix(const PaintFramesParams& aParams,
> -                   nsPoint& aOffsetToBoundingBox,
> -                   nsPoint& aToUserSpace,
> +SetupContextMatrix(nsIFrame* aFrame, const PaintFramesParams& aParams,
> +                   nsPoint& aOffsetToBoundingBox, nsPoint& aOffsetToUserSpace,
> +                   bool aClipCtx)

Can you add a comment above this function which documents the parameters?

::: layout/svg/nsSVGIntegrationUtils.cpp:698
(Diff revision 4)
>  
>    gfxPoint devPixelOffsetToUserSpace =
>      nsLayoutUtils::PointToGfxPoint(aOffsetToUserSpace,
> -                                   frame->PresContext()->AppUnitsPerDevPixel());
> -  aParams.ctx.SetMatrix(aParams.ctx.CurrentMatrix().Translate(devPixelOffsetToUserSpace));
> +                                   aFrame->PresContext()->AppUnitsPerDevPixel());
> +  gfxContext& context = aParams.ctx;
> +  context.SetMatrix(aParams.ctx.CurrentMatrix().Translate(devPixelOffsetToUserSpace));

The other "aParams.ctx" can be a "context" as now well.

::: layout/svg/nsSVGIntegrationUtils.cpp:846
(Diff revision 4)
>        : CreateBlendTarget(aParams, targetOffset);
>    if (!target) {
> +    context.Restore();
>      return DrawResult::TEMPORARY_ERROR;
>    }
> +  context.Restore();

Why not call Restore() just before the if?

It's a bit strange to have CreateBlendTarget rely on there being a tight clip on the context. You should at least add a comment that explains this, otherwise the Save+Restore calls seem a little arbitrary.

::: layout/svg/nsSVGIntegrationUtils.cpp:854
(Diff revision 4)
>                               shouldGenerateMaskLayer);
>  
>    /* Check if we need to do additional operations on this child's
>     * rendering, which necessitates rendering into another surface. */
>    if (shouldGenerateMask) {
>      context.Save();

You could use gfxContextAutoSaveRestore here.

::: layout/svg/nsSVGIntegrationUtils.cpp:894
(Diff revision 4)
>      context.Save();
> +    SetupContextMatrix(firstFrame, aParams, offsetToBoundingBox,
> +                       offsetToUserSpace, false);

Since these lines are repeated, can you do them first in an if (shouldApplyClipPath || shouldApplyBasicShape)? That would make it clearer that the Save is grouped with the restore below.

Comment 40

a year ago
mozreview-review
Comment on attachment 8792871 [details]
Bug 1304011 - Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing.

https://reviewboard.mozilla.org/r/79754/#review79236
Attachment #8792871 - Flags: review?(mstange) → review+

Comment 41

a year ago
mozreview-review
Comment on attachment 8793670 [details]
Bug 1304011 - Part 4. For css-mask, compute frame offset by the current frame.

https://reviewboard.mozilla.org/r/80380/#review79252

::: layout/svg/nsSVGIntegrationUtils.cpp:880
(Diff revision 1)
> +      SetupContextMatrix(firstFrame, aParams, offsetToBoundingBox,
> +                         offsetToUserSpace, true);

Not sure I completely understand this. Should this be inside the if (shouldGenerateMaskLayer)? Otherwise it seems like you stack up two transforms in the case where you have shouldGenerateMask && shouldGenerateClipMaskLayer && !shouldGenerateMaskLayer.

Comment 42

a year ago
mozreview-review
Comment on attachment 8793671 [details]
Bug 1304011 - Part 6. Handle nullptr returning from nsSVGClipPathFrame::GetClipMask.

https://reviewboard.mozilla.org/r/80382/#review79276
Attachment #8793671 - Flags: review?(mstange) → review+

Comment 43

a year ago
mozreview-review
Comment on attachment 8792873 [details]
Bug 1304011 - Part 7. Fix several coding convention violations.

https://reviewboard.mozilla.org/r/79758/#review79278
Attachment #8792873 - Flags: review?(mstange) → review+
(Assignee)

Comment 44

a year ago
mozreview-review-reply
Comment on attachment 8793670 [details]
Bug 1304011 - Part 4. For css-mask, compute frame offset by the current frame.

https://reviewboard.mozilla.org/r/80380/#review79252

> Not sure I completely understand this. Should this be inside the if (shouldGenerateMaskLayer)? Otherwise it seems like you stack up two transforms in the case where you have shouldGenerateMask && shouldGenerateClipMaskLayer && !shouldGenerateMaskLayer.

Stacking up two transform is possible under the condition you mentioned, will fix it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

a year ago
mozreview-review
Comment on attachment 8793670 [details]
Bug 1304011 - Part 4. For css-mask, compute frame offset by the current frame.

https://reviewboard.mozilla.org/r/80380/#review79536
Attachment #8793670 - Flags: review?(mstange) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 71

a year ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0de5ca60147c
Part 1. Do not merge nsDisplayMask with css mask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a0eb7d02a895
Part 2. typedef nsSVGIntegrationUtils::PaintFramesParams to shorten code length. r=mstange
https://hg.mozilla.org/integration/autoland/rev/bca355313274
Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f64615811ca8
Part 4. For css-mask, compute frame offset by the current frame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f57a57175a4b
Part 5. Test case. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8d425964393b
Part 6. Handle nullptr returning from nsSVGClipPathFrame::GetClipMask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/5011b0476532
Part 7. Fix several coding convention violations. r=mstange
You need to log in before you can comment on or make changes to this bug.