1230491 - Can't update/change styles in AUTHOR_SHEETS in browser toolbox

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mark Banner (:standard8)]

For Hello as a system add-on, we've now moved to loading style sheets as AUTHOR_SHEETS via the style sheet manager. :Gijs tells me that we should be able to edit/change the styles for these in the browser toolbox.

However, the browser toolbox shows the sheets as (user agent) <sheet name> with a grey background - and the styles can't be edited.

Comment 2

[:Gijs (he/him)]

Brian, why are these styles even tagged as user agent? I don't think they ought to be, if they're inserted as AUTHOR_SHEET... I think there's a separate bug here (ie I don't believe this is a dupe, though the two bugs are definitely related).

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #2)
> Brian, why are these styles even tagged as user agent? I don't think they
> ought to be, if they're inserted as AUTHOR_SHEET... I think there's a
> separate bug here (ie I don't believe this is a dupe, though the two bugs
> are definitely related).

I know we have a similar issue with devtools which are loading them with loadSheetUsingURIString, even when they are marked as "author" sheets: https://bugzilla.mozilla.org/show_bug.cgi?id=1076788#c6.

I just doubled checked and we are using this function to determine whether they show up as UA styles: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/styleinspector/css-logic.js#817.  Which is checking to see if the sheet has an ownerNode and otherwise considering it UA, which explains the problem.   Boris, do you know of a better way to determine if a CSSStyleSheet is a user agent sheet?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Boris, do you know of a better way to determine if a CSSStyleSheet is a user agent sheet?

Right now sheets have no real concept of being user agent or not.  But we could probably just add that.  The CSS loader certainly has such a concept when it calls ParseSheet (the mParsingMode of the SheetLoadData).  So we could go ahead and stash that state in the CSSStyleSheet, with a [ChromeOnly] getter.

Comment 5

[Brian Grinstead [:bgrins]]

I think that fixing the distinction for UA style sheets is the best thing to do here (instead of bug 1076788).  The use case for editing *actual* UA sheets is very narrow, the main problem is incorrectly identifying sheets as UA.

Comment 6

[Tom Tromey :tromey]

Attachment: add CSSStyleSheet::parsingMode

The platform patch.

Comment 7

[Tom Tromey :tromey]

Attachment: fix most eslint warnings in rule-view.js

Clean up rule-view.js.

Comment 8

[Tom Tromey :tromey]

Attachment: rewrite CssLogic.isContentStylesheet

The devtools patch.

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8700656 [details] [diff] [review]
add CSSStyleSheet::parsingMode

This implements the platform side of the plan as outlined in
comment #4.

I had to move the enum definition as css/Loader.h includes
CSSStyleSheet.h.

I chose to initialize mParsingMode as eUserSheetFeatures.
It shouldn't matter; but Maybe<> would be an ok choice as
well.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8700657 [details] [diff] [review]
fix most eslint warnings in rule-view.js

An earlier version of the patch touched rule-view, so while I was
there I removed most eslint warnings.  All that remains are some
false positives from the eventlistener check.

The patch no longer touches rule-view, but I thought this was worth
putting in regardless.  I can open a new bug if you'd prefer that.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8700658 [details] [diff] [review]
rewrite CssLogic.isContentStylesheet

This rewrites CssLogic.isContentStyleSheet to use the new platform feature,
fixing the original bug.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8700657 [details] [diff] [review]
fix most eslint warnings in rule-view.js

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

Changes look good to me, but I'd prefer if you filed a new bug and landed this patch with that ID so it's easier to track

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8700658 [details] [diff] [review]
rewrite CssLogic.isContentStylesheet

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

::: devtools/client/styleinspector/test/browser_ruleview_author-sheet.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Check that author sheet styles are editable via the UI.

It'd be nice if we could test CssLogic.isContentStylesheet more directly, like in browser_styleinspector_csslogic-content-stylesheets.js.

We should also have an integration test like this, but rather than adding a new one I'm thinking we could modify browser_ruleview_user-agent-styles.js.  It could load up this new doc_author-sheet.html file in the tab (with the current markup from that test plus the new authored stylesheet) and bump up numUserRules on one of the selectors.

::: devtools/shared/styleinspector/css-logic.js
@@ +814,5 @@
>   * @return {boolean} true if the given stylesheet is a content stylesheet,
>   * false otherwise.
>   */
>  CssLogic.isContentStylesheet = function(sheet) {
> +  return sheet.parsingMode !== Ci.nsIStyleSheetService.AGENT_SHEET;

Nice

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8700657 [details] [diff] [review]
fix most eslint warnings in rule-view.js

Moved to bug 1234306

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8700656 [details] [diff] [review]
add CSSStyleSheet::parsingMode

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

Since we're in Web IDL land, we probably should make parsingMode more usable by making it an enum.  How about we do this:

  enum SheetParsingMode {
    "agent",
    "user",
    "author"
  };

  interface CSSStyleSheet {
    ...
    [ChromeOnly]
    readonly attribute SheetParsingMode parsingMode;
  };

The values in the IDL-generated mozilla::dom::SheetParsingMode should correspond to mozilla::css::SheetParsingMode, so we can just cast them.

Comment 16

[Tom Tromey :tromey]

Attachment: add CSSStyleSheet::parsingMode

Updated per review.

Comment 17

[Tom Tromey :tromey]

Attachment: rewrite CssLogic.isContentStylesheet

Updated per review.

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8702652 [details] [diff] [review]
add CSSStyleSheet::parsingMode

Updated per review.

I gave the new enum a name other than "SheetParsingMode" because
using that caused ambiguities in the C++ code.  Choosing a new
name seemed simpler and less confusing than disambiguating (but
if you'd prefer the other way, I can redo this).

In order to ensure that the cast is valid I added a few static assertions.
One other option here would be to remove SheetParsingMode entirely
in favor of the new webidl-generated enum.

Comment 19

[Tom Tromey :tromey]

Comment on attachment 8702653 [details] [diff] [review]
rewrite CssLogic.isContentStylesheet

I believe this addresses the review comments.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8702652 [details] [diff] [review]
add CSSStyleSheet::parsingMode

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

Looks good, thanks.  You'll need a DOM peer's review for the .webidl change.

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8702653 [details] [diff] [review]
rewrite CssLogic.isContentStylesheet

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

Looks good to me, thanks

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8702652 [details] [diff] [review]
add CSSStyleSheet::parsingMode

Needs a DOM peer review for webidl changes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8702652 [details] [diff] [review]
add CSSStyleSheet::parsingMode

r=me for the webidl change

Comment 24

[Tom Tromey :tromey]

Attachment: add CSSStyleSheet::parsingMode

Rebased.

Comment 25

[Tom Tromey :tromey]

Attachment: rewrite CssLogic.isContentStylesheet

Rebased.  This required duplicating the new file
doc_author-sheet.html, due to how the styleinspector rearrangement was
done.  Note that there are other such duplications already in the
tree.

Comment 26

[Tom Tromey :tromey]

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

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2e6740e4993c
https://hg.mozilla.org/integration/fx-team/rev/80d8340b39c1

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e6740e4993c
https://hg.mozilla.org/mozilla-central/rev/80d8340b39c1

Comment 30

[Mayur Patil]

[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