I think it's quite useful if we had a CSS filter inspector. The CSS filters are about to be fully supported soon (see bug 948265). You can currently play around with blur() and drop-shadow(), others don't work yet. If you want to test this, set layout.css.filters.enabled to true. As for the CSS filter editor, we could put a filter swatch that will show a tooltip with some sliders like this : http://html5-demos.appspot.com/static/css/filters/index.html

How do you like the idea ?

Love it! A new swatch-based editor tooltip for this would be pretty awesome, especially that it's the kind of css feature for which sliders make perfect sense.

Setting myself as mentor on this if needed.

Interested to work in this bug.

(In reply to Nikhil Agarwal from comment #4) > Interested to work in this bug. Assigned it to you :) Patrick will mentor you through this.

Thanks for your interest Nikhil. This isn't a very simple bug, especially if this is your first bug, but that's fine, we can always split the work in several patches and work in steps. First you need to set up your development environment. Since you're now to bugzilla, I guess you haven't done this yet. Please follow the steps described here: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking Let me know when you're ready, then I'll guide you in the code. Don't hesitate to join IRC to ask for help (#devtools for devtools specific questions, #developers and #introduction for more general questions).

Here's some help to get started with the code. There are essentially 5 parts to this bug: - the filter editor UI - adding filter swatches in the rule-view - the tooltip itself - loading the filter editor UI in the tooltip - the mechanism to update the css on the page when changes are made in the editor tooltip Part 1) The hardest part is the filter editor itself. It requires a bunch of UI to be made and there's no code that already exists to get you started in mozilla. This should either be done from scratch or by reusing an open source project. The good news about this though is that you can work on this in a normal webpage. There's no sense integrating it into the devtools and building over and over again to work on this. Just set up a standard HTML page, with all the CSS and JS you need to create the wanted css filter editor. For the other 4 parts, you'll need some vocabulary first: - Rule-view: Open the devtools, switch to the inspector, the rule-view is the "Rules" tab in the sidebar. - When we talk about tooltips here, we mean tooltips in the rule-view: things that appear when you mouseover a background-image url for instance, or a font-family name, or click on a color-swatch. - Swatch is the small circle icon next to some properties in the rule-view: color swatches next to colors, timing-function swatches next to timing-functions. - SwatchBasedEditorTooltip: is a name for any tooltip that appears when you click on a little circular swatch in the rule-view. 2 examples: color picker tooltips, cubic-bezier timing-function tooltips. These types of tooltips are normally *editable* tooltips, which means that they allow the user to alter the css property value with a specific UI. Part 2) We need filter swatch icons to appear in the rule-view, next to css filter property values. /toolkit/devtools/output-parser.js is where css values (like colors and timing-functions) are parsed and converted into HTML elements with little swatches next to them. The thing about filters though is that they can't appear in random places. A color is different because you can have it in places like: background: red url(....); or color: green; or border: 1px solid black; So we actually need to parse the values to find out if there are colors and where. For filters though, they can only appear in a filter property: filter: blur(2px) drop-shadow(...); So we don't need to parse, we just need to append a swatch at the beginning of the value, if the property name is 'filter'. This should be a bit easier. I suggest modifying parseCssProperty (in output-parser.js) for this, if name is 'filter', the prepend a swatch element (you can take a look at how color swatches are created further down in _appendColor). To make the new swatch actually visible, it needs to have some style. See .ruleview-colorswatch in /browser/themes/shared/devtools/ruleview.css Part 3) Creating the tooltip itself is easier (and can be worked on first, if you want to) as we already have many tooltips in the devtools, so it's essentially about reusing/extending existing code. So, for this bug, we need to create a new type of SwatchBasedEditorTooltip which we can call SwatchCSSFilterTooltip or something like this. If you open /browser/devtools/shared/widgets/Tooltip.js, you will find a class called SwatchBasedEditorTooltip. This is the base class for all swatch-based tooltips. There is no need to change this class as it already provides most of what is needed here. Instead, you need to create a new class, much like SwatchColorPickerTooltip and SwatchCubicBezierTooltip. I suggest starting by copying one of these classes then getting rid of everything that is specific to them and you won't need. Once you have this new class set up (even if it doesn't do anything at first), you'll need to update the rule-view so that the new tooltip shows for filter properties. Open /browser/devtools/styleinspector/style-inspector-overlay.js This is where all rule-view (and computed-view) tooltips are managed. Check TooltipsOverlay.prototype.addToView for instance, you'll see this is where the color-picker is added (and removeFromView is where it's removed). Also, you'll need to modify /browser/devtools/styleinspector/rule-view.js. See TextPropertyEditor.prototype.update which is where properties are parsed and swatched are hooked up to tooltips. Search for colorPicker.addSwatch in this file: this.ruleEditor.ruleView.tooltips.colorPicker is an instance of SwatchColorPickerTooltip here (so in your case, you would have something like this.ruleEditor.ruleView.tooltips.filterTooltip which would be an instance of SwatchCSSFilterTooltip). You'll see the code calls .addSwatch(...) on colorPicker, this is to make it so that when you click on the swatch, the tooltip actually opens. Part 4) Loading the UI in the tooltip container. See /browser/devtools/shared/widgets/Tooltip.js, method setCubicBezierContent(). We'll need something similar for filter tooltips. Part 5) Going back to the rule-view, where addSwatch is called: this is where the magic happens. I've given a lot less details towards the end, on purpose, cause I think there's already a lot of work to be done on the first parts. Let's start and iterate. Apart from part 1, there's a lot of code to reuse/copy, so this isn't too hard, but it's a big amount of work.

Just a WIP of part 1 : http://jsfiddle.net/ntim/pc13goc6/ Note that I've only implemented the UI. Feel free to do the rest :)

Patrick, I wrote some edge cases in the fiddle, what do you think we should do in those cases ?

Thanks Tim for raising these questions, they mean we have some non trivial problems to address. Here's my take: > If multiple instances of a same filter are specified (that is allowed in the spec, and in Firefox too) , what to do ? Indeed, users can specify as many filter functions as they want, even several times the same functions. Each function is applied, in order. So 'blur(1px) blur(1px) blur(1px)' will blur 1px, then blur the result 1px again and then again once more. And of course things can be mixed up: 'blur(2px) contrast(10%) blur(5px)'. I think we should replicate the list of currently applied filters in the UI rather than showing a fixed list of filter functions, and add the ability to delete functions, add new functions and move functions around. Something like this could work: http://jsbin.com/hukazazovese/1/ > What to do if the value specified by the user is over the range that the editor provides ? Let's use the computed value. 'getComputedStyle(element).filter' will return exactly what gecko has interpreted and I think that's what we need to display. > What to do if the user provides an em unit ? Same here, computed style will have units in px. > Should we add url() support here ? This is complex, because you can do 'filter: blur(3px) url(myfilter.svg#filter) contrast(90%)'. We can choose to ignore it for now, but that means the tooltip will be useless for people that use it. Having said that we can probably safely assume the number of people mixing filter-functions and urls in the same filter to be 0 for version 1 of this tooltip, and iterate from there.

There's been no activity for 6 months, so unassigning.

Hi, I'm interested in this bug, can you assign it to me please? I made the UI, let me know what you think, anything missing or any changes you wish to see. https://github.com/mdibaiee/CSS-Filter-Tooltip note: I use chrome:// urls here, I'm pretty sure you're opening it in Firefox, aren't you? :D Also, I asked for the font used in DevTools in IRC, got no answer, but my guess is Clear Sans, is that right? Thanks

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #12) > Also, I asked for the font used in DevTools in IRC, got no answer, but my > guess is Clear Sans, is that right? The font is platform specific, but you can get the right font on each platform by using font: message-box;

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #12) > Hi, I'm interested in this bug, can you assign it to me please? > > I made the UI, let me know what you think, anything missing or any changes > you wish to see. > > https://github.com/mdibaiee/CSS-Filter-Tooltip Looks pretty cool ! In terms of features, drop-shadow should probably have a different UI. Maybe an input followed with a color picker ? Also, the add filter combobox could be shown all the time too, it would be faster that way to add filters.

Thanks Tim, I agree, having the combo box always visible and the button adding directly would be a lot faster. I hadn't thought of that, I have to take a look at the CSS Specification of filters to make sure they fit developers' needs.

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #15) > Thanks Tim, I agree, having the combo box always visible and the button > adding directly would be a lot faster. > > I hadn't thought of that, I have to take a look at the CSS Specification of > filters to make sure they fit developers' needs. For v1, a simple text input is fine. The user can then type anything he wants in it. As for the future, maybe an input for each shadow definition (x-axis, y-axis, spread, color input). I'm about to submit a PR to support all filter types.

Thanks Mahdi and Tim, that UI looks good. Some comments: - I like it :) - Sliders do make sense for most filters, except for drop shadow of course, as Tim said. - However, some filter support values with no upper bounds, and that's not really feasible with a slider. Like saturate for instance, you can set values far beyond 100%. Here's a different UI proposal for this: http://jsbin.com/zanaqicamo/2/ . It is inspired by Photoshop. The way this works is you can either use the text box to enter any value you want, or drag from the label, left or right, to decrement or increment the value. This could also be coupled with keybindings (holding alt would move in 0.1 increments, holding shift would move in 10 increments). Let me know what you think. - In any case, I'd like to see the value being displayed in the tooltip too. There should at least be the current value displayed next to each individual filter, and maybe the min/max values too, when applicable. - My first intention was to display all filters at all times. Because we have a limited number of existing filters, we could just display them all, with values set to 0 for those that aren't currently applied. But I like your approach with the dropdown to add new ones, I think it makes sense. And, I just realized that the order of filters is important, so displaying all filters in a list isn't going to work, we need to be able to add/remove/re-order filters. And your UI works better for this.

Thanks Patrick! I'm going to add support for string-based filters like drop-shadow. I really like the idea of dragging over label, keybindings will make it even better. You're right, we have to display the current value of the filter, Oh, back to drag/drop re-ordering :D Thanks Patrick and Tim, going to work on it soon, I'm on holidays so I have a plenty of free time, shouldn't take long.

I fixed the issue with #css() returning something like NaNnull for drop-shadow and filters alike. Added drag/drop re-ordering of elements using a little indicator at left side of each filter. I thought making the whole thing draggable would interfere with draggable labels, sliders, etc. I made it possible to drag labels to increase/decrease value of filters with no upper limit. Holding Alt and Shift work as expected (add 0.1 and 10 respectively). Something to note here is in order for this to work, I set all inputs' step property to 0.1, also replaced all |parseInt|s with |parseFloat|. Sliders' values are now shown. I renamed filter definitions' (filterList.js) "unit" property to "type" as I think it makes more sense, and there's no confusion between definition's type and filter's unit which is the unit defined by user (px, em, etc.)

I moved the master branch to gh-pages so you can test it online without pulling. (please update your git remote) https://github.com/mdibaiee/CSS-Filter-Tooltip

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #19) > I fixed the issue with #css() returning something like NaNnull for > drop-shadow and filters alike. > > Added drag/drop re-ordering of elements using a little indicator at left > side of each filter. I thought making the whole thing draggable would > interfere with draggable labels, sliders, etc. Cool, thanks for adding this. And yes, I agree, adding the indicator works better. One detail though, while dragging, you should set a class on the body that declares -moz-user-select:none, to avoid text from being selected while dragging. > I made it possible to drag labels to increase/decrease value of filters with > no upper limit. Holding Alt and Shift work as expected (add 0.1 and 10 > respectively). Something to note here is in order for this to work, I set > all inputs' step property to 0.1, also replaced all |parseInt|s with > |parseFloat|. I couldn't get the Alt+drag to work properly, holding Alt doesn't seem to do anything for me. Anyway, that's not important at this stage. I suggest dividing the distance by 2 or something. The sensitivity is too high right now I think (without alt or shift, dragging right increases the value pretty fast, a pixel should probably not be an increment of 1, but rather .5). > Sliders' values are now shown. Nice. > I renamed filter definitions' (filterList.js) "unit" property to "type" as I > think it makes more sense, and there's no confusion between definition's > type and filter's unit which is the unit defined by user (px, em, etc.) One other comment, we need some more alignment in the UI I think. The layout should basically be 2 columns, one for the labels, and one for the values. Also, with both the label dragging technique and the slider in place, it may be hard for a user to know why there are 2. What do you think about only have the label dragging think? And only text boxes for the values? The sliders are nice for sure, they're really made for this sort of UI, I'm just worried about having too many different ways to operate this. (In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #20) > I moved the master branch to gh-pages so you can test it online without > pulling. (please update your git remote) > > https://github.com/mdibaiee/CSS-Filter-Tooltip Thanks, that makes it easier. For other people reading: you can access Mahdi's demo here: http://mdibaiee.github.io/CSS-Filter-Tooltip/

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21) > (In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #19) > > I made it possible to drag labels to increase/decrease value of filters with > > no upper limit. Holding Alt and Shift work as expected (add 0.1 and 10 > > respectively). Something to note here is in order for this to work, I set > > all inputs' step property to 0.1, also replaced all |parseInt|s with > > |parseFloat|. > I couldn't get the Alt+drag to work properly, holding Alt doesn't seem to do > anything for me. Anyway, that's not important at this stage. You should be able to step like this, by focusing the input and using arrow keys anyway. > One other comment, we need some more alignment in the UI I think. The layout > should basically be 2 columns, one for the labels, and one for the values. Yeah, you should probably rely on some grid/table layout. My flexbox hacks don't work very well anyway. > Also, with both the label dragging technique and the slider in place, it may > be hard for a user to know why there are 2. What do you think about only > have the label dragging think? And only text boxes for the values? > The sliders are nice for sure, they're really made for this sort of UI, I'm > just worried about having too many different ways to operate this. I agree with this. I would personally go for the native techniques, since they provide great accessibility compared to the draggable label.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21) > One detail though, while dragging, you should set a class on the > body that declares -moz-user-select:none, to avoid text from being selected > while dragging. Done > I couldn't get the Alt+drag to work properly, holding Alt doesn't seem to do > anything for me. Anyway, that's not important at this stage. > I suggest dividing the distance by 2 or something. The sensitivity is too > high right now I think (without alt or shift, dragging right increases the > value pretty fast, a pixel should probably not be an increment of 1, but > rather .5). You should first hold the mouse down, then hold then Alt, holding modifiers first doesn't work (that's because I bind the event after the mousedown fires, should I change this?) I think with alt having a ratio of 0.1 there's no need to reduce the sensitivity. > One other comment, we need some more alignment in the UI I think. The layout > should basically be 2 columns, one for the labels, and one for the values. Fixed, we have a <table> layout now. > Also, with both the label dragging technique and the slider in place, it may > be hard for a user to know why there are 2. What do you think about only > have the label dragging think? And only text boxes for the values? > The sliders are nice for sure, they're really made for this sort of UI, I'm > just worried about having too many different ways to operate this. I thought the same, having label dragging only should be fine, sliders were making the layout ugly, too, with an added value indicator, so I removed the sliders and all the inputs are [type="number"] now. Label dragging respects min/max values now. I also fixed the issues mentioned by Tim on GitHub. Thanks Tim! You point out the small details that I miss.

Clearing out the NI? flag as Mahdi and I discussed via PRs at https://github.com/mdibaiee/CSS-Filter-Tooltip/pulls?q=is%3Apr+is%3Aclosed

I've integrated the tooltip in Firefox, but I see a few problems and I'd like to ask you. There is a weird problem: Tooltip updates the filter's value correctly if you define the filters first (i.e. enter blur(2px) brightness(2%) in the rule-view, then click swatch), or if you remove a filter from tooltip, but if you try adding a new filter from the toolbar, it doesn't work. I've checked the value returned be .css(), it's OK. The weird part is it works if you open Browser Toolbox and inspect the filter value span (.ruleview-filter I guess), span's textContent IS updated, but the value shown is not. Another thing I'd like to ask is what do you think about removing the tooltip wrapper (the transparent box wrapping our tooltip)? How can I do that? Thanks Patrick!

You forgot to add the file FilterWidget.js to your patch, which makes the build fail on my machine, because it's referenced in a build file. Can you update the patch so I can test locally?

Oh, sorry, added the missing files.

Comment on attachment 8580010 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8580010 [details] [diff] [review]: ----------------------------------------------------------------- I haven't done a full review because I think you were primarily concerned with the questions in comment 25. I did do a quick review though. I recommend for next time to only use the review flag (R?) when you think your patch is finalized. If you want to upload a patch for me to have a look and answer some questions, that's completely fine, but you should use the feedback flag instead (F?). (In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #25) > There is a weird problem: Tooltip updates the filter's value correctly if > you define the filters first (i.e. enter blur(2px) brightness(2%) in the > rule-view, then click swatch), or if you remove a filter from tooltip, but > if you try adding a new filter from the toolbar, it doesn't work. I've > checked the value returned be .css(), it's OK. > > The weird part is it works if you open Browser Toolbox and inspect the > filter value span (.ruleview-filter I guess), span's textContent IS updated, > but the value shown is not. First of all, instead of using a callback for your update mechanism, could you use EventEmitter? See the file CubicBezierWidget.js for an example on how to use it. Basically, you just need to make your class an event emitter, and once done, you can emit events with this.emit("event-name", someData); This makes it a lot easier for consumers of your class to react to changes. It would also make it more consistent with the rest of the devtools code. Secondly, congrats! you just discovered a nasty bug in our tooltip widget. You're the first person to use a <select> element inside a tooltip, and it turns out that <select>s fire the same show/shown/hide/hidden events that XUL <panel>s do, and our tooltip code isn't checking event targets when these events occur, and so because of event bubbling, the hidden event of the select was handled by the tooltip, and that caused the current filter value to be applied, and therefore the property in the rule-view to be refreshed, and what this means is that your tooltip was no longer modifying the right <span> in the rule-view, but one that had been detached from the DOM, hence you were not seeing the updates anymore. I'll file another bug to fix this issue and attach a patch there that you can work on top, to avoid having this problem. > Another thing I'd like to ask is what do you think about removing the > tooltip wrapper (the transparent box wrapping our tooltip)? How can I do > that? I don't think you should do that in fact. This wrapping element corresponds to our common tooltip styling, and removing it would make that tooltip look different from the other ones in devtools. What I suggest instead is removing the gray bordered box you have in your widget, it would look nicer. One other thing too, when more filters are added, the content of the tooltip scrolls, which is fine, but the add select+button is pushed down, that's not good, they need to be visible at all times. So you should absolutely position it at the bottom, and have the list of filters scroll instead. A better solution in fact would be to make the tooltip size adapt to its content automatically. ::: browser/devtools/jar.mn @@ +146,5 @@ > content/browser/devtools/cubic-bezier-frame.xhtml (shared/widgets/cubic-bezier-frame.xhtml) > content/browser/devtools/cubic-bezier.css (shared/widgets/cubic-bezier.css) > + content/browser/devtools/filter-frame.xhtml (shared/widgets/filter-frame.xhtml) > + content/browser/devtools/filter.css (shared/widgets/filter.css) > + content/browser/devtools/add.svg (shared/widgets/add.svg) This shouldn't be here, see my comment in add.svg This should be in: /browser/themes/<os>/jar.mn ::: browser/devtools/shared/widgets/FilterWidget.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + Please insert a general comment block here to explain what this file is about, what it contains, maybe how to use the classes in it, etc... It helps people opening the file for the first time. @@ +5,5 @@ > +"use strict"; > + > +const LIST_ITEM_HEIGHT = 32; > + > +function FilterToolTip(el, value = "") { FilterTooltip isn't the best name, this class doesn't contain code for displaying a tooltip. Instead, it defines a UI widget that could potentially be loaded in any container, not just a tooltip. So maybe CSSFilterEditorWidget, I think it doesn't hurt to be more specific with naming here. Also please add some jsdoc-style comment before the function name to describe the expected parameters. @@ +11,5 @@ > + this.win = this.doc.ownerGlobal; > + this.el = el; > + this.val = value; > + > + let list = el.querySelector(".filters"); Could you please try and keep the constructor function a bit smaller by moving all markup initializing code in one method, and all event initializing code in another one? The constructor could then just do: this._initMarkup(); this._startEventListeners(); Which would also help because you can then define a _destroyMarkup and a _stopEventListeners method which you could call from your destroy method (it's empty right now). It's very important to have a destroy method. Nothing tells you where your widget will be used, and how, and it may be that we want to use it in the future in another panel somewhere and init and destroy it dynamically. @@ +25,5 @@ > + let select = this.filterSelect = el.querySelector("select"); > + this.populateFilterSelect(); > + > + let addButton = el.querySelector("#add-filter"); > + addButton.addEventListener("click", e => { In order to be able to remove this handler in destroy, you'll need to define the click handler callback in the class instead. And here you should do: this.onAddButtonClicked = this.onAddButtonClicked.bind(this) to make sure 'this' is really the instance in the function. Then move all of the callback code in onAddButtonClicked, in the class' prototype. And in startEventListeners, just do addButton.addEventListener("click", this.onAddButtonClicked); Which will make it possible to do this in stopEventListeners: addButton.removeEventListener("click", this.onAddButtonClicked); @@ +129,5 @@ > + base.appendChild(name); > + base.appendChild(value); > + > + if (!this.filters.length) { > + this.list.innerHTML = "<p>No filter specified <br/> Add a filter using the list below</p>"; Hard-coded English labels need to go in localized properties files. You'll need to create a new properties file in /browser/locales/en-US/chrome/browser/devtools/ for your widget and load it in this file. One way is: const STRINGS_URI = "chrome://browser/locale/devtools/<name of your properties file>.properties"; const L10N = new ViewHelpers.L10N(STRINGS_URI); Which you can then use like this: L10N.getStr(<name of the localized string>); @@ +150,5 @@ > + label.textContent = filter.name; > + > + input.classList.add("item-editor"); > + > + drag.addEventListener("mousedown", e => { I actually think that for this widget, the native HTML5 drag/drop API should work just fine: http://www.html5rocks.com/en/tutorials/dnd/basics/#toc-examples Do you think you could rework this dragging implementation with this? @@ +317,5 @@ > + for(let filter of this.filters.slice(id)) { > + filter.index--; > + } > + if(this._onUpdate) this._onUpdate(this.css()); > + }, nit: we normally separate functions with 1 empty line. @@ +360,5 @@ > + > + > + > + > +let filterList = [ nit: should be a const, at the top of this file instead. ::: browser/devtools/shared/widgets/add.svg @@ +1,1 @@ > +<svg width="18" height="18" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> This file should not be in this directory, it should be in: /browser/themes/shared/devtools/images/ along with the other svg image files we have. ::: browser/devtools/shared/widgets/filter-frame.xhtml @@ +7,5 @@ > +<html xmlns="http://www.w3.org/1999/xhtml"> > +<head> > + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> > + <link rel="stylesheet" href="chrome://browser/content/devtools/filter.css" type="text/css"/> > + <link rel="stylesheet" href="chrome://browser/skin/devtools/light-theme.css" type="text/css" id="theme-stylesheet"/> You should not include the light-theme css here. But instead: <link rel="stylesheet" href="chrome://browser/skin/devtools/common.css" type="text/css"/> <script type="application/javascript;version=1.8" src="theme-switching.js"/> common.css gives you some shared devtools UI css, and theme-switching makes sure the right theme is used in the popup. @@ +17,5 @@ > + <div class="filters"> > + </div> > + <div id="editor-footer"> > + <select value=""> > + <option value="">Select a Filter</option> This and "Add" below are hard-coded English labels. They should be localized in a dtd file. Search for the file animation-inspector.xhtml for an example of how to do this (i.e. add an ENTITY to load the dtd and prefix string names with &) @@ +23,5 @@ > + <button id="add-filter">Add</button> > + </div> > + </div> > + > + <script src="chrome://browser/content/devtools/filterList.js" type="application/javascript;version=1.8"></script> This looks like a left-over script include. You don't have a filterList.js file anymore. ::: browser/devtools/styleinspector/rule-view.js @@ +2518,5 @@ > } > > let colorSwatchClass = "ruleview-colorswatch"; > let bezierSwatchClass = "ruleview-bezierswatch"; > + let filterSwatchClass = "ruleview-filterswatch"; Change to "ruleview-swatch ruleview-filterswatch" same for the color and bezier classes. See my comment in ruleview.css for more info. @@ +2570,5 @@ > + let span = this.valueSpan.querySelector("." + filterSwatchClass); > + if (this.ruleEditor.isEditable) { > + if(span) { > + let originalValue = this.valueSpan.textContent; > + nit: trailing whitespaces. ::: browser/themes/shared/devtools/ruleview.css @@ +68,5 @@ > } > > .ruleview-rule[uneditable=true] .ruleview-colorswatch, > +.ruleview-rule[uneditable=true] .ruleview-bezierswatch, > +.ruleview-rule[uneditable=true] .ruleview-filterswatch { Can you introduce a class called "ruleview-swatch" that all of these elements share? This way we can simplify this rule and another one further down. .ruleview-rule[uneditable=true] .ruleview-swatch { ... } We still need to keep the other, more specific classes, but only use them where needed. @@ +151,5 @@ > } > > .ruleview-colorswatch, > +.ruleview-bezierswatch, > +.ruleview-filterswatch { .ruleview-swatch { ... } @@ +184,5 @@ > background-size: 1em; > } > > +.ruleview-filterswatch { > + background: url("chrome://browser/skin/devtools/cubic-bezier-swatch.png"); We will need a new icons for filters. I'm thinking something along the lines of photoshop's "layer style", or "adjustment layer" icons: https://dl.dropboxusercontent.com/u/714210/Screenshot%202015-03-19%2013.32.17.png ::: toolkit/devtools/output-parser.js @@ +102,5 @@ > // It avoids parsing "linear" as a timing-function in "linear-gradient(...)" > options.expectCubicBezier = ["transition", "transition-timing-function", > "animation", "animation-timing-function"].indexOf(name) !== -1; > > + options.expectFilter = name === "filter"; Having to do this is a bit sad because it shows the current output-parser isn't adapted at all to this use case. The output-parser normally doesn't have to know the property name, but instead just goes through the value, trying to parse it as it goes into specific types (colors, timing-functions, urls, ...). In your case, we don't need this parsing at all, once we know the property-name is filter, we know for sure that the value is going to be a space separated list of filter functions. And so, having to bail out of the _parse function using this option seems weird. So, one option would be to change the |if| below to have a special code path for properties that don't need parsing. I'm not too happy with this either, but I think it's a bit better: if (name === "filter") { return this._parseFilter(value, options); } else if (this._cssPropertySupportsValue(name, value)) { return this._parse(value, options); } This way _parse remains untouched. But then again, would we want to show a color swatch near "black" in this filter: drop-shadow(16px 16px 10px black) ? I'd say yes, in which case, we'd still need _parse to go through the value. But at the same time, we don't want to add a filterswatch next to each and every filter function. Let me think about this more.

Thanks Patrick! I've fixed everything you mentioned but didn't touch the _parse thing. I'd like to ask something before a full review: in order to make iframe's height dynamic, I'm emitting a "render" event whenever the widget re-renders, and I'm updating iframe's height like so: |iframe.height = container.scrollHeight|, the problem is, the tooltip jumps around the first time the height get's updated. Re-opening the tooltip and re-rendering the widget again doesn't seem to reproduce the same problem again, do you have any idea why? Thanks again Patrick, your feedbacks/reviews are clear and awesome in details!

Comment on attachment 8580182 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8580182 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/FilterWidget.js @@ +396,5 @@ > + let dest = index + push; > + > + if (push === 0 || // there's no change > + push === 1 && index === this.list.children.length-1 || // moving down the last element > + push === -1 && index === 0) return; // moving up the first element nit : These lines need to be aligned. @@ +398,5 @@ > + if (push === 0 || // there's no change > + push === 1 && index === this.list.children.length-1 || // moving down the last element > + push === -1 && index === 0) return; // moving up the first element > + > + if (dest < 0 || nit : only one space is needed between if and ( @@ +411,5 @@ > + else > + this.list.appendChild(dragging); > + > + dragging.style.top = delta - push*LIST_ITEM_HEIGHT + "px"; > + dragging.startingY = e.pageY + push*LIST_ITEM_HEIGHT - delta; nit : spaces around "*" sign ::: browser/devtools/shared/widgets/Tooltip.js @@ +1551,5 @@ > + if (!this.activeSwatch) { > + return; > + } > + > + nit : Remove the extra blank line ::: browser/devtools/shared/widgets/filter-frame.xhtml @@ +21,5 @@ > + <div id="editor-footer"> > + <select value=""> > + <option value="">&select;</option> > + </select> > + <button id="add-filter">&add;</button> Can you use a prefixed name (and also more precise name) for each string ? &FilterWidget.addButton for example. Also, the dtd file seems to be missing from this patch. ::: browser/devtools/shared/widgets/filter.css @@ +35,5 @@ > + color: var(--theme-body-color); > + padding: 10px; > + /*border: 1px solid var(--theme-splitter-color);*/ > + font: message-box; > + /*width: 300px;*/ nit : Please remove all the commented out code. @@ +42,5 @@ > +#container.dragging { > + -moz-user-select: none; > +} > + > +#container i { Can you mention what this is for ? Either with a precise class name, or with a comment. @@ +56,5 @@ > + width: 10px; > + height: 1px; > + background: #EEF0F2; > + box-shadow: 0 3px 0 0 #EEF0F2, > + 0 -3px 0 0 #EEF0F2; Instead of #EEF0F2 and filter: invert(1), you can simply set currentColor instead of #EEF0F2 (only for the drag icon) @@ +108,5 @@ > +} > + > +#add-filter { > + -moz-appearance: none; > + background: url(chrome://browser/skin/devtools/add.svg); I think you forgot to `hg add` the `add.svg` file`.

Comment on attachment 8580182 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8580182 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/filter-frame.xhtml @@ +8,5 @@ > +]> > +<html xmlns="http://www.w3.org/1999/xhtml"> > + <head> > + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> > + <link rel="stylesheet" href="chrome://browser/content/devtools/filter.css" type="text/css"/> I'm not sure if the filter.css file belongs here (should it be in themes/shared ?), Patrick will have a better idea. Also, the stylesheet could use a more explicit name like filter-widget.css ::: browser/themes/windows/jar.mn @@ +263,5 @@ > skin/classic/browser/devedition/urlbar-history-dropmarker.svg (../shared/devedition/urlbar-history-dropmarker.svg) > * skin/classic/browser/devtools/common.css (../shared/devtools/common.css) > * skin/classic/browser/devtools/dark-theme.css (../shared/devtools/dark-theme.css) > * skin/classic/browser/devtools/light-theme.css (../shared/devtools/light-theme.css) > + skin/classic/browser/devtools/add.svg (../shared/devtools/images/add.svg) Please also add a reference for aero (at the end of the file).

(In reply to Tim Nguyen [:ntim] from comment #31) > Comment on attachment 8580182 [details] [diff] [review] > Bug 1055181 - CSS Filter Tooltip; r=pbrosset > > Review of attachment 8580182 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/shared/widgets/filter-frame.xhtml > @@ +8,5 @@ > > +]> > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > + <head> > > + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> > > + <link rel="stylesheet" href="chrome://browser/content/devtools/filter.css" type="text/css"/> > > I'm not sure if the filter.css file belongs here (should it be in > themes/shared ?), Patrick will have a better idea. > Also, the stylesheet could use a more explicit name like filter-widget.css Actually, I see the cubic-bezier css is also there. You can probably ignore that comment.

Comment on attachment 8580182 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset A few comments during my testing : - It seems that the value doesn't update at all when I add new filters, or when I dismiss the tooltip - String tooltip types (drop shadow & url) are not handled properly, they seem to expect a number.

- Dragging a filter label will update it's value but without an unit

Thanks Tim! You were right, I forgot to add the new files (localization and add.svg) I also fixed the string-typed filters expecting a number > - It seems that the value doesn't update at all when I add new filters, > or when I dismiss the tooltip Inability to add new filters is blocked by Bug 1145162 (see Depends on). About dismissing the tooltip, it seems clicking away commits the changes while hitting escape reverts changes back, I think it's the normal behaviour, but we can add a new "Save" button, what do you think? > Can you use a prefixed name (and also more precise name) for each string ? > &FilterWidget.addButton for example. I used animationinspector.dtd as a sample, and it didn't prefix names, do you think it's really necessary? I renamed strings to a more precise name (selectPlaceholder, addButton). > - Dragging a filter label will update it's value but without an unit I couldn't reproduce it, I tried dragging a |blur(2px)| label, it worked, can you please give instructions on how to reproduce? Thanks!

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #35) > Created attachment 8580651 [details] [diff] [review] > Bug 1055181 - CSS Filter Tooltip; r=pbrosset > > Thanks Tim! > > You were right, I forgot to add the new files (localization and add.svg) > I also fixed the string-typed filters expecting a number > > > - It seems that the value doesn't update at all when I add new filters, > > or when I dismiss the tooltip > > Inability to add new filters is blocked by Bug 1145162 (see Depends on). > About dismissing the tooltip, it seems clicking away commits the changes > while hitting escape reverts changes back, I think it's the normal > behaviour, but we can add a new "Save" button, what do you think? I think we can wait for bug 1145162. It should land pretty soon. > > Can you use a prefixed name (and also more precise name) for each string ? > > &FilterWidget.addButton for example. > > I used animationinspector.dtd as a sample, and it didn't prefix names, do > you think it's really necessary? I renamed strings to a more precise name > (selectPlaceholder, addButton). Well, if you take a look at debugger.dtd, it has prefixes. Also, having prefixes (in addition to comments) help localizers know where the string will be shown. > > - Dragging a filter label will update it's value but without an unit > > I couldn't reproduce it, I tried dragging a |blur(2px)| label, it worked, > can you please give instructions on how to reproduce? You can test a value like invert(0.5), or opacity(1) (which corresponds to invert(50%) and opacity(100%)). The current editor allows dragging to a value like invert(100) which acts like invert(1). The best solution for this would be converting all percent values (from 0 to 1), that don't have units, to values that have a percent unit (from 0% to 100%).

Comment on attachment 8580651 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8580651 [details] [diff] [review]: ----------------------------------------------------------------- I haven't tested this, but here are the nits I found. ::: browser/devtools/shared/widgets/FilterWidget.js @@ +17,5 @@ > + > +const filterList = [ > + { > + "name": "blur", > + "range": [0,null], nit : This needs a space after `,` @@ +42,5 @@ > + "type": "percentage" > + }, > + { > + "name": "hue-rotate", > + "range": [0,360], Same here @@ +47,5 @@ > + "type": "angle" > + }, > + { > + "name": "invert", > + "range": [0,100], Same here as well @@ +183,5 @@ > + input.classList.add("devtools-textinput"); > + break; > + } > + > + console.log(def.type, input.type); nit : Logging should be removed (but you can remove it later if it helps you) @@ +252,5 @@ > + }); > + } > + > + input.value = filter.value; > + el.id = "filter-"+index; nit : spaces around `+` sign @@ +264,5 @@ > + input.addEventListener("change", this._updateEvent.bind(this)); > + this.emit("render"); > + } > + > + let el = this.doc.querySelector("#filter-"+(sorted.length-1)+" input"); nit : spaces around `+` signs @@ +400,5 @@ > + push === 1 && index === this.list.children.length-1 || // moving down the last element > + push === -1 && index === 0) return; // moving up the first element > + > + if (dest < 0 || > + dest > this.list.children.length) return; nit : These need to be aligned @@ +402,5 @@ > + > + if (dest < 0 || > + dest > this.list.children.length) return; > + > + let target = push > 0 ? this.list.children[dest+1] : this.list.children[dest]; nit: Spaces around + sign @@ +456,5 @@ > + > + let unit = def.type === "string" ? "" : /[a-zA-Z%]+/.exec(value); > + unit = unit ? unit[0] : ""; > + > + console.log(name, def.type, unit); nit : Logging should be removed (but you can remove it later if it helps you) @@ +537,5 @@ > + * number for the rest (unit automatically determined) > + */ > + update(id, value) { > + let filter = this.get(id); > + console.log(value, filter); nit : Same comment about logging @@ +539,5 @@ > + update(id, value) { > + let filter = this.get(id); > + console.log(value, filter); > + filter.value = filter.unit ? fixFloat(value, true) : value; > + console.log(filter.value); nit : Same comment about logging ::: browser/devtools/shared/widgets/filter-widget.css @@ +28,5 @@ > + border: none; > + cursor: pointer; > +} > + > +#container { I would probably make more sense if the #container and #container.dragging were at the top (since they are more global styles). @@ +38,5 @@ > +#container.dragging { > + -moz-user-select: none; > +} > + > +/* drag/drop indicator */ Maybe handle would be better than indicator. ::: browser/devtools/styleinspector/rule-view.js @@ +2551,5 @@ > } > } > > // Attach the cubic-bezier tooltip to the bezier swatches > + this._bezierSwatchSpans = this.valueSpan.querySelectorAll("." + bezierSwatchClass.split(' ')[1]); This is probably not very clean. I'm not sure what's the best way to improve that though. Patrick will have a better idea. ::: browser/locales/en-US/chrome/browser/devtools/filterwidget.properties @@ +7,5 @@ > + > +# LOCALIZATION NOTE (FilterWidget.noFilter): > +# This string is displayed when filter's list is empty > +# (no filter specified / all removed) > +FilterWidget.noFilter=<p>No filter specified <br/> Add a filter using the list below</p> \ No newline at end of file \ No newline at end of file HTML tags don't belong in localization files. They should be created in the JS. ::: browser/themes/shared/devtools/ruleview.css @@ +191,5 @@ > background-size: 1em; > } > + .ruleview-filterswatch { > + background: url("chrome://browser/skin/devtools/cubic-bezier-swatch@2x.png"); > + background-size: 1em; Setting the background-size property here is redundant (same for the bezierswatch rule). It's already set outside the retina media query block.

Thanks Tim! I fixed everything you mentioned, but it seems like removing the |background-size: 1em| breaks on Retina displays. About using prefixes in .dtd and .properties files let's see what Patrick says, I don't see a single pattern followed by all files. Also, for the xSwatchClass variables, I used this: > const sharedSwatchClass = "ruleview-swatch "; > const colorSwatchClass = "ruleview-colorswatch"; > const bezierSwatchClass = "ruleview-bezierswatch"; > const filterSwatchClass = "ruleview-filterswatch"; > --------------------------------------------------------> > this.valueSpan.querySelectorAll("." + colorSwatchClass); What do you think? I think as soon as the tooltip jumping around and Bug 1145162 are fixed we're ready to review and test this.

Comment on attachment 8581092 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8581092 [details] [diff] [review]: ----------------------------------------------------------------- I don't have more comments to give, except those few nits. ::: browser/devtools/shared/widgets/FilterWidget.js @@ +135,5 @@ > + base.appendChild(name); > + base.appendChild(value); > + > + if (!this.filters.length) { > + this.list.innerHTML = `<p> ${L10N.getStr("noFilter")} <br /> nit: trailing whitespace @@ +141,5 @@ > + } > + > + let sorted = this.filters.sort((a, b) => a.index - b.index); > + > + for(let [index, filter] of sorted.entries()) { nit: Space after `for` @@ +208,5 @@ > + const mouseMove = e => { > + if (startX === null) return; > + lastX = e.clientX; > + let delta = e.clientX - startX; > + let value = startValue + delta*multiplier; nit : Spaces around * sign @@ +395,5 @@ > + let index = Array.prototype.indexOf.call(this.list.children, dragging); > + let dest = index + push; > + > + if (push === 0 || // there's no change > + push === 1 && index === this.list.children.length-1 || // moving down the last element nit : Spaces around "-" sign @@ +459,5 @@ > + if (def.type !== "string") { > + value = parseFloat(value); > + > + if(def.type === "percentage" && unit === "") { > + value = value*100; nit : Spaces around * @@ +493,5 @@ > + }, > + > + remove(id) { > + this.filters.splice(id, 1); > + for(let filter of this.filters.slice(id)) { nit : Space after `for` ::: browser/devtools/styleinspector/rule-view.js @@ +2519,5 @@ > > + const sharedSwatchClass = "ruleview-swatch "; > + const colorSwatchClass = "ruleview-colorswatch"; > + const bezierSwatchClass = "ruleview-bezierswatch"; > + const filterSwatchClass = "ruleview-filterswatch"; This approach looks good to me. ::: browser/locales/en-US/chrome/browser/devtools/filterwidget.properties @@ +10,5 @@ > +# (no filter specified / all removed) > +noFilter=No filter specified > + > +# LOCALIZATION NOTE (FilterWiddget.addUsingList): > +# This string is displayed under [noFilter] when filter's nit : trailing whitespace

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #38) > Created attachment 8581092 [details] [diff] [review] > Bug 1055181 - CSS Filter Tooltip; r=pbrosset > > Thanks Tim! > > I fixed everything you mentioned, but it seems like removing the > |background-size: 1em| breaks on Retina displays. That's pretty strange, background-size: 1em; is already set globally earlier in the file. Anyways, that's not very important.

An SVG you can use for the swatch. Let me know what you think.

Thanks Tim! Installed Whitespace plugin for Sublime to remove trailing spaces, that's certainly a must-have. :D And thanks for the swatch icon, let's think about it more. Personally I think the "fx" icon looks better, we might want to ask the UI/UX team, too, what do you think?

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #42) > Created attachment 8581112 [details] [diff] [review] > Bug 1055181 - CSS Filter Tooltip; r=pbrosset > > Thanks Tim! > > Installed Whitespace plugin for Sublime to remove trailing spaces, that's > certainly a must-have. :D > > And thanks for the swatch icon, let's think about it more. Personally I > think the "fx" icon looks better, we might want to ask the UI/UX team, too, > what do you think? I don't think the fx icon will fit inside a 12px circle anyway, and that's also the case for lots of icons.

I tweaked the icon to match other swatch icons, what do you think?

I tested the fx icon, as Tim said, it doesn't fit in 12px well. This patch uses the icon attached (8581332).

Comment on attachment 8581332 [details] filter-swatch.svg The border is useless on the svg, as the swatch css already has a border set. Other than that, this looks good to me.

(In reply to Tim Nguyen [:ntim] from comment #46) > Comment on attachment 8581332 [details] > filter-swatch.svg > > The border is useless on the svg, as the swatch css already has a border set. > Other than that, this looks good to me. Also, if you take a look at the cubic bezier swatch png icon, you'll see that there is no border (since the css sets it).

That's weird, this is what I get if I don't set stroke on SVG. Also, looking at rule-view.css I don't find anything relevant.

These selectors : [0], [1] need to be updated as well. [0] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#207 [1] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css#207

You're right, updated them and now it's working. Thanks Tim.

Comment on attachment 8581341 [details] [diff] [review] Bug 1055181 - CSS Filter Tooltip; r=pbrosset Review of attachment 8581341 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Mahdi for the patch. I see there's been quite a few changes since last time I looked and conversations with Tim, that's great. The tooltip looks really nice. A few global remarks before code review comments: - In the rule-view, add 'filter:none' to any element. Then click on the filter icon: ************************* A coding exception was thrown in a Promise resolution callback. See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise Full message: TypeError: value is undefined Full stack: setCSS/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/FilterWidget.js:527:33 setCSS@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/FilterWidget.js:525:5 SwatchFilterTooltip.prototype<.show/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/Tooltip.js:1548:11 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7 this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37 ************************* Can you take a look at this error and try and fix it? - The tooltip looks much nicer now, but can you maybe reduce the inner padding a bit? Try and make it similar to the padding in the color-picker tooltip. https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-padding.png - When there are no filters anymore (say you opened the tooltip and there were 3, but you deleted them 1 by 1), then the footer is actually below the fold, which creates a scrollbar: https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-scroll.png - When adding a url or drop-shadow filter functions, you should no prefill the value with '0', this may confuse the user as to what value is expected. More globally, I think the tooltip assumes a little too much that that user knows the syntax and possible values for each function. 2 things come to mind that could help: - add a placeholder attribute (greyed-out text in the field when it's empty) to the value fields that give an example of the expected value - for values that have a unit, that unit should be displayed somewhere (ideally after the field, and so, for those, you could make the field a bit shorter, so that things still align nicely). - About drag-dropping filters to change the order: it shouldn't be possible to drag to far towards the top and bottom: https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-drag-drop-bottom.png https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-drag-drop-top.png I think you should constrain the possibility to move to list container. However, you should listen to mousemove and mouseup on the whole tooltip window, because right now, if you mousedown on a handle, then start moving up by towards the left too, and you leave the list area, then the filter doesn't follow the mouse anymore. One other thing about this: re-ordering filters doesn't change the value in the rule-view, the order isn't being modified there. - Can you add title attributes to both drag handles (the one to re-order tooltips and the one to change a value) that say something like "move up or down to re-order filters" and "move left or right to decrease or increase the value". - What do you think about previewing at each keystroke in the value fields too? Right now if you want to enter a value by typing in the field, you need to exit to see the value applied. - The problem where the value didn't get updated when you clicked on the drop-down goes away if you apply my patch from bug 1145162 first (it hasn't been R+'d yet, but that shouldn't take long). *And* the bug where the tooltip jumps around when you resize the height automatically also disappears. So I suggest you import that patch in your patch queue and apply it first, before yours. - When adding a new filter (or opening the tooltip), there's a short amount of time where the height of the tooltip flickers a little bit. I'm assuming that's when you dynamically change the size to match the content. One thing that makes it more noticeable I think is that the scrollbar (at least on mac) appears and disappears quickly. Also, it gets even more visible when there are many filters. In that case, any filter addition or removal re-generates the whole list and re-size the tooltip slowly, as if it was doing it line per line: https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-resize.gif I haven't yet looked at the code, but there might be a better way to do this. (too bad the html5 <iframe seamless> attribute isn't supported yet, if it was, we could drop all of the resize code as this would happen automatically). - Which makes me wonder if we really need an iframe around the widget. I believe the first tooltip was the color-picker one, and we needed an iframe for that one for a reason I can't remember now. But We don't really need one here. Here's what the changes would look like if we go rid of it: - all strings should go in filterwidget.properties (but keys should be made really unique this time) and filterwidget.dtd should be removed, - the markup should be generated dynamically by CSSFilterEditorWidget, - there would be no need in setFilterContent to load an iframe, but instead just instantiate CSSFilterEditorWidget in this.content, - there would be no need to dynamically adjust the tooltip's size, this would happen automatically when content grows/shrinks (might need to do: this.panel.setAttribute("clamped-dimensions", "")), - the widget's css code should go in /browser/themes/shared/devtools/common.css. Overall, I think it would be a good thing to do, it should simplify things quite a bit. Sorry for not mentioning this earlier, but it just hit me when I saw the problems related to setting the height dynamically. One other advantage to this is it would make re-using this widgets elsewhere easier. You'd just have to import the widget, create a <div> somewhere, and instantiate it in it. No need to create and load an iframe. - With the dark theme, the filter swatch's border (or is it a box-shadow?) appears pixelated, at least on my mac (non-retina screen). See https://dl.dropboxusercontent.com/u/714210/css-filter-tooltip-icon-border.png Now, please find more review comments in the code below. For info, I have reviewed everything except FilterWidget.js which I haven't entirely finished. But I believe you've got enough to work on already as it is. I'd like to avoid having this review become a monster one (which it is already) and look again at this filter in the next review. ::: browser/devtools/shared/widgets/FilterWidget.js @@ +17,5 @@ > + > +const filterList = [ > + { > + "name": "blur", > + "range": [0, null], Why not use Infinity instead of null when there is no upper bound? It seems to convey the meaning better I think, and it's a valid js number that can be used for comparisons. @@ +32,5 @@ > + "type": "percentage" > + }, > + { > + "name": "drop-shadow", > + "range": [null, null], Why have a range at all for this value type? It's not going to be used, right? @@ +67,5 @@ > + "type": "percentage" > + }, > + { > + "name": "url", > + "range": [null, null], Same remark than for drop-shadow here. @@ +87,5 @@ > + * The widget container. > + * @param string value > + * CSS filter value > + */ > + nit: no empty line between a jsdoc comment and its associated function. @@ +92,5 @@ > +function CSSFilterEditorWidget(el, value = "") { > + this.doc = el.ownerDocument; > + this.win = this.doc.ownerGlobal; > + this.el = el; > + this.val = value; No need to save a couple of characters for this property, this.value is fine. @@ +111,5 @@ > + * binding required events. there are some delegated events > + * in _addEventListeners method > + * > + */ > + render() { I personally like this new syntax for defining function properties, but to my knowledge we don't use it yet in the devtools code, and so it would make this new widget inconsistent with the rest of the code. I'd prefer it if we stuck with render: function() for now. @@ +301,5 @@ > + > + this.emit("updated", this.css()); > + }, > + > + _initMarkup() { You could create the whole markup here instead of having to depend on what's in the HTML file. @@ +316,5 @@ > + }, > + > + _addEventListeners() { > + let select = this.filterSelect = this.el.querySelector("select"); > + this.populateFilterSelect(); Why is this called here? It sort of belongs more to _initMarkup I think, because it creates <option>s elements. @@ +434,5 @@ > + _definition(name) { > + return filterList.find(a => a.name === name); > + }, > + > + get(id) { Since we're accessing filters by index, I wonder if this getter is really needed. I think you should remove this function. @@ +446,5 @@ > + * filter name (e.g. blur) > + * @param string value > + * value of the filter (e.g. 30px, 20%) > + * @return number > + * new filter's id used to modify/remove/get the filter "The index of the new filter in the current list of filters" @@ +468,5 @@ > + if (min && value < min) value = min; > + if (max && value > max) value = max; > + } > + > + return this.filters.push({value, unit, name: def.name, index: this.filters.length}) - 1; Why store the index at filter item level here? A filter index is always going to be its index in the this.filters array, so it seems like duplicate information. @@ +476,5 @@ > + > + /** > + * returns value + unit of the specified filter > + * > + * @param number id this.filters is an array, and all modifications that you're making to it in this class are made by index, so id should be renamed index here. @@ +477,5 @@ > + /** > + * returns value + unit of the specified filter > + * > + * @param number id > + * filter id "The index of the filter in the current list of filters" @@ +483,5 @@ > + * css value of filter > + */ > + value(id) { > + let filter = this.get(id); > + if (!filter) return false; If there's no filter, return null instead, it conveys the intention better. @@ +491,5 @@ > + > + return val + unit; > + }, > + > + remove(id) { You should first check if id (which should be renamed index) actually exists in this.filters, and if it does not, bail out and return null. @@ +494,5 @@ > + > + remove(id) { > + this.filters.splice(id, 1); > + for (let filter of this.filters.slice(id)) { > + filter.index--; If you don't store the index, you don't need to run this loop at all. @@ +519,5 @@ > + * @param string value > + * css value to be parsed > + */ > + setCSS(value) { > + if (!value) return false; There isn't really a case where setting the CSS without a value is acceptable and so just returning false is too gentle :) You should throw an error. This also helps because it comes with a message: if (!value) { throw new Error("Missing CSS filter string value in setCSS"); } @@ +521,5 @@ > + */ > + setCSS(value) { > + if (!value) return false; > + this.filters = []; > + value.replace(/\s{2,}/g, " ").split(/\)\s/g).forEach((a, i, arr) => { You should start by trimming the value first: value.trim().replace.... I can see a problem with parsing the value like this though, try it with this value: "drop-shadow(3px 4px 5px rgba(0, 0,0,.4 ) )" The closing ) of the rgba color makes the splitting fail. In fact, this causes the following error when the tooltip is opened: ************************* A coding exception was thrown in a Promise resolution callback. See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise Full message: TypeError: value is undefined Full stack: setCSS/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/FilterWidget.js:527:33 setCSS@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/FilterWidget.js:525:5 SwatchFilterTooltip.prototype<.show/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/Tooltip.js:1549:11 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7 this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37 ************************* One way to make this parsing easier is to "compute" the value first by applying the value to a hidden DOM element and reading it again from its computed-style: hiddenDomElement.style.filter = value; let computedValue = getComputedStyle(hiddenDomElement).filter At least, after this, all extra spaces are removed, missing spaces are added, invalid values are removed or sanitized, things are properly in the right order. For instance: "drop-shadow(0 0 5px rgba(0,0,0,0.4 ) )" becomes: "drop-shadow(rgba(0, 0, 0, 0.4) 0px 0px 5px)" Of course it's still not an easy task because the rgb/rgba color functions have parenthesis, but it's more predictable. The next step is real tokenizing/parsing of the input, but to be honest here, because the input type can't really be anything and everything, I'd be ok to R+ a regex-based parsing. In your case, you might simply want to count the number of opening and closing parens to split the various filter functions. ::: browser/devtools/shared/widgets/Tooltip.js @@ +827,5 @@ > > return def.promise; > }, > > + setFilterContent: function(filter) { Can you add a jsdoc comment block before this function? /** * Fill the tooltip with a new instance of the CSSFilterEditorWidget * widget initialized with the given filter value, and return a promise * that resolves to the instance of the widget when ready. */ @@ +838,5 @@ > + iframe.setAttribute("flex", "1"); > + iframe.setAttribute("class", "devtools-tooltip-iframe"); > + > + let panel = this.panel; > + let xulWin = this.doc.ownerGlobal; This variable isn't used anywhere. @@ +848,5 @@ > + let container = win.document.getElementById("container"); > + let widget = new CSSFilterEditorWidget(container, filter); > + > + widget.on("render", e => { > + iframe.height = container.scrollHeight + 20; 20 is a magic number here, it needs to be defined as a named constant somewhere instead. Is it the size of the footer? If it's something that "belongs" to the widget, then add something like this in FilterWidget.js: exports.FOOTER_HEIGHT = 20; And import this variable and use it here. Also, the slow resizing thing I was mentioning in the general comments seems to be related to this. You should probably have an event that is only emitted once when something changes in the widget. Right now, if you have 10 filters, and you add one more, the render event is fired 11 times, which means the height of the iframe will be changed 11 times in a row, which is what makes the whole thing flickers. Having a render event is fine, if you need it for other things, but here, I believe you just need a simple "refreshed" event or something, that is emitted once after you've added/removed/changed/re-ordered a filter. @@ +1519,5 @@ > }); > } > }); > > +function SwatchFilterTooltip(doc) { jsdoc here too please: /** * The swatch-based css filter tooltip class is a specific class meant to be used * along with rule-view's generated css filter swatches. * It extends the parent SwatchBasedEditorTooltip class. * It just wraps a standard Tooltip and sets its content with an instance of a * CSSFilterEditorWidget. * * @param {XULDocument} doc */ @@ +1522,5 @@ > > +function SwatchFilterTooltip(doc) { > + SwatchBasedEditorTooltip.call(this, doc); > + > + // Creating a cubic-bezier instance. s/cubic-bezier/filter @@ +1530,5 @@ > +} > + > +exports.SwatchFilterTooltip = SwatchFilterTooltip; > + > +SwatchFilterTooltip.prototype = Heritage.extend(SwatchBasedEditorTooltip.prototype, { nit: the indentation inside this prototype object is off by 2 spaces. @@ +1532,5 @@ > +exports.SwatchFilterTooltip = SwatchFilterTooltip; > + > +SwatchFilterTooltip.prototype = Heritage.extend(SwatchBasedEditorTooltip.prototype, { > + show: function() { > + // Call then parent class' show function s/then/the @@ +1534,5 @@ > +SwatchFilterTooltip.prototype = Heritage.extend(SwatchBasedEditorTooltip.prototype, { > + show: function() { > + // Call then parent class' show function > + SwatchBasedEditorTooltip.prototype.show.call(this); > + // Then set the curve and listen to changes to preview them s/curve/filter value ::: browser/devtools/shared/widgets/filter-widget.css @@ +1,1 @@ > +#container { Missing license comment at the top of this new file: /* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ ::: browser/locales/en-US/chrome/browser/devtools/filterwidget.dtd @@ +1,4 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + It seems that we have every possible way of defining string keys in our dtd files (with prefixes, without prefixes, with . or _, using the type as a suffix, ...), so I don't think there is a right or wrong way to define those strings. The following criteria seem important: - string keys should be unique in the document there are used in, - it should be easy for a localizer to translate the value. You're building a shared widget, and so the uniqueness of the key is important if you think that perhaps that widget will be integrated into another document that already has strings of its own. But, since the widget can only be loaded in its own window (via an iframe), it's not a problem anymore. Also, in the case of this widget, it seems pretty improbable that the number of strings will grow a lot, and so having un-prefixed/un-suffixed string keys should work fine, as long as the keys are self-descriptive enough, and the comments help. @@ +2,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<!-- LOCALIZATION NOTE : FILE These strings are used in CSS Filter Editor Widget > + - which can be found in Rule View by clicking on a filter swatch --> "filter swatch" may be a bit hard to understand at first, could you rephrase to: "These strings are used in the CSS Filter Editor Widget which can be found in a tooltip that appears in the Rule View when clicking on a filter swatch displayed next to CSS declarations like 'filter: blur(2px)'. @@ +4,5 @@ > + > +<!-- LOCALIZATION NOTE : FILE These strings are used in CSS Filter Editor Widget > + - which can be found in Rule View by clicking on a filter swatch --> > + > +<!-- LOCALIZATION NOTE (select): This string is used as a preview option in a <select> --> Not just in "a" select but in "the list of possible filters <select>" @@ +5,5 @@ > +<!-- LOCALIZATION NOTE : FILE These strings are used in CSS Filter Editor Widget > + - which can be found in Rule View by clicking on a filter swatch --> > + > +<!-- LOCALIZATION NOTE (select): This string is used as a preview option in a <select> --> > +<!ENTITY selectPlaceholder "Select a Filter"> key should be: filterListSelectPlaceholder @@ +8,5 @@ > +<!-- LOCALIZATION NOTE (select): This string is used as a preview option in a <select> --> > +<!ENTITY selectPlaceholder "Select a Filter"> > + > +<!-- LOCALIZATION NOTE (add): This string is displayed on a button used to add new filters --> > +<!ENTITY addButton "Add"> Or addNewFilterButton ::: browser/locales/en-US/chrome/browser/devtools/filterwidget.properties @@ +2,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +# LOCALIZATION NOTE These strings are used in CSS Filter Editor Widget > +# which can be found in Rule View by clicking on a filter swatch Change this comment to be like the one in filterwidget.dtd @@ +7,5 @@ > + > +# LOCALIZATION NOTE (noFilter): > +# This string is displayed when filter's list is empty > +# (no filter specified / all removed) > +noFilter=No filter specified Or emptyFilterList @@ +9,5 @@ > +# This string is displayed when filter's list is empty > +# (no filter specified / all removed) > +noFilter=No filter specified > + > +# LOCALIZATION NOTE (FilterWiddget.addUsingList): Please change the key in () to be the actual key. ::: browser/themes/linux/jar.mn @@ +232,2 @@ > skin/classic/browser/devtools/filters.svg (../shared/devtools/filters.svg) > + skin/classic/browser/devtools/filter-swatch.svg (../shared/devtools/images/filter-swatch.svg) nit: can you align (../shared/...) with the other entries above and below, so that it looks like a column? ::: browser/themes/osx/jar.mn @@ +363,2 @@ > skin/classic/browser/devtools/filters.svg (../shared/devtools/filters.svg) > + skin/classic/browser/devtools/filter-swatch.svg (../shared/devtools/images/filter-swatch.svg) nit: can you align (../shared/...) with the other entries above and below, so that it looks like a column? ::: browser/themes/shared/devtools/dark-theme.css @@ +204,5 @@ > } > > +.ruleview-swatch, > +.computedview-colorswatch { > + box-shadow: 0 0 0 1px #c4c4c4; Why did you change the box-shadow color from #818181 to #c4c4c4 here? Copy/paste error from light-theme.css? Or intentional?

> - The problem where the value didn't get updated when you clicked on the > drop-down goes away if you apply my patch from bug 1145162 first (it hasn't > been R+'d yet, but that shouldn't take long). *And* the bug where the > tooltip jumps around when you resize the height automatically also > disappears. So I suggest you import that patch in your patch queue and apply > it first, before yours. No need to apply the patch yourself anymore if you work on the fx-team integration repo as I've just landed the patch for bug 1145162 there. It should make it to mozilla-central momentarily.

Mahdi left me a message on IRC about having to change the markup of the tooltip content to XUL if we get rid of the iframe. Mahdi: you don't necessarily need to do this, HTML can be inserted in XUL: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Adding_HTML_Elements

/r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull review -r 41db002be700550598a0a455c2f90b2047ee6d3a

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull review -r 41db002be700550598a0a455c2f90b2047ee6d3a

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5015 Thanks Mahdi. I've gone through the general comments in my previous review, and I see all of them have been addressed, awesome! The UI looks great too. Exciting. Here are some more general comments: * It looks like there was a regression in the last patch with dragging the label to increase/decrease values: dragging only changes the value in the field, not in the rule-view. Typing in the field works fine though. * Thanks for adding the units. This is minor, but maybe you could find a way to make sure all unit elements have the same width, so that the <input> fields that have units after them are still aligned nicely. * The title attribute doesn't seem to work indeed. This won't stop the patch from landing, but we should make sure to find the solution at some stage. So maybe file a follow-up bug about this. * I have a bug where, if I use the Shift or Alt key while dragging over a label once, then the multiplier stays, even if I then drag again later after having released the key. The x0.1 and x10 multipliers should really only apply __while__ I have the key pressed. * I have made a _lot_ of comments in FilterWidget.js below :) Don't worry, it's just that the class is really big and I hadn't had a chance to look at the code until now. I think most of my comments are about refactoring, trying to get the methods organized logically, short and sweet. Let's try and iterate on this front. Since mozreview shows interdiffs nicely, I'll stop here, let you go through another patch and I'll be able to see the interdiff and continue where I left off. Thanks for working on this Mahdi. It's not an easy bug, lot's of UI to get right, and UI is always really hard to get right the first time. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + "placeholder": "x y radius spread color", Thanks for adding the placeholders, that helps. However, I think this one is wrong, it should be: x y radius color I know spread is in the spec, but I haven't been able to make it work in Firefox. Also, this should be a localized string too, stored in the properties file, and loaded with L10N. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + switch (def.type) { Can you make the type <-> unit string mapping a const near the top of this file, like: const UNIT_MAPPING = { percentage: "%", length: "px", angle: "deg", string: "" }; const DEFAULT_UNIT = "length"; and then use it like: let unit = UNIT_MAPPING[def.type] || UNIT_MAPPING[DEFAULT_UNIT]; ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + value: function(id) { Rename id to index ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * filter id s/id/index ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + update: function(id, value) { rename id to index ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + nit: just one empty line between functions is enough. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > +function tokenize(css) { Rename to tokenizeComputedFilter ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * an array of [key, value] pairs Why not an array of {key, value} pair objects? ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + filters.push([key, value]); filters.push({key, value}); ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * CSS Filter value to be parsed Missing @return documentation. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + while (css.length) { This loop happens to work because you're making the assumption that the value contains at least one key(value). If you pass "none" as input to the function, it loops for ever. Also, the logic with parantheses, openingIndex and i is a little hard to read. Can you rewrite this function? Maybe something like this (not tested thoroughly): ``` function tokenize(css) { let filters = []; let current = ""; let depth = 0; while (css.length) { let char = css[0]; switch (char) { case "(": depth ++; if (depth === 1) { filters.push({name: current}); current = ""; } else { current += char; } break; case ")": depth --; if (depth === 0) { filters[filters.length - 1].value = current; } else { current += char; } break; default: current += char; break; } css = css.slice(1); } return filters; } ``` ::: toolkit/devtools/output-parser.js (Diff revision 2) > + options.expectFilter = name === "filter"; Can you ping Mike Ratcliffe (miker on IRC) to review the changes in this file? I made some comments about this in https://bugzilla.mozilla.org/show_bug.cgi?id=1055181#c28 but he's a better person to review this particular change. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * Parses CSS Filter value and returns s/Parses/Tokenizes Also, can you add something like this to this comment: This is only a very simple tokenizer that only works its way through parenthesis in the string to detect function names and values. It assumes that the string actually is a well-formed filter value (e.g. "blur(2px) hue-rotate(100deg)"). ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * nit: extra empty comment line not needed ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * Clears the list and renders filters nit: please re-phrase/re-format this comment keeping this in mind: - Cut lines at 80 chars, not before, especially if there's no dot, it makes it harder to know where the sentence ends. - Capitalize first letter after a dot. This is really minor, but it helps making comments easier to read, and therefore helps people reading the code for the first time. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + drag.title = "Drag up or down to re-order filter"; This should be localized in the properties file. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + label.title = "Drag left or right to decrease or increase the value"; This should be localized in the properties file. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + if (!this.filters.length) { If there are no filters, no need to create the markup above, so run this first in the function, and add a return statement in the if. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + let base = this.doc.createElement("div"); This function is really long. So, in an effort to try and make it smaller (therefore easier to understand), can you extract the DOM nodes creation into a separate function (called \_buildFilterItemMarkup or something like this). ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + let unitPreview = this.doc.createElement("span"); This markup creation seems misplaced, it should belong earlier in the function, where you create the rest of the markup for a filter item. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + multiplier = 10; Can you make the 0.1 and 10 multipliers const at the top of the file? const FAST_VALUE_MULTIPLIER = 10; const SLOW_VALUE_MULTIPLIER = 0.1; ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + const mouseMove = e => { I like how you have added delegation event listeners in addEventListeners, and I would like to see the key and mouse event listeners there too instead of adding them here. What you really need to do here is just add a mousedown listener on the label. Once that event occurs, you can store the clientX and a flag on the instance saying that the user is dragging a label and which one. `this._isDraggingLabel = label;` Or something like this. The advantage is that it would keep this function smaller too. And the dragging+keybinding logic would be moved to methods on the prototype of the class instead, where they are easier to read. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + input.addEventListener("input", e => { You end up adding 2 listeners for "input" here. Could you just add one and inside the callback, test for def.type !== "string". ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + let tmp = this.filters[a]; I don't think you need a temp variable if you replace the code in this function with: this.filters[a] = this.filters.splice(b, 1, this.filters[a])[0]; Also, this function doesn't really have anything to do with the core functionality of the parent class, so maybe move it out of the prototype, at the end of the file, as a generic helper function: function swapArrayIndices(array, a, b) { array[a] = array.splice(b, 1, array[a])[0]; } ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + * nit: empty comment line ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + let id = parseInt(li.id.slice(7), 10); The number 7 seems a bit magic here. Can you instead, add a const at the top of the file like: const FILTER_ITEM_ID_PREFIX = "filter-"; then use this const everywhere you have filter- in render, and here do this: parseInt(li.id.slice(FILTER_ITEM_ID_PREFIX.length), 10) ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + _initMarkup: function() { It's easier to read the code of a class you've never seen or used when the init/destroy methods are near the top of the prototype. Try and sort the methods in the prototype in a natural order. - createMarkup - addEvents - destroy - ... ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + this._addButtonClick = this._addButtonClick.bind(this); nit: other classes in devtools usually do the binding part in the constructor. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + let u = ""; Rename variable to unitLabel ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 2) > + this.update(id, e.target.value); The update method doesn't seem to be called from anywhere else, so I don't think it needs to exist at all. Can you remove it and put the code directly here instead. In fact, updateEvent is only called from one place too, but it looks like a generic place that all modifications should go to once done, but it's not the case. So either refactor it so that it's the only method that does this.emit("updated"...) in the class and all other methods that need it calls it, or move its code to where updateEvent is called.

https://reviewboard.mozilla.org/r/5965/#review5079 I've put comments on issues, but here's a full summary for reference: ####Second Patch: * I fixed the label-dragging not updating value in rule-view * The `update` method is now used in both label-dragging and `input` event, with _updateEvent removed * I disabled flex-shrink of filters' unit spans, so they all have the same width now * I think the modifier keys' bug might have been related to event listeners bound to label, now that they're delegated, they're fixed, at least I couldn't reproduce them * I put title attributes in .properties files and use them with L10N * You were right, moving filter items' markup creation to a method is better, and I thought, it was unnecessary to re-create the markup in every `render` call, so I moved it to constructor and set a private property `_filterItemMarkup` to be used in `render` * I declared constants for magic numbers, and also `DEFAULT_VALUE_MULTIPLIER = 1`. I think `DEFAULT_FILTER_TYPE` is a better name instead of `DEFAULT_UNIT`, what's your opinion? * Moved _swapIndexes out of prototype as you mentioned * I also privated the `populateFilterSelect`, I think it makes more sense * Ordered method definitions by their call time and relativity to others #####Questions * I'm not sure why drop-shadow's placeholder should be localized, but I put in .properties files anyway * `unitPreview` is created for number-type filters, so it shouldn't be created for all filters, that's why it's not in `_buildFilterItemMarkup`, do you think it's OK?

https://reviewboard.mozilla.org/r/5965/#review5077 > This markup creation seems misplaced, it should belong earlier in the function, where you create the rest of the markup for a filter item. `unitPreview` is created for number-type filters, so it shouldn't be created for all filters, that's why it's not in `_buildFilterItemMarkup`, what do u think? > Thanks for adding the placeholders, that helps. > However, I think this one is wrong, it should be: > > x y radius color > > I know spread is in the spec, but I haven't been able to make it work in Firefox. > > Also, this should be a localized string too, stored in the properties file, and loaded with L10N. I'm not sure why drop-shadow's placeholder should be localized, but I put in .properties files anyway > The update method doesn't seem to be called from anywhere else, so I don't think it needs to exist at all. > > Can you remove it and put the code directly here instead. > > In fact, updateEvent is only called from one place too, but it looks like a generic place that all modifications should go to once done, but it's not the case. > > So either refactor it so that it's the only method that does this.emit("updated"...) in the class and all other methods that need it calls it, or move its code to where updateEvent is called. The `update` method is now used in both label-dragging and `input` event, with _updateEvent removed > Can you make the type <-> unit string mapping a const near the top of this file, like: > > const UNIT_MAPPING = { > percentage: "%", > length: "px", > angle: "deg", > string: "" > }; > > const DEFAULT_UNIT = "length"; > > and then use it like: > > let unit = UNIT_MAPPING[def.type] || UNIT_MAPPING[DEFAULT_UNIT]; I think `DEFAULT_FILTER_TYPE` is a better name, what do you think?

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull review -r 972cc0156849d44045c335b7a2116c24a704fb16

https://reviewboard.mozilla.org/r/5965/#review5209 > Can you ping Mike Ratcliffe (miker on IRC) to review the changes in this file? > I made some comments about this in https://bugzilla.mozilla.org/show_bug.cgi?id=1055181#c28 but he's a better person to review this particular change. The changes to output-parser.js are fine for the moment. I really need to make time to revisit my fix to clearly identify which properties support which type.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5213 Nice! This is getting really good. As usual, first some global remarks: - Dragging the label to change the value works again normally (the value in the rule-view updates), great! - Also, the alt/shift multipliers work perfectly now. - I've also noticed that you can use the up/down arrow keys inside the number fields to change the value with the keyboard. That is nice (and I think this is provided natively by the number field, right?). The only thing I see with this is that we have a similar behavior with numbers in the rule-view, where you can use the arrow keys with the alt and shift modifiers to increment/decrement numbers. So, not for this patch (probably a follow-up bug), but I'd like the number fields in this tooltip to behave just like the fields in the rule-view. Can you file a follow-up bug? - I unfortunately found another regression with this patch: Add `filter: blur(1px)` to a rule in the rule-view, then click on the filter tooltip icon, then in the tooltip, add one more filter, hit ENTER on the keyboard, the tooltip closes. Now re-open the tooltip => You'll see the new filter you added in the tooltip is gone. This is a large piece of work, so it's not unusual that there are regressions of various features while you're working on it. One way to prevent this (or at least catch it) is to write tests as you go. You don't have to go full TDD on this, and I'm not sure it would be practical, but what you can do is, everytime you're done with a small feature (say dragging the label to update the value), then you write a small test just for this. The advantages are: everytime you finish another feature, you can run all the tests you've created so far and you're sure you haven't broken anything. Also, by the end, you'll have many small (maintainable) tests. - Another one is that I can't add drop-shadow filters anymore, the following error is thrown: def.range is undefined (line 269). Same for url filters. Other than that, I have a few comments below. I now have gone over most of the file. I think the only big remaining part that I haven't paid a lot of attention to is the drag/drop mechanism to re-order filters. It doesn't seem too complicated, so it should be ok, but I'd like to wait for the next update to review that last part. After that, we'll need several tests in /browser/devtools/shared/test/ for the widget itself (to be tested in isolation, in its own html frame, without a tooltip), one for each of the main features of the widget. And then a few tests (maybe just one) in /browser/devtools/styleinspector/test/ to ensure the integration of the tooltip with the rule-view works fine. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + "type": "string" // Well it's length, but it's also a special case nit: drop-shadow isn't really a length, I don't understand what this comment is trying to say. Better remove it I think. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + _buildFilterItemMarkup: function() { nit: add a jsdoc comment for this function that explains that the markup that is built here is stored as a DOM template to be cloned whenever a new filter is added. This isn't immediately obvious in the code so I think this comment would help. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + css: function() { Can you rename to 'getCssValue' ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + value: function(index) { Can you rename to 'getValueAt' ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + remove: function(index) { Can you rename to 'removeAt' ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + update: function(index, value) { Can you rename to updateValueAt ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + let filter = this.filters[index]; valueAt and removeAt both have a check to make sure index actually exist. This one should have a similar check. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + setCSS: function(value) { Rename to 'setCssValue' ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + this.add(a.name, a.value); You could also slightly simplify it with: for (let {name, value} of tokenizeComputedFilter(computedValue)) { this.add(name, value); } Or the same with the forEach, but at least this gets rid of the single letter 'a' variable. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + drag.addEventListener("mousedown", e => { The problem with adding the mousedown listener here (and the one below for the label) is that you never remove them when the list of filters is destroyed. What about you also delegate these events? In the `_addEventListeners` function, you do something like: ``` // These events are used for drag drop re-ordering and label dragging this.win.addEventListener("mousedown", this._onMouseDown); this.win.addEventListener("mousemove", this._mouseMove); this.win.addEventListener("mouseup", this._mouseUp); ``` So that you only have 3 event listeners in total for both drag/drop stuff, and you can add them only once and remove them in `_removeEventListeners`. Then in the callbacks, you simply have to use `e.target` to know which node has received the event and therefore to adapt what to do as a result. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + // These events are used for label dragging See my comment in `render` about this. I don't think you should be adding 2 listeners for mousemove and mouseup. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + let unitLabel = UNIT_MAPPING[def.type] || UNIT_MAPPING[DEFAULT_FILTER_TYPE]; This is incorrect. For url or drop-shadow for instance, `def.type` is string, then `UNIT_MAPPING[def.type]` is empty string, and therefore falsy, so instead, `UNIT_MAPPING[DEFAULT_FILTER_TYPE]` is used, so "px". Which is wrong for drop-shadow. And that's what causes the bug described in my general comments. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + if (e.target.tagName.toLowerCase() === "button") { Better to early return than enclose the whole function body in an indented condition: ``` let isButton = e.target.classList.contains("remove-button"); if (!isButton) { return; } ``` Also, to find the index, maybe something like this would be better: ``` let index = [...this.list.children].indexOf(li); ``` ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + if (!dragging || e.pageY > boundaries.bottom || e.pageY < boundaries.top) return; nit: this is a global comment for the whole file: we usually (in devtools code) always write if/else/elseif with { }. So, it'd be great if you could make sure you add the opening { then on the next line, indented, the block body, and then on the next line the closing } ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 3) > + let index = Array.prototype.indexOf.call(this.list.children, dragging); `[...this.list.children].indexOf(dragging)`

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull review -r 045605ed97b043290e916b8a9ec6cd640b0da663

https://reviewboard.mozilla.org/r/5965/#review5237 Sorry, I was busy when I wrote the last patch and didn't put enough time to test it completely. I think I had to wait, take my time to test and then submit for review, my bad. * Renamed method names with more descriptive names * I tested unit label widths again, it wasn't fixed in the last patch, fixed now * `tokenizeComputedFilter` was not resetting `content = ""` after setting value of a filter, and it caused *multiple* filters to break, fixed. * I think Tim meant most drop-shadow values are length (4 px values), and it has a special case for color * I made all the mouse events delegated, and separated the functions handling different cases (label-dragging and drag-drop) to avoid a monster method * You were right about the `UNIT_MAPPING` thing. * I was already finding index of filters using `indexOf` in drag-drop, I'm not sure why I was using id slicing in other methods, and your iterator spread technique was nice, a lot better than `Array.prototype.*.call` (since id was not needed anymore, I removed it) * `updateValueAt` checks for existance of index now * Added comment for _buildFilterItemMarkup * I found a bug myself, I used the `index` value in the for loop to update value of filters on "input" event in the last patch, but the value was getting updated in each iteration of loop and changing any filter would result in the last filter being updated, fixed. * As you said you're going to review drag-drop next, I reviewed it myself to see if anything should change, I'm not sure if it's perfect yet but I tried to minimize problems/bugs. * I found another bug which seems to be on Firefox's side: See this: http://note.io/195yc5T It's reproducible by opening tooltip, opening select then moving mouse away from tooltip e.g. on rule-view * There was also a bug that we were not emitting "render" if the value was "none" (returning early after printing localized messages) which was causing the scroll-bar problem to happen again The tokenize bug, string-typed filters error, `UNIT_MAPPING` bug, and unit label widths are all caused by submitting a patch without testing. +Exp here, never repeat a mistake. And you're right, I'll probably take a TDD-ish approach in the next bug, it helps a lot.

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #64) > * I think Tim meant most drop-shadow values are length (4 px values), and it > has a special case for color Yeah, that's what I meant, that comment can be removed. > * I found another bug which seems to be on Firefox's side: See this: > http://note.io/195yc5T > It's reproducible by opening tooltip, opening select then moving mouse > away from tooltip e.g. on rule-view I couldn't reproduce this bug on Windows, but here's an issue I found : if you double click the select box, very fast (and possibly multiple times as well), the select box will stop working, and the rule view starts acting strangely. There are no errors in the browser console though.

https://reviewboard.mozilla.org/r/5965/#review5263 > `unitPreview` is created for number-type filters, so it shouldn't be created for all filters, that's why it's not in `_buildFilterItemMarkup`, what do u think? I see. Would it be a problem to create it for all filters anyway, but make it be 0 width? You could have a css class "unitless" applied to the container of a filter when no unit is needed, and that class would be used to make the unit have a width of 0. At least, all filter markup would be created in the same place. Another solution is, since you clone the template markup when creating a filter, to delete the unit preview element for non number-type filters.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5307 I've tested the new patch, it works great, thanks! I believe we're close to finishing the implementation here. Can I suggest that you work on tests in a separate patch, to ease review? Here are my final comments for this patch: ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + this.emit("updated", this.getCssValue()); The `add` function already emits the `updated` event, so you shouldn't emit it again here. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + label.addEventListener("mousedown", e => { Should be removed. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + (index => { Why not also delegate this one in `_addEventListeners` ? This way, no need for the IIFE and shorter render function. ::: browser/devtools/shared/widgets/filter-widget.css (Diff revision 4) > + z-index: 10; Why 10 specifically here? Shouldn't 1 be enough? ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + li = label.parentNode.parentNode, Same comment here about the `li` name and using `closest` ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + let li = e.target.parentNode.parentNode; nit: the `li` name is confusing because the element isn't an `<li>` element. Also, could you change this to: ``` let filterEl = e.target.closest(".filter"); ``` That's easier to understand and doesn't rely as much on the markup structure. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + li.classList.add("dragging"); I believe we need a better way of knowing when we're dragging, and what we're dragging. Here you have the necessary 2 flags: - the container has the class "dragging" when dragging a filter up or down - `this._dragging` is set when dragging a label left or right. It's a bit confusing to know what's really happening when reading the code. I would advise having self-explanatory named properties for indicating what's currently being dragged. ``` this.isReorderingFilter = true/false this.isDraggingValue = true/false ``` This way, no need to read the className of an element to know if you're dragging. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + dragging.startX = dragging.lastX; I think it's possible not to have to reset the `startValue` and `startX` here and in `_keyUp` (and therefore it's possible not to have to access the input here either). In fact, you don't need `startValue` at all. See https://pastebin.mozilla.org/8827679 ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + let draggingElement = this.list.querySelector(".dragging"), If you create the 2 drag state properties I mentioned in `_mouseDown`, you don't need to do this, since you'll already have the right properties to test in the next `if` statement. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + let dragging = this.list.querySelector(".dragging"); nit: maybe rename the `dragging` variable to `filterEl`, since you're not using this variable to know whether you're dragging, but as a reference to the filter element. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + dragging.classList.remove("dragging"); You've already removed the class 3 lines ago. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + change < 0 ? Math.round(change) : change; Using `round` instead of `ceil` seems a little strange here. Also, if you change it to `ceil`, then you can remove the comment and code at line 347. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + dragging.style.top = delta - currentPosition + "px"; Related to my last comment: if you use `ceil` instead of `round`, then this becomes 0, and instead of having to add the comment and changing the top style, you can simply do: ``` dragging.removeAttribute("style"); ``` ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 4) > + // Label-dragging is disabled on mouseup Same remark as in `_mouseMove` here, if you have the 2 dragging state properties, this function would become easier to read.

https://reviewboard.mozilla.org/r/5965/#review5311 > I see. Would it be a problem to create it for all filters anyway, but make it be 0 width? You could have a css class "unitless" applied to the container of a filter when no unit is needed, and that class would be used to make the unit have a width of 0. > At least, all filter markup would be created in the same place. > Another solution is, since you clone the template markup when creating a filter, to delete the unit preview element for non number-type filters. I took the removing approach.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull review -r 6a3d0931695c8bc1bad4a68c7cbf1ea27ff55a8d

https://reviewboard.mozilla.org/r/5965/#review5313 Thanks Patrick, I'll start writing tests soon. Applied your tips and laughed hard at this: > You've already removed the class 3 lines ago. Something I'd like to ask: You said I should fill a follow-up bug about the title attribute not working, I'm not sure how this should be described. I thought it happens for XUL-wrapped iframes, but that's not the case, what do you suggest for the bug title? I've filled a bug for using alt/shift keys on inputs. Should I mark this as blocking the main bug? [Bug 1149671](https://bugzilla.mozilla.org/show_bug.cgi?id=1149671)

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #70) > You said I should fill a follow-up bug about the title attribute not > working, I'm not sure how this should be described. I thought it happens for > XUL-wrapped iframes, but that's not the case, what do you suggest for the > bug title? Have you tried using the tooltiptext attribute rather than title? I'm not very experienced with XUL, but that's the attribute we use for other things in the toolbox, so it's worth a shot. And if that works, no need to file a bug.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5345 Thanks for these changes. I found one more problem: open the filter tooltip and quit Firefox while that popup is opened: Full message: TypeError: ({}) is not extensible Full stack: SwatchFilterTooltip.prototype<.destroy/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/Tooltip.js:1583:7 Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23 this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7 this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37 Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5 this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7 Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:417:5 ElementStyle.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:174:1 CssRuleView.prototype.clear@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:1738:7 CssRuleView.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:1572:5 RuleViewTool.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/style-inspector.js:157:5 window.onunload@chrome://browser/content/devtools/cssruleview.xhtml:33:11 ToolSidebar.prototype.destroy<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/sidebar.js:526:7 TaskImpl_run@resource://gre/modules/Task.jsm:314:40 TaskImpl@resource://gre/modules/Task.jsm:275:3 createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14 InspectorPanel__destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:558:28 Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1712:26 DT_forgetBrowserWindow@resource:///modules/devtools/gDevTools.jsm:1289:9 gBrowserInit.onUnload@chrome://browser/content/browser.js:12215:5 onunload@chrome://browser/content/browser.xul:1:1 Other than this and the minor comments below, I think we're good to go. One last look and I'll R+ this. ::: browser/devtools/shared/widgets/filter-widget.css (Diff revisions 4 - 5) > - flex-grow: 0.6; > + /*flex-grow: 0.6;*/ If the layout works without this flex-grow property, then remove it entirely, no need to keep it commented out. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revisions 4 - 5) > let label = e.target, Can you add a `this.isDraggingLabel = true` here as well, just as a parity with `this.isReorderingFilter`. And use it in the `else if` in `_mouseMove` instead of `this._dragging`. `this._dragging` is still needed for sure, but should just be used to store the data you need while dragging, not to check what type of dragging is being done.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull -r f29d6c4e6103243e10b2cd30ca40b2ad4842f068 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5361 You were right, I forgot to change SwatchFilterTooltip's destroy method which was setting `widget._onUpdate = null` to use `.off` method.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5363 Nice work! Just a couple of remaining nits, but other than that, it's good to go. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revisions 5 - 6) > + input = dragging.input; The `input` variable doesn't seem like it's needed here. Also, the shorter `dragging` (instead of `this._dragging`) doesn't really help understand the following lines better, and adds one line. So maybe remove `dragging` here and just use `this._dragging` in both places below. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revisions 5 - 6) > + let dragging = this._dragging; Same comment here, making local variables as shortcuts is nice when you need them in a lot of places and they are a lot shorter than the original property. Here's not the case.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull -r f9156bc726ac3b6e3658eca99d26e7aa367c1252 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5365 Oh, I didn't have my JSHint set properly, otherwise I would get noticed about unused variables. I enabled it and found a few other unused variables and removed them. Thanks Patrick.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5375 Ship It!

For the tests, let me know if you need more help than what I quickly said in comment 62.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down this commit: hg pull -r 7f0cc8454cfdb111ee5f00424a1438ccb07370d9 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5401 Patch Rebase. (it seems the aero section of themes/windows/jar.mn was removed, also devtools/shared/moz.build was getting rejected);

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5403 Ship It!

Filled a bug about title attribute not working: https://bugzilla.mozilla.org/show_bug.cgi?id=1150665

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - UI Tests; r=pbrosset Pull down this commit: hg pull -r 8a084ab7b63f0c6d94ef8b009bbbe0098677d817 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - UI Tests; r=pbrosset /r/6587 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset Pull down these commits: hg pull -r 4483629cfcdff08eb8ccf046d3a1b40ce2fc243f https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5455 While writing tests I found a few bugs: * I was not removing input event listener in `_removeEventListeners` * After modifying label-dragging to be relative to last drag, it was no longer necessary to listen on keyboard events to update modifier, we could set the modifier depending on `altKey` and `shiftKey` properties of mouseMove event. * We were not importing ViewHelpers manually which caused the tests to throw an error. Not sure how it worked before, probably ViewHelpers was getting injected somehow. * I was checking for `tagName == "label"` to initialize label-dragging which was wrong, it should've been `target.classList.contains("devtools-draglabel")` which doesn't apply for string-type filters * Label's title attribute was being set for all labels, not only for the draggable ones. * I also found a few coding style mistakes (thanks to jscs and jshint) * We were not checking for value being in the filter's defined range in `updateValueAt`, fixed.

https://reviewboard.mozilla.org/r/5965/#review5457 Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ec21b393e51

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r da4b0060be822f3153437a5c32a7bf7c1ff2814a https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5461 Sorry, forgot to add doc_filter.html. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe221c0e7fc

https://reviewboard.mozilla.org/r/5965/#review5465 ::: browser/devtools/shared/test/browser_filter-editor-tooltip.js (Diff revision 11) > + yield new Promise(resolve => setTimeout(resolve, 500)); Same here. ::: browser/devtools/shared/test/browser_filter-editor-tooltip.js (Diff revision 11) > + yield new Promise(resolve => setTimeout(resolve, 500)); I think you can use yield wait(500), I'm not sure though. But waiting for the load event seems like a smarter way to do this. ::: browser/devtools/shared/test/browser_filter-editor-tooltip.js (Diff revision 11) > +// Tests the Filter Editor Tooltip I *think* this test belongs in the styleinspector directory.

I couldn't find the `wait` function in head.js, although I think I can add it. Are there events for different components of inspector panel? There is a small bug in the first test, I've fixed it locally, I'll wait for your review before updating the patch. You're right Tim, that test belongs to styleinspector, and it seems I have to add tests for ESC and and ENTER, revert/commit. Thanks.

https://reviewboard.mozilla.org/r/5965/#review5475 ::: browser/devtools/shared/test/browser_filter-editor-01.js (Diff revision 11) > + "setCssValue should work for stirng-typed values"); nit : typo on string

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #91) > I couldn't find the `wait` function in head.js, although I think I can add > it. I thought it was already defined, but apparently not. > Are there events for different components of inspector panel? I don't have deep knowledge of the inspector, but I think iframes have a load event, and that tooltips have a shown event. Anyways, you should use what's already in current styleinspector tests (cubic bezier & color swatch) > There is a small bug in the first test, I've fixed it locally, I'll wait for > your review before updating the patch. I don't really have much more comments to give.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r e53197a652f0d2cea64dc160f12d36aac930a424 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5505 I found a typo in the destroy method `filterItemmarkup` instead of `filterItemMarkup`. After looking at styleinspector tests (as Tim mentioned), I wrote tooltip-related tests just like the cubic bezier tooltip. Ran all the tests locally, they pass, pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=add5874fa615 I no longer use timeouts/waits as the functions provided by `head.js` in styleinspector tests are more reliable than timeouts.

https://reviewboard.mozilla.org/r/6587/#review5507 Forgot to `hg rm browser_filter-editor-tooltip.js`.

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #94) > Comment on attachment 8582472 [details] > MozReview Request: bz://1055181/mahdi > > /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset > /r/6587 - Bug 1055181 - UI Tests; r=pbrosset > > Pull down these commits: > > hg pull -r e53197a652f0d2cea64dc160f12d36aac930a424 > https://reviewboard-hg.mozilla.org/gecko/ I'm not able to edit string type filters (url, drop-shadow) with this patch.

(In reply to Tim Nguyen [:ntim] from comment #97) ... > I'm not able to edit string type filters (url, drop-shadow) with this patch. You'll probably want to create a test for that as well.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r b0346daf731303cf26b4528a6987260df4a693b6 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5513 oh, I had forgotten to check for string-typed filters in updateValueAt after adding range check. you're right, I'll have to write separate tests for number/string filters (methods test). I'm currently in a hurry and have to go, will take a look asap. Thanks Tim.

https://reviewboard.mozilla.org/r/6587/#review5565 ::: browser/devtools/shared/test/browser_filter-editor-02.js (Diff revision 5) > + const computedDropShadow = "rgb(0, 0, 0) 5px 5px 0px"; No need to define this const here, just use the "rgb(...." value in the `expected` object below. ::: browser/devtools/shared/test/browser_filter-editor-02.js (Diff revision 5) > + const cssValue = "blur(2px) contrast(200%) hue-rotate(20.2deg) drop-shadow(5px 5px black)"; I suggest adding more test cases in this test to make sure you test all filters and all combinations, as well as edge cases. ``` const TEST_DATA = [{ cssValue: "blur(2px) contrast(200%) hue-rotate(20.2deg) drop-shadow(5px 5px black)", expected: [{ label: "blur", value: "2", unit: "px" }, ...] }, { cssValue: "url(...)", expected: [{ ... }] }, { cssValue: "none", expected: [] }]; ``` You just need to add a `for (let {cssValue, expected} of TEST_DATA) {...}` loop around your test logic. ::: browser/devtools/shared/test/browser_filter-editor-04.js (Diff revision 5) > + You should also test the output of `getCssValue()` to make sure it wasn't changed. ::: browser/devtools/shared/test/browser_filter-editor-05.js (Diff revision 5) > + It would be nice to test that after having released the mouse button (mouseup), then you can drag again the same label, and that the multiplier value is back to default. ::: browser/devtools/shared/test/browser_filter-editor-06.js (Diff revision 5) > + add.click(); Here you do essentially the same sort of test 4 times, this is a good candidate to extract into a test data array and have a loop instead. This is good for several reasons: - less code duplication - short test file - test logic easier to read - easier to add more test cases in the future ``` const TEST_DATA = [{ filterName: "blur", valueAfterFilterAdded: "0px" }, { filterName: "contrast", valueAfterFilterAdded: "0%" }, { ... }]; for (let i = 0; i < TEST_DATA.length; i ++) { let {filterName, valueAfterFilterAdded} = TEST_DATA[i]; info(`Adding filter ${filterName}`); select.value = filerName; add.click(); is(widget.getValueAt(i), valueAfterFilterAdded, "Should ..."); } ``` ::: browser/devtools/styleinspector/test/browser_ruleview_filtereditor-appears-on-swatch-click.js (Diff revision 5) > + filterTooltip.widget.then(() => { I guess you're doing this to wait until the widget is initialized, right? The thing is that you're not yielding the promise returned, so is the widget was indeed async, this wouldn't work. I guess it does because the widget is sync. Please replace with: ``` yield filterTooltip.widget; filterTooltip.hide(); ``` ::: browser/devtools/styleinspector/test/browser_ruleview_filtereditor-commit-on-ENTER.js (Diff revision 5) > + const computed = content.getComputedStyle(content.document.body); This uses what we call CPOW (cross-process object wrapper) when the test is run with e10s enabled (multi-process firefox), and it is recommended not to use them. From now on, we need to always keep in mind that the test runs in one process, while the test page runs in another process, and so accessing things in the page from the test __must__ go through the Message Manager: https://developer.mozilla.org/en-US/docs/The_message_manager The nice thing is that everything's already set up in this test directory, you can simply use the getComputedStyleProperty(selector, pseudo, propName) helper function: ``` yield waitForSuccess(() => { let computed = yield getComputedStyleProperty("body", null, "filter"); return computed.filter === "blur(2px)"; }, "Waiting ..."); ``` Hmm, I just realized after writing these lines that waitForSuccess only works with sync callback functions though, so this won't work. But you could change it to also accept functions that return promises that resolve to booleans: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/head.js#500 Or, perhaps better, create a new message listener in the test page process script that waits until a given computed property has the expected value: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/doc_frame_script.js#84 Somewhere here, add another addMessageListener block named "Test:WaitForComputedStylePropertyValue" (instead of Get), which would do exactly like the "Test:GetComputedStylePropertyValue" listener except it would loop until the value is the expected one. ::: browser/devtools/styleinspector/test/browser_ruleview_filtereditor-revert-on-ESC.js (Diff revision 5) > + const computed = content.getComputedStyle(content.document.body); Same comment as in the commit-on-ENTER test.

Other than the above comments, great work. Those tests look good. We're very close to landing this.

https://reviewboard.mozilla.org/r/6587/#review5575 Applied your comments about tests. While writing tests I found another bug: We were returning early if the value passed to `setCssValue` was `none`, and forgot to call `this.render()` before returning, fixed. Also added more tests to browser_filter-editor-03.js (methods are now tested against string-type filters, too).

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

https://reviewboard.mozilla.org/r/6587/#review5577 ::: browser/devtools/styleinspector/test/doc_frame_script.js (Diff revision 6) > + * - {String} name: name of the property Please update the jsdoc here to mention the new `expected` property too. ::: browser/devtools/styleinspector/test/doc_frame_script.js (Diff revision 6) > + * @return {String} The value, if found, null otherwise Also remove this @return doc since the listener doesn't return any value. ::: browser/devtools/styleinspector/test/doc_frame_script.js (Diff revision 6) > + let start = Date.now(); `start` is not used, this should be removed. ::: browser/devtools/styleinspector/test/doc_frame_script.js (Diff revision 6) > + * it is true, the promise resolves. If validatorFn never returns true, then There's no timeout after several tries in this function, so please remove this part of the commnent. ::: browser/devtools/styleinspector/test/head.js (Diff revision 6) > + * @param {String} name: name of the property. s/name/propName ::: browser/devtools/styleinspector/test/head.js (Diff revision 6) > + * @param {String} name: the name used in test message I don't think this last parameter is needed. The test that uses `waitForComputedStyleProperty` should do the `ok(...)` part, not this helper function. And if you remove it, then you can simplify the code to: ``` function* waitForComputedStyleProperty(selector, pseudo, propName, expected) { yield executeInContent("Text:WaitForComputedStylePropertyValue", { selector, pseudo, name, expected }); } ``` (no need for name:value when name and value are the same). ::: browser/devtools/styleinspector/test/doc_frame_script.js (Diff revision 6) > +addMessageListener("Test:WaitComputedStylePropertyValue", function(msg) { s/WaitComputedStylePropertyValue/WaitForComputedStylePropertyValue

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r 8b9fb57b0049ce8f13fe490cba42a84f532c89b3 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5583 Thanks for the comments. Also found two unused variables in tests, removed. I think I'll have to review the whole thing later and see if I can refactor anything.

https://reviewboard.mozilla.org/r/5965/#review5597 ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 13) > + "range": [0, 100], // You can go over that, but it's the same as 100% The comment can be removed. I know I added it, but this pretty much applies to all percentage values. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revision 13) > + depth ++; nit : remove space before ++

https://reviewboard.mozilla.org/r/6587/#review5619 Those last changes look good to me. You said in the bug you wanted to take a final look and do some more refactoring, so just clearing the R? flag for now.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r 7b70d03f367641a4f5ce26b53cd2dd1ba0660459 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/5965/#review5625 I couldn't find anything to actually "refactor", just some small changes to make the code cleaner. Ran the tests again. Try (although nothing changed dramatically, there might be mistakes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2300a89025b

(In reply to Mahdi Dibaiee [:mahdi][:mdibaiee] from comment #111) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2300a89025b Looks like a couple of tests are failing consistently: 5654 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_filter-editor-01.js | setCssValue should work for spaced values - Got blur(2px) contrast(NaN), expected blur(2px) contrast(200%) 5678 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_filter-editor-03.js | Shouldn't allow values higher than max - Got NaN, expected 100% 5679 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_filter-editor-03.js | Shouldn't allow values less than INVERT_MIN - Got NaN, expected 0% 5681 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_filter-editor-03.js | Shouldn't allow values higher than max - Got NaN, expected 100% 5682 INFO TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_filter-editor-03.js | Shouldn't allow values less than INVERT_MIN - Got NaN, expected 0%

https://reviewboard.mozilla.org/r/5967/#review5647 I had lost count of which revision I had reviewed last for this patch, but I think I got it right in the end :) I see a lot of style changes, which are fine. Also a lot of `let` to `const` changes, we don't have a lot of them in the devtools code. We usually only use `const` for module-level constants, not for function variables which happen not to change. But that's ok, keep it this way. No need to ask for review on this patch again. ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revisions 7 - 13) > this.add(key, ""); "" is the default value for the second parameter of the `add` method, so you can just do `this.add(key);` ::: browser/devtools/shared/widgets/FilterWidget.js (Diff revisions 7 - 13) > - unit = unit ? unit[0] : ""; > + : (/[a-zA-Z%]+/.exec(value) || [])[0]; A better formatting would be: ``` let unit = def.type === "string" ? "" : (/[a-zA-Z%]+/.exec(value) || [])[0]; ``` Easier to read.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r 82f536aaca04d86bafde4f12d685c4f5af4a9433 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5653 Thanks Patrick. New Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28bc76279d2a I'll mark checkin-needed after the tests report success.

https://reviewboard.mozilla.org/r/6587/#review5655 Just a couple of final nits, and this is good to go (after try is green). ::: browser/devtools/styleinspector/test/head.js (Diff revisions 6 - 9) > * @param {String} name: name of the property. Final nit: This argument is called `name` in the doc, but `propName` in the function. I suggest keeping the `name` name because that would make the code in the function simpler. ::: browser/devtools/styleinspector/test/head.js (Diff revisions 6 - 9) > - expected: expected}) > + name: propName}) Same final nit here.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi /r/5967 - Bug 1055181 - CSS Filter Tooltip; r=pbrosset /r/6587 - Bug 1055181 - UI Tests; r=pbrosset Pull down these commits: hg pull -r d455b727e82bc8eb5817d470a20e5464b7358068 https://reviewboard-hg.mozilla.org/gecko/

Thanks Patrick, you're right, applied it. The try failed to build OS X 10.8 opt, I retriggered the job to see if it builds.

It didn't build. Looking at repo, it seems unrelated, all OS X Opt 10.8 are failing.

Comment on attachment 8582472 [details] MozReview Request: bz://1055181/mahdi https://reviewboard.mozilla.org/r/5965/#review5713 Ship It!

Rebased and landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/9ebe14f890e7 https://hg.mozilla.org/integration/fx-team/rev/d842435594f3

Thanks Mahdi for your work! Really great stuff.

Thanks Patrick and Tim for your great help! This was an awesome experience, I feel leveled up^. I'm a little busy this week, but I'm going to start working on the follow-up bugs as soon as I get free time.

Release Note Request (optional, but appreciated) [Why is this notable]: Cool new feature in the inspector rules view [Suggested wording]: New rules view tooltip to tweak CSS Filter values [Links (documentation, blog post, etc)]: http://youtu.be/t3NKmmWfklU

https://hg.mozilla.org/mozilla-central/rev/9ebe14f890e7 https://hg.mozilla.org/mozilla-central/rev/d842435594f3

It is so awesome that you made a video for the release notes. \o/