Closed Bug 1293929 Opened 8 years ago Closed 8 years ago

Yahoo! Finance stock chart can no longer compare more than one stock in Firefox 49

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
platform-rel --- ?
firefox48 --- unaffected
firefox49 + verified
firefox50 + fixed
firefox51 + verified

People

(Reporter: cpeterson, Assigned: u459114)

References

Details

(Keywords: regression, Whiteboard: [platform-rel-Yahoo])

Attachments

(2 files)

Bug 1293929 - Part 1. Treat unresolvable mask in svg frame as no-mask.
58 bytes, text/x-review-board-request
mstange
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Bug 1293929 - Part 2. add reftest.
58 bytes, text/x-review-board-request
mstange
: review+
Details 
[Tracking Requested - why for this release]:

This is a serious regression in Firefox 49.

@ C.J.: I bisected this regression to your fix for masking image bug 1275450.

@ Jenn: do we have partner contacts at Yahoo? I don't know yet if this is a Firefox bug that we should fix or a web content problem that Yahoo should fix.

STR:
1. Load https://finance.yahoo.com/chart/AAPL
2. Click the chart's "+ Compare" menu
3. Click "S&P 500" or enter another company's stock ticker symbol like MSFT.

RESULT:
Nothing happens! The chart should show the data for the S&P 500 or MSFT.

Alternately, here is a quick link to repro the problem:

https://finance.yahoo.com/chart/AAPL#eyJjb21wYXJpc29ucyI6Il5HU1BDLE1TRlQiLCJjb21wYXJpc29uc0NvbG9ycyI6IiMxYWM1NjcsI2YwMTI2ZiIsImNvbXBhcmlzb25zR2hvc3RpbmciOiIwLDAiLCJjb21wYXJpc29uc1dpZHRocyI6IjEsMSIsIm11bHRpQ29sb3JMaW5lIjpmYWxzZSwiYm9sbGluZ2VyVXBwZXJDb2xvciI6IiNlMjAwODEiLCJib2xsaW5nZXJMb3dlckNvbG9yIjoiIzk1NTJmZiIsIm1maUxpbmVDb2xvciI6IiM0NWUzZmYiLCJtYWNkRGl2ZXJnZW5jZUNvbG9yIjoiI2ZmN2IxMiIsIm1hY2RNYWNkQ29sb3IiOiIjNzg3ZDgyIiwibWFjZFNpZ25hbENvbG9yIjoiIzAwMDAwMCIsInJzaUxpbmVDb2xvciI6IiNmZmI3MDAiLCJzdG9jaEtMaW5lQ29sb3IiOiIjZmZiNzAwIiwic3RvY2hETGluZUNvbG9yIjoiIzQ1ZTNmZiIsInJhbmdlIjoiMXkifQ%3D%3D
Flags: needinfo?(jchaulk)
Flags: needinfo?(cku)
Checking. I think the reason should be similar with Bug 1287320 comment 2.
mask(#mask-4-eeda) is not resolvable. 
OK, this one is not totally the same with bug 1287320.
The unresolvable mask is belonged to a SVG document, which is embedded inside a HTML document. We should treat unresolvable mask as no-mask.
Assignee: nobody → cku
Flags: needinfo?(cku)
Directing this to BD for Yahoo contacts, as per email.
Flags: needinfo?(jchaulk) → needinfo?(mconnor)
Comment on attachment 8779660 [details]
Bug 1293929 - Part 1. Treat unresolvable mask in svg frame as no-mask.

https://reviewboard.mozilla.org/r/70630/#review68028
Attachment #8779660 - Flags: review?(mstange) → review+
Comment on attachment 8779661 [details]
Bug 1293929 - Part 2. add reftest.

https://reviewboard.mozilla.org/r/70632/#review68030

::: layout/reftests/w3c-css/submitted/masking/reftest.list:22
(Diff revision 2)
>  
>  # mask-image test cases
>  fails == mask-image-1a.html mask-image-1-ref.html
>  fails == mask-image-1b.html mask-image-1-ref.html
>  fails == mask-image-1c.html mask-image-1-ref.html
> +fails == mask-image-1d.html mask-image-1-ref.html

Why are you marking it "fail"? This test doesn't require mask as shorthand to be enabled in order to pass, does it?
Attachment #8779661 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b99bcfcb41b0
Part 1. Treat unresolvable mask in svg frame as no-mask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/03bc744ea8c0
Part 2. add reftest. r=mstange
platform-rel: --- → ?
Whiteboard: [platform-rel-Yahoo]
For clarity, given the comments and bugs, do we have an ask for Yahoo, or is this an issue we've created (and are now fixing)?
I wasn't part of the email thread, but as far as I can tell, the situation is as follows:
 - We unintentionally regressed how we deal with unresolvable masks on SVG elements. This is the bug being fixed here.
 - The fix in this bug will make Yahoo work again.
 - In parallel, Yahoo can change their site to remove the mask attribute on the affected element. They set mask="url(#mask-4-16b23)" on a <g> element with class="comparisons-container", but they do not define a mask with id="mask-4-16b23" anywhere. This looks unintentional and seems like something they'd want to fix anyway, since it's not doing anything. (Other than hiding the affected element in Firefox due to this Firefox bug.)
Tracking 51+.
tracking-firefox51: ? → +
Comment on attachment 8779660 [details]
Bug 1293929 - Part 1. Treat unresolvable mask in svg frame as no-mask.

Approval Request Comment
[Feature/regressing bug #]: 1275450
[User impact if declined]: A masked element become invisible if that mask is not resolvable.
[Describe test coverage new/current, TreeHerder]: manually + try
[Risks and why]: low risk. Correct a regression on SVG mask rendering.
[String/UUID change made/needed]:
Attachment #8779660 - Flags: approval-mozilla-aurora?
Comment on attachment 8779660 [details]
Bug 1293929 - Part 1. Treat unresolvable mask in svg frame as no-mask.

Approval Request Comment
[Feature/regressing bug #]: 1275450
[User impact if declined]: A masked element become invisible if that mask is not resolvable.
[Describe test coverage new/current, TreeHerder]: manually + try
[Risks and why]: low risk. Correct a regression on SVG mask rendering.
[String/UUID change made/needed]:
Attachment #8779660 - Flags: approval-mozilla-beta?
We need (part 1) for both 49 & 50.
The link rel="match" in mask-image-1d.html disagrees with the reftest.list as to what the correct referenence is.  (mask-image-3f and mask-image-3g have the same problem.)

Would you mind fixing?
Flags: needinfo?(cku)
Ugh, I need to remember to check those when reviewing. Sorry again.
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #21)
> The link rel="match" in mask-image-1d.html disagrees with the reftest.list
> as to what the correct referenence is.  (mask-image-3f and mask-image-3g
> have the same problem.)
> 
> Would you mind fixing?
Sure. Sorry for this.
Flags: needinfo?(cku)
Setting the 51 status flag back to affected so the sheriffs won't miss this. 
CJ, can you request uplift, once this lands on mozilla-central?  Thanks!
status-firefox51: fixed → affected
tracking-firefox49: ? → +
tracking-firefox50: ? → +
Oh never mind, I see you already requested uplift and the new change is just for the tests, which may not need uplift.
Comment on attachment 8779660 [details]
Bug 1293929 - Part 1. Treat unresolvable mask in svg frame as no-mask.

Fix for recent regression, please uplift to aurora and beta.
Attachment #8779660 - Flags: approval-mozilla-beta?
Attachment #8779660 - Flags: approval-mozilla-beta+
Attachment #8779660 - Flags: approval-mozilla-aurora?
Attachment #8779660 - Flags: approval-mozilla-aurora+
This change doesn't fix
https://webcompat.com/issues/2992 or Bug 1294171 
in Nightly 51.0a1 (2016-08-17) and DevEdition 50.0a2 (2016-08-17)
 
Chris, is the Yahoo issue resolved?  We can reopen and track this bug if we need to, but it sounds like maybe the issues left to resolve could be tracked in bug 1294171.
Flags: needinfo?(cpeterson)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32)
>  
> Chris, is the Yahoo issue resolved?  We can reopen and track this bug if we
> need to, but it sounds like maybe the issues left to resolve could be
> tracked in bug 1294171.

The Yahoo bug is fixed. The other broken sites should be tracked in other bugs. It sounds like they may require additional bug fixes.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox51: affected → verified
Flags: needinfo?(cpeterson)
Version: unspecified → 49 Branch
See Also: → 1361180
Clearing obsolete needinfo
Flags: needinfo?(mconnor)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: