Closed Bug 968762 Opened 6 years ago Closed 6 years ago

Plugin overlays are not displayed if plugin element is not fully in scroll view

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox28 --- unaffected
firefox29 - verified
firefox30 --- verified

People

(Reporter: pauly, Assigned: gfritzsche)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Actually this is a regression of bug 853973.
Is this expected?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
Priority: -- → P3
Duplicate of this bug: 951414
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.
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.
This is happening with the CTP overlay as well.
Summary: Missing plugin overlay is not displayed on FF 29 and up → Plugin overlays are not displayed if plugin element is not fully in scroll view
(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
I don't think it's expected to have completely different behaviour depending on initial scroll position or view area.
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?
Flags: needinfo?(benjamin)
OS: Windows 7 → All
Priority: P3 → P2
Hardware: x86_64 → All
Blocks: 853973
Duplicate of this bug: 974996
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?
Flags: needinfo?(benjamin) → needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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
Assignee: nobody → georg.fritzsche
Attachment #8379730 - Flags: feedback?(benjamin)
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.
Attachment #8379730 - Flags: feedback?(benjamin)
Are we sure we want to skip flushing layout?  Are we responding to a user click, so layout is already what it is?
We are not responding to clicks and probably should be flushing layout.
Where and how should we force a layout flush here?
You're passing false to nsIDOMWindowUtils.elementFromPoint(aFlushLayout) and should be passing true.
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.
Attachment #8380689 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/6d1aabc16a1b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Attachment #8380689 - Flags: approval-mozilla-aurora?
Attachment #8380689 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 977543
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.
Status: RESOLVED → VERIFIED
Verified as fixed on latest Aurora (build ID: 20140304004003) on Windows 7 64bit, Mac OS X 10.9 and Ubuntu 13.04 32 bit.
You need to log in before you can comment on or make changes to this bug.