1120570 - Rule view is not showing media query in source link text

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

At some point, the media text disappeared from the rule view link next to a selector.  This used to work but has regressed.

* Open http://www.webdesignerwall.com/demo/media-queries/
* Highlight the element with a background color that has a media query style applied to it

Expected: the link says something like: "inline:30 @media screen and .... (min-width: 900px)"
Actual: the link says something like "inline:30"

Comment 1

[Brian Grinstead [:bgrins]]

This is a regression - it is working in 34 but stopped working in 35

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: t.html

Mike's test case from Bug 722196

Comment 3

[Daniele "Mte90" Scasciafratte]

Mailing list reference about this regression: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/hma9yQiO9Jg

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: media-links.png

screenshot of the bug

Comment 5

[Ada [:adalucinet]]

Regression range:
(m-c):
Last good revision: 50b689feab5f (2014-10-10)
First bad revision: f74ad36bb97b (2014-10-11)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50b689feab5f&tochange=f74ad36bb97b   

(m-i):
Last good revision: a1e44f48ddbd
First bad revision: f74ad36bb97b
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a1e44f48ddbd&tochange=f74ad36bb97b

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have a simple fix... just need to add a test.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1120570-fix-media-query-in-rule-view-source-link.patch

Just about as simple as it gets.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44aca113e97
https://tbpl.mozilla.org/?tree=Try&rev=e44aca113e97

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8562078 [details] [diff] [review]
1120570-fix-media-query-in-rule-view-source-link.patch

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

::: browser/devtools/styleinspector/test/browser_ruleview_content_01.js
@@ +40,5 @@
> +
> +  let linkText = getRuleViewLinkTextByIndex(ruleView, 1);
> +  is(linkText, "inline:1 @screen and (min-width: 10px)",
> +    "link text contains media query text.");
> +

We should also have an assertion for the second rule link text (to confirm that it is ".testclass, .unmatched" with no media string).

::: toolkit/devtools/server/actors/styles.js
@@ +1254,5 @@
>          let location = {
>            href: source,
>            line: line,
> +          column: column,
> +          mediaText: this.parentRule ? this.parentRule.mediaText : ""

I believe that the `this.location` getter should be where the mediaText property is added, then it will just be passed it into this resolution and you can pass along `mediaText: mediaText`.

I'm not positive about it, but I'd like to at least discuss this before landing.  What is the case where there is no parentSheet?  If this is true, than I believe mediaText will be omitted from this call.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8562078 [details] [diff] [review]
1120570-fix-media-query-in-rule-view-source-link.patch

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

::: browser/devtools/styleinspector/test/browser_ruleview_content_01.js
@@ +40,5 @@
> +
> +  let linkText = getRuleViewLinkTextByIndex(ruleView, 1);
> +  is(linkText, "inline:1 @screen and (min-width: 10px)",
> +    "link text contains media query text.");
> +

Done

::: toolkit/devtools/server/actors/styles.js
@@ +1254,5 @@
>          let location = {
>            href: source,
>            line: line,
> +          column: column,
> +          mediaText: this.parentRule ? this.parentRule.mediaText : ""

This turned out to be more difficult than expected. nsIDOMCSSRule.MEDIA_RULEs are the actual @media statement block.

This means that to check if a rule is what we call a media rule we need to check it's parent rule if it has one. A small tweek to styles.js fixed this.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1120570-fix-media-query-in-rule-view-source-link.patch

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d60e5fc13e58
https://tbpl.mozilla.org/?tree=Try&rev=d60e5fc13e58

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8563459 [details] [diff] [review]
1120570-fix-media-query-in-rule-view-source-link.patch

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

There are test failures with this patch applied

::: toolkit/devtools/server/actors/styles.js
@@ +992,5 @@
>      if (this.rawRule.parentRule) {
>        form.parentRule = this.pageStyle._styleRef(this.rawRule.parentRule).actorID;
> +
> +      if (this.rawRule.parentRule.type === Ci.nsIDOMCSSRule.MEDIA_RULE) {
> +        form.media = [];

This is setting form.media, but form.mediaText is still being set directly to this.rawRule.media.mediaText (i.e. not relying on the parent rule).  I'm a little concerned about changing this behavior for backwards compatibility sake -- are you saying that your initial approach wouldn't work after all?

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8563459 [details] [diff] [review]
> 1120570-fix-media-query-in-rule-view-source-link.patch
> 
> Review of attachment 8563459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/styles.js
> @@ +992,5 @@
> >      if (this.rawRule.parentRule) {
> >        form.parentRule = this.pageStyle._styleRef(this.rawRule.parentRule).actorID;
> > +
> > +      if (this.rawRule.parentRule.type === Ci.nsIDOMCSSRule.MEDIA_RULE) {
> > +        form.media = [];
> 
> This is setting form.media, but form.mediaText is still being set directly
> to this.rawRule.media.mediaText (i.e. not relying on the parent rule).

mediaText comes from here:
```
get mediaText() {
  if (!this._form.media) {
    return null;
  }
  if (this._mediaText) {
    return this._mediaText;
  }
  this._mediaText = this.media.join(", ");
  return this._mediaText;
},
```

So it is set by the value in form.media, which is set from:
```
if (this.rawRule.parentRule.type === Ci.nsIDOMCSSRule.MEDIA_RULE) {
  form.media = [];
  for (let i = 0, n = this.rawRule.parentRule.media.length; i < n; i++) {
    form.media.push(this.rawRule.parentRule.media.item(i));
  }
}
```

> I'm
> a little concerned about changing this behavior for backwards compatibility
> sake.

This was not changed for backwards compatibility sake. The way we were doing it was wrong as it was based on a misunderstanding of what a Ci.nsIDOMCSSRule.MEDIA_RULE is. A rule that we generally call a media rule is actually a Ci.nsIDOMCSSRule.STYLE_RULE (.someclass {...}) that has a parent of a Ci.nsIDOMCSSRule.MEDIA_RULE (@media blah).

We were treating it as if the style rule (.someclass {...}) was a media rule when we should have been checking the parent... this would never work.

> Are you saying that your initial approach wouldn't work after all?

My initial approach works fine but simply masks an existing bug.

> 
> There are test failures with this patch applied

Of course, I will fix that.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> (In reply to Brian Grinstead [:bgrins] from comment #12)
> > Comment on attachment 8563459 [details] [diff] [review]
> > 1120570-fix-media-query-in-rule-view-source-link.patch
> > 
> > Review of attachment 8563459 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is setting form.media, but form.mediaText is still being set directly
> > to this.rawRule.media.mediaText (i.e. not relying on the parent rule).
> 

I see... you were looking in stylesheets.js. We are dealing with the actual Ci.nsIDOMCSSRule.MEDIA_RULEs there so we are doing it correctly.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1120570-fix-media-query-in-rule-view-source-link.patch

Addressed all comments and fixed bugs exposed by tests.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=2ab7a7135a67
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab7a7135a67

Comment 16

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Regression in the devtools

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

We can definitely take this up to 37, please nominate for uplift once it's landed on central.  36 is about to go out the door and the RC is already with users, this isn't a serious enough regression to justify a respin but I'll leave the nomination for tracking so Sylvestre, who's on point for 36, can confirm his opinion.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8565032 [details] [diff] [review]
1120570-fix-media-query-in-rule-view-source-link.patch

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

The changes look fine, but please address the comments before landing

::: browser/devtools/styleinspector/rule-view.js
@@ +436,5 @@
>    this._modificationDepth = 0;
>  
>    if (this.domRule) {
>      let parentRule = this.domRule.parentRule;
>      if (parentRule && parentRule.type == Ci.nsIDOMCSSRule.MEDIA_RULE) {

Can't we check `if (this.domRule.mediaText)` and get rid of the parentRule variable?  Then the whole complication of whether mediaText is set on the child or the parent can be kept out of the frontend.

@@ +437,5 @@
>  
>    if (this.domRule) {
>      let parentRule = this.domRule.parentRule;
>      if (parentRule && parentRule.type == Ci.nsIDOMCSSRule.MEDIA_RULE) {
> +      this.mediaText = this.domRule.mediaText;

Note: this looks like it could be a potential backwards compatibility problem.  On an old server the parentRule has media set while on a new server the rule itself does.  Looks like this is the only place in browser/devtools that references it and the feature hasn't been working anyway, so I guess it's OK

::: toolkit/devtools/server/actors/styles.js
@@ +1021,5 @@
>        case Ci.nsIDOMCSSRule.IMPORT_RULE:
>          form.href = this.rawRule.href;
>          break;
>        case Ci.nsIDOMCSSRule.MEDIA_RULE:
> +        // CSS rules that we call media rules are STYLE_RULES that are children

This comment should be above the code that's actually setting form.media, and this entire case should be removed from the switch statement.

@@ +1254,3 @@
>      let parentSheet = this.parentStyleSheet;
>      if (!parentSheet) {
>        return promise.resolve(this.location);

Can you add a comment here about how inline styles don't have any mediaText associated with them?  At least that's what I'm gathering after looking through the code to understand why parentStyleSheet is null, and why we wouldn't need to worry about passing along mediaText in that case.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1120570-fix-media-query-in-rule-view-source-link.patch

Addressed review comments.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8565923 [details] [diff] [review]
1120570-fix-media-query-in-rule-view-source-link.patch

https://hg.mozilla.org/integration/fx-team/rev/0799176cb906

Comment 21

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2018660&repo=fx-team

Comment 22

[Sylvestre Ledru [:Sylvestre]]

As Lukas said, too late for 36.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Michael - Your fix was backed out due to test failures. Do you have time to look into the failures this week so that we can consider this for uplift to 37?

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Joe - I haven't heard from Mike and we're running out of time for Beta. Can you help?

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23)
> Michael - Your fix was backed out due to test failures. Do you have time to
> look into the failures this week so that we can consider this for uplift to
> 37?

I am currently working on remoting the storage panel so I don't have time to look at this right now.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Based on comment 25 I'm dropping tracking and marking 37 as wontfix.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Can't reproduce the issue locally so pushing to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5868838e0e5

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

In fact, let's be more thorough than that try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf06a812ef4

Comment 29

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1120570-fix-media-query-in-rule-view-source-link.patch

Ah, it was a case of the test still expecting the value to be in the wrong place.

I would expect this to be green now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f9c178449a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8590713b55f