Closed Bug 1029371 Opened 6 years ago Closed 5 years ago

Make @media sidebar interact with responsive mode

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(firefox46 fixed, relnote-firefox 46+)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed
relnote-firefox --- 46+

People

(Reporter: ntim, Assigned: ntim)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 11 obsolete files)

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.
What do you guys think ?
Flags: needinfo?(paul)
Flags: needinfo?(fayearthur)
I like the idea a lot.
Flags: needinfo?(paul)
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?
Flags: needinfo?(fayearthur)
(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.
(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
Attached patch WIP (obsolete) — Splinter Review
Well, this works but I haven't figured out how to set the size of the responsive mode.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8469072 - Flags: feedback?(paul)
Attachment #8469072 - Flags: feedback?(fayearthur)
Attachment #8469072 - Attachment description: media-responsive.patch → WIP
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?
Attachment #8469072 - Flags: feedback?(fayearthur) → feedback+
It would be extremely cool if we could linkify the queries in codemirror too.
Linkifying seems nice, except, I'm not very talented in making regexes (which are I guess needed for this) :/
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).
Attachment #8469072 - Flags: feedback?(paul) → feedback-
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
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.
Mentor: fayearthur, paul
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
Assignee: nobody → ntim007
Mentor: fayearthur, paul
Status: NEW → ASSIGNED
Attached patch Patch v0.1 (obsolete) — Splinter Review
Attachment #8565031 - Flags: feedback?(paul)
Attachment #8565031 - Flags: feedback?(bgrinstead)
Comment on attachment 8565031 [details] [diff] [review]
Patch v0.1

The code is quite ugly, and still needs some tests.
Attachment #8469072 - Attachment is obsolete: true
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).
Attachment #8565031 - Flags: feedback?(bgrinstead) → feedback+
(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.
Flags: needinfo?(bgrinstead)
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;
  ....

}
(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.
Flags: needinfo?(bgrinstead)
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?
(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.
Attached patch Patch (obsolete) — Splinter Review
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/ntim.bugs@gmail.com-ff0287056bb6/
Try push that came with it : https://tbpl.mozilla.org/?tree=Try&rev=ff0287056bb6
Attachment #8565900 - Flags: ui-review?(shorlander)
Attachment #8565900 - Flags: review?(paul)
Attachment #8565900 - Flags: review?(bgrinstead)
Attachment #8565031 - Attachment is obsolete: true
Attachment #8565031 - Flags: feedback?(paul)
Attached image Screenshot
Stephen, here's a screenshot to ease up your ui-review.
The grayed out media queries are unmatched ones.
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
Attachment #8565900 - Flags: review?(bgrinstead) → feedback?(mgoodwin)
(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.
(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.
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 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)?
Attachment #8565900 - Flags: feedback?(mgoodwin) → feedback-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8565900 - Attachment is obsolete: true
Attachment #8565900 - Flags: ui-review?(shorlander)
Attachment #8565900 - Flags: review?(paul)
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.
Sorry for the spam :( I did not read your comment, that you purposely left this for later.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8592695 - Attachment is obsolete: true
Attachment #8592801 - Flags: review?(paul)
Attachment #8592801 - Flags: review?(bgrinstead)
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.
Attachment #8592801 - Flags: review?(paul) → review+
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?
Attachment #8592801 - Flags: review?(bgrinstead) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
This patch :
- Fixes the security issue
- Fixes the test
Attachment #8592801 - Attachment is obsolete: true
Attachment #8628290 - Flags: review?(bgrinstead)
Attached patch Patch v2.1 (obsolete) — Splinter Review
removes some leftover cruft and some trailing whitespaces
Attachment #8628290 - Attachment is obsolete: true
Attachment #8628290 - Flags: review?(bgrinstead)
Attachment #8628292 - Flags: review?(bgrinstead)
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.
Attachment #8628292 - Flags: review?(mgoodwin)
(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.
(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 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.
Attachment #8628292 - Flags: review?(mgoodwin) → review+
(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.
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
Addresses comments from paul and removes the textContent loop.
Attachment #8628292 - Attachment is obsolete: true
Attachment #8628292 - Flags: review?(bgrinstead)
Attachment #8629659 - Flags: review?(bgrinstead)
(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 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
Attachment #8629659 - Flags: review?(bgrinstead) → review+
Attachment #8629659 - Attachment is obsolete: true
Attachment #8631370 - Flags: review+
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
(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.
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.
Attachment #8631370 - Attachment is obsolete: true
Attachment #8631534 - Flags: review+
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).
Attachment #8631534 - Attachment is obsolete: true
Attachment #8702166 - Flags: review+
Forgot to qref the changes that actually fixed the test.
Attachment #8702166 - Attachment is obsolete: true
Attachment #8702167 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/075e39cc5f6e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Keywords: dev-doc-needed
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)]:
relnote-firefox: --- → ?
Seems like this is already in the relnotes, maybe the relnote-firefox flag should be set to 46+.
Flags: needinfo?(lhenry)
Yes, thanks!
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(ntim.bugs)
(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 :)
Flags: needinfo?(ntim.bugs)
[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
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) {}.
(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?
Flags: needinfo?(vertova)
(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
Flags: needinfo?(vertova)
Depends on: 1269479
Depends on: CVE-2017-7798
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.