1153184 - Persist CSS filter presets for reuse

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Bug 1055181 adds a nice CSS Filter tooltip to the rule-view, which helps with adding/removing/updating filter functions to the filter css property of the selected element.

It would be nice if this new tooltip allowed to save/restore filter presets.
Once you're happy with the changes you've made, you might be interested in saving the exact list of filter functions and their values with a name, so that you can later restore it.

User workflow:
- select the element to be styled
- add filter: none; in the rule-view
- click on the css filter tooltip button
- in the tooltip, add some functions, blur, drop-shadow, hue-rotate, ..., tweak the values, sort the functions, ...
- hit the "save filters" button
- the tooltip now shows a text input to enter a name for this new preset
Later, when inspecting another element, on another page:
- add filter: none; in the rule-view
- click on the css filter tooltip button
- in the tooltip, hit the "restore filters" button
- the tooltip now shows a list of saved presets, with their names, and filter functions
- hit one of the presets
- the tooltip now shows the filter values, with the right functions, in the right order, and with the right values

Comment 1

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Great idea! I'm going to notify you when I'm going to start working on this. Thanks.

Comment 2

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: presets.png

I think this is a good solution as it provides a way to search and save/load presets. This way they can search and replace old presets, too. It shouldn't be hard to implement either.

I've done this in Sketch, you can find the sketch file here in case you want to tweak something: https://github.com/mdibaiee/CSS-Filter-Tooltip/blob/presets/presets.sketch

Thanks!

Comment 3

[Patrick Brosset <:pbro>]

Sorry for the delay Mahdi.

I have some questions:
- How do users open this new panel from the tooltip? I think we need a button in the tooltip.
- Where do you enter a name for the new preset to be saved? Maybe we need 2 states for this new panel, one to save a new preset with a name, and one to load from the list.

And some suggestions:
- Not sure we need to worry about searching for now, it's nice, but this might give us too much work for a v1 of this new panel, and I'm not expecting this feature to be used so much that users suddenly need a search input to find their presets.
- We need a way to delete presets too.

And some UI remarks (but I'm no designer, so I'm going to flag the attachment with UI review/feedback):
- I'm not too sure about the popup in a popup. I don't think we need to be able to see the list of filters below the load/save screen. Maybe let's have the 2 main screens in the DOM, one for the filters list, one for the presets list and only display one or the other depending on the state.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8597691 [details]
presets.png

Hi Stephen, do you think you could give us some UI advices for this new piece of UI we're thinking of building here? Comment 0 should give you the necessary context for what we're after. And if you've never seen it before, this is about the css filter tooltip, which you can have in nightly, if you enter a css property like 'filter:blur(1px)' and click on the little icon that appears next to it.

Comment 5

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: presets.png

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> Sorry for the delay Mahdi.
> 
> I have some questions:
> - How do users open this new panel from the tooltip? I think we need a
> button in the tooltip.
Yes, I forgot the button in mockup.

> - Where do you enter a name for the new preset to be saved? Maybe we need 2
> states for this new panel, one to save a new preset with a name, and one to
> load from the list.
I would say they enter the name of their new preset in search input, and make sure it doesn't exist before, then they can click save and save it with the name specified in search input, although now I think again and I guess it's not good in terms UX.

> And some suggestions:
> - Not sure we need to worry about searching for now, it's nice, but this
> might give us too much work for a v1 of this new panel, and I'm not
> expecting this feature to be used so much that users suddenly need a search
> input to find their presets.
Knowing that there won't be more than 10 presets in most cases, I think you're right, that's not really required, they can just browse a list of presets.

> - We need a way to delete presets too.
> 
> And some UI remarks (but I'm no designer, so I'm going to flag the
> attachment with UI review/feedback):
> - I'm not too sure about the popup in a popup. I don't think we need to be
> able to see the list of filters below the load/save screen. Maybe let's have
> the 2 main screens in the DOM, one for the filters list, one for the presets
> list and only display one or the other depending on the state.

I had thought of it, this gives us more space and the user more focus. I think we can follow the same markup as filters, with a little tweak (no inputs, no drag handles). See attachment.

Stephan, can you please have a look at this new mockup, too? Thanks!

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Comment on attachment 8599221 [details]
presets.png

Hi Stephen, please see Comment 4 for the original ui-review request.

New mockup:

I have the idea of [contenteditable] preset names in list, so you can rename the preset. The "Add new..." field uses this to let the user add a preset easily, we can default the value to the current value of panel i.e. 'brightness(160%) contrast(93%) hue-rotate(11%)' here.

We can load the preset by double clicking on it, or we can add an icon like drag handles behind preset names, a "load" icon to load the preset.

Comment 7

[Patrick Brosset <:pbro>]

Hey Mahdi, sorry for the delay, I've been on vacation last week.
It'd be great to have some input from Stephen, but I kind of like your second screenshot, and I think we're now at a stage where it would probably be easier to discuss with an interactive mockup.
So maybe start coding? Either on your HTML mockup on github, or in firefox directly (the former would be faster to iterate).

Comment 8

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Hey Patrick. 

Welcome back! I hope you enjoyed your vacation.

The git repository is using some old code, but I think that's OK, I pushed to the presets branch.
https://github.com/mdibaiee/CSS-Filter-Tooltip/tree/presets

It's working. Some questions: we need some way of indicating that labels contentEditable, like a pencil icon or something, what's your idea?

Also how does the user know he can double-click the label to load the preset? I think we should use an open/load icon in the place of drag handle, or something like that.

Comment 9

[Patrick Brosset <:pbro>]

Thanks Mahdi for uploading this.

After using it, I have some comments, again I'm not a ux person, so the following remarks are just my point of view. But there are some things that I didn't immediately knew how to do with the mockup, and if you hadn't mentioned that the name was content-editable and you needed to dbl-click on it, I properly wouldn't have found out. So, I guess there is some truth anyway in these:

- Do we need the value of a preset to be editable in a textfield? I would say no because the whole point of the popup originally was to allow editing filters easily with one widget per function, and buttons to add/remove functions. So once you saved a preset and want to edit it, it feels weird to have to do it in a small textfield where the whole value is displayed. It's good to display it, but I think we should handle editing this way:
  - load a preset from the list
  - make changes in the filters list in the main popup screen
  - save again using the same name

- The previous point means that we'd need a way to save from the main filter screen instead of having to first go in the presets list. I think the main workflow is:
  - open the popup
  - play around with functions
  - ooh, this looks nice, I want to keep it for later
  - provide a name and hit the save button

- With this in place, I'd be happy (at least for a v1) to just have a presets list screen that only shows the names of the presets (non editable), next to the values (non editable), and next to each of them, a delete button (a cross like in the filters screen), and a load button that goes back to the filters screen.

What do you think?

Comment 10

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Thanks Patrick, I like it, pushed. 

The UI has become messy, it's hard for user to understand what's actually going on.

Also the Load button doesn't fit the UI, again I don't have any good idea about it, double click still works, it would be nice if we could tell the user about this.

It would be awesome if we could get a UX person to comment on this.

Comment 11

[Patrick Brosset <:pbro>]

Sorry for the delay again Mahdi, I've been swamped with reviews lately. Also, I'm not as used dealing with UI mockups than code changes :)
What about you start with writing the code we'll need for persisting the presets instead? I guess there'll be quite a bit of changes/additions to make to the current widget, and most of it can be done without touching the UI for now (tests too).
In the meantime, I'll try to play a little bit with your mockup.

Comment 12

[Patrick Brosset <:pbro>]

I just an idea for the UI: reuse the general layout of the timing-function tooltip (cubic-bezier) which has a list of presets already.
In that tooltip, presets are shown in a list on the left-hand side. Why not do the same here? Just make the tooltip a little bit wider, and have a left sidebar with the list of all the saved presets.
Each item in the list would be something like:

+----------------------------------+--------------------- - - -
| My super preset            [DEL] |
| blur(2px) sepia(100) saturate... |
|----------------------------------|
| Contrasty stuff            [DEL] |   ... The existing tooltip content here ...
| contrast(200)                    |
|----------------------------------|
| I don't know what I'm d... [DEL] |
| invert(50) sepia(100) brightn... |
|----------------------------------|
| [Name for new preset]     [SAVE] |
+----------------------------------+--------------------- - - -

First line would be the name.
Second line a preview of the functions, with an ellipsis.
The name could be content editable.
Floated to the right would be an icon buttons to delete the preset.
Clicking on the whole item would load that preset in the filter part of the tooltip.

I think this could work because:
- as I said, the timing-function tooltip is similarly built
- almost all tools in devtools have a sidebar with a list of stuff, so people are used to this (see the style-editor for example)
- it allows all required actions without having to switch screens or add more things to the most important part: the list of filters. There's a good change not a lot of people will use this, so it's important that it's on the side and doesn't clutter the list of filters.

Comment 13

[Patrick Brosset <:pbro>]

A few details I forgot in the previous comment:
- stuck at the bottom of the left sidebar: a textfield and save/add button to create a new preset from the current filters,
- the list will need to be scrollable too.

Comment 14

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

I think that's a good idea, I'm a little busy this week as I'm passing my final exams, I'll start working soon, sorry for the delay.

Thanks Patrick!

Comment 15

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Okay, I got some free time and implemented the UI, I think it looks good. I used flexbox, it's awesome.

Pull from GitHub: https://github.com/mdibaiee/CSS-Filter-Tooltip/tree/presets

I'll try integrating it into Firefox next.

Q: What's the best way of keeping presets in browser? Prefs (about:config)? localStorage?

Thanks Patrick!

Comment 16

[Patrick Brosset <:pbro>]

Nice, I like it Mahdi!
Just one suggestion: the presets list should overflow (scrollbar should appear) when there are many presets instead of pushing the popup size. Basically the list of presets should never be longer than the list of filters. But that's just a detail.

About browser persistence, I believe the best option for this particular use case is asyncStorage (/toolkit/devtools/shared/async-storage.js).
There's a usage example in /browser/devtools/webconsole/webconsole.js

Comment 17

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

It's done except for one thing, I couldn't get the presets list to stop growing past the filter list, I'm not sure how it can be done using CSS.

I also wrote the tests, and modified past tests as the markup had changed (had to change things like `button` to ids, etc)

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3335654e7a

Thanks.

Comment 18

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests; r=pbrosset

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8616326 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

A couple of comments about the CSS

::: browser/devtools/shared/widgets/filter-frame.xhtml
@@ +20,5 @@
> +        <div id="presets"></div>
> +
> +        <div id="presets-footer">
> +          <form id="save-preset">
> +            <input list="saved" value="" placeholder="&newPresetPlaceholder;">

Can you use the .devtools-textinput class here ? It matches better the devtools style.

@@ +22,5 @@
> +        <div id="presets-footer">
> +          <form id="save-preset">
> +            <input list="saved" value="" placeholder="&newPresetPlaceholder;">
> +            </input>
> +            <button>&savePresetButton;</button>

Can you use the .devtools-button class here for the same reason ?

::: browser/devtools/shared/widgets/filter-widget.css
@@ +70,5 @@
> +  cursor: pointer;
> +}
> +
> +.preset:hover {
> +  background: rgba(29, 79, 115, 0.8);

nit : use var(--theme-selection-background-semitransparent) or var(--theme-selection-background) (depending on which opacity you want)

@@ +198,2 @@
>    background: url(chrome://browser/skin/devtools/add.svg);
> +  background-size: 18px;

Actually, it may be better to keep cover here, since the asset has recently been tailored for 16px.

@@ +206,5 @@
>  }
> +
> +#toggle-view {
> +  width: 100%;
> +  /*margin: 10px auto 0;*/

nit : You should remove this comment

Comment 20

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

Thanks Tim!

Applying devtools classes to input and button gives me this:
http://i.imgur.com/eXlwabF.png

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8616886 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

::: browser/devtools/shared/widgets/filter-frame.xhtml
@@ +22,5 @@
> +        <div id="presets-footer">
> +          <form id="save-preset">
> +            <input list="saved" value="" placeholder="&newPresetPlaceholder;">
> +            </input>
> +            <button>&savePresetButton;</button>

.devtools-button needs a standalone attribute applied as well.

If things still look bad, you can try using .devtools-toolbarbutton

::: browser/devtools/shared/widgets/filter-widget.css
@@ +187,5 @@
>  
> +#save-preset input {
> +  flex-grow: 1;
> +  margin-right: 10px;
> +  border: none;

This is causing the border on the input to be removed.

Comment 22

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Thanks Tim, it looks slightly better, but the input has a bad appearance in dark theme and the button doesn't fit the input. I added |standalone="true"|, tried devtools-toolbarbutton, none of them look good.

This is the results:
http://i.imgur.com/ahnS7cv.png

Probably the dark text on button is caused by some of my CSS, but still, a rectangle button besides a rounded input doesn't look good, and they're both too dark. The light theme is OK in terms of colors.

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8616886 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

Thanks, that seems to work well.
It's definitely the right direction and the feature is nice.
I think there's still a bit of ground to cover, like with any new UI bits, it always takes time to get all the details and user flows right.
Also, sorry for the delay in reviewing here, my review queue seems to never want to settle.

I'd really like a designer person to look at this, but in the meantime, here are my general comments about the UI:

- it'd really be better if the presets list didn't grow longer than the filters list, but instead had a scrollbar,
- it'd make more sense (at least for me) if the presets list were on the right side, rather than the left. I don't know what's our story with RTL support, but in my locale, more important things are on the left, and that's where I think the filters list should be,
- I wonder if we should hide the presets list by default until you clicked on a button to show them (this could be a toggle button somewhere on the side). I just want to make sure that the filters popup remains as simple as possible for people that want to play with it. In my mind, presets are for more advanced people that use filters all the time and that can afford to learn about this toggle button.
- the color of the vertical 1px separate line between the presets and filters seems too strong, I think it should be a light grey or something, in any case, it should come from our list of theme variables (we have one named splitter-color or something like this),
- maybe make the preset "save" button a "+" instead, just like in the filters list
- the new preset name textfield is visible in the dark-theme, which is nice, it's not so easy to see it with the light-theme. It should look exactly like the other textfields in devtools (and btw, like the other textfields in the filters list), for better consistency,
- the background color when hovering items in the presets list doesn't seem like it's part of the theme,
- I would display the preset name and preset value a bit differently from each other. Maybe take a look at the style-editor sidebar, where the stylesheets are displayed. Try and do something more consistent with this.
- It shouldn't be possible to enter a new preset when no filters have been added to the list (it works and creates an entry with "none" next to the delete button).
- It shouldn't be possible to enter a new preset without a name.
- It shouldn't be possible to enter a new preset with the same name as an existing preset, or perhaps it should override it (right now you can enter as many presets with the same name as you want).

I haven't fully looked at the JS and CSS changes, but I guess there's already a bunch of things to do with this feedback, and I'll take a look at those at the next pass.

::: browser/devtools/shared/widgets/filter-frame.xhtml
@@ +15,5 @@
>    </head>
>    <body>
>  
>      <div id="container">
> +      <div class='left-side'>

nit: please only use double-quotes for attribute values.
Also, please use more self-explanatory class names, not class names that depict how the element is rendered, this is more future-proof.
So something like "presets-list"

@@ +29,3 @@
>        </div>
> +
> +      <div class='right-side'>

"filters-list"

::: browser/devtools/shared/widgets/filter-widget.css
@@ +6,5 @@
>    color: var(--theme-body-color);
>    padding: 5px;
>    font: message-box;
> +  width: 100%;
> +  border-radius: 2px;

Why is there a need for a border-radius since this will go inside the tooltip that already manages the border style itself.

@@ +41,5 @@
> +  flex: 1 0;
> +}
> +
> +#presets {
> +  overflow-y: auto;

I think you need to give it a height if you want the overflow to work.

::: browser/locales/en-US/chrome/browser/devtools/filterwidget.properties
@@ +12,5 @@
>  emptyFilterList=No filter specified
>  
> +# LOCALIZATION NOTE (emptyPresetList):
> +# This string is displayed when preset's list is empty
> +emptyPresetList=You don't have any saved presets

I think this string shouldn't just tell the user she has no presets but instead explain what this new side panel is about:

"You can store filter presets by adding a name and saving ... blah blah ..."

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8616327 [details] [diff] [review]
Bug 1153184 - UI Tests; r=pbrosset

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

These test changes look good to me.
One suggestion: can you keep in this patch only the changes to the existing tests (the simple id/classes changes) so that I can R+ this and get it out of the way, and create a new patch for the new tests you're adding?

You need to add more presets tests:
- test preset deletion
- test what happens when you try to save 2 presets under the same name
- test edge cases like: missing filters, missing name
- test that loading a preset replaces all current filter values (the test you added tests that values are loaded when there are no filters only).

::: browser/devtools/shared/test/browser_filter-presets-01.js
@@ +28,5 @@
> +  form.querySelector("button").click();
> +
> +  // should wait for asyncStorage
> +  yield widget.once("render");
> +

Please also test that the preset has been saved in asyncStorage here.

Comment 25

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

I tried setting height for presets list, something like 100%, but it doesn't work.

Comment 26

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Fix Filter Editor Tests; r=pbrosset

Comment 27

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests; r=pbrosset

Added the tests mentioned.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8621616 [details] [diff] [review]
Bug 1153184 - UI Tests; r=pbrosset

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

::: browser/devtools/shared/test/browser.ini
@@ +35,5 @@
>  [browser_filter-editor-09.js]
>  [browser_filter-editor-10.js]
> +[browser_filter-presets-01.js]
> +[browser_filter-presets-02.js]
> +[browser_filter-presets-03.js]

You forgot to add browser_filter-presets-03.js to the patch.

::: browser/devtools/shared/test/browser_filter-presets-02.js
@@ +23,5 @@
> +  widget.setCssValue(VALUE);
> +  widget._togglePresets();
> +
> +  yield widget.once("render");
> +  

nit: trailing whitespace

Comment 29

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests; r=pbrosset

added test 3

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8621614 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

A few general remarks first:

- Expand the presets panel, click anywhere in this panel but not on a preset:
Full message: TypeError: preset is null
Full stack: CSSFilterEditorWidget.prototype._presetClick/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/FilterWidget.js:513:25

- Expand the presets panel, try to delete the first filter at the top: nothing happens, it's like the delete icon that's close to the presets expand button doesn't work.

- Make sure you have no presets saved, and collapse the preset panel: the popup becomes really tall, and it shrinks back when the presets panel is expanded again.

- Save many presets, like a lot of them, so that the popup becomes really tall, then start deleting them one by one: the popup keeps its size and what should remain at the bottom (the filters list, and preset name input) starts going up. Even if you close and open the tooltip again, it remains tall.
It looks like many of the issue we're facing here are due to the xul panel being a little weird. I'll try to take a look at it, see if I can help with those.

- It's minor, but if someone enters a very very long preset name, maybe we should have an ellipsis after after a certain length, just to try and keep the popup look good, we could then display the whole preset name in a tooltip (title attribute).

I'd like to propose a new approach to handling the size of the popup, to avoid these weird problems: why not set the size of the popup to something fixed and have scrollable lists? I'll upload a patch with this approach a bunch of css cleanups to try and make the code easier to read.

::: browser/devtools/shared/widgets/FilterWidget.js
@@ +143,5 @@
>    this.filters = [];
>    this.setCssValue(value);
> +  this.renderPresets();
> +
> +  console.log(this.el);

please remove this console.log

@@ +624,5 @@
>    },
>  
> +  renderPresets: function() {
> +    this.getPresets().then(presets => {
> +      console.log('renderPresets', presets);

Please remove this console.log

Comment 31

[Patrick Brosset <:pbro>]

Attachment: some-css-fixes.patch

I spent some time trying to fix the popup size problems I noted in my last comment, and I ended up changing quite a lot of things, so I'm uploading my patch here for info (to be applied on top of your patches).
I tried to clean the CSS quite a bit and re-order the rules so the things that are related are close to each other.
I also simplified the flex layout quite a lot.
And, mainly, I introduced a new way to handle the popup size: fixed size, and presets list that slides in and out.

Comment 32

[Patrick Brosset <:pbro>]

By the way, this may help:
In order to quickly iterate between code changes and tests in the browser, and make it easier to use the browser toolbox to debug css problems, I used a scratchpad script to test the content of the popup.
1) change code
2) build
3) open firefox (no need to open the toolbox)
4) open scratchpad in "browser environment" mode
5) run this script:

let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
let {TargetFactory, require} = devtools;
let {console} = Cu.import("resource://gre/modules/devtools/Console.jsm", {});
let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", {});
let {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});
let {Hosts} = require("devtools/framework/toolbox-hosts");

let createHost = Task.async(function*(type = "bottom", src = "data:text/html;charset=utf-8,") {
  let host = new Hosts[type](gBrowser.selectedTab);
  let iframe = yield host.create();

  yield new Promise(resolve => {
    let domHelper = new DOMHelpers(iframe.contentWindow);
    iframe.setAttribute("src", src);
    domHelper.onceDOMReady(resolve);
  });

  return [host, iframe.contentWindow, iframe.contentDocument];
});

let TEST_URI = "chrome://browser/content/devtools/filter-frame.xhtml";
let {CSSFilterEditorWidget} = require("devtools/shared/widgets/FilterWidget");
createHost("bottom", TEST_URI).then(res => {
  let [host, win, doc] = res;
  
  let container = doc.querySelector("#container");
  let widget = new CSSFilterEditorWidget(container, "none");
  widget.setCssValue("blur(3px) hue-rotate(130deg) contrast(200%)")
});

6) test the filters widget
7) use the browser toolbox to inspect the content, ...

Comment 33

[Patrick Brosset <:pbro>]

Comment on attachment 8622979 [details] [diff] [review]
Bug 1153184 - UI Tests; r=pbrosset

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

Looks good, I think that's the right amount of testing we need. I'd like to see some code factored out in head.js and some changes, as described below:

::: browser/devtools/shared/test/browser_filter-presets-01.js
@@ +28,5 @@
> +  // Set preset name
> +  const form = widget.el.querySelector("#save-preset");
> +  form.querySelector("input").value = NAME;
> +  // Click save button
> +  widget._savePreset({

I like calling event handler callbacks directly when simulating the event is hard and doesn't bring much added value to the test, but here, I think it would be better to actually simulate the event properly, because it would simplify the code, something like this: EventUtils.synthesizeMouseAtCenter(button, {}, win);

@@ +34,5 @@
> +    target: form
> +  });
> +
> +  // should wait for asyncStorage
> +  yield widget.once("render");

It's good practice to always start listening before executing what will cause the event:

let onRender = widget.once("render");
EventUtils.synthesizeMouseAtCenter(button, {}, win);
yield onRender;

@@ +61,5 @@
> +    preventDefault: () => {},
> +    target: form
> +  });
> +
> +  yield widget.once("render");

Same comment here about starting to listen before, and using EventUtils.

@@ +85,5 @@
> +
> +  widget._savePreset({
> +    preventDefault: () => {},
> +    target: form
> +  });

Also here use EventUtils.

@@ +87,5 @@
> +    preventDefault: () => {},
> +    target: form
> +  });
> +
> +  yield wait(500);

The problem with using 500ms here is that on very very slow test machines, it might actually take longer than this to run the code you expected to run during this time, and so this test might not always catch the problem you're trying to catch.
Instead, I think it's better to add a new event (even if only used by the test) that signals that a preset save was aborted or something.

@@ +102,5 @@
> +    preventDefault: () => {},
> +    target: form
> +  });
> +
> +  yield wait(500);

Same remarks here.

@@ +108,5 @@
> +  is(list.length, 0,
> +     "Should not add a preset without filters (value: none)");
> +});
> +
> +let wait = (n) => {

Get rid of this wait function once you introduced the new event.

::: browser/devtools/shared/test/browser_filter-presets-02.js
@@ +31,5 @@
> +    preventDefault: () => {},
> +    target: form
> +  });
> +
> +  yield widget.once("render");

Same comment as in the first test (start to listen before, and use EventUtils)

@@ +43,5 @@
> +  widget._presetClick({
> +    target: preset
> +  });
> +
> +  yield widget.once("render");

And here.

::: browser/devtools/shared/test/browser_filter-presets-03.js
@@ +33,5 @@
> +    target: form
> +  });
> +
> +  // should wait for asyncStorage
> +  yield widget.once("render");

And here too.

It looks like making a helper function that toggles the presets sidebar and creates a new preset would be useful. This would allow to remove a lot of code from these 3 tests (and possibly new tests coming later in the future). You can put something like this in head.js:

/**
 * Show the presets list sidebar in the cssfilter widget popup
 * @param {CSSFilterWidget} widget
 * @return {Promise}
 */
function showFilterPopupPresets(widget) {
  let onRender = widget.once("render");
  widget._togglePresets();
  return onRender;
}

let showFilterPopupPresetsAndCreatePreset = Task.async(function*(widget, name, value) {
  yield showFilterPopupPresets(widget);
  
  widget.setCssValue(value);
  
  let form = widget.el.querySelector("#save-preset");
  form.querySelector("input").value = name;
  
  let onRender = widget.once("render");
  EvenUtils.synthesizeMouseAtCenter(form.querySelector("button"), {}, form.ownerDocument.defaultView);
  yield onRender;
});

Or something like this anyway, I'm not sure when "render" is emitted off the top of my head.

Comment 34

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

Comment 35

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests; r=pbrosset

Forgot to mention: I folded your patch into my patch.

I think after moving our function to head.js, we don't have to use EventUtils (as I said in IRC I had trouble getting it to work). If you still think using EventUtils is better, I'll try to make it work.

Thanks Patrick!

Comment 36

[Patrick Brosset <:pbro>]

Comment on attachment 8623634 [details] [diff] [review]
Bug 1153184 - UI Tests; r=pbrosset

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

Ok, only 2 final remarks below, R+ with these fixed.
Also, can you make the commit message more meaningful? Maybe this: "Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset"

::: browser/devtools/shared/test/browser_filter-presets-01.js
@@ +64,5 @@
> +
> +  info("Test saving a preset without name");
> +  input.value = "";
> +
> +  let onError = widget.on("saveError");

Minor remark: we usually don't use camelcasing for event names. Better rename that event save-error, and it doesn't hurt to be more self-explanatory: preset-save-error

::: browser/devtools/shared/test/head.js
@@ +294,5 @@
> +
> +let showFilterPopupPresetsAndCreatePreset = Task.async(function*(widget, name, value) {
> +  // First render of Widget
> +  let onRender = widget.once("render");
> +  yield onRender;

These 2 lines are a bit weird. If they are needed, they should in any case not be in this function. What about someone tries to use this function in a new test with a widget that's already been rendered? Then the function will time-out.
It looks like the tests that use this should probably wait for the render event themselves instead.

Comment 37

[Patrick Brosset <:pbro>]

Comment on attachment 8623632 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

Nice! I like it.

Just one general remark:
apparently it's possible to save a preset with invalid filters, and when you do, some weird things will happen. One example: add just one filter: url() with no path for value, and save the preset. Next, when you load the preset, it fills the value with the URL of the frame.xhtml page. Another example: add just contrast() and make sure there is no value in the field, save the preset: this will save contrast(NaN%), and when you try to load this preset, this empties the list of filters.
This might be hard to handle correctly, but if you manage to find a solution for this (btw, this could be a follow-up bug, in the interest of landing something), then it'd be good to cover it with a test.

This is close, I think one more review and we should be fine. Thanks Mahdi!

::: browser/devtools/shared/widgets/FilterWidget.js
@@ +91,5 @@
>      "type": "string"
>    }
>  ];
>  
> +// If it's the first run, initialize with an empty array

I don't think you should do this here. What about the (very low chance) case where getting the item takes longer than it takes the user to try and save a preset?
There is an inherent race condition here.
I would advise removing this code block entirely, and instead initialize the storage with an empty array the first time you ever need to access it in the widget class below.

@@ +92,5 @@
>    }
>  ];
>  
> +// If it's the first run, initialize with an empty array
> +asyncStorage.getItem("presets").then(presets => {

The name "presets" is too generic, we need to make sure it's self-explanatory enough for people to know what it's for. Proposal: "cssFilterPresets"

@@ +546,5 @@
> +
> +        this.setCssValue(p.value);
> +        this.addPresetInput.value = p.name;
> +      }
> +    }).catch(Cu.reportError);

I've seen a mix of catch(Cu.reportError) and catch(console.error), can you please consistently use 1. I advise Cu.reportError.

@@ +561,5 @@
> +    let name = this.addPresetInput.value,
> +        value = this.getCssValue();
> +
> +    if (!name || !value || value === "none") {
> +      this.emit("saveError");

As mentioned in the previous review, rename to preset-save-error.

@@ +679,5 @@
> +        this.presetsList.appendChild(el);
> +      }
> +
> +      this.emit("render");
> +    });

Please also define a rejection handler for the promise returned by getPresets.
It could fail, and if it does, we need an error in the logs.

@@ +844,5 @@
>      this.emit("updated", this.getCssValue());
>    },
>  
> +  getPresets: function() {
> +    return asyncStorage.getItem("presets");

So I think this is where you should initialize the presets empty array if presets does not exist.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +872,5 @@
>  
> +      // widget.on("render", () => {
> +      //   iframe.height = widget.el.offsetHeight;
> +      //   iframe.width = widget.el.offsetWidth;
> +      // });

Seems like you can get rid of these commented out lines now.

::: browser/devtools/shared/widgets/filter-widget.css
@@ +55,5 @@
> +  padding-left: 0;
> +}
> +
> +#container.show-presets .filters-list {
> +  width: 300px;

Is this size really needed? The filter-list has flex-grow: 1 which means it will adapt automatically to the full length of the available width.

@@ +147,5 @@
> +  display: flex;
> +  margin-bottom: 10px;
> +}
> +
> +.preset {

There are 2 .preset {} rules in a row, they should be combined into 1.

@@ +236,5 @@
> +#toggle-view {
> +  width: 100%;
> +}
> +
> +.placeholder {

Where is the placeholder class used from? I can't find it in the code. Are these lines really needed? (the placeholder, data-placeholder, ::focus, :hover, ...)

::: browser/locales/en-US/chrome/browser/devtools/filterwidget.dtd
@@ +14,5 @@
>  <!ENTITY addNewFilterButton "Add">
> +
> +<!-- LOCALIZATION NOTE (newPresetPlaceholder): This string is used as
> +   - a placeholder in the list of presets which is used to save a new preset -->
> +<!ENTITY newPresetPlaceholder "New Preset...">

There's a special character you can use for ..., but I don't know if it's such a good idea to have it here, it usually suggests that there will be a next step in the process, which there isn't here.
So maybe just "Preset Name"

Comment 38

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

I learn a lot from your reviews, from best practices to coding techniques, thank you Patrick!

About the invalid filter values, I'm going on a vacation and might not be available for a week or two, so I guess we would better land it now, I will fill a follow-up bug.

I had a fight with mercurial, but I think the patches are OK now, I hope so.

Comment 39

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset

Forgot to mention that: Yes, the 300px width is necessary. Without it, after adding filters, the filter-list would expand in width and presets list would shrink until add and remove buttons of presets were not visible.

I think I screwed up the patch queue because on try there are duplicate patches applied, O_O. I hope the final patches I sent are OK

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

Thanks.

Comment 40

[Tim Nguyen :ntim]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #38)
> Created attachment 8624687 [details] [diff] [review]
> Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset
> 
> I learn a lot from your reviews, from best practices to coding techniques,
> thank you Patrick!
> 
> About the invalid filter values, I'm going on a vacation and might not be
> available for a week or two, so I guess we would better land it now, I will
> fill a follow-up bug.
> 
> I had a fight with mercurial, but I think the patches are OK now, I hope so.

This patch looks like this one : https://hg.mozilla.org/try/rev/046cfc6adb31
Which should be folded with this one : https://hg.mozilla.org/try/rev/be8aa5171d0f

Comment 41

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

Thanks Tim, you were right, I think it's fixed now.

Comment 42

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a8c899a902d

Comment 43

[Patrick Brosset <:pbro>]

Comment on attachment 8625298 [details] [diff] [review]
Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset

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

Ok, looks good now, only a few minor-ish remarks about simplifying/cleaning the code, but otherwise good to go.

::: browser/devtools/shared/test/browser_filter-presets-01.js
@@ +10,5 @@
> +
> +add_task(function* () {
> +  yield promiseTab("about:blank");
> +
> +  let [host, win, doc] = yield createHost("bottom", TEST_URI);

host and win aren't used in this test, you could just write:
let [,, doc] = yield createHost("bottom", TEST_URI);
It's arguably less easy to read, so up to you.

@@ +88,5 @@
> +  is(list.length, 0,
> +     "Should not add a preset without filters (value: none)");
> +});
> +
> +function savePreset(widget) {

Maybe to simplify the test even more, you could rewrite this to:

function savePreset(widget, expectEvent="render") {
  let onEvent = widget.once(expectEvent);
  widget._savePreset({
    preventDefault: () => {}
  });
  return onEvent;
}

Which means that you can then use it like:

yield savePreset(widget);
and
yield savePreset(widget, "preset-save-error");
in the test.

::: browser/devtools/shared/test/browser_filter-presets-02.js
@@ +10,5 @@
> +
> +add_task(function* () {
> +  yield promiseTab("about:blank");
> +
> +  let [host, win, doc] = yield createHost("bottom", TEST_URI);

Same comment about host and win not being used here.

@@ +23,5 @@
> +
> +  yield showFilterPopupPresetsAndCreatePreset(widget, NAME, VALUE);
> +
> +  // reset value
> +  widget.setCssValue("saturate(100%) brightness(150%)");

Shouldn't you wrap this call in:
let onRender = widget.once("render");
and
yield onRender;

::: browser/devtools/shared/test/head.js
@@ +291,5 @@
> +  widget._togglePresets();
> +  return onRender;
> +}
> +
> +let showFilterPopupPresetsAndCreatePreset = Task.async(function*(widget, name, value) {

Missing jsdoc comment.

Comment 44

[Patrick Brosset <:pbro>]

Comment on attachment 8625297 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

Ok looks good, I still see a few of my earlier remarks not addressed, but most have been fixed.
The invalid filters problems are still here, but let's file a follow-up for this.

::: browser/devtools/shared/widgets/FilterWidget.js
@@ +531,5 @@
> +    this.getPresets().then(presets => {
> +      if (el.classList.contains("remove-button")) {
> +        // If the click happened on the remove button.
> +        presets.splice(id, 1);
> +        this.setPresets(presets).then(this.renderPresets, console.error);

s/console.error/Cu.reportError

@@ +539,5 @@
> +
> +        this.setCssValue(p.value);
> +        this.addPresetInput.value = p.name;
> +      }
> +    });

Missing rejection handler here: Cu.reportError

@@ +567,5 @@
> +      } else {
> +        presets.push({name, value});
> +      }
> +
> +      this.setPresets(presets).then(this.renderPresets, console.error);

s/console.error/Cu.reportError

@@ +568,5 @@
> +        presets.push({name, value});
> +      }
> +
> +      this.setPresets(presets).then(this.renderPresets, console.error);
> +    });

Missing rejection handler here: Cu.reportError

::: browser/devtools/shared/widgets/filter-widget.css
@@ +173,5 @@
> +.theme-light .preset:hover .remove-button {
> +  filter: invert(0);
> +}
> +
> +.preset {

This rule needs to be merged with the other .preset {} rule defined earlier.

Comment 45

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset

Comment 46

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

Couldn't push to try as it's closed. The tests passed locally and I don't think a try is really necessary, but I'll try to push if I don't forget to.

Comment 47

[Patrick Brosset <:pbro>]

Comment on attachment 8628784 [details] [diff] [review]
Bug 1153184 - UI Tests for the css filter presets popup changes; r=pbrosset

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

I had already R+'d this patch. The last minor comments didn't require a re-review form me. Just marking as R+ again.

Comment 48

[Patrick Brosset <:pbro>]

Comment on attachment 8628785 [details] [diff] [review]
Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset

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

I had already R+'d this patch. The last minor comments didn't require a re-review form me. Just marking as R+ again.

Please do push to try.

Comment 49

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fdbd989cb9f

Comment 50

[Carsten Book [:Tomcat]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #48)
> Comment on attachment 8628785 [details] [diff] [review]
> Bug 1153184 - Persist CSS filter presets for reuse; r=pbrosset
> 
> Review of attachment 8628785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had already R+'d this patch. The last minor comments didn't require a
> re-review form me. Just marking as R+ again.
> 
> Please do push to try.

that part failed to apply:

patching file browser/devtools/shared/widgets/Tooltip.js
Hunk #1 FAILED at 850
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/shared/widgets/Tooltip.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh presets-old.diff

can you take a look, thanks!

Comment 51

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Weird, I updated and applied the patches without any issues on top of your latest commit:

changeset:   251455:cef11c3e86c3
tag:         central
parent:      251434:0bc555946d99
parent:      251454:f9aaae416bbb
user:        Carsten "Tomcat" Book <[e-mail]>
date:        Mon Jul 06 11:31:27 2015 +0200
summary:     merge mozilla-inbound to mozilla-central a=merge


mahdi :: ~/Documents/Workshop/Firefox ~ $ hg qseries
presets-old.diff
bug1153184_css-filter-presets-tests.diff
bug1153184_fix-filter-editor-tests.diff

What might be causing the problem so I can investigate?

Comment 52

[Carsten Book [:Tomcat]]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #51)
> Weird, I updated and applied the patches without any issues on top of your
> latest commit:
> 
> changeset:   251455:cef11c3e86c3
> tag:         central
> parent:      251434:0bc555946d99
> parent:      251454:f9aaae416bbb
> user:        Carsten "Tomcat" Book <[e-mail]>
> date:        Mon Jul 06 11:31:27 2015 +0200
> summary:     merge mozilla-inbound to mozilla-central a=merge
> 
> 
> mahdi :: ~/Documents/Workshop/Firefox ~ $ hg qseries
> presets-old.diff
> bug1153184_css-filter-presets-tests.diff
> bug1153184_fix-filter-editor-tests.diff
> 
> What might be causing the problem so I can investigate?

hmm did you apply all 3 patches or do we only need to checkin the 2 patches ?

like bug1153184_css-filter-presets-tests.diff
> bug1153184_fix-filter-editor-tests.diff

Comment 53

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

All three patches, actually `preset-old.diff` is the main patch.

Comment 54

[Carsten Book [:Tomcat]]

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #53)
> All three patches, actually `preset-old.diff` is the main patch.

ok setting checkin-needed again then :) thanks!

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8b5c398e30d7
https://hg.mozilla.org/integration/fx-team/rev/f087e6f929c9
https://hg.mozilla.org/integration/fx-team/rev/8d615b05f65d

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b5c398e30d7
https://hg.mozilla.org/mozilla-central/rev/f087e6f929c9
https://hg.mozilla.org/mozilla-central/rev/8d615b05f65d

Comment 57

[Tim Nguyen :ntim]

Just tested this out, and the feature is pretty awesome ! 
Although, I think the "Presets" button could look prettier as an icon button, the pseudo class lock icon could possibly be used there. What do you think ?

Comment 58

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #57)
> Just tested this out, and the feature is pretty awesome ! 
> Although, I think the "Presets" button could look prettier as an icon
> button, the pseudo class lock icon could possibly be used there. What do you
> think ?
Agreed, we need a pretty icon button there. Filed bug 1184110.

Comment 59

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Good idea! Let's talk in the new bug.

Comment 60

[Will Bamberg [:wbamberg]]

I've updated the page on editing filters, please let me know if this covers it: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Edit_CSS_filters

Thanks!

Comment 61

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Great! The only thing I'd like to have added is an screenshot with the presets list visible, that would be helpful.

Thank you Will, awesome work!

Comment 62

[Will Bamberg [:wbamberg]]

Good idea Mahdi, I've added that.

Comment 63

[Tim Nguyen :ntim]

Release Note Request (optional, but appreciated)
[Why is this notable]: Great addition to the Filter Tooltip.
[Suggested wording]: Ability to save filter presets inside CSS Filter Tooltip.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Edit_CSS_filters#Saving_filter_presets

Comment 64

[Sylvestre Ledru [:Sylvestre]]

Added to the 42 release notes using Tim's wording.