Closed Bug 1287588 Opened 3 years ago Closed 3 years ago

Flash content is cropped in Firefox 49 depending on the order of <embed> scale and salign attributes

Categories

(Core :: Plug-ins, defect, P1)

49 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 + fixed
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: cpeterson, Assigned: qdot)

References

()

Details

(Keywords: flashplayer, regression, Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge])

Attachments

(2 files, 2 obsolete files)

[Tracking Requested - why for this release]:

This is a regression in FF 49, on both Windows and OS X. I bisected this regression to bug 1264270.

STR:
1. Load http://thefederalist.com/2016/07/17/ben-domenech-the-gop-is-going-to-cleveland-to-die/
2. Scroll down to the two CBS News Flash videos.

RESULT:
Flash videos are offset (down and to the left) and cropped in Firefox.

Based on the comments in bug 1264270 and bug 1271439 and that this doesn't appear to affect Flash videos on other websites, this bug might be WONTFIX.
Do you know why this is happening? Is it because we changed the ordering of plugin parameters passed through NPAPI? Or does this have something to do with the DOM/CSS itself?
Flags: needinfo?(cpeterson)
Attached file test.html
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Do you know why this is happening? Is it because we changed the ordering of
> plugin parameters passed through NPAPI? Or does this have something to do
> with the DOM/CSS itself?

I think this is a regression in Firefox, not a bug in Flash or the video player SWF, because Safari and Firefox behave differently even though (AFAIK) they both use the same NPAPI Flash plugin. Note that IE11/Edge has different behavior than Chrome, Safari, and Firefox.

Here is a minimal test case that does not use the CBS video player. Moving the <embed> tag's salign=lt attribute before scale=noscale fixes the SWF position.

Compare the position (IN viewport or OUT of viewport) of this test SWF's "v. 1.5" version number in the upper-left corner of these <embeds>:

* Firefox <= 48: #1 is IN. #2 is OUT.
* Firefox >= 49: #1 is OUT. #2 is IN.
* Chrome: #1 is IN. #2 is OUT.
* Safari: #1 is IN. #2 is OUT.
* IE11/Edge: both #1 and #2 are IN!
Flags: needinfo?(cpeterson)
Summary: Flash videos on thefederalist.com are offset and cropped in Firefox 49 → Flash content is cropped in Firefox 49 depending on the order of <embed> scale and salign attributes
Whiteboard: [parity-chrome][parity-safari][parity-ie][parity-edge]
Kyle, seems we need to reverse the order of attributes passed to the plugin here, to revert back to passing them in the order we used to pass them in before the fix for bug 1264270 landed. Can you look into that?
Assignee: nobody → kyle
I'll take a look.
This patch makes sure that scale always comes before salign in parameter ordering. Seems to fix cpeterson's test.
Attachment #8779939 - Flags: review?(benjamin)
Comment on attachment 8779939 [details] [diff] [review]
Patch 1 (v1) - Fix scale/salign ordering in flash embed tags

Are you sure that this is better than simply giving Flash all the attributes in the order it has always received them? In terms of risk management, we don't know what other code may depend on ordering and be subtly broken.

I'm going to mark r+ on this in any case, but I'd strongly suggesting doing strong backwards-compat. And then documenting the behavior in npapi.h on github.
Attachment #8779939 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg [:bsmedberg] from comment #6)
> Comment on attachment 8779939 [details] [diff] [review]
> Patch 1 (v1) - Fix scale/salign ordering in flash embed tags
> 
> Are you sure that this is better than simply giving Flash all the attributes
> in the order it has always received them? In terms of risk management, we
> don't know what other code may depend on ordering and be subtly broken.

Yeah, that's a good point. I tend to default to scalpel versus shotgun when given the choice, but in this case it makes sense. I'll submit a new patch today.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #7)
> Yeah, that's a good point. I tend to default to scalpel versus shotgun when
> given the choice, but in this case it makes sense. I'll submit a new patch
> today.

Even better, I just realized my current patch puts us in the same situation as Edge, where scale and salign are ordered in ALL cases, and not back to the FF48 case we want to be in. Oops.
Swapping order on the whole array this time, but BEFORE we add anything to it. Outputs same as FF48 now, this should give us what we used to have in all cases.
Attachment #8779939 - Attachment is obsolete: true
Attachment #8780330 - Flags: review?(benjamin)
Tracking since this looks like a recent regression (from 49)  Let me know once it lands if you think it is worth the risk to uplift to beta. We are heading in to beta 5 on Thursday.
Comment on attachment 8780330 [details] [diff] [review]
Patch 1 (v2) - Fix scale/salign ordering in flash embed tags

This loops feels a little dirty: I so much want an iterator of some sort. In particular, relying on integer division to truncate to get the correct behavior is unintuitive. Is there a more readable way of writing this?

Going to mark r+ because this does the right thing and could land this way if there's not a more readable way.
Attachment #8780330 - Flags: review?(benjamin) → review+
Priority: -- → P1
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1361e4b73c4b
Fix parameter ordering in embed tags; r=bsmedberg
Oops, I was still following the old "no std" rule. Should've just done a dxr to see if std::reverse was already in the codebase (which it is, in a few places). Ah well. 

Patch now much cleaner, landed on inbound.
Attachment #8780330 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1361e4b73c4b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Kyle, should we uplift the fix to Aurora50, Beta49 given that this is a P1 regression?
Flags: needinfo?(kyle)
Comment on attachment 8782540 [details] [diff] [review]
Patch 1 (v3) - Fix scale/salign ordering in flash embed tags; r=bsmedberg

Approval Request Comment
[Feature/regressing bug #]: bug 1264270
[User impact if declined]: Some flash embeds will show up shifted
[Describe test coverage new/current, TreeHerder]: Hand tested (could be tested in testplugin but not sure it's worth the development time since this is getting phased out), passes mochis otherwise
[Risks and why]: None
[String/UUID change made/needed]: None
Flags: needinfo?(kyle)
Attachment #8782540 - Flags: approval-mozilla-beta?
Attachment #8782540 - Flags: approval-mozilla-aurora?
Hi Chris, could you please verify this fix works as expected on a latest Nightly build? Thanks!
Flags: needinfo?(cpeterson)
Comment on attachment 8782540 [details] [diff] [review]
Patch 1 (v3) - Fix scale/salign ordering in flash embed tags; r=bsmedberg

Fixes a P1 regression, stabilized on Nightly for 2 days, Aurora50+
Attachment #8782540 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed in Nightly 50 (2016-08-22) and still broken in Aurora 50.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cpeterson)
Comment on attachment 8782540 [details] [diff] [review]
Patch 1 (v3) - Fix scale/salign ordering in flash embed tags; r=bsmedberg

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

This patch fixes a P1 regression and is also verified. Take it in 49 beta. Should be in 49 beta 7.
Attachment #8782540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1307694
Version: unspecified → 49 Branch
You need to log in before you can comment on or make changes to this bug.