1258650 - Yahoo finance chart comparison "overlays" not displayed properly because of bad interaction with scaled clip and mask combo

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Ravi S]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160315153207

Steps to reproduce:

1. Go to http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22range%22:%221d%22,%22allowChartStacking%22:true}
2. Click on the +Comparison "button" on the chart
3. Add any other stock symbol (I added HCLTECH.NS) into the text box



Actual results:

The graph for WIPRO.NS displays alright. The second "comparison" graph is only displayed when the mouse is moved over the chart. As soon as you stop moving the mouse, the comparison graph is not displayed.


Expected results:

The graphs of both stocks should have been displayed even without moving the mouse over the chart.

The problem started to occur only after the latest update of Firefox to 45.0.1

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Firefox: 45.0.1, Build ID  20160315153207
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0

Hi Ravi,

I have tested this issue on the latest Firefox (45.0.1) release, latest Nightly (48.0a1 - Build ID: 20160322030417) build, but I could not reproduce it. I have followed your provided steps and both graphs are correctly displayed.

Can you please retest this using latest FF release and latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E). 

Thanks,
Cosmin.

Comment 2

[Ravi S]

Attachment: Screen capture showing disappearing overlay plot

I restarted Firefox in safe-mode and the problem still occurs. I've also tried doing a "refresh firefox" with the same result. Attached is a screen capture showing the problem - notice how the 2nd plot is visible while the mouse is being moved in the chart area but disappears as soon as it is stopped or leaves the chart area. I will try with the nightly shortly and update.

Comment 3

[Ravi S]

Looks like this problem exists only if the 1d or 5d chart resolutions are chosen. For lower chart resolutions such as 1m, 3m, 6m, etc. the second plot is displayed just fine.

On the nightly: 48.0a1 (2016-03-23) the problem has a slightly different manifestation. The second plot will not appear even if the mouse is moved (in the 1d and 5d resolutions). Lower chart resolutions display fine.

Comment 4

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi Ravi,

I have tested again and I could reproduce this bug for lower char resolution. I have also performed a regression and this are the results:

Last good revision: 1e5f3d1151d60a1edd6424a35a2e38b5ab17adad
First bad revision: af5c9cf6898b0e2931a809774387cad5953363c3
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e5f3d1151d60a1edd6424a35a2e38b5ab17adad&tochange=af5c9cf6898b0e2931a809774387cad5953363c3

Looks like the following bug has the changes which introduced the regression: bug 1210560

Thanks,
Cosmin.

Comment 5

[Kohei Yoshino]

[Tracking Requested - why for this release]: A regression is Firefox 45, affecting SVG charts on Yahoo! Finance.

Comment 6

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/some-elements-in-interactive-svg-become-invisible-under-certain-conditions/

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 45+

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Milan, Jeff, could you please help find an owner who can investigate this? I thought I'd start with you guys first. Thanks!

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm seeing similar problems in Safari.  Also, when I add the third stock, the first and third show up, but not the second one (for the 1 day and 5 day ranges.)  I also don't see the problem when, say, comparing Apple and Google stocks.

Have we reached out to Yahoo on this?

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Karl, I have seen you looking at sitecompat issues. Milan suggested we get in touch with Yahoo on this issue as this might not be something we can fix. Would you be able to help with that? Thanks!

Comment 11

[Ravi S]

Hi,

I don't see the problem either on Safari or Chrome (OS X) - the charts display alright.
I see the problem on Firefox 45.0.1 irrespective of the stock chosen - including for AAPL and GOOG.

Thanks,
Ravi

Comment 12

[Kohei Yoshino]

I can't reproduce the issue on Safari or Chrome neither. We already have a clear regression window; should be fixed on the Firefox side.

Comment 13

[Karl Dubost💡 :karlcow]

Attachment: two-plots-on-yahoo.png

When going to 
http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22range%22:%221d%22,%22allowChartStacking%22:true}

I'm getting redirected to
http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22allowChartStacking%22:true}

fwiw I do not have the issue in Firefox Nightly 48.0a1 (2016-03-27)

Comment 14

[Karl Dubost💡 :karlcow]

Ah yes interesting behavior is different for 1d 5d.

ok. Also when changing the settings in visualization aka the gear.

Comment 15

[Karl Dubost💡 :karlcow]

Agreed with Kohei, we need first to understand where the issue is coming from.

Either Firefox and fix it
Or Yahoo! and know why it doesn't work anymore.

This requires more analysis.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Safari - 1 day chart, Google stock doesn't show

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Safari - 5 day chart, Google stock doesn't show

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Safari - 1 month chart, Google stock shows

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ravi S from comment #11)
> I don't see the problem either on Safari or Chrome (OS X) - the charts
> display alright.
> I see the problem on Firefox 45.0.1 irrespective of the stock chosen -
> including for AAPL and GOOG.

(In reply to Kohei Yoshino [:kohei] from comment #12)
> I can't reproduce the issue on Safari or Chrome neither. We already have a
> clear regression window; should be fixed on the Firefox side.

I attached the screen grabs from the bug reproducing in Safari.  1 and 5 day are missing Google stock graph, 1 month has it.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Chrome - 1 day chart, Google stock doesn't show

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Chrome - 5 day chart, Google stock doesn't show

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: On Chrome - 1 month chart, Google stock shows

Comment 23

[Kohei Yoshino]

That's not related to this bug. You see GOOGL N/A in that range. HCLTECH.NS in comment 0 works.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

Fair enough.  It does feel like a push group regression, mentioned in comment 4.  Some observations:

* Adding another stock (e.g., Google) after HCLTECH.NS makes that one show up (nothing to show for Google)
* Panning the whole graph, especially to the left, I can see the HCLTECH.NS graph show up, in parts, usually clipped. 

(N/A is not necessarily an indicator, because it shows where the mouse cursor is, so if you don't have the data for the whole range, you'll see N/A when you'd doing a clip box type of screen grab.)

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

I see this on Windows as well.

Comment 26

[Karl Dubost💡 :karlcow]

Another interesting pattern with this bug.

If I display 1d, then 5d Indeed I receive a Chart unavailable most of the time.
Then I go to 1 month, and 3 months, etc.
Then go down from 3m to 1m, then 1m to 5d, then 5d to 1d, and all charts are being displayed fine.

I even managed to have the chart not displayed for 1m. We still need to understand why it's happening and more or less randomly.

Each time we change the view, we get a series of 3 HTTP requests.

* http://geo.yahoo.com/p?s=2022773886&t=rbK4LU6Mjxzvic4p,0.0373193842344699&_I=&_AO=0&_NOL=0&_R=&_P=3.21%05_pl%031%04A_v%033.21%04test%03DDCONTROL%04_bt%03rapid%04A_sid%03mmnGZQPMKP7Z0XVY%04_w%03finance.yahoo.com%2Fecharts%3Fs%3DWIPRO.NS%2BInteractive%23%7B%2522range%2522%3A%25221d%2522%2C%2522allowChartStacking%2522%3Atrue%7D%04version%03td%20app%04site%03mobile-web-quotes%04_ts%031459212240%04sec%03chart-controls-range%04slk%031mo%04_E%03click

* http://y.analytics.yahoo.com/fpc.pl?_cb=rtx1TJHhJLlEQbey&.ys=2022773886&a=1000911397279&b=WIPRO.NS%20Interactive%20Stock%20Chart%20%7C%20Yahoo!%20Inc.%20Stock%20-%20Yahoo!%20Finance&d=Tue%20Mar%2029%202016%2009%3A44%3A00%20GMT%2B0900%20(JST)&f=http%3A%2F%2Ffinance.yahoo.com%2Fecharts%3Fs%3DWIPRO.NS%2BInteractive%23%7B%2522range%2522%3A%25223mo%2522%2C%2522allowChartStacking%2522%3Atrue%7D&j=1920x1200&k=24&t=1459212240&l=true&c=interactive-chart&dpid=1846106777&x=12&fpc=ZXVaI2ZY%7C%7C&cf14=DDCONTROL&cf18=chart-controls-range&cf19=1mo&cf42=mobile-web-quotes&ca=1&resp=img

* https://finance-yql.media.yahoo.com/v7/finance/chart/WIPRO.NS?period2=1459212240&period1=1449189840&interval=1d&indicators=quote&includeTimestamps=true&includePrePost=true&comparisons=HCLTECH.NS&events=div%7Csplit%7Cearn&corsDomain=finance.yahoo.com


There's a recurring error in the console.log but again not systematic.

NS_ERROR_FAILURE: 
e.exports.init/c</<()
combo:54
m/<()
combo:53
forEach()
self-hosted
m()
combo:53
e.exports.init/c<()
combo:54
p/<()
combo:53
Kr/<()


Exceptions are raised on 
http://l.yimg.com/zz/combo?os/mit/td/td-applet-charts-0.3.1/td-charts-model/td-charts-model-min.js&os/mit/td/td-applet-charts-0.3.1/td-charts-image-model/td-charts-image-model-min.js&os/mit/td/td-applet-charts-0.3.1/td-charts-virgo-view/td-charts-virgo-view-min.js

 !c && t.xLabelText && (c = t.xLabelText.node().getBBox().width, p = c / 2, d = Math.max(0, Math.min(e.width - c - l, o - p - f))),


When the graph is working, every is working fine continuously.

Comment 27

[Ravi S]

(In reply to Milan Sreckovic [:milan] from comment #24)
> Fair enough.  It does feel like a push group regression, mentioned in
> comment 4.  Some observations:
> 
> * Adding another stock (e.g., Google) after HCLTECH.NS makes that one show
> up (nothing to show for Google)
> * Panning the whole graph, especially to the left, I can see the HCLTECH.NS
> graph show up, in parts, usually clipped. 
> 
> (N/A is not necessarily an indicator, because it shows where the mouse
> cursor is, so if you don't have the data for the whole range, you'll see N/A
> when you'd doing a clip box type of screen grab.)

You're seeing these inconsistencies because you're comparing stocks from two different exchanges with different market durations and times. Please use any two stocks from the same exchange to reproduce the original issue.

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, I can reproduce the original issue on multiple platforms, and the panning problem I mentioned is probably in support of this being push group related issue and a fallout from bug 1210560.

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Moving to Jeff, Bas has too many tracked bugs.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Anthony, this bug is a good sanity test case to add to our list.  The panning as well.  I know it's broken now, but to track and test in the future.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Attachment: Reduced test cast

It looks like the problem comes from having a clip-path and a mask

Comment 32

[u279076]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> Created attachment 8735983 [details]
> Reduced test cast
> 
> It looks like the problem comes from having a clip-path and a mask

This testcase doesn't do anything for me -- all I get is a blank page. What steps are needed to get this to work?

Comment 33

[Jeff Muizelaar [:jrmuizel]]

(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #32)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> > Created attachment 8735983 [details]
> > Reduced test cast
> > 
> > It looks like the problem comes from having a clip-path and a mask
> 
> This testcase doesn't do anything for me -- all I get is a blank page. What
> steps are needed to get this to work?

Fixing the bug :)

The testcase displays blank in broken Firefox. If you use one from before bug 1210560 it will show a graph.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Attachment: Change how we combine clip and path masks

This switches us to using FillRect for combining Mask and Clip path surfaces. Doing so allows us to avoid allocating a new source and making a copy. More importantly it uses the whole matrix instead of just the offset. This was the cause of this bug. aExtraMaskTransform has a scale component that we were ignoring.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Attachment: A readable version of the test case

I'll turn this into a reftest

Comment 36

[Markus Stange [:mstange]]

Have you tested this patch with DrawTargetCG? I'm worried that bug 1231643 might bite you there.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #36)
> Have you tested this patch with DrawTargetCG? I'm worried that bug 1231643
> might bite you there.

It seems to be biting me.

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

Since it got mentioned today, wanted to record it - the original test case does assert in debug.

Comment 39

[Markus Stange [:mstange]]

Attachment: a testcase with nested clip paths for Jeff

Comment 40

[Jeff Muizelaar [:jrmuizel]]

In writing a test case for this it looks like I've discovered an additional bug.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Attachment: Change how we combine clip and path masks v2

This version adds a test and corrects some of the problems of the previous version. 

1. DrawTargetCG doesn't properly support OPERATOR_IN with A8 surfaces and is not easily fixed (cairo-quartz falls back to pixman). We use a Cairo draw target in that case.

2. We were not properly handling the area outside of the mask image. The test I added caught this.

Comment 42

[Jeff Muizelaar [:jrmuizel]]

There's apparently something broken with D2D as well...

Comment 43

[Bas Schouten (:bas.schouten)]

Comment on attachment 8736568 [details] [diff] [review]
Change how we combine clip and path masks v2

Review of attachment 8736568 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGClipPathFrame.cpp
@@ +124,5 @@
>    }
>  
> +  RefPtr<DrawTarget> maskDT;
> +  // The CoreGraphics backend doesn't support IN with A8 surfaces
> +  if (aReferenceDT.GetBackendType() == BackendType::COREGRAPHICS)

nit: {}

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Attachment: Change how we combine clip and path masks v3

Comment 45

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

Review of attachment 8736729 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGClipPathFrame.cpp
@@ +245,1 @@
>    }

There is some serious transformation complexity involved here :)

Some random notes, for what they're worth.

mat starts as reference context transform * -devSpaceClipExtents; it gets inverted a few lines above.

Above, where we do:
mat.Invert();
we could move the code from further down right after it:
*aMaskTransform = ToMatrix(mat);

Then this:

maskDT->MaskSurface(SurfacePattern(currentMask, ExtendMode::CLAMP, aExtraMasksTransform.Inverse() * ToMatrix(mat))...

becomes this:

maskDT->MaskSurface(SurfacePattern(currentMask, ExtendMode::CLAMP, aExtraMasksTransform.Inverse() *(*aMaskTransform))...

And we could add a comment that reminds us that:

aExtraMasksTransform.Inverse() *(*aMaskTransform)

is the same as:

(original mat, pre Invert call * aExtraMasksTransform ).Inverse()

in other words, the matrix we're using in the MaskSurface call is:

(aReferenceContext.CurrentMatrix() *
 gfxMatrix::Translation(-devSpaceClipExtents.TopLeft()) *
 aExtraMasksTransform).Inverse()

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb08f003648

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afb08f003648

Comment 48

[Kohei Yoshino]

This bug is a driver of the potential 45.0.2 release. The patch needs to be uplifted.

https://wiki.mozilla.org/Firefox/Channels/Meetings/2016-04-05

Comment 49

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

Approval Request Comment
[Feature/regressing bug #]: 1210560
[User impact if declined]: Some SVG scenarios are not properly drawn.  The example we have is from a high traffic site.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low.  The case where the results are different should only be those that asserted before (and were wrong.)
[String/UUID change made/needed]:

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

Let's take it for 45.0.2

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This does not apply cleanly to aurora, so likely doesn't apply to any of the other branches this was approved for. Can we get rebased patches?

Comment 52

[Jeff Muizelaar [:jrmuizel]]

Attachment: Untested Aurora patch

Comment 53

[Jeff Muizelaar [:jrmuizel]]

Attachment: Untested beta patch

Comment 54

[Jeff Muizelaar [:jrmuizel]]

The beta patch fails tests. I expect it will be broken on release as well.

Comment 55

[Jeff Muizelaar [:jrmuizel]]

Attachment: Partially test patch against release

Comment 56

[Jeff Muizelaar [:jrmuizel]]

It's worth noting that this fix might require part1 of bug 1251431 to work on Windows with D2D

Comment 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jeff mentioned on IRC that he will be running exhaustive tests on this patch which may take a couple of hours. We both agree that landing this patch without running it through all tests is risky. Let's wait for the test run to finish before deciding to uplift the rebased patch or whether we need more changes as mentioned in comment 56. Sylvestre, fyi.

Comment 58

[Jeff Muizelaar [:jrmuizel]]

Beta try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c544764fff9c

Comment 59

[Jeff Muizelaar [:jrmuizel]]

Release try results w/ part 1 from bug 1251431
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe5e6ee38ff

Comment 60

[Jeff Muizelaar [:jrmuizel]]

It looks like this should be ok to land on release on top of part 1 from bug 1251431

Comment 61

[Sylvestre Ledru [:Sylvestre]]

Well, not sure I want to do that in a dot release...  :/

Comment 62

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hey Jeff, we uplifted this patch to Beta, Aurora and ESR45. Should we uplift part1 from bug 1251431 to those branches as well to ensure we have a complete and fully working fix?

Comment 63

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ritu Kothari (:ritu) from comment #62)
> Hey Jeff, we uplifted this patch to Beta, Aurora and ESR45. Should we uplift
> part1 from bug 1251431 to those branches as well to ensure we have a
> complete and fully working fix?

Yes.

Comment 64

[Sylvestre Ledru [:Sylvestre]]

Not taking it to 45. The bug 1251431 didn't get enough user coverage to take the risk.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1caddf0eaae5

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b858565ed9a

Comment 67

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: I'm assuming this needs to be 46+ now since it's not landing for an ESR45 dot release (looks like the approval flags need some updating too).

Also, my understanding is that bug 1251431 also needs ESR45 approval before this can land for 45.1?

Comment 68

[Sylvestre Ledru [:Sylvestre]]

I am not even sure I want to take it in ESR... Bug 1251431 didn't have time to back properly...

Comment 69

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

Reproduced the initial issue with Firefox 45.0.2.

Confirming the fix across platforms (Win 10x64, Ubuntu 14.04x64 and Mac OS X 10.11 using:
- Latest 48.0a1 Nightly, buildID 20160413030239
- Latest 47.0a2 Aurora, buildID 20160413004016
- Firefox 46.0b10, buildID 20160411042519.

Comment 70

[Liz Henry (:lizzard) (relman/hg->git project)]

Jeff or Milan, do you think this is worth a release note? If so can you suggest wording? Thanks.

Comment 71

[Jeff Muizelaar [:jrmuizel]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #70)
> Jeff or Milan, do you think this is worth a release note? If so can you
> suggest wording? Thanks.

Sure. "Fix an issue where scaled SVG that used a clip and a mask was not rendered properly (bug 1258650)"