[e10s] Get responsive design mode working in e10s

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: trishul.goel, Assigned: paul)

Tracking

(Blocks 1 bug)

35 Branch
Firefox 38
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s-m6])

Attachments

(1 attachment, 9 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140913030206

Steps to reproduce:

Entered responsive mode (Ctrl + shift + M)  
Pressed the close button


Actual results:

Responsive design did not close 


Expected results:

Should switch back to normal mode

Comment 1

5 years ago
In fact, I feel they can be integrated into a bug.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Responsive Mode
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
See Also: → 1067114
Summary: unable to quit responsive design → [e10s] unable to quit responsive design

Comment 2

5 years ago
I'm seeing something similar:

1. Enable e10s 
2. View a site with Responsive Design View
3. Press the little 'x' in the upper left to leave Responsive Design View - nothing happens.
4. Disable e10s and the little 'x' works.
There's an error thrown from gcli: "this.browser.contentWindow is null"
Paul, I know you had this working at the meetup a couple weeks ago. Any way you can upload a patch for it? We can worry about enabling the tests later.

Just having this able to close is a big deal, as right now you have to close the whole tab to get out of responsive mode with e10s enabled.
Flags: needinfo?(paul)
Assignee

Comment 5

5 years ago
Coming soon. A couple of other bugs to fix first.
Flags: needinfo?(paul)
Duplicate of this bug: 1067038
Duplicate of this bug: 1068916
Relatedly, clicking “Responsive Design Mode” button (or ^M; not ^m) toggles the responsive design view. In e10s additional responsive design toolbars will appear instead of closing the already enabled mode.

Comment 9

5 years ago
In last nightly (36.0a1 2014-10-21), an error is thrown in the browser console:

responsivedesign.jsm.js:186
TypeError: this.browser.contentWindow is null
Assignee

Comment 10

5 years ago
Posted patch wip (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Assignee

Comment 11

5 years ago
Posted patch wip (obsolete) — Splinter Review
Attachment #8508699 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 937169
Duplicate of this bug: 1081585
devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.
Whiteboard: [e10s-m6]
Posted patch Rebased WIP (obsolete) — Splinter Review
Rebased from previous patch. Seems to be working well: opening and closing works, screenshot button works, and touch events are disabled on e10s.

There's one test failing under regular mode: browser_responsiveuiaddcustompreset.js, and some tests disabled under e10s. My guess is that the only thing left to do is get the test passing. Is that right, Paul? I'm willing to check it out.
Attachment #8508945 - Attachment is obsolete: true
Flags: needinfo?(paul)
Assignee

Comment 15

5 years ago
Sorry for the long delay. Looking at this now.
Flags: needinfo?(paul)
Assignee

Comment 16

5 years ago
Posted patch v1 (obsolete) — Splinter Review
Alex, can you make sure I didn't break the mulet?
Attachment #8529505 - Attachment is obsolete: true
Attachment #8530262 - Flags: review?(poirot.alex)
Summary: [e10s] unable to quit responsive design → [e10s] Get responsive design mode working in e10s
Assignee

Updated

5 years ago
Duplicate of this bug: 1108533
Comment on attachment 8530262 [details] [diff] [review]
v1

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

Yes, mulet is broken!
In b2g/chrome/content/desktop.js, you should convert tab.__responsiveUI to ResponsiveUIManger.getResponsiveUIForTab(tab).
Same thing in b2g/chrome/content/screen.js.

There is still some failures in last try, in browser/devtools/responsivedesign/test/browser_responsiveui.js.

Otherwise the patch looks good, I made some random comments.

::: browser/devtools/responsivedesign/responsivedesign-child.jsm
@@ +17,5 @@
> +    return;
> +  }
> +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> +  addEventListener("load", makeScrollbarsFloating);

By listening to `load` you are also going to trigger makeScrollbarsFloating for iframes.
Either call makeScrollbarsFloating only for top level document load event,
or make it so that makeScrollbarsFloating only applies the stylesheet for the document which dispatched the load event.
(The second option looks better as the first one would miss applying to iframe created after top level document load event)

@@ +20,5 @@
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> +  addEventListener("load", makeScrollbarsFloating);
> +  docShell.deviceSizeIsPageSize = true;
> +  gRequiresFloatingScrollbars = data.requiresFloatingScrollbars;
> +  if (docShell.contentViewer) {

A comment about this condition could be helpful.

@@ +54,5 @@
> +    try {
> +      winUtils.removeSheet(gFloatingScrollbarsStylesheet, win.AGENT_SHEET);
> +    } catch(e) { }
> +  }
> +  flushStyle();

Could you move all the code related to floating scrollbars to a dedicated cleanup function?

@@ +91,5 @@
> +  let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +  let width = content.innerWidth;
> +  let height = content.innerHeight;
> +  canvas.mozOpaque = true;
> +  canvas.mozImageSmoothingEnabled = true;

Could you justify the usage of these properties in comments?

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +139,5 @@
> +  }
> +  this.mm.addMessageListener("ResponsiveMode:Start:Done", childOn);
> +
> +  let requiresFloatingScrollbars = !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;
> +  this.mm.loadFrameScript("resource:///modules/devtools/responsivedesign-child.jsm", true);

Frame scripts are named .js and not .jsm.
Attachment #8530262 - Flags: review?(poirot.alex)
Assignee

Comment 20

5 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Comment on attachment 8530262 [details] [diff] [review]
> v1
> 
> Review of attachment 8530262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, mulet is broken!
> In b2g/chrome/content/desktop.js, you should convert tab.__responsiveUI to
> ResponsiveUIManger.getResponsiveUIForTab(tab).
> Same thing in b2g/chrome/content/screen.js.

Ok.

> There is still some failures in last try, in
> browser/devtools/responsivedesign/test/browser_responsiveui.js.

Fixed: https://tbpl.mozilla.org/?tree=Try&rev=3190dc10d1fd

> Otherwise the patch looks good, I made some random comments.
> 
> ::: browser/devtools/responsivedesign/responsivedesign-child.jsm
> @@ +17,5 @@
> > +    return;
> > +  }
> > +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > +  addEventListener("load", makeScrollbarsFloating);
> 
> By listening to `load` you are also going to trigger makeScrollbarsFloating
> for iframes.
> Either call makeScrollbarsFloating only for top level document load event,
> or make it so that makeScrollbarsFloating only applies the stylesheet for
> the document which dispatched the load event.
> (The second option looks better as the first one would miss applying to
> iframe created after top level document load event)

Actually, listening to "load" is useless because of the webprogress listener
that does the same job.

But your comment still applies. I'm not sure we can just add the ua stylesheet
just to the loaded document  (there are some corners cases I don't understand).
No matter what, you need to flush the top level document, and inserting the stylesheet
twice is just a no-op operation. It works well this way, there are some
useless sheet injection, but I think it's safer that way. If you know precisely
when to insert the sheet and invalidate the top level document, I'd be happy to
implement it the right way.

> @@ +20,5 @@
> > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > +  addEventListener("load", makeScrollbarsFloating);
> > +  docShell.deviceSizeIsPageSize = true;
> > +  gRequiresFloatingScrollbars = data.requiresFloatingScrollbars;
> > +  if (docShell.contentViewer) {
> 
> A comment about this condition could be helpful.

Ok.

> @@ +54,5 @@
> > +    try {
> > +      winUtils.removeSheet(gFloatingScrollbarsStylesheet, win.AGENT_SHEET);
> > +    } catch(e) { }
> > +  }
> > +  flushStyle();
> 
> Could you move all the code related to floating scrollbars to a dedicated
> cleanup function?

Ok.

> @@ +91,5 @@
> > +  let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> > +  let width = content.innerWidth;
> > +  let height = content.innerHeight;
> > +  canvas.mozOpaque = true;
> > +  canvas.mozImageSmoothingEnabled = true;
> 
> Could you justify the usage of these properties in comments?

Removed mozImageSmoothingEnabled, kept mozOpaque that is self explanatory.

> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +139,5 @@
> > +  }
> > +  this.mm.addMessageListener("ResponsiveMode:Start:Done", childOn);
> > +
> > +  let requiresFloatingScrollbars = !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;
> > +  this.mm.loadFrameScript("resource:///modules/devtools/responsivedesign-child.jsm", true);
> 
> Frame scripts are named .js and not .jsm.

Meh. Fixed.
Assignee

Comment 21

5 years ago
Posted patch v1.1 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3190dc10d1fd
Attachment #8536502 - Flags: review?(poirot.alex)
Still broken for me in
37.0a1 (2014-12-14)

12:11:04.995 TypeError: this.browser.contentWindow is null responsivedesign.jsm:187:2
12:11:09.963 TypeError: this.touchEventHandler is undefined responsivedesign.jsm:825:0
12:11:23.729 TypeError: this._deviceSizeWasPageSize is undefined responsivedesign.jsm:273:4
Comment on attachment 8536502 [details] [diff] [review]
v1.1

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #20)
> > ::: browser/devtools/responsivedesign/responsivedesign-child.jsm
> > @@ +17,5 @@
> > > +    return;
> > > +  }
> > > +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> > > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > > +  addEventListener("load", makeScrollbarsFloating);
> > 
> > By listening to `load` you are also going to trigger makeScrollbarsFloating
> > for iframes.
> > Either call makeScrollbarsFloating only for top level document load event,
> > or make it so that makeScrollbarsFloating only applies the stylesheet for
> > the document which dispatched the load event.
> > (The second option looks better as the first one would miss applying to
> > iframe created after top level document load event)
> 
> Actually, listening to "load" is useless because of the webprogress listener
> that does the same job.
> 
> But your comment still applies. I'm not sure we can just add the ua
> stylesheet
> just to the loaded document  (there are some corners cases I don't
> understand).

Do you have test case for these corner cases, I have one last suggestion mentioned after...


Otherwise, it no longer breaks the mulet!

::: browser/devtools/responsivedesign/responsivedesign-child.js
@@ +110,5 @@
> +                              .getInterface(Ci.nsIWebProgress);
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_ALL);
> +  },
> +  onLocationChange: function onLocationChange() {
> +    makeScrollbarsFloating();

What about passing the loading docshell to makeScrollbarsFloating and only inject the sheet to it, while always flushing toplevel?
onLocationChange: function onLocationChange(webProgress) {
  webProgress.QueryInterface(Ci.nsIDocShell);
  makeScrollbarsFloading(webProgress);
}
function makeScrollbarsFloating(targetDocshell) {
  ...
  let allDocShells = [];
  if (targetDocshell)
    allDocShells.push(targetDocshell);
  else {
    allDocShell.push(docShell);
    for (let i = 0; i < docShell.childCount; i++) {
      let child = docShell.getChildAt(i).QueryInterface(Ci.nsIDocShell);
      allDocShells.push(child);
    }
  }
  ...
  flushStyle();
}
Attachment #8536502 - Flags: review?(poirot.alex) → review+
Assignee

Comment 24

5 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Do you have test case for these corner cases

I tried to do like you describe. The issues I run into: scrollbars not always being restored (with codepen.io), and some deep docshells (tab > iframe > iframe) to not be affected in jsbin. Also - width and height of the page not being  properly recorded when the responsive mode is re-opened.

I'm sure I could figure out what's going on, but it's going to take even more time. I prefer to land this working patch first.
Assignee

Comment 25

5 years ago
Posted patch v1.2 (obsolete) — Splinter Review
Attachment #8543787 - Flags: review+
Depends on: 1074180
Depends on: 1098984
Assignee

Updated

5 years ago
Blocks: 1074835
Assignee

Comment 27

5 years ago
Posted patch 1.2 (obsolete) — Splinter Review
Fixed test failure (hopefuly).
Attachment #8530262 - Attachment is obsolete: true
Attachment #8536502 - Attachment is obsolete: true
Attachment #8543787 - Attachment is obsolete: true
Attachment #8545710 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/66ec04bc10eb
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]

Comment 31

5 years ago
In case you didn't see that: currently (Fx 38a01 2015-01-14) if you press several times on cmd+alt+m to trigger responsive design mode, it create multiples responsive toolbar.
Assignee

Comment 32

5 years ago
Posted patch v1.2 (obsolete) — Splinter Review
Fixes leak.

https://tbpl.mozilla.org/?tree=Try&rev=b33870d7c5f5
Attachment #8545710 - Attachment is obsolete: true
Attachment #8549543 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/553cdfa00c2b
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]

Updated

5 years ago
Blocks: 1045689

Updated

5 years ago
No longer blocks: 1045689
Assignee

Comment 35

5 years ago
Posted patch patch.diff (obsolete) — Splinter Review
Alright. I think I go it right this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad673a194a27
Attachment #8549543 - Attachment is obsolete: true
Attachment #8551675 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 36

5 years ago
Posted patch v1.2Splinter Review
Better commit message.
Attachment #8551675 - Attachment is obsolete: true
Attachment #8551676 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ae9cc9b5f5dd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [e10s-m6][fixed-in-fx-team] → [e10s-m6]
Target Milestone: --- → Firefox 38
Depends on: 1126639
Depends on: 1145010

Updated

4 years ago
Depends on: 1141760
Depends on: 1189702

Updated

3 years ago
Depends on: 1249967

Updated

3 years ago
No longer depends on: 1249967

Updated

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