749628 - Implement a "Responsive Design" tool

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

Attachment: mockup

Comment 1

[Paul Rouget [:paul]]

Attachment: patch v0.1 // browser code

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v0.1 // devtools code

Comment 3

[Paul Rouget [:paul]]

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]]

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 5

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=11269737&tree=Try

Comment 6

[Kevin Dangoor]

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]]

(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/[e-mail]-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)]

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]]

Attachment: patch v0.1 // browser code

Comment 10

[Paul Rouget [:paul]]

Attachment: patch v0.2 // devtools code

Comment 11

[Paul Rouget [:paul]]

Attachment: patch v0.2 // browser code

Comment 12

[Paul Rouget [:paul]]

Attachment: patch v0.2 // browser code

Now with tests.

Comment 13

[Paul Rouget [:paul]]

Attachment: patch v0.2 // browser code

rebased

Comment 14

[Paul Rouget [:paul]]

Attachment: patch v0.3 // browser code

Comment 15

[Paul Rouget [:paul]]

Attachment: patch v0.3 // tool code

Comment 16

[Paul Rouget [:paul]]

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 17

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=e92a43401cad
Mac and Linux builds logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e92a43401cad

Comment 18

[Paul Rouget [:paul]]

Attachment: patch v1 // tabbrowser changes

Comment 19

[Paul Rouget [:paul]]

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]]

Attachment: patch v0.4 // tool code

Comment 21

[Paul Rouget [:paul]]

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]]

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)]

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]]

Attachment: patch v0.5 // tool code

Windows support.

Comment 25

[Paul Rouget [:paul]]

Attachment: patch v0.6 // tool code

Documentation. Addressed Dave's comments. Better Windows support.

Comment 26

[Paul Rouget [:paul]]

I think the crash was unrelated (can't reproduce).

Comment 27

[Paul Rouget [:paul]]

Attachment: patch v1 // tool code

Comment 28

[Paul Rouget [:paul]]

Attachment: patch v1 // tool code

Comment 29

[Paul Rouget [:paul]]

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]]

I removed the queryInterface for this.contentViewer. This is actually needed:
this.contentViewer = aBrowser.docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer);

Comment 31

[Justin Dolske [:Dolske]]

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]]

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]]

Attachment: patch v1.1 // tabbrowser changes

Addressed Dolske's comments

Comment 34

[Paul Rouget [:paul]]

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]]

Attachment: patch v1.1 // tool code

Addressed harth's comments

Comment 36

[Dave Camp (:dcamp)]

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]]

Attachment: patch v1.3

Comment 38

[Paul Rouget [:paul]]

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: [e-mail]]]

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]]

(In reply to :Gavin Sharp (use [e-mail] 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]]

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]]

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]]

Attachment: 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)]

These patches don't apply anymore.  Paul, can you rebase them please?

Comment 45

[Kevin Dangoor]

(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]]

Attachment: patch v1.1 // tabbrowser changes

Comment 47

[Paul Rouget [:paul]]

Attachment: patch v1.4 // tool code

Comment 48

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=ad8943fe25ca
Builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ad8943fe25ca

Comment 49

[David Flanagan [:djf]]

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]]

(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)]

Review ping Dao?  Thanks.

Comment 52

[David Flanagan [:djf]]

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]]

(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]]

Attachment: patch v1.5

Addressed Shorlander's comments.

Comment 55

[Paul Rouget [:paul]]

Dão, review ping?

Comment 56

[Paul Rouget [:paul]]

(there are 2 patches to apply here, first "patch v1.1 // tabbrowser changes" then "patch v1.5")

Comment 57

[:Gavin Sharp [email: [e-mail]]]

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]]

(In reply to :Gavin Sharp (use [e-mail] 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]]

Attachment: patch to land

Addressed Gavin's comments. Merged the 2 patches. Ready to land

Comment 61

[Paul Rouget [:paul]]

Attachment: patch to land

preffed on as all the blockers in bug 752857 are fixed

Comment 62

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/9bad0808a691

Comment 63

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/9bad0808a691

Comment 64

[Rimas Kudelis [use needinfo to ping]]

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]]

(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]

"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]]

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 69

[pd]

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]]

(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]

(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]]

(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 (he/him) [:past] (please ni?)]

In my Galaxy S II Fennec Nightly says 320x460.

Comment 74

[pd]

Attachment: http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S

Comment 75

[pd]

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]

Attachment: http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S

Comment 77

[Panos Astithas (he/him) [:past] (please ni?)]

Indeed, XUL Fennec (the release version of Firefox for Android) is 320x508 in my S II as well.

Comment 78

[pd]

(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]]

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]

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]]

(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.