Last Comment Bug 749628 - Implement a "Responsive Design" tool
: Implement a "Responsive Design" tool
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal with 2 votes (vote)
: Firefox 15
Assigned To: Paul Rouget [:paul]
:
Mentors:
: 731153 752857 (view as bug list)
Depends on: 752848 758011 771046 774055 778169 749626 752473 752850 752851 755953 761165 761169 761431 762395 762846 762848 765413 771043 777972 778174 799471
Blocks: k9o-dev-resources
  Show dependency treegraph
 
Reported: 2012-04-27 08:06 PDT by Paul Rouget [:paul]
Modified: 2013-09-14 13:49 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
mockup (3.66 MB, image/png)
2012-04-27 08:06 PDT, Paul Rouget [:paul]
no flags Details
patch v0.1 // browser code (5.97 KB, patch)
2012-04-27 08:11 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.1 // devtools code (26.62 KB, patch)
2012-04-27 08:12 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.1 // browser code (5.97 KB, patch)
2012-05-02 10:09 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 // devtools code (51.42 KB, patch)
2012-05-02 10:11 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 // browser code (5.97 KB, patch)
2012-05-02 10:11 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 // browser code (51.16 KB, patch)
2012-05-03 05:09 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 // browser code (44.87 KB, patch)
2012-05-03 05:34 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.3 // browser code (5.97 KB, patch)
2012-05-03 09:29 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.3 // tool code (55.31 KB, patch)
2012-05-03 09:29 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 // tabbrowser changes (6.98 KB, patch)
2012-05-04 07:44 PDT, Paul Rouget [:paul]
dolske: review+
Details | Diff | Splinter Review
patch v0.4 // tool code (57.69 KB, patch)
2012-05-04 09:12 PDT, Paul Rouget [:paul]
dcamp: feedback+
Details | Diff | Splinter Review
patch v0.5 // tool code (63.03 KB, patch)
2012-05-07 05:21 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.6 // tool code (68.12 KB, patch)
2012-05-08 03:24 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 // tool code (69.73 KB, patch)
2012-05-08 06:06 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 // tool code (69.71 KB, patch)
2012-05-08 06:18 PDT, Paul Rouget [:paul]
fayearthur: feedback+
Details | Diff | Splinter Review
patch v1.1 // tabbrowser changes (7.86 KB, patch)
2012-05-11 02:29 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.1 // tool code (72.39 KB, patch)
2012-05-11 05:02 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review
patch v1.3 (70.73 KB, patch)
2012-05-16 03:16 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.4 (56.91 KB, patch)
2012-05-18 06:51 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.1 // tabbrowser changes (6.92 KB, patch)
2012-05-22 05:00 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.4 // tool code (56.03 KB, patch)
2012-05-22 05:01 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.5 (64.39 KB, patch)
2012-05-28 07:15 PDT, Paul Rouget [:paul]
gavin.sharp: review+
Details | Diff | Splinter Review
patch to land (71.59 KB, patch)
2012-06-01 05:46 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch to land (71.59 KB, patch)
2012-06-01 05:56 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S (47.92 KB, image/png)
2012-06-18 20:14 PDT, pd
no flags Details
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S (46.78 KB, image/png)
2012-06-18 20:22 PDT, pd
no flags Details

Description Paul Rouget [:paul] 2012-04-27 08:06:45 PDT
Created attachment 619038 [details]
mockup
Comment 1 Paul Rouget [:paul] 2012-04-27 08:11:40 PDT
Created attachment 619041 [details] [diff] [review]
patch v0.1 // browser code
Comment 2 Paul Rouget [:paul] 2012-04-27 08:12:20 PDT
Created attachment 619042 [details] [diff] [review]
patch v0.1 // devtools code
Comment 3 Paul Rouget [:paul] 2012-04-27 08:16:28 PDT
Todo:
- tests
- force toolbar to LTR in RTL
- should we make the dimensions editable?
- get a comprehensive list of presets
- pref it off by default
- file a legal bug to know if we can get the name of the devices
Comment 4 Paul Rouget [:paul] 2012-04-28 10:49:33 PDT
Todo:
- toggle scrollbar
- Test fail:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug462673.js | Panel for remaining tab is selected - Got [object XULElement @ 0xba04638 (native @ 0xbb669c0)], expected [object XULElement @ 0xb179020 (native @ 0xbb66968)]
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4331579 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of AsyncStatement with size 48 bytes each (96 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1994 instances of AtomImpl with size 24 bytes each (47856 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 38 instances of BodyRule with size 16 bytes each (608 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of CalculateFrecencyFunction with size 12 bytes
Comment 6 Kevin Dangoor 2012-04-30 10:12:37 PDT
This is very cool, and I can't wait to try it!

One thing I'll note is that I think people want the ability to browse and interact with their apps in "responsive design mode" in addition to inspecting.
Comment 7 Paul Rouget [:paul] 2012-04-30 10:31:42 PDT
(In reply to Kevin Dangoor from comment #6)
> This is very cool, and I can't wait to try it!

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/prouget@mozilla.com-2c54c22b660d/try-macosx64/

In a chrome scratchpad, type: ResponsiveUI.start();

(there's some styling problem with Mac)

> One thing I'll note is that I think people want the ability to browse and
> interact with their apps in "responsive design mode" in addition to
> inspecting.

You don't need to use the inspector to use this tool.
Comment 8 Maniac Vlad Florin (:vladmaniac) 2012-05-02 08:48:04 PDT
Guys, where should I apply those patches to play along? 

I tried a mozilla-central build with no luck (developer tools code is rejected) 

Can you point out some directions? :)
Comment 9 Paul Rouget [:paul] 2012-05-02 10:09:59 PDT
Created attachment 620365 [details] [diff] [review]
patch v0.1 // browser code
Comment 10 Paul Rouget [:paul] 2012-05-02 10:11:13 PDT
Created attachment 620366 [details] [diff] [review]
patch v0.2 // devtools code
Comment 11 Paul Rouget [:paul] 2012-05-02 10:11:56 PDT
Created attachment 620367 [details] [diff] [review]
patch v0.2 // browser code
Comment 12 Paul Rouget [:paul] 2012-05-03 05:09:40 PDT
Created attachment 620653 [details] [diff] [review]
patch v0.2 // browser code

Now with tests.
Comment 13 Paul Rouget [:paul] 2012-05-03 05:34:36 PDT
Created attachment 620661 [details] [diff] [review]
patch v0.2 // browser code

rebased
Comment 14 Paul Rouget [:paul] 2012-05-03 09:29:18 PDT
Created attachment 620735 [details] [diff] [review]
patch v0.3 // browser code
Comment 15 Paul Rouget [:paul] 2012-05-03 09:29:44 PDT
Created attachment 620736 [details] [diff] [review]
patch v0.3 // tool code
Comment 16 Paul Rouget [:paul] 2012-05-03 09:33:14 PDT
New TODO list:
* Windows Code
* Panel FIXME in //browser code
* initial animation
* add some comments (probably followup)
* toggle scrollbars (probably followup)
* add icons (phone/tablet/labtop/desktop) to the menulist (probably followup)
* are current (anonymous) presets ok?
* name the presets (probably followup)
* file bug for pref the tool on
* the zoom is jumpy (might use a transform instead)
* I got a segfault 2 or 3 times on Linux
* if the inspector is open, the inspector should close first (when hitting Escape)
* need a ux-review
Comment 18 Paul Rouget [:paul] 2012-05-04 07:44:30 PDT
Created attachment 621048 [details] [diff] [review]
patch v1 // tabbrowser changes
Comment 19 Paul Rouget [:paul] 2012-05-04 07:48:48 PDT
Comment on attachment 621048 [details] [diff] [review]
patch v1 // tabbrowser changes

This patch adds a <vbox> around the browser stack. In the tool code, I insert a toolbar in this box, and resize the stack to achieve this "Responsive Mode".

https://tbpl.mozilla.org/?tree=Try&rev=42c5f0c7f0ac
Comment 20 Paul Rouget [:paul] 2012-05-04 09:12:16 PDT
Created attachment 621068 [details] [diff] [review]
patch v0.4 // tool code
Comment 21 Paul Rouget [:paul] 2012-05-04 09:12:59 PDT
New TODO list:
* Windows Code
* add some comments
* we need a good list of presets
* investigate crash on Linux (not sure if related)

Nice to have (follow ups):
* initial animation
* toggle scrollbars
* add icons (phone/tablet/labtop/desktop) to the menulist
* name/categorize the presets
* pref the tool on
* if the inspector is open, the inspector should close first (when hitting Escape)
Comment 22 Paul Rouget [:paul] 2012-05-04 09:15:43 PDT
Comment on attachment 621068 [details] [diff] [review]
patch v0.4 // tool code

Dave, can you take a quick look?

Queue: patch in bug 749626, then tabbrowser patch in this bug, then this patch.
Comment 23 Dave Camp (:dcamp) 2012-05-04 09:35:29 PDT
Comment on attachment 621068 [details] [diff] [review]
patch v0.4 // tool code

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

Looks nice.  Left out the parts I'm sure you could predict about documentation and l10n etc.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +106,5 @@
> +  presets[0].width = bbox.width - 40;
> +  presets[0].height = bbox.height - 80;
> +
> +  this.container.setAttribute("expose", "true");
> +  this.stack.setAttribute("expose", "true");

We should probably find another name for this attribute.  I can't think of one offhand, but "expose" seems ambiguous (especially without the grave).

@@ +122,5 @@
> +  this.startResizing = this.startResizing.bind(this);
> +  this.stopResizing = this.stopResizing.bind(this);
> +  this.onDrag = this.onDrag.bind(this);
> +  this.onKeypress = this.onKeypress.bind(this);
> +  this.close = this.close.bind(this);

The browser folks have told us they prefer to tag bound methods rather than overwrite them... something like this._close = this.close.bind() or this.boundClose = this.close.bind().  I'll defer to Rob whether he wants that request to apply in the devtools module.

@@ +139,5 @@
> +}
> +
> +ResponsiveUI.prototype = {
> +  close: function RUI_unload() {
> +    let style = (<r><![CDATA[

Can you just use a string for this?
Comment 24 Paul Rouget [:paul] 2012-05-07 05:21:55 PDT
Created attachment 621567 [details] [diff] [review]
patch v0.5 // tool code

Windows support.
Comment 25 Paul Rouget [:paul] 2012-05-08 03:24:11 PDT
Created attachment 621927 [details] [diff] [review]
patch v0.6 // tool code

Documentation. Addressed Dave's comments. Better Windows support.
Comment 26 Paul Rouget [:paul] 2012-05-08 03:56:07 PDT
I think the crash was unrelated (can't reproduce).
Comment 27 Paul Rouget [:paul] 2012-05-08 06:06:27 PDT
Created attachment 621948 [details] [diff] [review]
patch v1 // tool code
Comment 28 Paul Rouget [:paul] 2012-05-08 06:18:10 PDT
Created attachment 621949 [details] [diff] [review]
patch v1 // tool code
Comment 29 Paul Rouget [:paul] 2012-05-08 06:20:31 PDT
Comment on attachment 621949 [details] [diff] [review]
patch v1 // tool code

(you'll need to apply the patch from bug 749626 first)
Comment 30 Paul Rouget [:paul] 2012-05-09 11:11:45 PDT
I removed the queryInterface for this.contentViewer. This is actually needed:
this.contentViewer = aBrowser.docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer);
Comment 31 Justin Dolske [:Dolske] 2012-05-09 17:05:48 PDT
Comment on attachment 621048 [details] [diff] [review]
patch v1 // tabbrowser changes

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

::: browser/base/content/tabbrowser.xml
@@ +1784,5 @@
>  
>              // This will unload the document. An unload handler could remove
>              // dependant tabs, so it's important that the tabbrowser is now in
>              // a consistent state (tab removed, tab positions updated, etc.).
> +            panel.removeChild(browser.parentNode.parentNode);

*groan* How many .parentNodes will it take to break the camel's back? :)

Let's just go ahead and change these?

  var panel = this.getNotificationBox(browser);
  ...
  panel.removeChild(this.getBrowserContainer(browser))

@@ +2555,5 @@
>        </method>
>  
>        <constructor>
>          <![CDATA[
> +          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild.firstChild.firstChild;

We can save cleaning this up for the next person for add a .firstChild! :)
Comment 32 Heather Arthur [:harth] 2012-05-10 13:46:27 PDT
Comment on attachment 621949 [details] [diff] [review]
patch v1 // tool code

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

There are some issues with the globality of the checked state of the toolbar menuitem. For example, if you go into responsive mode on a page, then switch to a different tab, the menuitem remains checked. This is confusing since the responsive mode isn't enabled on the page you're viewing. Same goes for closing responsive mode and switching to a tab that's still in responsive mode.

I think there could be a bit more margin between the "size" and the "zoom" toolbars.

This is smaller and up for debate, but pressing ESC when you have the responsive mode and style inspector open will close the responsive mode. My preference is for closing the style inspector first because I imagine that's enabled after enabling the responsive mode.

Looks good!

::: browser/base/content/browser.js
@@ +1788,5 @@
> +#ifdef MENUBAR_CAN_AUTOHIDE
> +    document.getElementById("appmenu_responsiveUI").hidden = false;
> +#endif
> +    let toolbarbutton = document.getElementById("developer-toolbar-responsiveui");
> +    if (toolbarbutton) toolbarbutton.hidden = false;

Small nit: other single-line if statements in this file went on a separate line.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +96,5 @@
> +
> +  // Default size. The first preset (custom) is the one that will be used.
> +  let bbox = this.stack.getBoundingClientRect();
> +  this.presets[0].width = bbox.width - 40;
> +  this.presets[0].height = bbox.height - 80;

Explain the 40 and 80.

@@ +130,5 @@
> +  let toolbaritem = this.chromeDoc.getElementById("developer-toolbar-responsiveui");
> +  if (toolbaritem) toolbaritem.setAttribute("checked", "true");
> +
> +  // We compute the CSS transition duration. We will need to simulate a
> +  // on the top and left attributes.

simulate a what?

@@ +428,5 @@
> +
> +    let zoomH, zoomV;
> +    zoomH = zoomV = this.currentZoomValue;
> +    zoomH = width / this.currentWidth;
> +    zoomV = height / this.currentHeight;

It seems like zoomH and zoomV are set here then set again the line after. I don't immediately see the point of this.

@@ +541,5 @@
> +      let currentDate = Date.now();
> +      if (currentDate >= arrivalDate) {
> +        aElement.setAttribute(aAttribute, aFinalValue);
> +      } else {
> +        deltaDate = currentDate - initialDate;

deltaDate not declared
Comment 33 Paul Rouget [:paul] 2012-05-11 02:29:04 PDT
Created attachment 623082 [details] [diff] [review]
patch v1.1 // tabbrowser changes

Addressed Dolske's comments
Comment 34 Paul Rouget [:paul] 2012-05-11 02:35:22 PDT
Thank you for the review.

(In reply to Heather Arthur [:harth] from comment #32)
> Comment on attachment 621949 [details] [diff] [review]
> patch v1 // tool code
> 
> Review of attachment 621949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are some issues with the globality of the checked state of the toolbar
> menuitem. For example, if you go into responsive mode on a page, then switch
> to a different tab, the menuitem remains checked. This is confusing since
> the responsive mode isn't enabled on the page you're viewing. Same goes for
> closing responsive mode and switching to a tab that's still in responsive
> mode.

Oh, I didn't think about that. Good catch.

> 
> I think there could be a bit more margin between the "size" and the "zoom"
> toolbars.

I'll let the UX guys decide (see bug 751910).

> 
> This is smaller and up for debate, but pressing ESC when you have the
> responsive mode and style inspector open will close the responsive mode. My
> preference is for closing the style inspector first because I imagine that's
> enabled after enabling the responsive mode.

I agree (see bug 752851).
Comment 35 Paul Rouget [:paul] 2012-05-11 05:02:47 PDT
Created attachment 623105 [details] [diff] [review]
patch v1.1 // tool code

Addressed harth's comments
Comment 36 Dave Camp (:dcamp) 2012-05-14 13:40:52 PDT
Comment on attachment 623105 [details] [diff] [review]
patch v1.1 // tool code

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

Looks good, a few small comments:

::: browser/app/profile/firefox.js
@@ +1077,5 @@
> +//   {width: 2048, height: 1536},  // iPad 3
> +//   {width: 2880, height: 1800},
> +// ];
> +pref("devtools.responsiveUI.presets", "[{\"width\":240,\"height\":400},{\"width\":320,\"height\":480},{\"width\":360,\"height\":640},{\"width\":480,\"height\":800},{\"width\":600,\"height\":1024},{\"width\":640,\"height\":960},{\"width\":720,\"height\":1280},{\"width\":768,\"height\":1024},{\"width\":800,\"height\":1280},{\"width\":1280,\"height\":1024},{\"width\":1366,\"height\":768},{\"width\":1440,\"height\":900},{\"width\":1600,\"height\":900},{\"width\":1920,\"height\":1600},{\"width\":2048,\"height\":1536},{\"width\":2880,\"height\":1800}]");
> +

Maybe we should have the default set in the code somewhere?  This seems annoying to update.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +57,5 @@
> +      this.open(aWindow, aTab);
> +    }
> +  },
> +  open: function(aWindow, aTab) {
> +    aTab.responsiveUI = new ResponsiveUI(aWindow, aTab);

If open is a public function it should probably defend itself a bit better against bad callers (calling open multiple times).

If only toggle is public, we should _prefix the other methods.

Can we put the function name tags in these methods?

@@ +73,5 @@
> +  this.chromeDoc = aWindow.document;
> +  this.container = aWindow.gBrowser.getBrowserContainer(this.browser);
> +  this.stack = this.container.querySelector("[anonid=browserStack]");
> +
> +  this.strings = Services.strings.createBundle("chrome://browser/locale/devtools/responsiveUI.properties");

Is createBundle smart about reusing properties?  Should strings be done in a lazy getter in the global scope like it usually is?

@@ +86,5 @@
> +  let presets;
> +  try {
> +    presets = JSON.parse(Services.prefs.getCharPref("devtools.responsiveUI.presets"));
> +  } catch(e) {
> +    // User pref is malformated.

We should just use the default set here.
Comment 37 Paul Rouget [:paul] 2012-05-16 03:16:50 PDT
Created attachment 624335 [details] [diff] [review]
patch v1.3
Comment 38 Paul Rouget [:paul] 2012-05-16 08:35:49 PDT
Comment on attachment 624335 [details] [diff] [review]
patch v1.3

Justin, this second patch implements the tool-side of the responsive mode tool.

I already got a r+ from :dcamp for the devtools changes (99% of the code), but there's still some browsers changes in here (look at the browser.js change, one line, and some CSS code).

(also, can you please confirm the pref in browser/branding/unofficial/pref/firefox-branding.js will only be taken into account for Firefox Nightly? First time I do that.)

Thank you.
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-16 10:38:34 PDT
Comment on attachment 624335 [details] [diff] [review]
patch v1.3

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

I'm not sure whether these changes belong here or in the theme. Changing the overflow on browserContainer also scares me a little (will it mess up layout or cause performance issues?). Dao should probably sign-off on these changes.

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

>   onUpdateCurrentBrowser: function XWB_onUpdateCurrentBrowser(aStateFlags, aStatus, aMessage, aTotalProgress) {
>-    if (FullZoom.updateBackgroundTabs)
>+    // If the browser is in ResponsiveUI mode, we don't update its zoom value.
>+    if (FullZoom.updateBackgroundTabs && !gBrowser.selectedBrowser.responsiveUI)

This doesn't look like a complete fix - don't you also need to change the other FullZoom.onLocationChange caller? I think this part might be best split off into another bug.

>diff --git a/browser/branding/unofficial/pref/firefox-branding.js b/browser/branding/unofficial/pref/firefox-branding.js

>+// DevTools
>+pref("devtools.responsiveUI.enabled", true);

I don't really like using the branding pref file for functionality like this. Having an override in branding makes blame harder to figure out later, and leads to confusion when trying to figure out whether something is actually enabled.
Comment 40 Paul Rouget [:paul] 2012-05-16 10:54:39 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39)
> Comment on attachment 624335 [details] [diff] [review]
> patch v1.3
> 
> >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css
> 
> I'm not sure whether these changes belong here or in the theme.

These are functional changes.

> Changing the overflow on browserContainer also scares me a little (will it mess up layout
> or cause performance issues?). Dao should probably sign-off on these changes.

Just to be clear, these changes are only effective when the responsive mode is activated.

> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >   onUpdateCurrentBrowser: function XWB_onUpdateCurrentBrowser(aStateFlags, aStatus, aMessage, aTotalProgress) {
> >-    if (FullZoom.updateBackgroundTabs)
> >+    // If the browser is in ResponsiveUI mode, we don't update its zoom value.
> >+    if (FullZoom.updateBackgroundTabs && !gBrowser.selectedBrowser.responsiveUI)
> 
> This doesn't look like a complete fix - don't you also need to change the
> other FullZoom.onLocationChange caller? I think this part might be best
> split off into another bug.

I'll do that.

> 
> >diff --git a/browser/branding/unofficial/pref/firefox-branding.js b/browser/branding/unofficial/pref/firefox-branding.js
> 
> >+// DevTools
> >+pref("devtools.responsiveUI.enabled", true);
> 
> I don't really like using the branding pref file for functionality like
> this. Having an override in branding makes blame harder to figure out later,
> and leads to confusion when trying to figure out whether something is
> actually enabled.

Ok, I'll just remove that.
Comment 41 Paul Rouget [:paul] 2012-05-18 04:02:51 PDT
I am considering removing the zoom feature.

The initial reason I added the ability to zoom was to be able to work with huge resolution (like the iPad 3 or the iPhone 4). But I was mistaken. iPad 3 hardware resolution has nothing to do with the resolution exposed by the browser. And I don't think making this tool work for huge screen resolution on small screen justifies adding confusing controls.
Comment 42 Paul Rouget [:paul] 2012-05-18 04:13:18 PDT
Comment on attachment 624335 [details] [diff] [review]
patch v1.3

Cancelling review until I know if we should include the zoom feature or not.
Comment 43 Paul Rouget [:paul] 2012-05-18 06:51:03 PDT
Created attachment 625089 [details] [diff] [review]
patch v1.4

Removed the zoom feature. Let's add it later if we consider it's an important thing to have.
Comment 44 Dave Camp (:dcamp) 2012-05-21 13:15:02 PDT
These patches don't apply anymore.  Paul, can you rebase them please?
Comment 45 Kevin Dangoor 2012-05-21 14:46:17 PDT
(In reply to Paul Rouget [:paul] from comment #41)
> I am considering removing the zoom feature.
> 
> The initial reason I added the ability to zoom was to be able to work with
> huge resolution (like the iPad 3 or the iPhone 4). But I was mistaken. iPad
> 3 hardware resolution has nothing to do with the resolution exposed by the
> browser. And I don't think making this tool work for huge screen resolution
> on small screen justifies adding confusing controls.

Yeah, good call.
Comment 46 Paul Rouget [:paul] 2012-05-22 05:00:31 PDT
Created attachment 625973 [details] [diff] [review]
patch v1.1 // tabbrowser changes
Comment 47 Paul Rouget [:paul] 2012-05-22 05:01:19 PDT
Created attachment 625974 [details] [diff] [review]
patch v1.4 // tool code
Comment 49 David Flanagan [:djf] 2012-05-23 14:05:35 PDT
Paul,

Any chance you can make this tool set the layout.css.dpi preference so that we can try sites at different pixel densities as well as different resolutions?

Style sheets that use mozmm units would be affected, as would media queries that use min-resolution and max-resolution.
Comment 50 Paul Rouget [:paul] 2012-05-23 14:13:29 PDT
(In reply to David Flanagan from comment #49)
> Paul,
> 
> Any chance you can make this tool set the layout.css.dpi preference so that
> we can try sites at different pixel densities as well as different
> resolutions?
> 
> Style sheets that use mozmm units would be affected, as would media queries
> that use min-resolution and max-resolution.

This is something we are considering (I even have some code for that). Can you please file a specific bug for that? (and add it to the dependency list)
Comment 51 Dave Camp (:dcamp) 2012-05-24 13:52:35 PDT
Review ping Dao?  Thanks.
Comment 52 David Flanagan [:djf] 2012-05-25 12:16:09 PDT
For the B2G desktop client, I've implemented a --screen command line argument that allows you to specify screen size and density at startup, and optionally scales the screen to match the simulated pixel density to the actual pixel density of your monitor. Maybe Firefox could have an option like that in addition to this devtool?

See https://bugzilla.mozilla.org/show_bug.cgi?id=758129 if this is of interest.
Comment 53 Paul Rouget [:paul] 2012-05-28 07:14:44 PDT
(In reply to David Flanagan from comment #52)
> For the B2G desktop client, I've implemented a --screen command line
> argument that allows you to specify screen size and density at startup, and
> optionally scales the screen to match the simulated pixel density to the
> actual pixel density of your monitor. Maybe Firefox could have an option
> like that in addition to this devtool?
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=758129 if this is of
> interest.

Thank you for the reference.
These are the physical resolution. Different beast. But still useful for bug 758011.
Comment 54 Paul Rouget [:paul] 2012-05-28 07:15:44 PDT
Created attachment 627693 [details] [diff] [review]
patch v1.5

Addressed Shorlander's comments.
Comment 55 Paul Rouget [:paul] 2012-05-29 02:13:57 PDT
Dão, review ping?
Comment 56 Paul Rouget [:paul] 2012-05-29 02:16:13 PDT
(there are 2 patches to apply here, first "patch v1.1 // tabbrowser changes" then "patch v1.5")
Comment 57 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-30 09:18:18 PDT
Comment on attachment 627693 [details] [diff] [review]
patch v1.5

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

>+  // Enable Responsive UI?

I think it'd be super-swell if all of this devtools-enabling/disabling code lived in a separate module, so that the only code in delayedStartup looked something like:

DevToolUtils.setInitialToolState(window);

or some such. Something for a followup bug.

Following comments apply to all three themes:

>diff --git a/browser/themes/gnomestripe/browser.css b/browser/themes/gnomestripe/browser.css

>+vbox[anonid=browserContainer][responsivemode] {
>+  background: #2a3643 url(data:image/png;base64,...

I think this should be split out into a separate file.

>diff --git a/browser/themes/gnomestripe/devtools/responsive-se-resizer.png b/browser/themes/gnomestripe/devtools/responsive-se-resizer.png
>diff --git a/browser/themes/gnomestripe/devtools/responsive-vertical-resizer.png b/browser/themes/gnomestripe/devtools/responsive-vertical-resizer.png

Why are these custom resizer images necessary?

r=me on the non-browser/devtools parts.
Comment 58 Paul Rouget [:paul] 2012-06-01 05:43:07 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #57)
> Comment on attachment 627693 [details] [diff] [review]
> patch v1.5
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+  // Enable Responsive UI?
> 
> I think it'd be super-swell if all of this devtools-enabling/disabling code
> lived in a separate module, so that the only code in delayedStartup looked
> something like:
> 
> DevToolUtils.setInitialToolState(window);
> 
> or some such. Something for a followup bug.

Filed bug 760409.

> Following comments apply to all three themes:
> 
> >diff --git a/browser/themes/gnomestripe/browser.css b/browser/themes/gnomestripe/browser.css
> 
> >+vbox[anonid=browserContainer][responsivemode] {
> >+  background: #2a3643 url(data:image/png;base64,...
> 
> I think this should be split out into a separate file.

ok.

> >diff --git a/browser/themes/gnomestripe/devtools/responsive-se-resizer.png b/browser/themes/gnomestripe/devtools/responsive-se-resizer.png
> >diff --git a/browser/themes/gnomestripe/devtools/responsive-vertical-resizer.png b/browser/themes/gnomestripe/devtools/responsive-vertical-resizer.png
> 
> Why are these custom resizer images necessary?

chrome://global/skin/icons/resizer.png looks bad on a dark background.

> 
> r=me on the non-browser/devtools parts.

Thank you!
Comment 59 Paul Rouget [:paul] 2012-06-01 05:46:50 PDT
Created attachment 629154 [details] [diff] [review]
patch to land

Addressed Gavin's comments. Merged the 2 patches. Ready to land
Comment 60 Paul Rouget [:paul] 2012-06-01 05:52:05 PDT
*** Bug 752857 has been marked as a duplicate of this bug. ***
Comment 61 Paul Rouget [:paul] 2012-06-01 05:56:24 PDT
Created attachment 629157 [details] [diff] [review]
patch to land

preffed on as all the blockers in bug 752857 are fixed
Comment 63 Panos Astithas [:past] (away until 7/21) 2012-06-02 02:26:06 PDT
https://hg.mozilla.org/mozilla-central/rev/9bad0808a691
Comment 64 Rimas Kudelis 2012-06-05 03:02:25 PDT
Any hints about localizing the name of this feature?

For me, "Responsive design" or "Responsive Mode" doesn't ring a bell at all, and from looking at it in action, it seems that all it does is add an ability to resize the viewport. So, what does the term "Responsive Mode" really mean?
Comment 65 Dão Gottwald [:dao] 2012-06-05 04:06:53 PDT
(In reply to Rimas Kudelis from comment #64)
> Any hints about localizing the name of this feature?
> 
> For me, "Responsive design" or "Responsive Mode" doesn't ring a bell at all,
> and from looking at it in action, it seems that all it does is add an
> ability to resize the viewport. So, what does the term "Responsive Mode"
> really mean?

As I understand it (and I'm not at all sure about this), it's supposed to mean "mode for testing whether a page is responsive to different view port sizes." "Responsive mode" seems like an inadequate shortening and I think we should probably fix that rather than trying to explain it to localizers...
Comment 66 Kevin Dangoor 2012-06-05 09:15:22 PDT
"Responsive Web Design" has, over the past year, become a fairly common term for pages that "adapt the layout to the viewing environment"[1]. Twitter Bootstrap, which has become a popular starting point for new sites/apps supports "Responsive Design"[2]

Googling "responsive design" (quotes included) yields 1.4 million results.

The terminology is fairly new, but seems to have reached a high level of industry acceptance. Webmonkey seemed okay with it[3].

I recognize that this may be unfamiliar to localizers, but I worry that whatever other terminology we may pick would actually be less clear to practicing web developers than a term that uses "responsive".

FWIW, the similar capability in Chrome Canary is hidden away in the settings as "Override device metrics" in a section headed "User Agent". This is probably easier to translate but I don't think it's very clear in English!

[1]: http://en.wikipedia.org/wiki/Responsive_Web_Design
[2]: http://twitter.github.com/bootstrap/
[3]: http://www.webmonkey.com/2012/06/new-firefox-developer-tools-will-help-you-build-responsive-websites/
Comment 67 Dão Gottwald [:dao] 2012-06-05 09:54:15 PDT
It's not just unfamiliarity. I can make sense of "responsive [web] design" without having come across the term. "Responsive mode" or "responsive UI" (used internally) I don't think make sense. It seems that "design" is missing there, because it's not the mode or the UI that's responsive.
Comment 68 Paul Rouget [:paul] 2012-06-07 07:15:28 PDT
*** Bug 731153 has been marked as a duplicate of this bug. ***
Comment 69 pd 2012-06-14 20:08:28 PDT
The defaults for this feature seem a tad strange. AFAIK the Galaxy S * is one of the more popular mobiles on the market today yet it's resolution is not on the list.

Galaxy S * resolution is 480 x 800 pixels

In addition it doesn't seem clear how to set the 'custom' resolution. 

In addition it would be an easier to use tool if the x and y sides were individually adjustable to a custom size rather than only adjustable together via the bottom right corner.
Comment 70 Paul Rouget [:paul] 2012-06-15 06:36:36 PDT
(In reply to pd from comment #69)
> The defaults for this feature seem a tad strange. AFAIK the Galaxy S * is
> one of the more popular mobiles on the market today yet it's resolution is
> not on the list.
> 
> Galaxy S * resolution is 480 x 800 pixels

Are you sure the Galaxy S resolution is 480px?
(be careful, we are talking about the resolution exposed by the browser, not the actual device resolution)

> In addition it doesn't seem clear how to set the 'custom' resolution. 

Did you see the handles? See bug 761169 & bug 762848.

> In addition it would be an easier to use tool if the x and y sides were
> individually adjustable to a custom size rather than only adjustable
> together via the bottom right corner.

You can resize x alone (see the right handle).
There's no good reason to be able to resize the y size though. So I didn't put a y-resizer.
Comment 71 pd 2012-06-17 20:25:12 PDT
(In reply to Paul Rouget [:paul] from comment #70)
> (In reply to pd from comment #69)
> > The defaults for this feature seem a tad strange. AFAIK the Galaxy S * is
> > one of the more popular mobiles on the market today yet it's resolution is
> > not on the list.
> > 
> > Galaxy S * resolution is 480 x 800 pixels
> 
> Are you sure the Galaxy S resolution is 480px?
> (be careful, we are talking about the resolution exposed by the browser, not
> the actual device resolution)

I just did a quick test with http://resruler.com/ and aside from resruler.com strangely using 10px margin, the Galaxy S running Firefox 10.0.5 indeed does appear to support 480 pixels width in portrait mode. 

Unfortunately I am not sure that supplying a screenshot would be convincing due to the aforementioned 10px issue with this tool. If anybody has a better tool I'd be happy to do the test again.
 
> > In addition it doesn't seem clear how to set the 'custom' resolution. 
> 
> Did you see the handles? See bug 761169 & bug 762848.

I don't think I saw handles on the x or y edges, just the bottom right corner. At the moment, Nightly's quite broken for me. I can't use it.

> > In addition it would be an easier to use tool if the x and y sides were
> > individually adjustable to a custom size rather than only adjustable
> > together via the bottom right corner.
> 
> You can resize x alone (see the right handle).
> There's no good reason to be able to resize the y size though. So I didn't
> put a y-resizer.

Wouldn't developers want to know what the user will see first, on either axis? I have attended usability seminars that mention 'above the fold' as something to be aware of.
Comment 72 Paul Rouget [:paul] 2012-06-18 02:38:56 PDT
(In reply to pd from comment #71)
> (In reply to Paul Rouget [:paul] from comment #70)
> > (In reply to pd from comment #69)
> > > The defaults for this feature seem a tad strange. AFAIK the Galaxy S * is
> > > one of the more popular mobiles on the market today yet it's resolution is
> > > not on the list.
> > > 
> > > Galaxy S * resolution is 480 x 800 pixels
> > 
> > Are you sure the Galaxy S resolution is 480px?
> > (be careful, we are talking about the resolution exposed by the browser, not
> > the actual device resolution)
> 
> I just did a quick test with http://resruler.com/ and aside from
> resruler.com strangely using 10px margin, the Galaxy S running Firefox
> 10.0.5 indeed does appear to support 480 pixels width in portrait mode.

I don't know how resruler works. Can you please load this page http://jsbin.com/eyecap/9 and tell what size is displayed? Thank you!

If, indeed, it exposes 480px for the width, I'll add it to the list.
Comment 73 Panos Astithas [:past] (away until 7/21) 2012-06-18 06:20:01 PDT
In my Galaxy S II Fennec Nightly says 320x460.
Comment 74 pd 2012-06-18 20:14:44 PDT
Created attachment 634277 [details]
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S
Comment 75 pd 2012-06-18 20:18:27 PDT
Comment on attachment 634277 [details]
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S

Sorry, wrong screenshot.
Comment 76 pd 2012-06-18 20:22:20 PDT
Created attachment 634279 [details]
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S
Comment 77 Panos Astithas [:past] (away until 7/21) 2012-06-19 00:20:06 PDT
Indeed, XUL Fennec (the release version of Firefox for Android) is 320x508 in my S II as well.
Comment 78 pd 2012-06-19 00:59:12 PDT
(In reply to Paul Rouget [:paul] from comment #70)
> (In reply to pd from comment #69)
> > The defaults for this feature seem a tad strange. AFAIK the Galaxy S * is
> > one of the more popular mobiles on the market today yet it's resolution is
> > not on the list.
> > 
> > Galaxy S * resolution is 480 x 800 pixels
> 
> Are you sure the Galaxy S resolution is 480px?
> (be careful, we are talking about the resolution exposed by the browser, not
> the actual device resolution)

I think you're talking about the resolution when using the meta viewport tag which AFAIK is not necessarily a requirement for displaying web pages on a mobile device or an official standard but is instead more of an unofficial convention:

https://developer.mozilla.org/en/Mobile/Viewport_meta_tag#Standards

I suspect the browser can still render 480px but scales down the entire page until is fits that resolution, often rendering pages unreadable.
Comment 79 Paul Rouget [:paul] 2012-06-19 01:39:14 PDT
The preset only include exposed "device-width". No meta tag will size the viewport up to 920px depending on the content. See bug 752473 comment 19.
Comment 80 Jean-Marie COUPRIE 2012-12-13 06:52:22 PST
Badly implemented for a normal user :

1) Cathodic screens had no preferential definition but recent monitors are LCD and have a native definition corresponding to the number of cells. Used with other definitions the quality of image is poor. So the default definition should be the native definition of my screen that should also be included in the list of proposed definitions.

2) when I receive a screen from the net with some strange definition and press the key this replace (and loose) the previous customized definition I used !

3) the way of inactivating this feature should be documented.

Please reopen this bug.
Comment 81 Paul Rouget [:paul] 2012-12-13 07:22:39 PST
(In reply to Jean-Marie COUPRIE from comment #80)
> Badly implemented for a normal user :

This is not for normal user, But for developers.

> 1) Cathodic screens had no preferential definition but recent monitors are
> LCD and have a native definition corresponding to the number of cells. Used
> with other definitions the quality of image is poor. So the default
> definition should be the native definition of my screen that should also be
> included in the list of proposed definitions.

I don't understand what you're saying here. This feature doesn't change any definition (do you mean resolution?), but the size of the viewport.

> 2) when I receive a screen from the net with some strange definition and
> press the key this replace (and loose) the previous customized definition I
> used !

Do you use the same Firefox? Can you share some STR?

> 3) the way of inactivating this feature should be documented.

Press escape.

> Please reopen this bug.

No.

Note You need to log in before you can comment on or make changes to this bug.