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

NEW
Unassigned

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P3
enhancement
2 years ago
2 months ago

People

(Reporter: pbro, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox49 affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8755479 [details] [diff] [review]
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.
(Reporter)

Updated

2 years ago
Summary: Refactor output-parser.js to support help adding more features to the css rule-view like inline editors → Refactor output-parser.js to support adding more features to the css rule-view like inline editors
(Reporter)

Comment 2

2 years ago
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.
(Reporter)

Comment 3

2 years ago
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.
Flags: needinfo?(ttromey)
Flags: needinfo?(jdescottes)
Flags: needinfo?(gl)
Flags: needinfo?(bgrinstead)
(Reporter)

Updated

2 years ago
Assignee: nobody → pbrosset
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 4

2 years ago
I commented.
Flags: needinfo?(ttromey)
Looks very good! Commented on the gist.
Flags: needinfo?(jdescottes)
(Reporter)

Comment 6

2 years ago
Created attachment 8759202 [details] [diff] [review]
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.
Attachment #8755479 - Attachment is obsolete: true
Left comments on the gist
Flags: needinfo?(bgrinstead)

Comment 8

2 years ago
Thanks for working on this! Left comments on gist.
Flags: needinfo?(gl)
(Reporter)

Comment 9

2 months ago
I haven't made progress on this lately. Not a P1.
Assignee: pbrosset → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.