walker should support mozbrowser iframes

RESOLVED FIXED in Firefox 26

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: kgrandon, Assigned: paul)

Tracking

unspecified
Firefox 26
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Reporter

Description

6 years ago
This is working in the currently released Firefox, but broken in Firefox Nightly.

When I open gaia inside of nightly and launch an App, it's difficult to inspect those elements because the inspector will not jump to the right element.

My guess is that this might be somehow related to mozbrowser/mozapp iframes.
Assignee

Comment 1

6 years ago
Could it be your fault?

(iframes don't expand in the markupview)
Flags: needinfo?(dcamp)
Assignee

Comment 2

6 years ago
I meant:

Dcamp, could it be your fault?

(iframes don't expand in the markupview)
Assignee

Comment 3

6 years ago
Kevin, Rik, how do you select the element? Right-click -> Inspect?
It doesn't matter, it fails with both methods: "right-click > inspect" or "activate select element with mouse > click"
Could you attach a screenshot?
Assignee

Comment 6

6 years ago
Anthony just showed me:
- gaia with mozbrowser iframes
- iframe don't expand in the markup view
- breadcrumbs are correctly displayed with the right selection
- it works in Firefox 24
Does this only happen for mozbrowser iframes?  Do normal iframes work as expected?
Flags: needinfo?(paul)
Normal iframes work as expected.
Flags: needinfo?(paul)
Assignee

Comment 9

6 years ago
While debugging apps, iframes are empty.
Assignee

Comment 10

6 years ago
What would help a lot is a reduced test case.
Kevin: Do you have any idea if we're doing something special to insert those iframes that could trip up the devtools?
Flags: needinfo?(kgrandon)
Assignee

Comment 12

6 years ago
mozbrowser iframes host a window that runs in a different process. We don't support that yet.
Assignee

Updated

6 years ago
Summary: Inspector does not jump to correct element in nightly → walker should support mozbrowser iframes (e10s)
Assignee

Comment 13

6 years ago
actually, afair, what Anthony showed me was running in a regular Firefox desktop (no multiprocess). So i'm not sure about the e10s part.
Summary: walker should support mozbrowser iframes (e10s) → walker should support mozbrowser iframes
Assignee

Updated

6 years ago
Blocks: 889889
Assignee

Comment 14

6 years ago
it appears to work when I use the markup view. It might be at the highlighter level.
Assignee

Comment 15

6 years ago
In markup-view.js, in _ensureVisible, in the while loop, we get this exception:

> TypeError: value is not a non-null object

Because parentNode() return null:

>      let parent = node.parentNode();
>      if (!container.elt.parentNode) {
>        let parentContainer = this._containers.get(parent);

Where node is the <html> tag.
Assignee

Comment 16

6 years ago
I figured it out. Patch coming.
Assignee

Updated

6 years ago
Duplicate of this bug: 889889
Assignee

Comment 18

6 years ago
Here is the issue:

In Gaia code, we use mozbrowser iframes: <iframe mozbrowser>.

These iframes work in a self contained way. That means we can do `window.parent` and `window.frameElement`.

The solution is to use `nsIDocShell.getSameTypeParentIgnoreBrowserAndAppBoundaries()`.

From this, we can get the parent window, but then, 2 problems:
1. it doesn't solve the window.frameElement issue
2. we don't know when to stop going up in the window hierarchy. We don't stop at the tab level, but at the very top window level (browser.xul in Firefox Desktop, and shell.xul in B2G)

The coming patch solves these 2 issues this way:

For 1, we just get the parent window, go through all the iframes, if one has a contentWindow that matches the window, we found the iframe. There's probably a better way to do that, but not sure how.

For 2, we could do some magic trick and guess when to stop (if docshell types are different, if we end-up in a chrome window, ...), but it doesn't sound safe, and I can think of cases where we want to stop earlier (chrome window in a tab). So I introduce a "top window" variable, that says when to stop. This top window has to be defined every time we want to use something similar to window.frameElement and window.parent. Which is essentially in toolkit/LayoutHelpers.jsm and toolkit/inspector.js.
Assignee

Updated

6 years ago
Flags: needinfo?(kgrandon)
Flags: needinfo?(dcamp)
Assignee

Comment 19

6 years ago
Removed irrelevant code (prettyKey) (moved to another file in another patch).
LayoutHelpers is now a class.
One instance of LayoutHelpers is bound by a top level window.
Implement getFrameElement, getParentWindow and isTopLevelWindow.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #801258 - Flags: review?(dcamp)
Assignee

Comment 20

6 years ago
We used LayoutHelpers.jsm to put some random helpers in it. Let's get a dedicated file and move there the not-related methods.
Attachment #801259 - Flags: review?(dcamp)
Assignee

Comment 22

6 years ago
Attachment #801261 - Flags: review?(dcamp)
Assignee

Comment 23

6 years ago
This also solves bug 889889.
Assignee

Comment 24

6 years ago
I'm working on a test (not sure how I'll test that...).
Assignee

Updated

6 years ago
Blocks: appmgr_v1

Updated

6 years ago
Attachment #801258 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #801259 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #801260 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #801261 - Flags: review?(dcamp) → review+
I'm r+'ing before the tests are written because I want to get testing from the Gaia work week.

But we should still get a test.
Assignee

Comment 28

6 years ago
orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=27562069&tree=Fx-Team

Silly me, testing (bc) instead of (oth).
Assignee

Comment 30

6 years ago
Attachment #801405 - Flags: review?(paul)
Assignee

Updated

6 years ago
Attachment #801405 - Attachment description: Patch D. Fix more tests → Patch E. Fix more tests
Attachment #801405 - Flags: review?(paul) → review+
Assignee

Updated

6 years ago
Whiteboard: [land-in-fx-team]
Assignee

Updated

6 years ago
Whiteboard: [land-in-fx-team]
Reporter

Comment 34

6 years ago
This is a huge help for us, thank you!
For future reference, it's better if the commit message actually explains what the patch does.

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.