Implement a "Responsive Design" tool

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

(Depends on: 5 bugs, Blocks: 1 bug)

Trunk
Firefox 15
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(4 attachments, 23 obsolete attachments)

3.66 MB, image/png
Details
64.39 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
71.59 KB, patch
Details | Diff | Splinter Review
46.78 KB, image/png
Details
(Assignee)

Description

5 years ago
Created attachment 619038 [details]
mockup
(Assignee)

Updated

5 years ago
Depends on: 749626
(Assignee)

Comment 1

5 years ago
Created attachment 619041 [details] [diff] [review]
patch v0.1 // browser code
(Assignee)

Comment 2

5 years ago
Created attachment 619042 [details] [diff] [review]
patch v0.1 // devtools code
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

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

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Updated

5 years ago
blocking-kilimanjaro: --- → ?
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? :)

Updated

5 years ago
blocking-kilimanjaro: ? → +
(Assignee)

Comment 9

5 years ago
Created attachment 620365 [details] [diff] [review]
patch v0.1 // browser code
(Assignee)

Updated

5 years ago
Attachment #619041 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 620366 [details] [diff] [review]
patch v0.2 // devtools code
(Assignee)

Updated

5 years ago
Attachment #619042 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 620367 [details] [diff] [review]
patch v0.2 // browser code
(Assignee)

Updated

5 years ago
Attachment #620365 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 746390
(Assignee)

Comment 12

5 years ago
Created attachment 620653 [details] [diff] [review]
patch v0.2 // browser code

Now with tests.
(Assignee)

Updated

5 years ago
Attachment #620366 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 620661 [details] [diff] [review]
patch v0.2 // browser code

rebased
(Assignee)

Updated

5 years ago
Attachment #620653 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 620735 [details] [diff] [review]
patch v0.3 // browser code
(Assignee)

Updated

5 years ago
Attachment #620367 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 620736 [details] [diff] [review]
patch v0.3 // tool code
(Assignee)

Updated

5 years ago
Attachment #620661 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 17

5 years ago
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/prouget@mozilla.com-e92a43401cad
(Assignee)

Comment 18

5 years ago
Created attachment 621048 [details] [diff] [review]
patch v1 // tabbrowser changes
(Assignee)

Updated

5 years ago
Attachment #620735 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
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
Attachment #621048 - Flags: review?(dolske)
(Assignee)

Updated

5 years ago
Depends on: 751910
(Assignee)

Comment 20

5 years ago
Created attachment 621068 [details] [diff] [review]
patch v0.4 // tool code
(Assignee)

Updated

5 years ago
Attachment #620736 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
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)
(Assignee)

Comment 22

5 years ago
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.
Attachment #621068 - Flags: feedback?(dcamp)

Comment 23

5 years ago
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?
Attachment #621068 - Flags: feedback?(dcamp) → feedback+
(Assignee)

Comment 24

5 years ago
Created attachment 621567 [details] [diff] [review]
patch v0.5 // tool code

Windows support.
(Assignee)

Updated

5 years ago
Attachment #621068 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 752473
(Assignee)

Comment 25

5 years ago
Created attachment 621927 [details] [diff] [review]
patch v0.6 // tool code

Documentation. Addressed Dave's comments. Better Windows support.
(Assignee)

Updated

5 years ago
Attachment #621567 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #621927 - Attachment description: patch v0.6 → patch v0.6 // tool code
(Assignee)

Updated

5 years ago
Depends on: 752847
(Assignee)

Updated

5 years ago
Depends on: 752848
(Assignee)

Updated

5 years ago
Depends on: 752850
(Assignee)

Updated

5 years ago
Depends on: 752851
(Assignee)

Comment 26

5 years ago
I think the crash was unrelated (can't reproduce).
(Assignee)

Updated

5 years ago
Blocks: 752857
(Assignee)

Updated

5 years ago
No longer blocks: 752857
No longer depends on: 751910, 752473, 752847
(Assignee)

Updated

5 years ago
Blocks: 752857
(Assignee)

Comment 27

5 years ago
Created attachment 621948 [details] [diff] [review]
patch v1 // tool code
(Assignee)

Updated

5 years ago
Attachment #621927 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 621949 [details] [diff] [review]
patch v1 // tool code
(Assignee)

Updated

5 years ago
Attachment #621948 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Comment on attachment 621949 [details] [diff] [review]
patch v1 // tool code

(you'll need to apply the patch from bug 749626 first)
Attachment #621949 - Flags: review?(dcamp)

Updated

5 years ago
Attachment #621949 - Flags: feedback?(fayearthur)
(Assignee)

Updated

5 years ago
No longer blocks: 752857
(Assignee)

Comment 30

5 years ago
I removed the queryInterface for this.contentViewer. This is actually needed:
this.contentViewer = aBrowser.docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer);
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! :)
Attachment #621048 - Flags: review?(dolske) → review+
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
Attachment #621949 - Flags: feedback?(fayearthur) → feedback+
(Assignee)

Comment 33

5 years ago
Created attachment 623082 [details] [diff] [review]
patch v1.1 // tabbrowser changes

Addressed Dolske's comments
(Assignee)

Updated

5 years ago
Attachment #621048 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
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).
(Assignee)

Comment 35

5 years ago
Created attachment 623105 [details] [diff] [review]
patch v1.1 // tool code

Addressed harth's comments
(Assignee)

Updated

5 years ago
Attachment #621949 - Attachment is obsolete: true
Attachment #621949 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Attachment #623105 - Flags: review?(dcamp)

Comment 36

5 years ago
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.
Attachment #623105 - Flags: review?(dcamp) → review+
(Assignee)

Comment 37

5 years ago
Created attachment 624335 [details] [diff] [review]
patch v1.3
(Assignee)

Updated

5 years ago
Attachment #623105 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
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.
Attachment #624335 - Flags: review?(dolske)
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.
(Assignee)

Comment 40

5 years ago
(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.
Depends on: 755953
(Assignee)

Comment 41

5 years ago
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.
(Assignee)

Comment 42

5 years ago
Comment on attachment 624335 [details] [diff] [review]
patch v1.3

Cancelling review until I know if we should include the zoom feature or not.
Attachment #624335 - Flags: review?(dolske)
(Assignee)

Comment 43

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #624335 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625089 - Flags: review?(dao)

Comment 44

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

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
Created attachment 625973 [details] [diff] [review]
patch v1.1 // tabbrowser changes
(Assignee)

Updated

5 years ago
Attachment #623082 - Attachment is obsolete: true
(Assignee)

Comment 47

5 years ago
Created attachment 625974 [details] [diff] [review]
patch v1.4 // tool code
(Assignee)

Updated

5 years ago
Attachment #625089 - Attachment is obsolete: true
Attachment #625089 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #625974 - Flags: review?(dao)
(Assignee)

Comment 48

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ad8943fe25ca
Builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/prouget@mozilla.com-ad8943fe25ca
(Assignee)

Updated

5 years ago
Depends on: 757477
(Assignee)

Updated

5 years ago
Depends on: 752473
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.
(Assignee)

Comment 50

5 years ago
(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)

Updated

5 years ago
Depends on: 758011

Comment 51

5 years ago
Review ping Dao?  Thanks.
(Assignee)

Updated

5 years ago
No longer depends on: 757477
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.
(Assignee)

Comment 53

5 years ago
(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.
(Assignee)

Comment 54

5 years ago
Created attachment 627693 [details] [diff] [review]
patch v1.5

Addressed Shorlander's comments.
(Assignee)

Updated

5 years ago
Attachment #625974 - Attachment is obsolete: true
Attachment #625974 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #627693 - Flags: review?(dao)
(Assignee)

Comment 55

5 years ago
Dão, review ping?
(Assignee)

Comment 56

5 years ago
(there are 2 patches to apply here, first "patch v1.1 // tabbrowser changes" then "patch v1.5")
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.
Attachment #627693 - Flags: review?(dao) → review+
(Assignee)

Comment 58

5 years ago
(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!
(Assignee)

Comment 59

5 years ago
Created attachment 629154 [details] [diff] [review]
patch to land

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

Updated

5 years ago
Attachment #625973 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Duplicate of this bug: 752857
(Assignee)

Comment 61

5 years ago
Created attachment 629157 [details] [diff] [review]
patch to land

preffed on as all the blockers in bug 752857 are fixed
(Assignee)

Updated

5 years ago
Attachment #629154 - Attachment is obsolete: true
(Assignee)

Comment 62

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/9bad0808a691
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9bad0808a691
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Assignee)

Updated

5 years ago
Depends on: 761169, 761165

Comment 64

5 years ago
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?
(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...
(Assignee)

Updated

5 years ago
Depends on: 761431

Comment 66

5 years ago
"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/
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.

Updated

5 years ago
Depends on: 762395
(Assignee)

Updated

5 years ago
Duplicate of this bug: 731153
(Assignee)

Updated

5 years ago
Depends on: 762846
(Assignee)

Updated

5 years ago
Depends on: 762848

Comment 69

5 years ago
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.
(Assignee)

Comment 70

5 years ago
(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.
Depends on: 765413

Comment 71

5 years ago
(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.
(Assignee)

Comment 72

5 years ago
(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.
In my Galaxy S II Fennec Nightly says 320x460.

Comment 74

5 years ago
Created attachment 634277 [details]
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S

Comment 75

5 years ago
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

5 years ago
Created attachment 634279 [details]
http://jsbin.com/eyecap/9 in Firefox 10.0.5 on a Samsung Galaxy S
Attachment #634277 - Attachment is obsolete: true
Indeed, XUL Fennec (the release version of Firefox for Android) is 320x508 in my S II as well.

Comment 78

5 years ago
(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.
(Assignee)

Comment 79

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 771046
(Assignee)

Updated

5 years ago
Depends on: 771043
(Assignee)

Updated

5 years ago
Depends on: 777972
(Assignee)

Updated

5 years ago
Depends on: 774055
(Assignee)

Updated

5 years ago
Depends on: 778174
(Assignee)

Updated

5 years ago
Depends on: 778169
(Assignee)

Updated

5 years ago
Depends on: 799471

Comment 80

5 years ago
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.
(Assignee)

Comment 81

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.