Closed Bug 1522422 Opened 2 years ago Closed 11 months ago

Opacity not obeyed on SVG USE elements when a Mask is utilized with Direct2D

Categories

(Core :: Graphics, defect, P2)

64 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 73+ fixed
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: jonreeves, Assigned: ktaeleman)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

  1. Create an SVG inline on a HTML page
  2. Define a Mask
  3. Create a USE element to use the Mask
  4. Set opacity to (0.5) via Attribute

Example:
https://jsfiddle.net/bg4o5y63/

Actual results:

On Windows (Firefox 64.0.2) "Opacity" attribute seems to have no affect. "Fill-Opacity" Attribute does work. MacOS does not have the same problem.

Expected results:

Opacity attribute should be obeyed and shape should appear semi transparent.

Component: Untriaged → SVG
Product: Firefox → Core

From my testing (using old versions of Portable Firefox). The last released version that this worked correctly in was 54.0.1. Version 55.0 seems to show it incorrectly.

https://sourceforge.net/projects/portableapps/files/Mozilla%20Firefox%2C%20Portable%20Ed./

Hope that helps.

(Which landed over a year and a half ago.)

Priority: -- → P3

Sean, could you please find someone to look into this for our 67 soft freeze (Mar 11)?

Flags: needinfo?(svoisen)

Looks like a long-standing regression based on comment 2 and comment 3.

Will keep an eye on it but believe this will need to be handled by someone on graphics given the regressing bug.

@jrmuizel: Does this seem correct?

Component: SVG → Graphics
Flags: needinfo?(svoisen) → needinfo?(jmuizelaar)

Yeah. It looks like this only happens with Direct2D.

Flags: needinfo?(jmuizelaar)
Summary: Opacity not obeyed on SVG USE elements when a Mask is utilized → Opacity not obeyed on SVG USE elements when a Mask is utilized with Direct2D

I saw the cause for this when reading the code a little while ago. It shouldn't be too hard to fix. We're basically assuming the opacity was applied in one case when it actually hasn't been applied

Jeff, should we expect a patch for this one for 69?

Flags: needinfo?(jmuizelaar)

Last poke for this bug as I drop it from weekly regression triage.

Flags: needinfo?(jbonisteel)

I believe this is caused by DrawTargetD2D1::IntoLuminanceSource not handling the opacity argument.

https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/gfx/2d/DrawTargetD2D1.cpp#141

We can either bail on opacity != 1.0 or handle it properly.

Flags: needinfo?(jmuizelaar)

Kris, can you take a look at this when you get a chance.

Assignee: nobody → ktaeleman
Attachment #9089188 - Attachment is obsolete: true
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52cff0e39802
Fixing opacity issue on SVG USE elements in D2D. r=jrmuizel
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
No longer blocks: 1359527
Flags: in-testsuite+
Regressed by: 1359527

Is this something we should consider uplifting to Beta or can it ride Fx71 to release?

Flags: needinfo?(ktaeleman)

I think this is a bit of an edge case (svg + use + mask), so in my opinion it could ride to Fx71.

Flags: needinfo?(ktaeleman)

Given the complaint filed on bug 1601898, we might as well ask to uplift this to esr.

Comment on attachment 9089218 [details]
Bug 1522422 - Fixing opacity issue on SVG USE elements in D2D.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Certain web-content is broken without this fix. See bug 1601898.
  • User impact if declined: Certain web-content is broken without this fix. See bug 1601898.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This is pretty low risk as it has shipped in 71 and 72 without issue. It is, however, a little involved and does have some additional uses of D2D so there is some risk.
  • String or UUID changes made by this patch:
Attachment #9089218 - Flags: approval-mozilla-esr68?

Comment on attachment 9089218 [details]
Bug 1522422 - Fixing opacity issue on SVG USE elements in D2D.

This will need a rebased patch if we want it on ESR68.

Flags: needinfo?(jmuizelaar)
Attachment #9089218 - Flags: approval-mozilla-esr68?

Kris can you rebase on ESR?

Flags: needinfo?(jmuizelaar) → needinfo?(ktaeleman)

Setting back to you Jeff as I won't have time to do this before thursday

Flags: needinfo?(ktaeleman) → needinfo?(jmuizelaar)
Attached patch Patch for ESR68Splinter Review

This also pulls in the fix from bug 1556470

Flags: needinfo?(jmuizelaar)

Comment on attachment 9122444 [details] [diff] [review]
Patch for ESR68

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Certain web-content is broken without this fix. See bug 1601898.
  • User impact if declined: Certain web-content is broken without this fix. See bug 1601898.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This is pretty low risk as it has shipped in 71 and 72 without issue. It is, however, a little involved and does have some additional uses of D2D so there is some risk.
  • String or UUID changes made by this patch:
Attachment #9122444 - Flags: approval-mozilla-esr68?
Attachment #9089218 - Flags: approval-mozilla-esr68?
Comment on attachment 9122444 [details] [diff] [review]
Patch for ESR68

Fixes a web compat issue being reported in the wild. Baked on release for awhile now without known issues. Approved for 68.5esr.
Attachment #9122444 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.