about:PreferenceStyleSheet links now clickable in rule view

RESOLVED FIXED in Firefox 46

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 46

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8707480 [details]
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.
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8707589 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Comment 6

3 years ago
Created attachment 8708030 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

Updated per review.
Attachment #8707589 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8708030 - Flags: review+
(Assignee)

Comment 8

3 years ago
Created attachment 8708505 [details] [diff] [review]
set parsing mode on about:PreferenceStyleSheet

Fixed browser_rules_user-agent-styles.js test.
Attachment #8708030 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
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
(Assignee)

Comment 12

3 years ago
(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+

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39ddc3588d53
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 15

3 years ago
[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: → bug 1436782

Updated

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