Paul Silaghi, QA [:pauly] Reporter
	Description • 11 years ago

STR:
1. Remove Flash
2. Start Firefox
3. Open a page that uses Flash (ie. http://wickedgoodgames.com/flash9/bongoboom.html)

AR: Missing plugin overlay is not displayed

Repro on FF 29.0a2 (2014-02-05), 30.0a1 (2014-02-05)
Not repro on FF 27, 28b1.
Repro on Win, Linux.
Not repro on Mac OS X.

	Paul Silaghi, QA [:pauly] Reporter
	Comment 1 • 11 years ago

Actually this is a regression of bug 853973.
Is this expected?

	Benjamin Smedberg
	Comment 2 • 11 years ago

No. I tried to make a testcase for this at bug 968762 but it works. I'm looking at another possibility now. But this is not high priority.

	Benjamin Smedberg
	Comment 4 • 11 years ago

Is the scrollbar covering the content, as in bug 951414? If that's the case then this is by design and WONTFIX. But I didn't see that when testing.

	Elbart
	Comment 5 • 11 years ago

It has nothing to do with the scrollbar. I just used it to partially "hide" the embed in my example.

The same thing is happening in this bug's URL, though.
Depending on the vertical size of the browser-window, the site's content at the top could be large enough to position the lower part of the embed just below the initially visible area, as seen on the right side in the screenshot.
This is unnecessarily causing the clickable overlay to be hidden and the notification-bar to be shown.
A measure, which, if I'm understanding bug 853973 and related correctly, should only be happening when the embed and the overlay are otherwise not clickable at all.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 6 • 11 years ago

This is happening with the CTP overlay as well.

	Paul Silaghi, QA [:pauly] Reporter
	Comment 7 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> This is happening with the CTP overlay as well.
With the CTP, it's expected I think, due to https://bugzilla.mozilla.org/show_bug.cgi?id=853973#c15

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 11 years ago

I don't think it's expected to have completely different behaviour depending on initial scroll position or view area.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 9 • 11 years ago

So, the problem is the elementFromPoint() approach here:
http://hg.mozilla.org//mozilla-central/annotate/bb030d47c946/browser/base/content/browser-plugins.js#l138

When one of the tested point on the element is outside of the initially visible scroll area/view (not sure about the terminology here), this fails.
bsmedberg, do you happen to know of an alternative to elementFromPoint() here?

	Benjamin Smedberg
	Comment 11 • 11 years ago

It's clear that I introduced a bug: from https://developer.mozilla.org/en-US/docs/Web/API/Element.getBoundingClientRect

"The amount of scrolling that has been done of the viewport area (or any other scrollable element) is taken into account when computing the bounding rectangle. This means that the top and left property change their values as soon as the scrolling position changes"

What I don't know is whether simply adding the .scrollX/.scrollY offsets will fix the problem, or whether .elementFromPoint actually doesn't return the correct results for points that aren't currently on screen. bz, do you know if there are .elementFromPoint limits in this regard?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

elementFromPoint will return null if given a point that's not in the viewport...  See http://dev.w3.org/csswg/cssom-view/#dom-document-elementfrompoint step 1.

If this script is privileged, then nsIDOMWindowUtils.elementFromPoint might work better: it lets you specify whether points outside the viewport should return null or the actual element at the point.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 11 years ago

nsIDOMWindowUtils.elementFromPoint() works well in my local testing.
bsmedberg, do you see any other potential problems with this?

https://tbpl.mozilla.org/?tree=Try&rev=a0fe966884f7

	Benjamin Smedberg
	Comment 14 • 11 years ago

Comment on attachment 8379730 [details] [diff] [review]
Switch to using nsIDOMWindowUtils.elementFromPoint

The test seems a little contrived: couldn't we just statically have a <div style="height: 100%"> and then have the plugin element below that in order to test this without all the scripting?

Also we should add a test that plugin inside iframes are correctly marked as visible in this case.

The actual code change seems fine.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

Are we sure we want to skip flushing layout?  Are we responding to a user click, so layout is already what it is?

	Benjamin Smedberg
	Comment 16 • 11 years ago

We are not responding to clicks and probably should be flushing layout.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 17 • 11 years ago

Where and how should we force a layout flush here?

	Benjamin Smedberg
	Comment 18 • 11 years ago

You're passing false to nsIDOMWindowUtils.elementFromPoint(aFlushLayout) and should be passing true.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 19 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=f4364241b2b7

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 11 years ago

Comment on attachment 8380689 [details] [diff] [review]
Switch to using nsIDOMWindowUtils.elementFromPoint

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

::: browser/base/content/test/general/plugin_outsideScrollArea.html
@@ +19,5 @@
> +}
> +</style>
> +</head>
> +<body>
> +  <div id="container" style="position:fixed; top:0; bottom: 0; height:100%; width: 100%; background:red;"></div>

You can remove this style attribute.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 21 • 11 years ago

Ah, thanks for catching that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1aabc16a1

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6d1aabc16a1b

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 23 • 11 years ago

Comment on attachment 8380689 [details] [diff] [review]
Switch to using nsIDOMWindowUtils.elementFromPoint

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 853973.
User impact if declined: Plugin overlays are hidden when the plugin is partially or fully outside of the scroll view area.
Testing completed (on m-c, etc.): Test coverage, now baking on m-c.
Risk to taking this patch (and alternatives if risky): Low-risk, this is just changing to a different visibility check that is not bound to the scroll view.
String or IDL/UUID changes made by this patch: None.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 24 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/de2794d8eeec

	Paul Silaghi, QA [:pauly] Reporter
	Comment 25 • 11 years ago

The missing plugin overlay and the scenario in bug 951414 work fine now.
Verified fixed in nightly 30.0a1(2014-02-27) Win 7, Mac OS X 10.8.5, Ubuntu 13.04.
Further work continues in bug 977543.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 26 • 11 years ago

Verified as fixed on latest Aurora (build ID: 20140304004003) on Windows 7 64bit, Mac OS X 10.9 and Ubuntu 13.04 32 bit.