1415963 - Graphs in YouTubes analytics broken

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Dennis Schubert [:denschub]]

Attachment: Screenshot illustrating the issue

STR:

1. Sign into a Google account with a YouTube channel and some views.
2. Head to https://www.youtube.com/analytics and observe the statistics.

Expected results:

Normal charts visible

Actual results:

Top charts are drawn as a single line, see the attached screenshot. Hovering over the chart shows the points at the correct locations, but the lines are drawn at y=0.

Comment 1

[Dennis Schubert [:denschub]]

This is a regression from bug 1382534, as verified with mozregression. The chart is an inline SVG, and as far as I can see, the <g> contains valid points for the chart. 

Botond, as you wrote the patches for bug 1382534, do you have an idea what's going wrong here?

Comment 2

[Dennis Schubert [:denschub]]

Attachment: Example chart with broken lines

Attached is an example for a chart, to make debugging easier.  This issue is likely related to the calculation of clipping paths, since the chart seems to work fine if I remove the `clip-path` attribute of the group containing the path.

Comment 3

[Gerry Chang [:gchang]]

Track 58+ as new regression in 58.

Comment 4

[Botond Ballo [:botond]]

The problem here is that when calculating the contribution of SVG elements to the clip rect computed for clip paths, we were taking intro consideration fill but not stroke.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1415963 - Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path.

Review commit: https://reviewboard.mozilla.org/r/199260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199260/

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8928032 [details]
Bug 1415963 - Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path.

https://reviewboard.mozilla.org/r/199260/#review204974

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1415963 - Add a reftest.

Review commit: https://reviewboard.mozilla.org/r/199870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199870/

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8928636 [details]
Bug 1415963 - Add a reftest.

https://reviewboard.mozilla.org/r/199870/#review204988

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b98f2fff0e17
Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f592e4962e2b
Add a reftest. r=mstange

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b98f2fff0e17
https://hg.mozilla.org/mozilla-central/rev/f592e4962e2b

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out 2 changesets (bug 1415963) for failing reftests on Android 4.3 API16 css-invalid/select/select-disabled-fieldset-1.html

Backout: https://hg.mozilla.org/mozilla-central/rev/2c78cf003670a4beeb8730a78daf1edf9e744c1c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f592e4962e2b01125cce9d406a4f01ad413ea821

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=145304960&repo=autoland&lineNumber=1743 

 REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/css-invalid/select/select-disabled-fieldset-1.html | 2 / 623 (0%)
1742
[task 2017-11-16T13:37:09.580Z] 13:37:09     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/css-invalid/select/select-fieldset-ref.html | 2 / 623 (0%)
1743
[task 2017-11-16T13:37:19.890Z] 13:37:19     INFO -  REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/css-invalid/select/select-disabled-fieldset-1.html == http://10.0.2.2:8854/tests/layout/reftests/css-invalid/select/select-fieldset-ref.html | image comparison, max difference: 8, number of differing pixels: 1

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The backout didn't fix the issue.

Comment 14

[Botond Ballo [:botond]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #13)
> The backout didn't fix the issue.

Yeah, I don't think this bug has anything to do with that reftest failure. The code modified in this bug isn't even called when running that reftest.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5956fbf8648
Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path. r=mstange
https://hg.mozilla.org/integration/autoland/rev/74555d82b8e1
Add a reftest. r=mstange

Comment 16

[Botond Ballo [:botond]]

Relanded per comments 13 and 14.

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a5956fbf8648
https://hg.mozilla.org/mozilla-central/rev/74555d82b8e1

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8928032 [details]
Bug 1415963 - Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path.

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 1382534

[User impact if declined]:
  Some SVG images are rendered incorrectly.
  Affected SVG images can be found on at least one
  popular website, https://www.youtube.com/analytics

[Is this code covered by automated tests?]:
  Yes

[Has the fix been verified in Nightly?]:
  Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
  No

[List of other uplifts needed for the feature/fix]:
  None

[Is the change risky?]:
  No

[Why is the change risky/not risky?]:
  It's a one line, easy to understand change

[String changes made/needed]:
  None

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8928636 [details]
Bug 1415963 - Add a reftest.

(See previous comment)

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8928032 [details]
Bug 1415963 - Take into account stroke (not just fill) when calculating the contribution of an SVG element to a clip rect for a clip path.

Fix a SVG image rendering issue. Beta58+.

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/520fd735565e
https://hg.mozilla.org/releases/mozilla-beta/rev/93b43c6d7ab4

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Botond Ballo [:botond] from comment #18)
> [Is this code covered by automated tests?]:
>   Yes
> [Needs manual test from QE? If yes, steps to reproduce]: 
>   No

Based on Botond's assessment for manual testing needs and given the fact that it is already covered by automation, marking as qe-verify-.