Closed Bug 1556511 Opened 3 months ago Closed 3 months ago

initial render of a Coinbase svg graph is wrong in Firefox Nightly

Categories

(Core :: SVG, defect, P2)

69 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla69
Webcompat Priority ?
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed

People

(Reporter: ksenia, Assigned: dholbert)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(5 files)

  1. On Firefox Nightly
  2. Go to https://www.coinbase.com/dashboard
  3. Create an account
  4. Once you're in, observe the graphs

Expected:
Graphs svgs render the same as in Chrome/Firefox release

Actual:
Svgs look different corrupted, their height is bigger than their parent's container. This goes away on page resize/css change

Unfortunately I wasn't able to create a reproducible reduced test case. To access the affected elements search for: SparkLine__ in the inspector.

The output of mozregression:

20:27.87 INFO: Last good revision: bb69a96aea8403f807a74652168c71c97da6fbeb
20:27.87 INFO: First bad revision: 09ef16b1c51fa6917986aea632e61a3f153b7ce8
20:27.87 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bb69a96aea8403f807a74652168c71c97da6fbeb&tochange=09ef16b1c51fa6917986aea632e61a3f153b7ce8

Could you find a regression range?

Flags: needinfo?(kberezina)

D'oh, sorry, had missed it! :(

Daniel, could you take a look? Otherwise lmk and I can try :)

Flags: needinfo?(dholbert)
Priority: -- → P2

Sure. I've created an account and I can reproduce. Hopefully I'll know more tomorrow.

Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Keywords: regression
Has Regression Range: --- → yes
Has STR: --- → yes

It looks like the issue here is that the aspect ratio of the SVG container is wrong (i.e. the frame's size does not match the aspect ratio).

I'm guessing that this is due to some attribute being set dynamically on that element, which needs to update the frame size, but doesn't currently.

Summary: initial render of an svg is different from Chrome/Firefox release → initial render of a Coinbase svg graph is wrong in Firefox Nightly

In particular, we end up with each graph looking like this:

<svg viewBox="0 0 360 75" ..>

...with no 'height' or 'width' specified, and for some reason we're giving it a box that is 359.667 x 359.667 pixels (roughly 360x360, rather than 360x75 as the viewBox clearly wants).

Actually, testcase 1 is broken in builds at least as far back as 2014-06-01 (that's as far back as I tested). It seems like we don't trigger a reflow for a viewBox attribute change, and never have!

In Coinbase, they were inadvertently working around this by making some subsequent changes inside the SVG (e.g. setting the d attribute on a path), which does trigger a reflow, which was sufficient to mask the bug. But as of bug 1550535, the reflow from e.g. <path d="..."> changes is more localized (as it should be able to be), and it's no longer sufficient tot get the outer <svg> element's frame reflowed by its own parent anymore, so it doesn't change size to account for its new viewBox value anymore.

So we need to fix the actual (old) bug here -- we need to handle viewBox changes by requesting a reflow.

Attachment #9070073 - Attachment description: testcase 1 → testcase 1 (demonstrating the underlying quite-old bug)
Component: Layout → SVG

This works correctly for the "height" attribute (e.g. if I start it out at "1" and then change it to "450"). For that attribute, we call PresShell::FrameNeedsReflow at the bottom of nsSVGOuterSVGFrame::AttributeChanged.

We probably should do that for viewBox, too.

The viewBox attribute establishes an aspect ratio, which may influence the size
of the outer SVG element. So when the viewBox attribute changes, we have to
reflow to potentially update the frame size. This patch achieves this by
sharing an existing codepath for handling changes to width & height.

The attached patch fixes this for the attached testcases and for Coinbase. I'll extend it to add automated testcases tomorrow and request review.

(Note to self: I need to be sure to add testcases for object/embed, too, since those have a different codepath here.)

Here's a try run to make sure that our existing testsuite is all right with this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b0d30347c1c40834b755e2a1fb22bd86bd00c1

Attachment #9070149 - Attachment description: Bug 1556511: Mark the outer SVG frame as needing a reflow, when its viewBox attribute changes. → Bug 1556511: Mark the outer SVG frame as needing a reflow, when its viewBox attribute changes. r?longsonr

Chrome has a version of this bug, too (though it's not triggered in as many situations), and they fail the final check in the included WPT test as a result. Here's a reduced testcase for their bug: https://jsfiddle.net/g75rmhz6/ When you click the button, the svg element should shrink to show only the 100x100 green square (and that's what happens, in Firefox with this bug's patch applied). In Chrome and pre-patch Firefox, the SVG element stays the same size and red is still visible (and the content jumps around in response to the updated transform associated with the new viewBox).

I'll file a Chrome bug.

(In reply to Daniel Holbert [:dholbert] (reduced availability until June 12) from comment #14)

(Note to self: I need to be sure to add testcases for object/embed, too, since those have a different codepath here.)

(Note: my patch includes a test for SVG-inline-in-HTML, but I still need to add this ^^ sort of test. I also should add a test with viewBox implicitly changing via an SVG View if possible, too.)

Attachment #9070149 - Attachment description: Bug 1556511: Mark the outer SVG frame as needing a reflow, when its viewBox attribute changes. r?longsonr → Bug 1556511 part 1: Mark the outer SVG frame as needing a reflow, when its viewBox attribute changes. r?longsonr

Firefox fails some of this test's subtests right now (per the included .ini
file), and I've filed bug 1557612 to track these failures.

Depends on D33898

(In reply to Daniel Holbert [:dholbert] from comment #17)

I also should add a test with viewBox implicitly changing via an SVG View if possible, too.)

I started writing this ^^ sort of test, but it turned out that the size got reset to 0x0 (synchronously), and a new load was initiated, with each #-ref navigation, AFAICT. So the test ended up being a bit more trouble (due to an async load at each step) and it didn't actually exercise any problematic codepaths, since we were doing a fresh reflow/resize at each #-ref navigation for each view change.

So, I'm not including a SVG View testcase after all.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aba9b5d525af
part 1: Mark the outer SVG frame as needing a reflow, when its viewBox attribute changes. r=longsonr
https://hg.mozilla.org/integration/autoland/rev/bab948f512c2
part 2: Add a test variant that uses SVG embedded in an `<object>` element. r=longsonr
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17253 for changes under testing/web-platform/tests
Upstream PR merged
Regressions: 1559344
You need to log in before you can comment on or make changes to this bug.