Closed
Bug 1269174
Opened 8 years ago
Closed 8 years ago
SVG icon missing or too small, at wix.com
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox46 | --- | unaffected |
firefox47 | + | fixed |
firefox48 | --- | fixed |
firefox49 | --- | verified |
People
(Reporter: dholbert, Assigned: dholbert)
References
()
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
15.95 KB,
image/png
|
Details | |
71.48 KB,
image/png
|
Details | |
465 bytes,
text/html
|
Details | |
6.20 KB,
patch
|
longsonr
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Ran into a bug on a private URL for a private Wix page that my fiance, Jess, is setting up. (I'll try to create a public version of the page soon; can't post the URL of the actual site since it contains details for a private event. I'll set needinfo=me so I don't forget.) STR: 1. Load a Wix page which reproduces the bug. 2. Scroll down and look at the icons (which are inline SVG content, and which are provided as stock icons by Wix) ACTUAL RESULTS: The icons are too small. EXPECTED RESULTS: The icons should be consistently large, like they are in Chrome and in older Firefox versions. I can't reproduce in a saved version of the page, unfortunately -- it only reproduces on the live site. (Hence, no reduced testcase yet.) Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3329b93589df2613f87fb475eace4b30c1cd0af4&tochange=4894d75bf376b60766430de3b76106023bab4ac3 --> Regression from bug 1250773
Assignee | ||
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: Rendering regression introduced in Firefox 47
status-firefox46:
--- → unaffected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
tracking-firefox47:
--- → ?
Assignee | ||
Updated•8 years ago
|
Summary: SVG icon appears too small at wix.com → SVG icon missing or too small, at wix.com
Assignee | ||
Comment 2•8 years ago
|
||
Here's a screenshot, comparing Chrome (top) to Firefox (bottom). The left icon is completely missing in Firefox, and the right one is just extremely small.
Assignee | ||
Comment 3•8 years ago
|
||
Given the nature of the regressing bug, I'm guessing Wix might be doing something silly with a negative viewBox. (I don't see any viewBox at all on the final page's DOM, but maybe Wix temporarily sets a negative viewBox and then later clears it, or something.) anyway, needinfo=me to come up with a public URL that reproduces this (and perhaps to debug as well)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 4•8 years ago
|
||
OK, here's a testcase: http://dholbert5.wix.com/bug-1269174-test This testcase has two affected SVG icons: a bow-tie and a clock. There's also a PNG owl clip-art, as a reference for the correct size. (The bow-tie and clock should be slightly larger than the owl.) Comparing Nightly 49 against Chrome & Firefox 46, I see the following results: - Nightly renders the bow tie as too small. - Nightly renders the clock in the wrong position, so it's mostly clipped.
Flags: needinfo?(dholbert)
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8747495 -
Attachment description: screenshot of bug, from private Wix site → screenshot #1, from private Wix site
Comment 6•8 years ago
|
||
I think a regression window would help.
Assignee | ||
Comment 7•8 years ago
|
||
We have one -- see comment 0. Regression from bug 1250773.
Assignee | ||
Comment 8•8 years ago
|
||
The problem in bug 1250773's patch is with the "GetAnimValue()" call here: bool HasRect() const - { return (mAnimVal && !mAnimVal->none) || - (!mAnimVal && mHasBaseVal && !mBaseVal.none); } + { + const nsSVGViewBoxRect& rect = GetAnimValue(); + return !rect.none && rect.width >= 0 && rect.height >= 0; + } GetAnimValue() is implemented as "return mAnimVal ? *mAnimVal : mBaseVal". In this case, mAnimVal is null, so it's just returning mBaseVal, which is 0,0,200,200. BUT, we should not be caring about mBaseVal at all, because it turns out mHasBaseVal is false! So we're reading the contents of mBaseVal, effectively, despite the fact that mHasBaseVal is false. (And as a result, we're behaving as if there's a viewBox even though there is not actually an explicit viewBox.)
Assignee | ||
Comment 9•8 years ago
|
||
And so the obvious next question is "why do we have valid-looking stuff in mBaseVal, if mHasBaseVal is false?" Here's how: 1) The page dynamically sets viewBox="0 0 200 200". * This is parsed via nsSVGElement::ParseAttribute calling nsSVGViewBox::SetBaseValueString, which calls ToSVGViewBoxRect.) * We're left with mHasBaseVal=true, and mBaseVal = 0,0,200,200 (in nsSVGViewBoxRect form). * All is well, so far... 2) The page dynamically sets viewBox="" * Again, this is parsed via nsSVGViewBox::SetBaseValueString which calls ToSVGViewBoxRect. * ToSVGViewBoxRect returns an error code; nsSVGViewBox::SetBaseValueString follows suit. * nsSVGElement::ParseAttribute gracefully handles the error by calling viewBox->Init(); * BUT, viewBox->Init() **ONLY** resets the "mHasBaseVal" flag -- it doesn't touch mBaseVal itself. So, this is how we're left with a valid-looking mBaseVal, which is nonetheless stale (per mHasBaseVal), which is causing us trouble as described in comment 8.
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
So, the root problem here is that bug 1250773 made an incorrect assumption. (Specifically, it assumed it was OK to call & trust the result of GetAnimValue(), without first having checked mHasBaseVal.) I see two easy ways to fix this: (1) Remove the incorrect assumption -- adjust HasRect() to check the "mHasBaseVal" flag. (And perhaps set assert mHasBaseVal is true, in the branch of GetAnimVal that returns mBaseVal, to catch future instances of this bug.) (2) Change the code so that this incorrect assumption *becomes* valid -- i.e. make viewBox->Init() a bit more thorough so it sets mBaseVal.none to "true". This way, if we do happen to use a reinitialized mBaseVal, we'll at least see that it has none=true and hence represents the value "none". I think either of these would be sufficient to fix this, but I think we should do both for thoroughness.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 12•8 years ago
|
||
Slight tangent: (In reply to Daniel Holbert [:dholbert] from comment #11) > (And perhaps assert mHasBaseVal is true, in the > branch of GetAnimVal that returns mBaseVal, to catch future instances of > this bug.) I don't think I can add this assertion, actually (not without some more significant rearchitecting, at least). So I'm going to pass on this suggestion of mine. (I tried adding it -- by making GetAnimValue() call GetBaseValue(), and making GetBaseValue() assert that mHasBaseVal is true, so that we'll always assert before returning a stale mBaseValue. But this assertion failed in some local testing, in a testcase that does the following: mySVG.setAttribute("viewBox", "0 0 40 40"); var savedBaseval = mySVG.viewBox.baseVal; mySVG.removeAttribute("viewBox"); savedBaseval.width = 45; This test code ends up invoking nsSVGViewBox::DOMBaseVal::SetWidth() on the |savedBaseval| object, which involves a call to GetBaseValue under the hood, which trips the assertion because at this point the nsSVGViewBox has had "Init()" called on it which resets mHasBaseVal.)
Assignee | ||
Comment 13•8 years ago
|
||
This patch basically: (1) Moves our current HasRect() impl from the .h file to the .cpp file. (2) Expands its GetAnimValue() call, and inserts a check & early-return if mAnimVal and mBaseVal are both unset. (where "unset" for mBaseVal means "mHasBaseVal is false") This restores the behavior that HasRect() had (with respect to mHasBaseVal) before this regressed.
Attachment #8747918 -
Flags: review?(longsonr)
Assignee | ||
Comment 14•8 years ago
|
||
I was going to attach a patch that tweaks nsSVGViewBox::Init, as described in comment 11, but after further reflection, I think I'll file a followup for that change (in part because I don't think we'd need or want to backport *that* patch to fix this bug on branches -- we only need to backport the patch that I've just attached).
Assignee | ||
Updated•8 years ago
|
Attachment #8747918 -
Attachment description: part 1: Fix nsSVGViewBox::HasRect() to return false instead of using stale mBaseVal → fix v1: Fix nsSVGViewBox::HasRect() to return false instead of using stale mBaseVal
Attachment #8747918 -
Attachment is obsolete: true
Attachment #8747918 -
Flags: review?(longsonr)
Assignee | ||
Comment 15•8 years ago
|
||
Re-posting the fix, now with reftests. (The tests are identical, except: one of them removes the viewBox attribute, one sets it to the empty string, and one sets it to a bogus invalid value.) I verified with a local reftest run that all three tests fail without the fix, and pass with the fix.
Attachment #8747933 -
Flags: review?(longsonr)
Assignee | ||
Comment 16•8 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b9b8bea1b59
Updated•8 years ago
|
Attachment #8747933 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8747933 [details] [diff] [review] fix v2 (now with reftest): Fix nsSVGViewBox::HasRect() to return false instead of using stale mBaseVal Approval Request Comment (for Aurora & Beta) [Feature/regressing bug #]: Regression from bug 1250773 [User impact if declined]: Broken sizing/positioning for SVG content, on sites that use dynamically generated/modified SVG (like wix.com, a popular website creation platform) [Describe test coverage new/current, TreeHerder]: Reftests included; and we do have a pretty thorough testsuite for SVG rendering. [Risks and why]: This is pretty low-risk -- it effectively just returns us to checking a SVG sizing-related flag (the nsSVGViewBox "mHasBaseVal" flag), which we *were checking* in the past (up until Firefox 46), but which the regressing bug inadvertantly made us stop checking. Anyway -- worst-case, in terms of risks, this bug could make SVG content mis-render in some scenario we haven't forseen. But given that this is just restoring an accidentally-removed check, I think that's unlikely. [String/UUID change made/needed]: None.
Attachment #8747933 -
Flags: approval-mozilla-beta?
Attachment #8747933 -
Flags: approval-mozilla-aurora?
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e1f60f95d08
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hi Dan, did you get a chance to verify the fix manually on the site from where you could repro the original issue? If yes, please mark this bug as verified. I will A+ fixes for uplift anyways.
Flags: needinfo?(dholbert)
Comment on attachment 8747933 [details] [diff] [review] fix v2 (now with reftest): Fix nsSVGViewBox::HasRect() to return false instead of using stale mBaseVal Recent regression in Fx47, Aurora48+, Beta47+
Attachment #8747933 -
Flags: approval-mozilla-beta?
Attachment #8747933 -
Flags: approval-mozilla-beta+
Attachment #8747933 -
Flags: approval-mozilla-aurora?
Attachment #8747933 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #20) > Hi Dan, did you get a chance to verify the fix manually on the site from > where you could repro the original issue? If yes, please mark this bug as > verified. Yes -- I just verified that today's nightly 2016-05-04 gives EXPECTED RESULTS on the original (private) site, and that yesterday's nightly 2016-05-03 gives buggy ACTUAL RESULTS. So, the fix (merged in today's nightly) definitely worked. > I will A+ fixes for uplift anyways. Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(dholbert)
Assignee | ||
Updated•8 years ago
|
(In reply to Daniel Holbert [:dholbert] from comment #22) > > Yes -- I just verified that today's nightly 2016-05-04 gives EXPECTED > RESULTS on the original (private) site, and that yesterday's nightly > 2016-05-03 gives buggy ACTUAL RESULTS. So, the fix (merged in today's > nightly) definitely worked. > Awesome! :)
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/995ecc8dbf0b
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f67fcfa24a72
You need to log in
before you can comment on or make changes to this bug.
Description
•