1029371 - Make @media sidebar interact with responsive mode

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Tim Nguyen :ntim]

It'd be nice if the dimension related media queries in the media sidebar could interact with the responsive mode.

The UI could be like this :  
(max-width: 500px)   [\] :22

[\] corresponds to the responsive mode button, clicking on it would launch and resize responsive mode to 500px.

Comment 1

[Tim Nguyen :ntim]

What do you guys think ?

Comment 2

[Paul Rouget [:paul]]

I like the idea a lot.

Comment 3

[Heather Arthur [:harth]]

I've thought about this. Didn't know what to do about resizing to like 1200px if your screen isn't that big, what do you think?

Comment 4

[Paul Rouget [:paul]]

(In reply to Heather Arthur [:harth] from comment #3)
> I've thought about this. Didn't know what to do about resizing to like
> 1200px if your screen isn't that big, what do you think?

This is a corner case. I would not worry too much about that. The responsive mode is supposed to correctly handle bigger-than-viewport pages.

Comment 5

[Tim Nguyen :ntim]

(In reply to Heather Arthur [:harth] from comment #3)
> I've thought about this. Didn't know what to do about resizing to like
> 1200px if your screen isn't that big, what do you think?

Well, currently, we show a horizontal scrollbar.

Another case : if the media query includes 2 width/2 height conditions. In that case, we could show the responsive mode next to each condition. For example :

(min-width:200px) [/] and (max-width:700px) [/]

Chrome uses a slider for that edge case (well chrome has a totally different UI too) : note.io/1woyvj1

Comment 6

[Tim Nguyen :ntim]

Attachment: WIP

Well, this works but I haven't figured out how to set the size of the responsive mode.

Comment 7

[Heather Arthur [:harth]]

Comment on attachment 8469072 [details] [diff] [review]
WIP

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

Awesome! The icon is a little big/jarring right now. What do ya'll think about linkifying the words "max-width:790px" instead of adding an extra button?

Comment 8

[Heather Arthur [:harth]]

It would be extremely cool if we could linkify the queries in codemirror too.

Comment 9

[Tim Nguyen :ntim]

Linkifying seems nice, except, I'm not very talented in making regexes (which are I guess needed for this) :/

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 8469072 [details] [diff] [review]
WIP

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

This feature will sadly break the e10s boundaries, and will fail with non-firefox targets (Firefox OS for example). I guess we should make the responsive mode remotable first (should be fairly simple).

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +802,5 @@
> +          let mediaHeight = isHeightCond ? mediaVal : null;
> +          let responsiveButton = this._panelDoc.createElement("button");
> +          responsiveButton.textContent = "R";
> +          responsiveButton.className = "media-responsive-mode devtools-toolbarbutton";
> +          responsiveButton.addEventListener("click", this._launchResponsiveMode.bind(this, mediaWidth, mediaHeight));

Use a bound_ function, like other functions in this file. And don't forget the removeEventListener.

@@ +836,5 @@
>  
> +  _launchResponsiveMode: function(width, height) {
> +    let tab = this._target._tab;
> +    let win = this._target._tab.ownerGlobal;
> +    ResponsiveUIManager.toggle(win, tab);

So this is not e10s-proof. I don't know if we want to introduce a e10s breakage now.

Also - you'll need to write a test for that (then at least it will turn orange when we activate e10s tests).

Comment 11

[Tim Nguyen :ntim]

This should be good to work on now that e10s support has landed in bug 1067145. I won't have time to work on this though.

Comment 12

[Tim Nguyen :ntim]

A few notes to myself :
* /^(min\-|max\-)(width|height): \d+\w+/ig
* http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm#862
* http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#819

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v0.1

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8565031 [details] [diff] [review]
Patch v0.1

The code is quite ugly, and still needs some tests.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8565031 [details] [diff] [review]
Patch v0.1

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

Cool idea - took me a minute to figure it out since you need to use it on the right page.  I'd suggest just linkifying the "NNNpx" portion of the string "max-width: NNNpx" just to make it more clear when it's applying and not (because right now it is just using text color to specify that so you only see the parens change).

Comment 16

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Comment on attachment 8565031 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8565031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool idea - took me a minute to figure it out since you need to use it on
> the right page.  I'd suggest just linkifying the "NNNpx" portion of the
> string "max-width: NNNpx" just to make it more clear when it's applying and
> not (because right now it is just using text color to specify that so you
> only see the parens change).
I think it's better to linkify everything, so we can clearly see the link between width/height and the value. As for the clarity, I could just set the opacity to 0.5 when the media query isn't applying. 

Also, I'm not sure if this is coded the right way, the code is quite ugly (mainly the changes in StyleEditorUI.jsm), but I'm not sure what to do to improve it (mainly don't know where things belong). Can you guide me a bit ? Thanks.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8565031 [details] [diff] [review]
Patch v0.1

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

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +826,5 @@
>          div.appendChild(cond);
> +        if (this._target._tab.tagName == "tab") {
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> +          var widthAndHeightCond = cond.querySelectorAll(".media-responsive-mode-toggle");
> +          for (let $el of widthAndHeightCond) {

I would separate the templating here from the event handlers.  This will cut down on nesting and make it easier to follow.

You could set up a single click handler on cond (named this._onMediaSizeClick or similar) and then check to make sure that a link was clicked with some code like:

_onMediaSizeClick: function(e) {
  if (!e.target.matches(".media-responsive-mode-toggle")) {
    return;
  }

  let conditionText = e.target.textContent;
  ....

}

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #16)
> (In reply to Brian Grinstead [:bgrins] from comment #15)
> > Comment on attachment 8565031 [details] [diff] [review]
> > Patch v0.1
> > 
> > Review of attachment 8565031 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Cool idea - took me a minute to figure it out since you need to use it on
> > the right page.  I'd suggest just linkifying the "NNNpx" portion of the
> > string "max-width: NNNpx" just to make it more clear when it's applying and
> > not (because right now it is just using text color to specify that so you
> > only see the parens change).
> I think it's better to linkify everything, so we can clearly see the link
> between width/height and the value. As for the clarity, I could just set the
> opacity to 0.5 when the media query isn't applying. 

This would be a good feature to ask UX about (while providing screenshots/screencast of what is happening for context).  Linking to open up responsive mode is a cool idea, but we also need to make sure that it isn't confusing when responsive mode isn't available (like if debugging a target on a device).  Also, see comment 17 for a recommendation on how to clean up the code.

Comment 19

[Paul Rouget [:paul]]

Comment on attachment 8565031 [details] [diff] [review]
Patch v0.1

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

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +122,5 @@
> +   * @param aWidth width of the browser.
> +   * @param aHeight height of the browser.
> +   */
> +  setSize: function(aTab, aWidth, aHeight) {
> +    ActiveTabs.get(aTab).setSize(aWidth, aHeight);

No need to create a new method.
Use mgr.getResponsiveUIForTab(aTab).setSize() from the styleeditor.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +866,5 @@
> +  _launchResponsiveMode: function(options = {}) {
> +    let tab = this._target._tab;
> +    let win = this._target._tab.ownerGlobal;
> +
> +    ResponsiveUIManager.launch(win, tab);

So this works with E10S?

Comment 20

[Tim Nguyen :ntim]

(In reply to Paul Rouget [:paul] from comment #19)
> Comment on attachment 8565031 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8565031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +122,5 @@
> > +   * @param aWidth width of the browser.
> > +   * @param aHeight height of the browser.
> > +   */
> > +  setSize: function(aTab, aWidth, aHeight) {
> > +    ActiveTabs.get(aTab).setSize(aWidth, aHeight);
> 
> No need to create a new method.
> Use mgr.getResponsiveUIForTab(aTab).setSize() from the styleeditor.
Will do. Thanks.

> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +866,5 @@
> > +  _launchResponsiveMode: function(options = {}) {
> > +    let tab = this._target._tab;
> > +    let win = this._target._tab.ownerGlobal;
> > +
> > +    ResponsiveUIManager.launch(win, tab);
> 
> So this works with E10S?
Yes, it does.

Comment 21

[Tim Nguyen :ntim]

Attachment: Patch

Addressed all comments and added a test.
Paul, The test leaks just after the responsive mode launches, I'm not sure why, can you take a look ? Thanks. Also, is there a way to set *only* the height or the width of the responsive mode ?

--
Stephen, basically this bug lets you launch the responsive mode from the style editor @media sidebar. It linkifies all width and height media conditions, and when you click on those links, it launches the responsive mode at the right size.
Can you check if the UI is right for you and if the difference between unmatched media queries and match media queries is pronounced enough ? Thanks.

Here's a try build : https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ff0287056bb6/
Try push that came with it : https://tbpl.mozilla.org/?tree=Try&rev=ff0287056bb6

Comment 22

[Tim Nguyen :ntim]

Attachment: Screenshot

Stephen, here's a screenshot to ease up your ui-review.
The grayed out media queries are unmatched ones.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8565900 [details] [diff] [review]
Patch

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

The style editor code changes look generally OK (I'll let Paul review the responsive design changes).

Mark, can you take a look at the change with regards to replacing media query text with a link?  The situation is if you open up the style editor on http://bgrins.github.io/devtools-demos/styleeditor/media.html then flip to the 'media.css' file, you will see a sidebar on the right that lists media rules.  This patch would match 'max-width: 400px' and replace that text with a link.  I think it's alright (see note below), but I want to confirm that this code doesn't somehow open up an XSS hole that would allow a specially-crafted media query to execute scripts with chrome privileges.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +827,5 @@
>          if (!rule.matches) {
>            cond.classList.add("media-condition-unmatched");
>          }
> +        if (this._target._tab.tagName == "tab") {
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");

I'm worried about setting innerHTML here since media query text is user created.  But I think it's OK since the value was originally set with textContent and is fetched again with textContent before doing the replacement.

Also, media queries seem to only be returned if they are generally well formed and either don't parse or come back as 'not all' if I enter poorly formed values like &lt;script&gt; but I don't know if there is some concoction of css escaping that would allow malicious input to be added into a media query.  So I'd feel better about having a security person take a look at this.

@@ +849,5 @@
>      }.bind(this)).then(null, Cu.reportError);
>    },
>  
>    /**
> +    * Called when a media condition is clicked

Add a sentence about what this function is meant to do

@@ +859,5 @@
> +    if (!e.target.matches(".media-responsive-mode-toggle")) {
> +      return;
> +    }
> +    let conditionText = e.target.textContent;
> +    let isWidthCond = conditionText.indexOf("width") > -1;

This isn't case insensitive, but the comparison up above is.  It seems like the platform is lowercasing this text anyway, but let's be consistent.

@@ +875,5 @@
> +   * @param  {object} options
> +   *         Object with width or/and height properties.
> +   */
> +  _launchResponsiveMode: function(options = {}) {
> +    let tab = this._target._tab;

use the non-underscored version of this._target.tab here

@@ +883,5 @@
> +    if (options.width && options.height) {
> +      ResponsiveUIManager.getResponsiveUIForTab(tab).setSize(options.width, options.height);
> +    }
> +    else if (options.width) {
> +      ResponsiveUIManager.getResponsiveUIForTab(tab).setSize(options.width, 480);

If responsive ui was already opened before the link in this case it would end up changing it's height to 480.  Not sure if this is intended

Comment 24

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Comment on attachment 8565900 [details] [diff] [review]
> Patch
> 
> Review of attachment 8565900 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +827,5 @@
> >          if (!rule.matches) {
> >            cond.classList.add("media-condition-unmatched");
> >          }
> > +        if (this._target._tab.tagName == "tab") {
> > +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> 
> I'm worried about setting innerHTML here since media query text is user
> created.  But I think it's OK since the value was originally set with
> textContent and is fetched again with textContent before doing the
> replacement.
> 
> Also, media queries seem to only be returned if they are generally well
> formed and either don't parse or come back as 'not all' if I enter poorly
> formed values like &lt;script&gt; but I don't know if there is some
> concoction of css escaping that would allow malicious input to be added into
> a media query.  So I'd feel better about having a security person take a
> look at this.
I just tried typing in a script tag in multiple ways, it (and more generally invalid conditions) gets parsed as `not all` all the time. I don't think we should worry too much about that, since we also use textContent on the previous lines, but I'll let Mark verify that.

> @@ +859,5 @@
> > +    if (!e.target.matches(".media-responsive-mode-toggle")) {
> > +      return;
> > +    }
> > +    let conditionText = e.target.textContent;
> > +    let isWidthCond = conditionText.indexOf("width") > -1;
> 
> This isn't case insensitive, but the comparison up above is.  It seems like
> the platform is lowercasing this text anyway, but let's be consistent.
If you mean the regex vs this condition, will do.

> @@ +875,5 @@
> > +   * @param  {object} options
> > +   *         Object with width or/and height properties.
> > +   */
> > +  _launchResponsiveMode: function(options = {}) {
> > +    let tab = this._target._tab;
> 
> use the non-underscored version of this._target.tab here
This is cleaner, but I'm curious, are there other differences between the two ?

> @@ +883,5 @@
> > +    if (options.width && options.height) {
> > +      ResponsiveUIManager.getResponsiveUIForTab(tab).setSize(options.width, options.height);
> > +    }
> > +    else if (options.width) {
> > +      ResponsiveUIManager.getResponsiveUIForTab(tab).setSize(options.width, 480);
> 
> If responsive ui was already opened before the link in this case it would
> end up changing it's height to 480.  Not sure if this is intended
Yeah, I'll need to split up setSize into setWidth and setHeight, there's no way to set only one dimension yet.

Comment 25

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> I'm worried about setting innerHTML here since media query text is user
> created.  But I think it's OK since the value was originally set with
> textContent and is fetched again with textContent before doing the
> replacement.

It may well be OK but I'm not sure (and can't be without a bit of testing).

Since we already know what element we're adding, is there a reason we can't just add the nodes rather than using innerHTML? That would be the safer option.

Comment 26

[Frederik Braun [:freddy]]

Mark asked me to do some cursory testing.
Let's not play with a footgun when there are easy & safer alternatives:

* Use the ParserUtils to sanitize the DOM string (this is probably too much overhead)
* Add the link with document.createElement, <template>, etc. and then fill out the text using textContent.

Comment 27

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8565900 [details] [diff] [review]
Patch

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

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +827,5 @@
>          if (!rule.matches) {
>            cond.classList.add("media-condition-unmatched");
>          }
> +        if (this._target._tab.tagName == "tab") {
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");

Can't we just set the textContent on the resulting <a> after you've created it? That way, there can't be any user data supplied to innerHTML. Better still, can we not use innerHTML at all (instead, append a new <a> child)?

Comment 28

[Tim Nguyen :ntim]

Attachment: Patch

This patch adds setHeight and setWidth functions to the responsive mode, and makes use of them.

Gave an attempt to fix the test, but ResponsiveUIManager is still leaking.
This patch also doesn't address the security issue yet.

Comment 29

[Frederik Braun [:freddy]]

Comment on attachment 8592695 [details] [diff] [review]
Patch

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

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +840,5 @@
>          }
> +        if (this._target._tab.tagName == "tab") {
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> +          cond.addEventListener("click", this._onMediaConditionClick.bind(this));
> +        }

Please remember to address the feedback from comment 25 to comment 27 about innerHTML in your next iteration of this patch.

Comment 30

[Frederik Braun [:freddy]]

Sorry for the spam :( I did not read your comment, that you purposely left this for later.

Comment 31

[Tim Nguyen :ntim]

Attachment: Patch

Brian, can you review the style editor changes ? The test still needs to be fixed, but can you give a first pass review of the test ?

Paul, can you review the responsive mode changes ? I have added a setHeight and a setWidth function.

Comment 32

[Paul Rouget [:paul]]

Comment on attachment 8592801 [details] [diff] [review]
Patch

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

r+ with comments addressed.

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +60,5 @@
> +   *
> +   * @param aWindow the main window.
> +   * @param aTab the tab targeted.
> +   */
> +  launch: function(aWindow, aTab) {

s/launch/runIfNeeded/

@@ +93,5 @@
>     */
>    handleGcliCommand: function(aWindow, aTab, aCommand, aArgs) {
>      switch (aCommand) {
>        case "resize to":
>          if (!this.isActiveForTab(aTab)) {

The `if` is not needed anymore.

@@ +99,5 @@
>          }
>          ActiveTabs.get(aTab).setSize(aArgs.width, aArgs.height);
>          break;
>        case "resize on":
>          if (!this.isActiveForTab(aTab)) {

same.

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8592801 [details] [diff] [review]
Patch

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

Style editor changes look generally good (see comments).  f+ because we still need a solution for the sec issue (something from comment 25, 26, or 27)

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +838,4 @@
>          if (!rule.matches) {
>            cond.classList.add("media-condition-unmatched");
>          }
> +        if (this._target._tab.tagName == "tab") {

Should be 

if (this._target.tab)

::: browser/devtools/styleeditor/test/browser_styleeditor_media_sidebar.js
@@ +91,5 @@
> +     "media width rule should have a link to toggle responsive mode");
> +
> +  conditions[2].querySelector(responsiveModeToggleClass).click();
> +  ResponsiveUIManager.on("on", () => {
> +    ok(ResponsiveUIManager.isActiveForTab(tab), "Responsive mode should be active.");

Can you also assert that the correct width/height have been set here?

Comment 34

[Tim Nguyen :ntim]

Attachment: Patch v2

This patch :
- Fixes the security issue
- Fixes the test

Comment 35

[Tim Nguyen :ntim]

Attachment: Patch v2.1

removes some leftover cruft and some trailing whitespaces

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8628292 [details] [diff] [review]
Patch v2.1

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

Mark, do the changes in StyleEditorUI.jsm satisfy any security issues you raised in Comment 25 / 27?  Here's an interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8592801&action=interdiff&newid=8628292&headers=1.

Tim, can you take a look at the performance of this?  I checked it out on http://www.cnn.com/ and it seemed extremely slow when clicking one of the (many) links on the first CSS file in the style editor.  Not sure if that's just my profile or what.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +887,5 @@
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> +          cond.addEventListener("click", this._onMediaConditionClick.bind(this));
> +          let links = cond.querySelectorAll("a");
> +          for (let link of links) {
> +            link.textContent = link.textContent;

I'm not sure I get the scenario where this is doing anything.  The contents of $& has already been set into the innerHTML so presumably any script could have run at that time if it were able to be injected.  However, cond.innerHTML is being set to cond.textContent.replace(...) so presumably it's already been sanitized then.  Forwarding this to mgoodwin.

Comment 37

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #36)
> Comment on attachment 8628292 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8628292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mark, do the changes in StyleEditorUI.jsm satisfy any security issues you
> raised in Comment 25 / 27?  Here's an interdiff:
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8592801&action=interdiff&newid=8628292&headers=1.
> 
> Tim, can you take a look at the performance of this?  I checked it out on
> http://www.cnn.com/ and it seemed extremely slow when clicking one of the
> (many) links on the first CSS file in the style editor.  Not sure if that's
> just my profile or what.
> 
> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +887,5 @@
> > +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> > +          cond.addEventListener("click", this._onMediaConditionClick.bind(this));
> > +          let links = cond.querySelectorAll("a");
> > +          for (let link of links) {
> > +            link.textContent = link.textContent;
> 
> I'm not sure I get the scenario where this is doing anything.  The contents
> of $& has already been set into the innerHTML so presumably any script could
> have run at that time if it were able to be injected.  However,
> cond.innerHTML is being set to cond.textContent.replace(...) so presumably
> it's already been sanitized then.  Forwarding this to mgoodwin.

cond is being appended after this code. So this should be ok. Also, I don't think any of this security stuff is necessary. I mean it's already sanitized once using textContent, and invalid values get parsed as not all.

Comment 38

[Mark Goodwin [:mgoodwin]]

(In reply to Tim Nguyen [:ntim] from comment #37)
> cond is being appended after this code. So this should be ok. Also, I don't
> think any of this security stuff is necessary. I mean it's already sanitized
> once using textContent, and invalid values get parsed as not all.

I'm also pretty sure this is safe in this context - but innerHTML is a footgun we shouldn't use unless there is no alternative, IMO.

Can we please get into the habit of not doing this?

Comment 39

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8628292 [details] [diff] [review]
Patch v2.1

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

Grudging r+ from me since there's no obvious exploitable flaw - but please stop using innerHTML when you have options.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +883,5 @@
>          if (!rule.matches) {
>            cond.classList.add("media-condition-unmatched");
>          }
> +        if (this._target._tab.tagName == "tab") {
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");

Please don't use innerHTML when it's avoidable. At the moment this is probably fine but there's the potential for future problems when changes are made to this code.

@@ +887,5 @@
> +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> +          cond.addEventListener("click", this._onMediaConditionClick.bind(this));
> +          let links = cond.querySelectorAll("a");
> +          for (let link of links) {
> +            link.textContent = link.textContent;

I also think this doesn't do anything; my issue is solely with the use of innerHTML in the first place.

Comment 40

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #36)
> Tim, can you take a look at the performance of this?  I checked it out on
> http://www.cnn.com/ and it seemed extremely slow when clicking one of the
> (many) links on the first CSS file in the style editor.  Not sure if that's
> just my profile or what.

Seems like CNN is pretty slow, I recorded a perf profile when resizing CNN, and it seems that there is a lot of DOM event stuff going on when the page is being resized.

Comment 41

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #40)
> (In reply to Brian Grinstead [:bgrins] from comment #36)
> > Tim, can you take a look at the performance of this?  I checked it out on
> > http://www.cnn.com/ and it seemed extremely slow when clicking one of the
> > (many) links on the first CSS file in the style editor.  Not sure if that's
> > just my profile or what.
> 
> Seems like CNN is pretty slow, I recorded a perf profile when resizing CNN,
> and it seems that there is a lot of DOM event stuff going on when the page
> is being resized.

But it also seems that the code that is updating the @media list is a bit slow as well.

Comment 42

[Tim Nguyen :ntim]

Attachment: Patch v3

Addresses comments from paul and removes the textContent loop.

Comment 43

[Tim Nguyen :ntim]

(In reply to Mark Goodwin [:mgoodwin] from comment #39)
> Comment on attachment 8628292 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 8628292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Grudging r+ from me since there's no obvious exploitable flaw - but please
> stop using innerHTML when you have options.
> 
> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +883,5 @@
> >          if (!rule.matches) {
> >            cond.classList.add("media-condition-unmatched");
> >          }
> > +        if (this._target._tab.tagName == "tab") {
> > +          cond.innerHTML = cond.textContent.replace(/(min\-|max\-)(width|height):\s\d+(px)/ig, "<a href='#' class='media-responsive-mode-toggle'>$&</a>");
> 
> Please don't use innerHTML when it's avoidable. At the moment this is
> probably fine but there's the potential for future problems when changes are
> made to this code.
It's pretty tricky to do this without innerHTML. Using the regex is the most powerful way to do this.

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8629659 [details] [diff] [review]
Patch v3

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

Looks good

::: browser/devtools/styleeditor/test/browser_styleeditor_media_sidebar_links.js
@@ +1,3 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Can you add a simple comment explaining what this test is doing?

@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +Cu.import("resource:///modules/devtools/responsivedesign.jsm");

Please add a newline before this.  Also, can you do `let {ResponsiveUIManager} = Cu.import("resource:///modules/devtools/responsivedesign.jsm", {});`

@@ +52,5 @@
> +
> +  is(conditions[3].querySelectorAll(responsiveModeToggleClass).length, 2,
> +       "There should be 2 linkified items in the media rule");
> +
> +  delete window.ResponsiveUIManager;

You shouldn't need to delete on window anymore if it's imported as suggested above

Comment 45

[Tim Nguyen :ntim]

Attachment: Patch v3.1 (r=bgrins, paul, mgoodwin)

Comment 46

[Tim Nguyen :ntim]

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aa7e3a8be53

Comment 47

[Tim Nguyen :ntim]

Comment on attachment 8631370 [details] [diff] [review]
Patch v3.1 (r=bgrins, paul, mgoodwin)

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

Note to self : remove trailing spaces

Comment 48

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #46)
> Try push :
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aa7e3a8be53

Urgh, the try push was busted, it didn't push my changes with it.

Comment 49

[Tim Nguyen :ntim]

Attachment: Patch v3.2 (r=bgrins, paul, mgoodwin)

Oh, so the test did really fail, anyway, fixed it. It's weird that the revision didn't show up on the left side of the page.

Comment 50

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=589c36c69be0

Comment 51

[Tim Nguyen :ntim]

Attachment: Patch v4 (r=bgrins, paul, mgoodwin)

I think this patch has the intermittent fixed (I can't seem to reproduce it locally after 20 runs of the test). I'll push to try to be sure (it was always failing on XP, so let's see if that's fixed).

Comment 52

[Tim Nguyen :ntim]

Attachment: Patch v4.1 (r=bgrins, paul, mgoodwin)

Forgot to qref the changes that actually fixed the test.

Comment 53

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f8794217552

Comment 54

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=205f81da85e5

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/075e39cc5f6e

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/075e39cc5f6e

Comment 57

[Tim Nguyen :ntim]

Release Note Request (optional, but appreciated)
[Why is this notable]: New Style Editor feature
[Suggested wording]: Links to launch the responsive mode from the Style Editor @media sidebar
[Links (documentation, blog post, etc)]:

Comment 58

[Tim Nguyen :ntim]

Seems like this is already in the relnotes, maybe the relnote-firefox flag should be set to 46+.

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, thanks!

Comment 60

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Tools/Style_Editor#The_media_sidebar for this. Let me know if this covers it.

Comment 61

[Tim Nguyen :ntim]

(In reply to Will Bamberg [:wbamberg] from comment #60)
> I've updated
> https://developer.mozilla.org/en-US/docs/Tools/
> Style_Editor#The_media_sidebar for this. Let me know if this covers it.

Great, thanks! Nice screencast :)

Comment 62

[Mayur Patil]

[bugday-20160323]

STR:
1. Open Web Developer -> Style Editor
2. Open Responsive web design
3. Now go to Style editor. On right side, you will find @media rules

Status: RESOLVED,FIXED --> VERIFIED

Component: 
Name 	        Firefox
Version 	46.0b2
Build ID 	20160314144540
Update Channel 	beta
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS              Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Media rules are working in Firefox 46.0b2. Also they are responding to the RWD.

Thanks to https://developer.mozilla.org/en-US/docs/Tools/Style_Editor#The_media_sidebar

Comment 63

[Francesco]

I've seen this nice new feature after upgrading to FF46.

One thing: media queries in the @media sidebar are clickable if (max|min)-(height|width) is defined in px, e.g. @media screen and (min-width: 800px) {}. They are not if it's defined in em, e.g. @media screen and (min-width: 50em) {}.

Comment 64

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Francesco from comment #63)
> I've seen this nice new feature after upgrading to FF46.
> 
> One thing: media queries in the @media sidebar are clickable if
> (max|min)-(height|width) is defined in px, e.g. @media screen and
> (min-width: 800px) {}. They are not if it's defined in em, e.g. @media
> screen and (min-width: 50em) {}.

Thanks for the note!  Could you file a new bug for this issue?

Comment 65

[Francesco]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #64)
> (In reply to Francesco from comment #63)
> > I've seen this nice new feature after upgrading to FF46.
> > 
> > One thing: media queries in the @media sidebar are clickable if
> > (max|min)-(height|width) is defined in px, e.g. @media screen and
> > (min-width: 800px) {}. They are not if it's defined in em, e.g. @media
> > screen and (min-width: 50em) {}.
> 
> Thanks for the note!  Could you file a new bug for this issue?

I've filed bug 1269479