inspector should display color swatch in filter function

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

Trunk
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
The drop-shadow filter function can take a color as an argument.
The rule view should put a color swatch next to this.

See the last test case here:
 https://bugzilla.mozilla.org/show_bug.cgi?id=971234#c18

(The test is already in the patch in that bug, just commented out.)

This will require changes to output-parser and/or FilterWidget.

This will be simplest if the various other changes go in first, so
setting dependencies for this.
(Assignee)

Comment 1

3 years ago
This also affects browser_styleinspector_output-parser.js,
which tests linear-gradient and -moz-radial-gradient.
(Assignee)

Comment 2

3 years ago
This also affects at least:
browser_ruleview_colorpicker-and-image-tooltip_01.js
browser_ruleview_colorpicker-edit-gradient.js
(Assignee)

Comment 3

3 years ago
On second thought, the problems in comment #1 and comment #2 are just
regressions from the CSS lexing work, and I'll fix them in the relevant
bug.  Comment #0 remains as a new feature, though.
(Assignee)

Comment 4

2 years ago
Created attachment 8615406 [details] [diff] [review]
minimal changes for eslint

This patch makes a bunch of minor changes for eslint.  I updated the
files I touched in the real patch.

There are still some eslint warnings after this patch.  I only fixed
up the really trivial things.
(Assignee)

Comment 5

2 years ago
Created attachment 8615434 [details] [diff] [review]
show color swatch in drop-shadow function

This changes the way the filter swatch is emitted.  Now, it wraps the
result of the "ordinary" parse of the property value.  (The change to
OutputParser._parse is largely just reindenting, but buried in there
is a small change to this effect.)

Then, this adds a new mode, via a new option, to output-parser so that
updates in the filter editor will re-parse the property.  This makes
it so that the color swatch continues to be visible while the property
is edited in the filter widget.
(Assignee)

Updated

2 years ago
Attachment #8615406 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8615434 - Flags: review?(pbrosset)
Comment on attachment 8615406 [details] [diff] [review]
minimal changes for eslint

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

Thanks for cleaning this up.
There's a few warnings for DOMUtils not being declared in output-parser.js, you can fix this by adding this at the top of the file (right below the license header comment):

/* globals DOMUtils */

We already have a bunch of globals declared in the main .eslintrc (for things like Cu, require, exports, ...), so all other globals (which usually come from Cu.import) should be declared as globals comments within each file.
Attachment #8615406 - Flags: review?(pbrosset) → review+
Comment on attachment 8615434 [details] [diff] [review]
show color swatch in drop-shadow function

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

Looks mostly good to me. I'd like if you could avoid having to re-declare a whole set of parser options in rule-view.js and instead reuse the ones that are already defined, adding the new filterSwatch property.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +1666,5 @@
>  
> +    // Remove the old children and reparse the property value to
> +    // recompute them.
> +    while (this.currentFilterValue.firstChild) {
> +      this.currentFilterValue.removeChild(this.currentFilterValue.firstChild);

or just:
this.currentFilterValue.firstChild.remove();

@@ +1698,5 @@
> +   *        |outputParser| an @see OutputParser object
> +   *        |options| options to pass to the output parser, with
> +   *          the option |filterSwatch| set.
> +   */
> +  addSwatch: function(swatchEl, callbacks, parser) {

I think I'd rather have 2 separate parameters than a new type of object. I know we often try and keep the total number of parameters low, but constructing the new object with the parser and its options in rule-view feels weird to me.

::: browser/devtools/styleinspector/rule-view.js
@@ +3081,5 @@
>      // Attach the filter editor tooltip to the filter swatch
>      let span = this.valueSpan.querySelector("." + filterSwatchClass);
>      if (this.ruleEditor.isEditable) {
>        if (span) {
> +        let parser = {

With my comment in Tooltip.js, this new parser object isn't needed, you can pass outputParser directly in addSwatch below.
Then you can move the parser options declaration (above near 'let frag ...') into an object you can reuse here.
Attachment #8615434 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8616689 [details] [diff] [review]
minimal changes for eslint

Rebased and updated to add "globals" comments.
There are still a few eslint warnings remaining -- I only addressed
the trivial ones.
(Assignee)

Comment 9

2 years ago
Created attachment 8616690 [details] [diff] [review]
show color swatch in drop-shadow function

Updated to address review comments.
Attachment #8615406 - Attachment is obsolete: true
Attachment #8615434 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f412ee20e5
(Assignee)

Updated

2 years ago
Attachment #8616689 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8616690 - Flags: review+
(Assignee)

Comment 11

2 years ago
Created attachment 8616847 [details] [diff] [review]
minimal changes for eslint

Fix a typo that caused a regression.
Attachment #8616689 - Attachment is obsolete: true
Attachment #8616690 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8616849 [details] [diff] [review]
show color swatch in drop-shadow function
(Assignee)

Comment 13

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb658bced438
(Assignee)

Updated

2 years ago
Attachment #8616849 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8616847 - Flags: review+
(Assignee)

Comment 14

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd2d351cd13d
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/34d7922bed37
https://hg.mozilla.org/integration/fx-team/rev/d94caa738b3f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34d7922bed37
https://hg.mozilla.org/mozilla-central/rev/d94caa738b3f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.