Rule view is not showing media query in source link text

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: bgrins, Assigned: miker)

Tracking

({regression})

unspecified
Firefox 39
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 wontfix, firefox36- wontfix, firefox37- wontfix, firefox38- affected, firefox39 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

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

Comment 1

5 years ago
This is a regression - it is working in 34 but stopped working in 35
Keywords: regression
Reporter

Comment 2

5 years ago
Posted file t.html
Mike's test case from Bug 722196
Reporter

Comment 4

5 years ago
Posted image media-links.png
screenshot of the bug
Assignee: nobody → mratcliffe

Comment 5

5 years ago
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
QA Contact: alexandra.lucinet
I have a simple fix... just need to add a test.
Reporter

Comment 9

4 years ago
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.
Attachment #8562078 - Flags: review?(bgrinstead)
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.
Reporter

Comment 12

4 years ago
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?
Attachment #8563459 - Flags: review?(bgrinstead) → review-
(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.
(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 16

4 years ago
[Tracking Requested - why for this release]: Regression in the devtools
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.
Reporter

Comment 18

4 years ago
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.
Attachment #8565032 - Flags: review?(bgrinstead) → review+
Addressed review comments.
Attachment #8565032 - Attachment is obsolete: true
Attachment #8565923 - Flags: review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2018660&repo=fx-team
Flags: needinfo?(mratcliffe)
As Lukas said, too late for 36.
Flags: needinfo?(mratcliffe)
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?
Flags: needinfo?(mratcliffe)
Joe - I haven't heard from Mike and we're running out of time for Beta. Can you help?
Flags: needinfo?(jwalker)
(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.
Flags: needinfo?(mratcliffe)
Flags: needinfo?(jwalker)
Based on comment 25 I'm dropping tracking and marking 37 as wontfix.
Attachment #8565923 - Flags: checkin+ → checkin-
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
Attachment #8565923 - Attachment is obsolete: true
Attachment #8581769 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/c8590713b55f
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c8590713b55f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Duplicate of this bug: 1064854

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.