530835 - Flash: Chrome UI shows up on background of flash videos

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b4pre) Gecko/20091124 Firefox/3.6b4pre Fennec/1.0b6pre

Steps to Reproduce:
1. Go to any flash site and play a video
2. Click on the favicon to open the identity panel.
3. Click on the bookmarks star and click on "edit"

Actual Results:
http://www.flickr.com/photos/42893104@N04/4130864331/

Expected Results:
Chrome elements (edit bookmarks dialog and identity panel) show up behind flash videos

Comment 2

[Madhava Enros [:madhava]]

(examples from the duped bug)

Example here: http://www.flickr.com/photos/madhava_work/4131129147/

The flash ad here seems to be layered on top of the bookmark editing panel.

Actually, this seems to happen for the identity panel:
http://www.flickr.com/photos/madhava_work/4131154227/

And the bookmarking popup as well:
http://www.flickr.com/photos/madhava_work/4131916778/in/photostream/

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Patch

I tried to solve this as generally as possible for popups.  This patch calculates the area difference between the critical region and the popup rects.  The result is always a union of rects.  If it's one rect, the popup area is clipped.  If the region is more complex, rendering is stopped.  A padding hack is currently needed so that flash doesn't "leak" into the shadows of the dialogs.

The setTimeout trick is now smarter.  Rendering is now always immediate if rendering previously.  This way, when critical region is reduced flash doesn't accidentally draw on top.

pushPopup and popPopup now fire an event.

I did a refactoring of browser-ui.js--take it or leave it.  I split "updatePopup" into two functions because the name is not only ambiguous, but deceiving.  "_hidePopup" now always hides the current popup, and "_isEventInsidePopup" does what it says on the tin.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 415464 [details] [diff] [review]
Patch

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js

>-  setBounds: function(t, l, b, r) {
>+  setBounds: function(l, t, r, b) {

CSS's t,r,b,l is more familiar to me. I suppose we'll probably have to look it up all the time either way, so maybe doesn't matter. Could have it take an object with the right properties, but perhaps that's too verbose for callers?

>+  /** Subtract other area from this. Returns array of rects whose union is this-other. */
>+  subtract: function(other) {

can we get tests for this added to tests/browser_rect.js ?

>+    // left strip
>+    r.setBounds(this.left, this.top, other.left, this.bottom);
>+    if (!r.isEmpty()) result.push(r.clone());

put the result.push()es on a new line? makes things much easier to read IMO.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>@@ -926,17 +938,18 @@ var BookmarkHelper = {

>-    Util.executeSoon(function() { self._editor.startEditing(); });
>+    Browser.forceChromeReflow();
>+    self._editor.startEditing();

Is this related to the fix, or just independent cleanup?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

> PluginObserver.prototype = {
>+  // When calculating critical rect, subtract N pixels around popup boxes
>+  POPUP_PADDING: 4,

Where does this number come from? Related to CSS, or just derived experimentally?

>   start: function() {

>+    let stack = document.getElementById("stack");

could add this to Elements: http://mxr.mozilla.org/mobile-browser/source/chrome/content/Util.js#179

>@@ -3059,18 +3070,42 @@ PluginObserver.prototype = {

>+    let popup = BrowserUI._popup;
>+    if (popup) {
>+      let p = this.POPUP_PADDING;
>+      let elements = BrowserUI._popup.elements;
>+      for (let i = elements.length - 1; i >= 0; i--) {

This is a bit strange... we sometimes include unrelated elements in the "elements" array just to determine click behavior (e.g. the star button, see BookmarkPopup.show()). We don't really want those to be included in these calculations. In this specific case the star button can't appear over content, so doesn't matter in practice, but maybe we should eventually have a way to get to just the actual "box" element to avoid having to do any work for other stuff.

>+        popupRect.setBounds(popupRect.left - p, popupRect.top - p, popupRect.right + p, popupRect.bottom + p);

hmm, thought we had an expandBy, but I guess that got lost in one of the Rect rewrites.

Comment 6

[Benjamin Stover (:stechz)]

> CSS's t,r,b,l is more familiar to me. I suppose we'll probably have to look it
> up all the time either way, so maybe doesn't matter. Could have it take an
> object with the right properties, but perhaps that's too verbose for callers?

Yes, might as well set all the properties manually as it will end up taking four lines.  (Actually if you wanted to use object behavior you could use setRect.)

I think my default is to leave this alone.  I wouldn't be offended if we removed setBounds since the order is so ambiguous.

> can we get tests for this added to tests/browser_rect.js ?

Yes.

> >-    Util.executeSoon(function() { self._editor.startEditing(); });
> >+    Browser.forceChromeReflow();
> >+    self._editor.startEditing();
> Is this related to the fix, or just independent cleanup?

Related.  Edit bookmarks popup height would sometimes be incorrect when even was called.

> Where does this number come from? Related to CSS, or just derived
> experimentally?

Experimentally.

> This is a bit strange... we sometimes include unrelated elements in the
> "elements" array just to determine click behavior (e.g. the star button, see
> BookmarkPopup.show()). We don't really want those to be included in these
> calculations. In this specific case the star button can't appear over content,
> so doesn't matter in practice, but maybe we should eventually have a way to get
> to just the actual "box" element to avoid having to do any work for other
> stuff.

We could separate the box element out from the other elements.  Alternatively, what about a class like "keep-popup" that would inform a mouseclick that the popup should stay open for this element?  That way, only the box element needs to be passed in.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Try using keep-popup class

Code review changes, and the suggestion I made in the previous comment.

This simplifies push popup by only taking one element instead of an array.  Buttons now can just add the class "keep-popup" so that the current popup doesn't hide.  Slightly less powerful than before, but I couldn't think of a use case where this doesn't do the trick.

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Revert keep-class

keep-popup was not favorable, Gavin was worried about extensions already using it this way.

Comment 9

[Benjamin Stover (:stechz)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/339f988d5d9b

Comment 11

[Aakash Desai [:aakashd]]

I haven't seen this in a month or so, so verified FIXED on trunk and 1.9.2.