1016944 - Events are processed twice in BrowserElementPanning.js for nested in-process mozbrowser iframe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just run into that while working on bug 1005815.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.deepest.iframes.for.mozbrowser.patch

Fabrice, this patch ensure that the events are processed by the correct BrowserElementPanning.js script related to the event target. Otherwise events are processed by the embedder BEP.js script as well as the one from the embedded content.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8430014 [details] [diff] [review]
use.deepest.iframes.for.mozbrowser.patch

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

::: dom/browser-element/BrowserElementPanning.js
@@ +90,5 @@
>        return;
>      }
>  
> +    // For nested in-process <iframe mozbrowser> the events are handled
> +    // by the deepest <iframe mozbrowser>.

This comment is not super clear. We could think that the <iframe mozbrowsre> case is already dealt with by the previous test.

@@ +92,5 @@
>  
> +    // For nested in-process <iframe mozbrowser> the events are handled
> +    // by the deepest <iframe mozbrowser>.
> +    var targetWindow = evt.target.ownerDocument.defaultView;
> +    var frameElement = targetWindow.frameElement;

s/var/let

@@ +96,5 @@
> +    var frameElement = targetWindow.frameElement;
> +    while (frameElement) {
> +      targetWindow = frameElement.ownerDocument.defaultView;
> +      frameElement = targetWindow.frameElement;
> +    }

which guarantee do we have to not loop forever?
are we sure that we never have frameElement.ownerDocument.defaultView.frameElement == frameElement ?

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.deepest.iframes.for.mozbrowser.patch

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Comment on attachment 8430014 [details] [diff] [review]
> use.deepest.iframes.for.mozbrowser.patch
> 
> Review of attachment 8430014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +96,5 @@
> > +    var frameElement = targetWindow.frameElement;
> > +    while (frameElement) {
> > +      targetWindow = frameElement.ownerDocument.defaultView;
> > +      frameElement = targetWindow.frameElement;
> > +    }
> 
> which guarantee do we have to not loop forever?
> are we sure that we never have
> frameElement.ownerDocument.defaultView.frameElement == frameElement ?

The spec at https://developer.mozilla.org/fr/docs/Web/API/window.frameElement says that window.frameElement === null if the element is top level.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3f42130258

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e3f42130258