Flash: Chrome UI shows up on background of flash videos

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
--
blocker
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: aakashd, Assigned: stechz)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Duplicate of this bug: 530874
(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/

Updated

9 years ago
Assignee: nobody → webapps
(Assignee)

Comment 3

9 years ago
Created attachment 415464 [details] [diff] [review]
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.
Attachment #415464 - Flags: review?(gavin.sharp)

Updated

9 years ago
tracking-fennec: ? → 1.0+
(Reporter)

Updated

9 years ago
Duplicate of this bug: 532181
Attachment #415464 - Flags: review?(gavin.sharp) → review+
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.
(Assignee)

Comment 6

9 years ago
> 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.
(Assignee)

Comment 7

9 years ago
Created attachment 416120 [details] [diff] [review]
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.
Attachment #415464 - Attachment is obsolete: true
Attachment #416120 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

9 years ago
Created attachment 416138 [details] [diff] [review]
Revert keep-class

keep-popup was not favorable, Gavin was worried about extensions already using it this way.
Attachment #416120 - Attachment is obsolete: true
Attachment #416120 - Flags: review?(gavin.sharp)
(Assignee)

Comment 9

9 years ago
Pushed: http://hg.mozilla.org/mobile-browser/rev/339f988d5d9b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Updated

9 years ago
Duplicate of this bug: 534574
(Reporter)

Comment 11

9 years ago
I haven't seen this in a month or so, so verified FIXED on trunk and 1.9.2.
Status: RESOLVED → VERIFIED

Updated

8 years ago
Component: Linux/Maemo → General
OS: All → Linux (embedded)
QA Contact: maemo-linux → general
Hardware: All → ARM
You need to log in before you can comment on or make changes to this bug.