Closed Bug 1239336 Opened 4 years ago Closed 4 years ago

about:PreferenceStyleSheet links now clickable in rule view

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file css-to.html
Since the AUTHOR_SHEETS change (bug 1230491), about:PreferenceStyleSheet
links are clickable in the rule view.  This is a regression.

The issue is that such rules are no longer recognized as system rules here:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1992

It's also not entirely clear that these rules should show up when not
showing browser styles.  Open the attached test case and inspect the div.
Even with browser styles off, you will see rules from this sheet.
I think the bug here is that the style sheet simply isn't marked as a system
sheet when it is created.

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#811
This code in ReparseSheet also looks suspect to me:

https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#2265

It doesn't seem to account for the possibility of user sheets.
Comment on attachment 8707589 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

This fixes a regression due to bug 1230491.

There I failed to set the parsing mode for the special
about:PreferenceStyleSheet.

The BuildPreferenceSheet hunk is necessary, but not sufficient,
because BuildPreferenceSheet uses ReparseSheet, which has its
own logic to decide the parsing mode.  However, this logic
seems suspect to me; not only does it do the wrong thing (IMO)
for this sheet, but it also fails to account for the possibility
of eUserSheetFeatures.  So, this patch changes ReparseSheet to
preserve the sheet's parsing mode, which is available since bug 1230491.
Attachment #8707589 - Flags: review?(cam)
Comment on attachment 8707589 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

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

::: dom/tests/mochitest/chrome/test_parsingMode.html
@@ +37,5 @@
>  	is(sheet.parsingMode, "author",
>  	  "author sheet has expected mode");
>  	results[sss.AUTHOR_SHEET] = 1;
> +      } else {
> +        if (sheet.href === "about:PreferenceStyleSheet") {

Nit: Maybe make this an "else if" to follow the pattern in the rest of the if..else if.. chain.
Attachment #8707589 - Flags: review?(cam) → review+
Updated per review.
Attachment #8707589 - Attachment is obsolete: true
Attachment #8708030 - Flags: review+
Fixed browser_rules_user-agent-styles.js test.
Attachment #8708030 - Attachment is obsolete: true
Comment on attachment 8708505 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

This required an additional fix to doc_author-sheet.html.
My previous change there (in a different bug) seems to have been
totally wrong :(
This updates it to do what was originally intended.
Attachment #8708505 - Flags: review?(bgrinstead)
Comment on attachment 8708505 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

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

::: devtools/client/inspector/rules/test/doc_author-sheet.html
@@ -24,5 @@
>    </script>
>  
>  </head>
>  <body>
> -  <div id="target"> the ocean </div>

Looks like this might break client/inspector/shared/test/browser_styleinspector_csslogic-content-stylesheets.js which relies on a #target element
(In reply to Brian Grinstead [:bgrins] from comment #11)

> Looks like this might break
> client/inspector/shared/test/browser_styleinspector_csslogic-content-
> stylesheets.js which relies on a #target element

As discussed on irc there are two copies of doc_author-sheet.html,
and this only changes one of them.

This change is needed because the test in question looks like this:

  {
    selector: "a",
    numUserRules: 3,
    numUARules: 0
  }

This means to select the <a> element and notice that there are 3 user rules
and 0 agent rules.

This "worked" previously because I didn't notice when writing the test that
this was doing the wrong thing for about:PreferenceStyleSheet; it's really like
I just got halfway through redoing the test and then forgot what I was aiming
to do.
Attachment #8708505 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/39ddc3588d53
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
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

Expected Results: 
Developer specific testing

Actual Results: 
As expected
See Also: → 1436782
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.