1275029 - Refactor output-parser.js to support adding more features to the css rule-view like inline editors

Status: NEW

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

As part of the effort to bring inline-editors to the rule-view (see bug 1258390), we want us and addon authors to be able to easily add new functionality to how declarations are displayed and interacted with in the css rule-view.

Right now, the DOM used to display and interact with a declaration is generated by output-parser.js, there's a bit of logic in text-property-editor.js and yet again some in style-inspector-overlay.js.
These have aged differently, there's a lot of things that could be de-coupled better, and it's not very extensible.
Adding a new swatch type proves hard every time.

We want to make it simpler for us to support existing and add new features, and have an extensible model for extensions at the same time.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: WIP_-_New_architecture_for_the_rule-view_to_suppor.diff

Very much a WIP at this point, but starting to work, and I wanted to attach it to give an idea of what I had in mind with this bug.

Comment 2

[Patrick Brosset <:pbro>]

Here's the idea so far:

We get a list of css rules, each having a list of declarations.
Each declaration is handled today in devtools\client\inspector\rules\views\text-property-editor.js
I want to keep this in order to reduce this refactoring to the minimum. The goal here isn't to rewrite the rule-view entirely, this would be too big of a project.

The TextPropertyEditor then hands off the declaration name:value pair to the output parser that somewhat parses the value, recognizing colors, timing-functions, filters, etc. and outputs a DOM fragment that can be inserted in the DOM in the rule-view.
Then the TextPropertyEditor goes over the various elements in this fragment to find color swatches (and other things) and binds them to tooltips.

I'd like to stop using the output parser altogether here.
Instead, here's the proposed architecture:

Take a declaration, parse it into a custom AST that makes sense for the rule-view.
Pass this AST to a renderer. This renderer would visit the AST and, for each node in it, call a list of visitors that know how to generate some output for a given node.
It would be possible to define more visitors, making it extensible.

Comment 3

[Patrick Brosset <:pbro>]

Before I continue implementing this and start asking for reviews, it will be helpful if reviewers read about the reasoning first and get some doc about the high level architecture.
So I wrote this: https://gist.github.com/captainbrosset/937fb40a95b59cfe5e8496b16bccde86

Pinging a few people who I'd like to get their opinions on this doc before progressing more.
Please leave your comments in the gist.

Comment 4

[Tom Tromey :tromey]

I commented.

Comment 5

[Julian Descottes [:jdescottes]]

Looks very good! Commented on the gist.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1275029_-_Introduce_a_new_css_declaration_rend.diff

Many broken or un-implemented things, but getting closer. And I've moved this behind a pref so that the old codepath still exist and is used by default. This should help land it and get people helping rather than me working on this giant patch on my own.

Comment 7

[Brian Grinstead [:bgrins]]

Left comments on the gist

Comment 8

[u454729]

Thanks for working on this! Left comments on gist.

Comment 9

[Patrick Brosset <:pbro>]

I haven't made progress on this lately. Not a P1.