Closed Bug 1294171 Opened 8 years ago Closed 8 years ago

Material Design Light SVG checkbox doesn't show check with Firefox 50

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox48 --- unaffected
firefox49 - verified
firefox50 - verified
firefox51 + verified

People

(Reporter: danny, Assigned: u459114)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1294171 - Part 1. Treat unresolvable mask as no mask before support image mask.
58 bytes, text/x-review-board-request
mstange
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Bug 1294171 - Part 2. Create a reftest for data url mask.
58 bytes, text/x-review-board-request
mstange
: review+
Details 
Google has a CSS/JS package for emulating their Material Design aesthetic on the Web. Their reskin of a standard checkbox is broken in Firefox 50.

If you go to https://getmdl.io/components/index.html#lists-section  and scroll down to the checkbox demo, you can see the HTML in its native habitat. It should show a blue SVG (in a data URI) checkmark when clicked, but stays empty.

Here's a minimal testcase.
https://codepen.io/dannyobrien/pen/JKwYap

This looks like a regression -- it worked fine in my copy of Firefox 48. 

I ran mozregression on it, and it ended up at
 
changeset: 373e67c2caaddfec3d3901e038804543c3da9afb
pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=373e67c2caaddfec3d3901e038804543c3da9afb&tochange=37c7fec8a0c130033c7e8462b058ea360c6d36d9

..so it looks like it might be in the SVG rendering code, possibly interacting with it being in a DATA URI?
CJ, might this be from your fix from bug 1275450?
status-firefox48: --- → unaffected
status-firefox49: --- → affected
Flags: needinfo?(cku)
Keywords: regression
It's a duplication of bug 1293929
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(cku)
Resolution: --- → DUPLICATE
According to https://github.com/webcompat/web-bugs/issues/2992 this is not fixed, and I think we need to be concerned about shipping this regression.
Status: RESOLVED → REOPENED
status-firefox49: fix-optional → affected
status-firefox50: fix-optional → affected
status-firefox51: fixed → affected
Flags: needinfo?(cku)
Resolution: DUPLICATE → ---
[Tracking Requested - why for this release]: web compat regression
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
it's kind of dup of 1287320, but not the totally the same either.
This test site give a data url as mask(which is not supported yet before enable-mask-as-shorthand)
  url("data:image/svg+xml;base64,xxxxx")
I think the correct solution for this kind of regression is go back to original behavior(treat unresolvable masks as no-mask) when MASK_AS_SHORTHAND is undefined.
Working on the patch
Flags: needinfo?(cku)
bug 1294171(this one) - This web site five a valid mask value, but not valid for firefox since we are not support image mask yet.
bug 1287320 - This web site give a invalid svg mask in a SVG document, which is embedded in a HTML document. (fixed)
bug 1293929 - This web site give a invalid svg mask in HTML document.

The patch here can accidentally fix bug 1293929 before turning on MOZ_ENABLE_MASK_AS_SHORTHAND.
Attachment #8782252 - Flags: review?(mstange)
Attachment #8782273 - Flags: review?(mstange)
Assignee: nobody → cku
Tracking 51+ for this web compat regression.
tracking-firefox51: ? → +
Comment on attachment 8782252 [details]
Bug 1294171 - Part 1. Treat unresolvable mask as no mask before support image mask.

https://reviewboard.mozilla.org/r/72444/#review70394
Attachment #8782252 - Flags: review?(mstange) → review+
Comment on attachment 8782273 [details]
Bug 1294171 - Part 2. Create a reftest for data url mask.

https://reviewboard.mozilla.org/r/72474/#review70396
Attachment #8782273 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23fcd6d28628
Part 1. Treat unresolvable mask as no mask before support image mask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/dab9f89ca163
Part 2. Create a reftest for data url mask. r=mstange
https://hg.mozilla.org/mozilla-central/rev/23fcd6d28628
https://hg.mozilla.org/mozilla-central/rev/dab9f89ca163
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8782252 [details]
Bug 1294171 - Part 1. Treat unresolvable mask as no mask before support image mask.

Approval Request Comment
[Feature/regressing bug #]: 1275450
[User impact if declined]: A masked element become invisible if that mask is an image mask.
[Describe test coverage new/current, TreeHerder]: manually + try
[Risks and why]: low risk. Correct a regression on SVG mask rendering.
[String/UUID change made/needed]:

Please uplift (part 1) to aurora and beta
Attachment #8782252 - Flags: approval-mozilla-beta?
Attachment #8782252 - Flags: approval-mozilla-aurora?
This can still make it into the beta 6 build if we can land it by early Monday morning. 
Andrei can your team help verify the fix? You can check dbaron's link from comment 3 for more cases.
Flags: needinfo?(andrei.vaida)
Danny, want to test this from mozilla-central's test builds? >^..^< 
You can get the appropriate build for your OS from here, you don't even have to wait for tomorrow's Nightly: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=3da4d64410c002418eab4b025dfb24a6f4f57fd6
Flags: needinfo?(danny)
Yep, I can confirm that this is now working. Nice job! Thank you so much.
Flags: needinfo?(danny)
Flags: needinfo?(andrei.vaida)
Karl (or Andrei) can you test just to make sure, with Karl's test cases here, https://github.com/webcompat/web-bugs/issues/2992 ?
Flags: needinfo?(kdubost)
Flags: needinfo?(andrei.vaida)
Comment on attachment 8782252 [details]
Bug 1294171 - Part 1. Treat unresolvable mask as no mask before support image mask.

Fix for regression in 49, let's uplift this for the beta 6 build on Monday so that we don't ship this to release.
Attachment #8782252 - Flags: approval-mozilla-beta?
Attachment #8782252 - Flags: approval-mozilla-beta+
Attachment #8782252 - Flags: approval-mozilla-aurora?
Attachment #8782252 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)
> Karl (or Andrei) can you test just to make sure, with Karl's test cases
> here, https://github.com/webcompat/web-bugs/issues/2992 ?

Confirmed. This is fixed! 
Thanks a lot everyone for the quick fix.
With <3 from Web compat.
Flags: needinfo?(kdubost)
Since this is already fixed, we don't really need to track it.
tracking-firefox49: ? → -
tracking-firefox50: ? → -
I've managed to reproduce the bug using the sample URL from Comment 0,
on 50.0a2 (2016-08-10) and 51.0a1 (2016-08-08).

I can also confirm that this is verified fixed on Windows 10 x64, Mac
OS X 10.11.1 and Ubuntu 16.04 x64, using:
    - 48.0.2-build1 (20160823121617)
    - 49.0b6-build1 (20160822111414)
    - 50.0a2 (2016-08-24)
    - 51.0a1 (2016-08-24)
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Flags: needinfo?(andrei.vaida)
Depends on: 1385911
No longer depends on: 1385911
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: