Closed Bug 1269174 Opened 8 years ago Closed 8 years ago

SVG icon missing or too small, at wix.com

Categories

(Core :: SVG, defect)

defect
Not set
normal

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)

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
[Tracking Requested - why for this release]: Rendering regression introduced in Firefox 47
Summary: SVG icon appears too small at wix.com → SVG icon missing or too small, at wix.com
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.
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)
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)
Attachment #8747495 - Attachment description: screenshot of bug, from private Wix site → screenshot #1, from private Wix site
I think a regression window would help.
We have one -- see comment 0. Regression from  bug 1250773.
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.)
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.
Attached file reduced testcase 1
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: nobody → dholbert
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.)
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)
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).
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)
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)
Attachment #8747933 - Flags: review?(longsonr) → review+
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?
Blocks: 1269885
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+
(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)
(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! :)
You need to log in before you can comment on or make changes to this bug.