Vaclav Vodicka Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

After opening our website http://www.ibobr.cz/url-otazka/?type=full&determine=317458148063897458921127145799&code=42746374636183879445 in Firefox 49 the flash object is not properly positioned The same situation is with many of our flash objects.


Actual results:

It seems that the flash salign attribute is ignored and the flash object is centered in the area (the center of the object is in the center of the area). See file "ibobr-firefox49.png"


Expected results:

According to the salign attribute, the flash object should be aligned to the left top corner (the top left corner of the object should be in the top left corner of the area). We know that the flash object is bigger than the area, but it should not be the problem - but in Firefox 48 or later as well as in current version of Coogle Chrome the alignment works properly. See file "ibobr-chrome53.png"

	Alice0775 White
	Comment 1 • 8 years ago

[Tracking Requested - why for this release]: breaks website layout


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b6174c5855f49d9ea91c588165f4728363ff5866&tochange=1361e4b73c4b3d01e421669c0a8cad6e25c45bf4

Regressed by:
1361e4b73c4b	Kyle Machulis — Bug 1287588 - Fix parameter ordering in embed tags; r=bsmedberg

	Marcia Knous [:marcia]
	Comment 2 • 8 years ago

Tracking 52+ for this layout issue.

	Benjamin Smedberg
	Comment 3 • 8 years ago

I'm going to be really ashamed if my suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=1287588#c6 somehow caused this.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 4 • 8 years ago

Not sure what's up here, as the test case we had in bug 1287588 still works? Big difference is this flash file is being passed in via swfobject instead of using a static embed tag. Maybe that's causing some sort of weirdness or code path execution that I missed?

Will research more and figure it out.

	Andrew Overholt [:overholt]
	Comment 5 • 8 years ago

Have you had a chance to investigate more, Kyle?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 6 • 8 years ago

Yeah, I've been poking at it, still trying to get a solid repro that's not the website in Comment 0. I backed up to 48, and sure enough, things work there. The argument reversal in bug 1287588 /should/ make everything happen in the same order in 48 and 49, but this bug says otherwise, so I'm trying to trace down where that's happening.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 7 • 8 years ago

Ok, so our problem is that we're doing this reversal too late in the call chain. We wanted to make sure we only reversed the work that was done by bug 1262470, which changed the order of attributes coming out of the HTML parser. However, this is reversing the order of ALL plugin attributes, regardless of if they came from the HTML parser or were generated on the fly by javascript. That's why this only breaks in SWFObject instances.

This is where things get complicated. At the point we have the attribute reversal right now, we don't know who our plugin instance owner is. So we need to find a place where we can both detect that our owner is a dynamically created node, AND that our plugin instance will be flash, and do the reversal there. Working my way through the call stack now to figure out where that would be done.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 8 • 8 years ago

Ok, this ended up being way easier than planned, just took more code reading. Turns out there was already a manual reversing of parameters from the HTML parser when building the parameter array in nsObjectLoadingContent. Bug 1287588 was then reversing THAT reversal, except for both parser and dynamically generated nodes. Removing the old and new code seems to make both things work fine.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 9 • 8 years ago

Helps if I attach the right patch.

	Benjamin Smedberg
	Comment 10 • 8 years ago

Comment on attachment 8800895 [details] [diff] [review]
Patch 1 (v2) - Remove attribute reversal for plugins

Do we need another test for this specific behavior?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 11 • 8 years ago

Ugh, yeah, we probably should have more tests even though we're trying to spin this down, since that's gonna take a while. I'll try to get some written early this week and land everything together.

	Pulsebot
	Comment 14 • 8 years ago

Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c6abc7afed
Remove reversal of attributes for plugins; r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f18ae4d64f7
Mochitests for plugin parameter ordering; r=bsmedberg

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE) Assignee
	Comment 15 • 8 years ago

Comment on attachment 8800895 [details] [diff] [review]
Patch 1 (v2) - Remove attribute reversal for plugins

Approval Request Comment
[Feature/regressing bug #]: Bug 1287588
[User impact if declined]: Some websites using swfobject to display flash will display incorrectly
[Describe test coverage new/current, TreeHerder]: mochitest included
[Risks and why]: This patch is all code removal, and should restore how things used to be in <= FF48. Hopefully little to no risk involved.
[String/UUID change made/needed]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/63c6abc7afed
https://hg.mozilla.org/mozilla-central/rev/6f18ae4d64f7

	Kohei Yoshino
	Comment 17 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/flash-objects-are-not-positioned-properly-according-to-salign-attribute/

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 8 years ago

Hello Vaclav, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 8 years ago

Comment on attachment 8800895 [details] [diff] [review]
Patch 1 (v2) - Remove attribute reversal for plugins

This is mostly a patch backout to revert things back to the good state (48), Aurora51+, Beta50+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 8 years ago

Comment on attachment 8801976 [details] [diff] [review]
Patch 2 (v1) - Mochitest for static/dynamic embed parameter ordering

Test only changes don't need relman review.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff645fb30194
https://hg.mozilla.org/releases/mozilla-aurora/rev/34354988012e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/35de3a532fe1
https://hg.mozilla.org/releases/mozilla-beta/rev/91e9715a7065

	Vaclav Vodicka Reporter
	Comment 23 • 8 years ago

Hello Ritu,
I am sorry that I have not checked my secondary email for ages :(
Anyway, I have just verified the issue and it works very well. Thank you for your work!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 8 years ago

(In reply to Vaclav Vodicka from comment #23)
> Hello Ritu,
> I am sorry that I have not checked my secondary email for ages :(
> Anyway, I have just verified the issue and it works very well. Thank you for
> your work!

Great! Thanks.