Closed Bug 1415963 Opened 2 years ago Closed 2 years ago

Graphs in YouTubes analytics broken

Categories

(Core :: Panning and Zooming, defect)

58 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: denschub, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(4 files)

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.
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?
Blocks: 1382534
Flags: needinfo?(botond)
Keywords: regression
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.
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Track 58+ as new regression in 58.
Assignee: nobody → botond
Flags: needinfo?(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 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
Attachment #8928032 - Flags: review?(mstange) → review+
Comment on attachment 8928636 [details]
Bug 1415963 - Add a reftest.

https://reviewboard.mozilla.org/r/199870/#review204988
Attachment #8928636 - Flags: review?(mstange) → review+
Pushed by bballo@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b98f2fff0e17
https://hg.mozilla.org/mozilla-central/rev/f592e4962e2b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Flags: needinfo?(botond)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---
The backout didn't fix the issue.
(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.
Flags: needinfo?(botond)
Pushed by bballo@mozilla.com:
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
Relanded per comments 13 and 14.
https://hg.mozilla.org/mozilla-central/rev/a5956fbf8648
https://hg.mozilla.org/mozilla-central/rev/74555d82b8e1
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8928032 - Flags: approval-mozilla-beta?
Comment on attachment 8928636 [details]
Bug 1415963 - Add a reftest.

(See previous comment)
Attachment #8928636 - Flags: approval-mozilla-beta?
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+.
Attachment #8928032 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928636 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.