Closed Bug 1304011 Opened 9 years ago Closed 9 years ago

css-mask works not correctly on a inline element

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

Attachments

(7 files)

Bug 1304011 - Part 1. Do not merge nsDisplayMask with css mask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 3. Add clip function in SetupContextMatrix, and give a frame to this function as the source of offset computing.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 5. Test case.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 7. Fix several coding convention violations.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 2. typedef nsSVGIntegrationUtils::PaintFramesParams to shorten code length.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 4. For css-mask, compute frame offset by the current frame.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1304011 - Part 6. Handle nullptr returning from nsSVGClipPathFrame::GetClipMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
as title.
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.
Attachment #8792870 - Flags: review?(mstange)
Attachment #8792871 - Flags: review?(mstange)
Attachment #8792872 - Flags: review?(mstange)
Attachment #8792873 - Flags: review?(mstange)
(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 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 on attachment 8792872 [details] Bug 1304011 - Part 5. Test case. https://reviewboard.mozilla.org/r/79756/#review78486
Attachment #8792872 - Flags: review?(mstange) → 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 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?
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 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 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 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 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 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 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+
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 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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: