1361180 - Graphics on Yahoo Finance not displaying properly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jon]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

When I updated from Firefox 52.0.2 to 53.0, certain graphics on Yahoo Finance no longer work.  I frequently use technical indicators (such as EMA), and many of these indicators no longer plot properly.  When I revert back to Firefox 52.0.2 everything works properly.

To reproduce, go to Yahoo and look at the chart for a stock (e.g. https://finance.yahoo.com/chart/DEO).  Click "Add Indicator" and choose "Exponential Moving Average".  In Firefox 53.0 nothing appears on the chart.  In Firefox 52.0.2, a line showing the EMA appears properly.

This also happens for several other indicators I frequently use (MACD and RSI).


Actual results:

With Firefox 53.0, certain graphics do not plot correctly on Yahoo Finance.  These graphics plot correctly on Firefox 52.0.2.


Expected results:

Lines representing various technical indicators should be plotted on Yahoo's chart.  These lines properly appear on Firefox 52.0.2.

Comment 1

[Alice0775 White]

regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ac16df8810f60d0bd4514b5d4975e63e7004e3ce&tochange=d9b26b438895944ea6764976577436383de9e23d

regressed by:Bug 1319667

Comment 2

[u459114]

Attachment: Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

In an SVG doc, giving an invalid local-ref mask, e.g. mask="url"#foo"", and
clipState.mDirtyRectInDevPx be empty at the same time:
1. PaintStyleImageLayerWithSC early continue at [1], and the value of result is
still the initial value, DrawResult::SUCCESS.
2. The caller is not aware of that the  mask image was left unresolved since it get
DrawResult::SUCCESS from #1.

This patch fixed this problem by calling PrepareImageLayer before early return to
update correct drawing result.

[1]
https://hg.mozilla.org/mozilla-central/file/ebf68ba9b098/layout/painting/nsCSSRendering.cpp#l2682

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

Comment 3

[u459114]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135560/diff/1-2/

Comment 4

[u459114]

Attachment: Bug 1361180 - Part 2. Reftest for unresolved mask in svg.

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

Comment 5

[u459114]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135560/diff/2-3/

Comment 6

[u459114]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135560/diff/3-4/

Comment 7

[u459114]

Comment on attachment 8863850 [details]
Bug 1361180 - Part 2. Reftest for unresolved mask in svg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135582/diff/1-2/

Comment 8

[u459114]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135560/diff/4-5/

Comment 9

[u459114]

Comment on attachment 8863850 [details]
Bug 1361180 - Part 2. Reftest for unresolved mask in svg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135582/diff/2-3/

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

https://reviewboard.mozilla.org/r/135560/#review140258

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8863850 [details]
Bug 1361180 - Part 2. Reftest for unresolved mask in svg.

https://reviewboard.mozilla.org/r/135582/#review140260

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d6cfbbc07a4
Part 1. Return error DrawResult to the caller when the mask url can not be resolved. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ce92201af9b7
Part 2. Reftest for unresolved mask in svg. r=mstange

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d6cfbbc07a4
https://hg.mozilla.org/mozilla-central/rev/ce92201af9b7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 15

[u459114]

Attachment: Part 2 for beta

Comment 16

[u459114]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1319667
[User impact if declined]:trend line on Yahoo Finance may disappear
[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]: this patch, Part 1, and "Part 2 for beta"
[Is the change risky?]:no
[Why is the change risky/not risky?]: pass auto/manual test, the logic changed in the patch is simple.
[String changes made/needed]: N/A

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8863813 [details]
Bug 1361180 - Part 1. Return error DrawResult to the caller when the mask url can not be resolved.

Fix an UI issue and include test. Beta54+. Should be in 54 beta 7.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6922fb2c5500
https://hg.mozilla.org/releases/mozilla-beta/rev/7250535bd343

Comment 19

[Andrei Vaida [:avaida]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #16)
> [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 

Setting qe-verify- based on C.J. Ku's assessment on manual testing needs and the fact that this fix has automated coverage.