1267414 - Migrate Color picker editor widget to HTML Tooltip

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

The goal of this bug is to migrate the current Color picker / Spectrum Tooltip editor (defined in client/shared/widgets/Tooltip.js) to no longer rely on XUL panels.

An HTML tooltip API is developed in Bug 1259834. Keeping in mind the rule-view editor tooltips are planned to be turned into inline editors.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Julian, This patch just convert the color picker to HTML Tooltip,
but its height is too big. In typical toolbox size, the tooltip ends up cut.
Do you have ways to workaround that or this bug isn't about HTML Tooltip,
but really about converting all these things to be inline widget??

Comment 2

[Julian Descottes [:jdescottes]]

Sorry about that Alex. I would like to discuss this with :pbro if we go forward with the migration or wait for the inline widgets. I realized a bit late that with the height restrictions on the HTML Tooltip, simply migrating the editors does not make sense for now unless we tackle Bug 1267403, and use a XUL panel to host the HTML Tooltip. I am going to block this bug (and other editor migration bugs) on 1267403.

(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Do you have ways to workaround that or this bug isn't about HTML Tooltip,
> but really about converting all these things to be inline widget??

The inline widget for the color picker is being worked on in Bug 1277352. I will update the title of our bugs to make it more self-explanatory.

Comment 3

[Brian Grinstead [:bgrins]]

Does it make sense to work on this while there is planned / ongoing work for inline editors?  Seems like there's a risk this work will be thrown out when they get converted.  Not sure how much work / complexity this and Bug 1267403 are going to be but I hope that we can talk with Patrick about this.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Does it make sense to work on this while there is planned / ongoing work for
> inline editors?  

Depends when the inline editors are planned to land (according to :gl, color picker should land in Q2, the rest is not planned), and how much we care if some parts of the inspector are still using XUL.

Regarding Bug 1267403 it would be nice to implement it regardless of the inline-editors discussion.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Does it make sense to work on this while there is planned / ongoing work for
> > inline editors?  
> 
> Depends when the inline editors are planned to land (according to :gl, color
> picker should land in Q2, the rest is not planned), and how much we care if
> some parts of the inspector are still using XUL.

So, I think the colorpicker in particular is really important to always be supported.  And it looks like this patch is pretty far along so I don't want to stop progress on it if it's close to landing.  But what's not clear to me is if we will have a deployment target for the inspector that doesn't have XUL available before the inline editors are there - are we talking about Q2 of next year?

> Regarding Bug 1267403 it would be nice to implement it regardless of the
> inline-editors discussion.

Agreed, my main concern is that it might need to introduce some async behavior that's currently sync (for hiding the popup), or general complexity.  But I haven't seen the patch so we can discuss further in bug 1267403.

Comment 6

[Alexandre Poirot [:ochameau]]

This is the new trend in devtools: "wait there is another rewrite somewhere else".
This bug shouldn't have been set as P1 of a track if some other bugs, like bug 1277352 (no patch, no priority, nor part of any tracks) has a bigger precedence.

So I'll polish these patches to convert all the rule view pickers to HTML in order to close Track#1 sooner than later. Hopefully julian can figures things out to run within xul panels.

Comment 7

[Brian Grinstead [:bgrins]]

We made a decision in our planning meeting that we'd consult with Patrick about whether these bugs are necessary and remove them from our P1 list if they weren't.  Either way, looks like you already have good progress on this bug

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

New patch that now passes the tests.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Support closeOnKeys in HTMLTooltip - v1

I had to add support for closeOnKeys to keep the same behavior.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: fix simulateColorPickerChange

Also, this test helper looks racy.
It isn't required by this changes (i.e tests pass without that locally),
but it looks like a safe thing to do!

Comment 11

[Alexandre Poirot [:ochameau]]

The tricky things with all rules view tooltips here is that they are all based on SwatchBasedEditorTooltip, so, I'll most likely have to patch them all at once.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Fixing the tests.

Comment 13

[Alexandre Poirot [:ochameau]]

Here is a first attempt on try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=814765c6f606
I had to convert all the three tooltip at once.
I'm wondering how to land these patches?
It doesn't look correct to land them in distinct bugs as they have to land at the same time...

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Here is a first attempt on try:
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=814765c6f606
> I had to convert all the three tooltip at once.
> I'm wondering how to land these patches?
> It doesn't look correct to land them in distinct bugs as they have to land
> at the same time...

That's a good problem to have :) Thanks for taking care of all the widgets!
We can land everything in this bug, modify the summary and close the others as duplicate. Let's discuss during standup.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Support closeOnKeys in HTMLTooltip

So. I'll attach all tooltip patch related to the rule view in this bug.
Let's start with this one. Color picker is expected to close on Escape/Return key press.
So port the existing closeOnKeys attribute from Tooltip to HTMLTooltip.

This is being covered by color picker tests which depend on this,
but tell me if you want some unit test specific to HTMLTooltip for this.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Implement HTMLTooltip.isShown

Some test depends on this. Also closeOnKeys patch depends on this.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: color picker patch v4

A few comments about this patch:
- (new SwatchColorPickerTooltip()).spectrum is no longer a promise, but a Spectrum instance.
That's because we can create the Spectrum instance synchronously from SwatchColorPickerTooltip constructor.
So we do not have to yield on colorPicker.spectrum anymore.
- I removed spectrum-frame.xhtml as it is no longer usefull.
I had to tweak tests to accommodate this loss. We no longer need a document, we just need a container Element,
which has the spectrum css injected.

You will see that next patches for cubic bezier and filter follow the same pattern.
I'm wondering if I should move the css file injection to HTMLTooltip or SwatchBasedEditorTooltip?

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: fix simulateColorPickerChange

This change isn't required, but this helper looks racy!

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: cubic bezier patch v1

For cubic, tooltip.widget still needs to be a promise.
Otherwise this patch is very similar to color picker one.

Feel free to say: "apply same fixes than color picker to this one".

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: filter tooltip v1

Same.

filter.widget is no longer a promise.

Comment 23

[Alexandre Poirot [:ochameau]]

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

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8765904 [details] [diff] [review]
Implement HTMLTooltip.isShown

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

HTMLTooltip::isVisible() is already available with the same implementation. 
It doesn't seem like "isShown" is massively used in the current codebase, so we could probably migrate the relevant calls to use isVisible()?

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8765908 [details] [diff] [review]
color picker patch v4

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

Implementation looks good so far! I just would like to discuss the dynamic include of the spectrum stylesheet.

::: devtools/client/shared/widgets/Tooltip.js
@@ +647,5 @@
>   * tooltips
>   *
>   * @param {XULDocument} doc
>   */
> +function SwatchBasedEditorTooltip(doc, toolbox) {

JSDoc to be updated for the toolbox parameter.
doc is no longer used and can be removed.

@@ +689,5 @@
>        // When the tooltip is closed by clicking outside the panel we want to
>        // commit any changes. Because the "hidden" event destroys the tooltip we
>        // need to do this before the tooltip is destroyed (in the "hiding"
>        // event).
> +      this.tooltip.once("hidden", () => {

The comment above needs to be updated as it explains why we should listen to "hiding" instead of "hidden" and we now do the opposite :)

I think this comment was already outdated, from looking at the code + quick tests on Nightly it doesn't seem like hiding the tooltip "destroys" anything. So I guess listening to "hidden" here is fine. But if need be we could add the "hiding" event to the HTMLTooltip.

@@ +697,5 @@
>          this._reverted = false;
>        });
>  
>        // Once the tooltip is hidden we need to clean up any remaining objects.
>        this.tooltip.once("hidden", () => {

Should be merged with the "hidden" handler above.

@@ +838,5 @@
>  SwatchColorPickerTooltip.prototype =
>  Heritage.extend(SwatchBasedEditorTooltip.prototype, {
>    /**
> +   * Fill the tooltip with a new instance of the spectrum color picker widget
> +   * initialized with the given color, and return a promise that resolves to

nit: jsdoc to be updated, no longer returns a promise.

@@ +857,5 @@
> +      link.rel = "stylesheet";
> +      link.href = spectrumCss;
> +      (doc.head || doc.documentElement).appendChild(link);
> +    }
> +

This implementation adds a new stylesheet for each instance of ColorPickerTooltip. We only have one instance at the moment, but still I think we should make sure we add the stylesheet only once.

Now regarding the approach, I would like to discuss another option:

What about including spectrum.css in tooltips.css, in the same way that tooltips.css is included in common.css ? Of course here we are lazily loading the CSS which has some advantages, but given that the set*Content methods are called as soon as the inspector starts, the stylesheets will end up in the document anyway.

If we include the stylesheets in tooltips.css, we also have a predictable stylesheet insertion order, which can be useful to avoid selector priority issues based on the selector order. 

What do you think?

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8765903 [details] [diff] [review]
Support closeOnKeys in HTMLTooltip

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

Thanks for the patch, two overall comments on the implementation:
 - as you mentioned, would be nice to add a HTMLTooltip test dedicated to this feature.
 - could we use the key-shortcut module here? (would need to support useCapture in key-shortcut, but looks like this should work!)

I initially did not want to port the current closeOnKeys API, because callers can take care of the keyboard navigation. But having an easy way to define a close key can be a nice feature, so thanks for working on this!

Can I suggest a modification of the API: what about removing the "keypress" event fired by the tooltip? Today, it is only used by the SwatchBasedEditorTooltip and I find it confusing ; to understand what happens when pressing Escape/Return you have to switch between the Tooltip and the SwatchBasedEditorTooltip. My opinion is that if the caller needs to listen to the "keypress" event from the tooltip, it might as well fully define its keyboard navigation, relying on isVisible()/hide() methods from the tooltip if it needs to. Let me know what you think.

::: devtools/client/shared/widgets/HTMLTooltip.js
@@ +185,5 @@
>  function HTMLTooltip(toolbox,
> +  {type = "normal",
> +   autofocus = false,
> +   consumeOutsideClicks = true,
> +   closeOnKeys = [ESCAPE_KEYCODE]} = {}) {

What about using an empty array as default value?
I would prefer pushing callers to explicitly say which keys they want to use rather than see "closeOnKeys: []" to override the behaviour.

@@ +309,5 @@
>  
>      this.doc.defaultView.clearTimeout(this.attachEventsTimer);
>      this.attachEventsTimer = this.doc.defaultView.setTimeout(() => {
>        this._maybeFocusTooltip();
>        this.topWindow.addEventListener("click", this._onClick, true);

Fine to leave this here, so just for information: 
I should have added a comment here, but the reason we add the click handler on show() and in a setTimeout() is because if the tooltip was opened by a mousedown event, it will be hidden immediately after because of the click handler.

A better approach would probably be to add the event handlers only once and use a flag to know if events should be processed or not.

Mentioning this because it could be relevant if we want to use the key-shortcuts module.

@@ +393,5 @@
>  
> +  _onKeyDown: function (e) {
> +    this.emit("keypress", e.keyCode);
> +
> +    if (this.closeOnKeys.includes(e.keyCode) && this.isShown()) {

Given the current implementation, this.isShown() should always be true here since we add the listener on show() and remove it on hide().

Comment 27

[Julian Descottes [:jdescottes]]

Comment on attachment 8765909 [details] [diff] [review]
fix simulateColorPickerChange

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

Looked racy indeed, r+ on green try, thanks!

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8765910 [details] [diff] [review]
cubic bezier patch v1

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

Change looks good, thanks! 
Just keeping the review flag for discussion about the CSS inclusion.

::: devtools/client/shared/test/browser_cubic-bezier-02.js
@@ +68,5 @@
>  
>    bezier = yield onUpdated;
>    is(bezier.P2[0], 1, "The new P2 time coordinate is correct");
>    is(bezier.P2[1], 0, "The new P2 progress coordinate is correct");
> +

nit: blank line should be removed.

::: devtools/client/shared/widgets/Tooltip.js
@@ +956,5 @@
>   * CubicBezierWidget.
>   *
>   * @param {XULDocument} doc
>   */
> +function SwatchCubicBezierTooltip(doc, toolbox) {

Same as for colorpicker: doc argument no longer used, JSDoc shoud be updated.

@@ +990,5 @@
> +      link.rel = "stylesheet";
> +      link.href = css;
> +      (doc.head || doc.documentElement).appendChild(link);
> +    }
> +

Same comment about the CSS inclusion as for the colorpicker.

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8765910 [details] [diff] [review]
cubic bezier patch v1

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

There is another issue I didn't spot before importing the Filter patch: the "preset" CSS class is used by both the bezier and filter editors. Now that the stylesheet are no longer isolated their respective iframes, we should either change the classnames to bezier-preset / filter-preset or specialize the selectors.

::: devtools/client/shared/widgets/Tooltip.js
@@ +994,5 @@
> +
> +    let container = doc.createElementNS(XHTML_NS, "div");
> +    container.id = "cubic-bezier-container";
> +
> +    this.tooltip.setContent(container, { width: 500, height: 360 });

height should be 370 (cf cubic-bezier-frame.xhtml)

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8765911 [details] [diff] [review]
filter tooltip v1

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

Looks good! See my comment about the CSS below.

::: devtools/client/shared/widgets/Tooltip.js
@@ +1063,5 @@
>   *
>   * @param {XULDocument} doc
>   */
> +function SwatchFilterTooltip(doc, toolbox) {
> +  SwatchBasedEditorTooltip.call(this, doc, toolbox);

Same as for the other editors.

@@ +1094,5 @@
> +      let link = doc.createElement("link");
> +      link.rel = "stylesheet";
> +      link.href = css;
> +      (doc.head || doc.documentElement).appendChild(link);
> +    }

Same as for the other editors, let's discuss the stylesheet include.

::: devtools/client/shared/widgets/filter-widget.css
@@ -6,5 @@
> -  height: 100%;
> -  margin: 0;
> -  overflow: hidden;
> -  font: message-box;
> -  color: var(--theme-body-color);

not directly related to your patch, but I realized that none of the tooltip container has the theme-body or theme-comment classname, therefore the color is currently not updated depending on the theme.

We can either keep the color rule for the filter container or add the theme-comment classname to the tooltip.container element. (the reason I would prefer to use "theme-comment" rather than "theme-body", is that theme-body forces a background color that overrides the one defined on the tooltip-container)

@@ +198,5 @@
>    border: none;
>    cursor: pointer;
>  }
>  
>  .hidden {

This comment depends on how we decide to include the stylesheet, but since we are no longer isolated in an iframe, we should review the following classnames: add, filter, footer, hidden, preset. Also the "input {width: 8em;}" should probably be specialized.

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: Apply scoped stylesheet to HTMLTooltip container.

New patch in this queue. Quite simple...

Comment 32

[Alexandre Poirot [:ochameau]]

Attachment: color picker patch v5

Addressed most of the comments if not all.
Waiting for try to be working again before r?.

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: cubic bezier patch v2

These patches now use the scoped style.

Comment 34

[Alexandre Poirot [:ochameau]]

Attachment: filter patch v2

Things left to do:
- address comments on the closeOnKeys patch
- ensure tests are green

Comment 35

[Julian Descottes [:jdescottes]]

Comment on attachment 8766839 [details] [diff] [review]
Apply scoped stylesheet to HTMLTooltip container.

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

Looks good to me, thanks!

::: devtools/client/shared/widgets/HTMLTooltip.js
@@ +594,5 @@
>    },
> +
> +  /**
> +   * Apply a scoped stylesheet to the container so that this css file only
> +   * apply to it.

nit: s/apply/applies

@@ +599,5 @@
> +   */
> +  _applyStylesheet: function (url) {
> +    let style = this.doc.createElementNS(XHTML_NS, "style");
> +    style.setAttribute("scoped", "true");
> +    url = url.replace(/"/g, "\\\"");

Why do we need to escape quotes here? Is this for data-uri stylesheets ? 
Maybe add a comment?

Comment 36

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #35)
> @@ +599,5 @@
> > +   */
> > +  _applyStylesheet: function (url) {
> > +    let style = this.doc.createElementNS(XHTML_NS, "style");
> > +    style.setAttribute("scoped", "true");
> > +    url = url.replace(/"/g, "\\\"");
> 
> Why do we need to escape quotes here? Is this for data-uri stylesheets ? 
> Maybe add a comment?

No, this is just to prevent escaping css syntax and allow arbitrary css execution.
Do you think I should put a comment?

Comment 37

[Alexandre Poirot [:ochameau]]

Comment on attachment 8765903 [details] [diff] [review]
Support closeOnKeys in HTMLTooltip

I'll attach a patch for the color picker where SwatchBasedEditor listen for key shortcut by itself.

Comment 38

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98be849d532b

Comment 39

[Alexandre Poirot [:ochameau]]

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

Comment 40

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ef96fdeb06

Comment 41

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13638ef194b0

Comment 42

[Alexandre Poirot [:ochameau]]

Attachment: Apply scoped stylesheet to HTMLTooltip container

Fixed typo.

Comment 43

[Alexandre Poirot [:ochameau]]

Attachment: cubic bezier patch v3

New patches where I tried to address all previous comments.
It now uses the scoped stylesheets.

I do have some weird error on try, but I'm wondering if that's because of XUL wrapper,
I'll push to try without it to see...

Comment 44

[Alexandre Poirot [:ochameau]]

Attachment: Filter patch v3

Should I do something about --theme-* vars in this patch?
I haven't changed anything regarding your comment about that.

Comment 45

[Alexandre Poirot [:ochameau]]

Attachment: color picker patch v6

So, I droped the closeOnKeys patch and inlined key listener logic.

Comment 46

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #43)
> Created attachment 8767967 [details] [diff] [review]
> cubic bezier patch v3
> 
> New patches where I tried to address all previous comments.
> It now uses the scoped stylesheets.
> 
> I do have some weird error on try, but I'm wondering if that's because of
> XUL wrapper,
> I'll push to try without it to see...

Will review later today, but just mentioning it here in case you miss my message on IRC: the failures could be linked to event listeners leaks. Try to destroy this.shortcuts in the destroy() method of SwatchBasedEditorTooltip. I could see some improvements locally at least.

Comment 47

[Alexandre Poirot [:ochameau]]

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

Comment 48

[Julian Descottes [:jdescottes]]

Comment on attachment 8767967 [details] [diff] [review]
cubic bezier patch v3

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

Looks good to me, but as discussed on IRC we need to figure out why the .dot is no longer animated here.

::: devtools/client/shared/test/browser_cubic-bezier-02.js
@@ +12,5 @@
>  
> +const TEST_URI = `data:text/html,
> +  <html><body>
> +    <div id="cubic-bezier-container"/>
> +  </body></html>`;

Any reason not to use the simple TEST_URI you used in other tests (ie const TEST_URI = `data:text/html,<div id="cubic-bezier-container" />`; )

::: devtools/client/shared/test/browser_cubic-bezier-06.js
@@ +13,5 @@
>  
> +const TEST_URI = `data:text/html,
> +  <html class="theme-light">
> +    <div id="cubic-bezier-container" />
> +  </html>`;

Any reason not to use the simple TEST_URI you used in other tests (ie const TEST_URI = `data:text/html,<div id="cubic-bezier-container" />`; )

::: devtools/client/shared/widgets/CubicBezierWidget.js
@@ +264,2 @@
>      p1.className = "control-point";
>      p1.id = "P1";

Now that the widget is no longer isolated in an iframe, I think we should swap for classnames here. 

I would prefer to deal with this in a follow up though, in order not to block this patch. Let me know if you agree.

::: devtools/client/shared/widgets/Tooltip.js
@@ +627,5 @@
>    // It will also close on <escape> and <enter>
>    this.tooltip = new HTMLTooltip(toolbox, {
>      type: "arrow",
>      consumeOutsideClick: true,
>      closeOnKeys: [ESCAPE_KEYCODE, RETURN_KEYCODE],

Same comments as for the color picker patch: consumeOutsideClicks (plural), remove closeOnKeys.

@@ +980,5 @@
> +  setCubicBezierContent: function (bezier) {
> +    let { doc } = this.tooltip;
> +
> +    let container = doc.createElementNS(XHTML_NS, "div");
> +    container.id = "cubic-bezier-container";

Same comment about ids vs classes

Comment 49

[Julian Descottes [:jdescottes]]

Comment on attachment 8767969 [details] [diff] [review]
color picker patch v6

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

Looks good to me, thanks!

::: devtools/client/shared/widgets/Tooltip.js
@@ +661,1 @@
>      consumeOutsideClick: true,

the option for the HTMLTooltip is named consumeOutsideClicks (plural). 

FYI, I did it because the corresponding XUL panel attribute is consumeOutsideClicks.

@@ +661,2 @@
>      consumeOutsideClick: true,
>      closeOnKeys: [ESCAPE_KEYCODE, RETURN_KEYCODE],

remove unused closeOnKeys

@@ +664,5 @@
>    });
>  
>    // By default, swatch-based editor tooltips revert value change on <esc> and
>    // commit value change on <enter>
> +  this.shortcuts = new KeyShortcuts({

Call this.shortcuts.destroy() in the destroy method of SwatchBasedEditorTooltip

Comment 50

[Julian Descottes [:jdescottes]]

Comment on attachment 8767968 [details] [diff] [review]
Filter patch v3

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

Great work, thanks!

A last comment that applies to all three patches. The XUL tooltip container had a default 5px padding which has not been preserved in the HTML version.
Could you add a 5px padding on the containers and increase the dimensions of 10px in each call to setContent ?

::: devtools/client/shared/widgets/filter-widget.css
@@ +8,5 @@
>    height: 100%;
>    display: flex;
>    position: relative;
> +  /* when opened in a xul:panel, a gray color is applied to text */
> +  color: var(--theme-body-color);

I think it would be better to add the theme-comment class to the ".tooltip-panel" element created in HTMLTooltip::_createContainer.

Comment 51

[Alexandre Poirot [:ochameau]]

Attachment: color picker patch v7

Removed closeOnKeys, renamed consumeOutsideClick, call shortcuts.destroy().

I'm carrying r+ over, but do not hesitate to throw new comments.

Comment 52

[Alexandre Poirot [:ochameau]]

Attachment: cubic bezier patch v4

Added the 5px padding. But note that the container overlaps the arrow on the bottom,
so the padding at the bottom doesn't work as expected.
I imagine that's a good followup...

Otherwise I migrate a bunch of IDs to class.
There were no use of P1/P2 IDs...
But there is still some other elements with IDs in presets/categories
which would be good to convert as followup.

browser_cubic-bezier-06.js doesn't require a complex TEST_URI (simplified it in the new patch),
but browser_cubic-bezier-02.js does (kept the same TEST_URI).
That's because there is some assertion on Element positions,
and the <body> padding mess up with these assertions.
I droped a comment in this test.

For the animation breakage, I now use animate() in JS and remove the keyframes from the CSS.
It also happen to fix the animation restart...

Comment 53

[Alexandre Poirot [:ochameau]]

Attachment: filter patch v4

Apply the 5px padding (color picker patch was already having it).
Use theme-comment class for tooltip container.
But are you sure of that? It is gray text whereas it was black (or close to black)
before.
Shouldn't we use theme-body instead?!

Comment 54

[Julian Descottes [:jdescottes]]

Comment on attachment 8768390 [details] [diff] [review]
cubic bezier patch v4

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

Looks good to me, thanks for fixing the animation auto restart :)

Comment 55

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #53)
> Created attachment 8768391 [details] [diff] [review]
> filter patch v4
> 
> Apply the 5px padding (color picker patch was already having it).
> Use theme-comment class for tooltip container.
> But are you sure of that? It is gray text whereas it was black (or close to
> black)
> before.
> Shouldn't we use theme-body instead?!

Oh you're right, I thought theme-comment applied the same color as theme-body, just without the background color.
Looks like I was wrong, please revert this back to use what you had before. Sorry about this!

Comment 56

[Julian Descottes [:jdescottes]]

adding a ni? to make sure you see my last comment.

Comment 57

[Julian Descottes [:jdescottes]]

Comment on attachment 8768391 [details] [diff] [review]
filter patch v4

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

Looks good!
For the padding overflow I will have a look before landing Bug 1284461. I think it might fix the issue.

Comment 58

[Alexandre Poirot [:ochameau]]

Attachment: filter patch v5

Revert back to settting theme-body-color on filter container.

Comment 59

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58db1b92f2b7

Comment 60

[Alexandre Poirot [:ochameau]]

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

Comment 61

[Alexandre Poirot [:ochameau]]

Just one intermittent left on taskcluster around the Filter widget.
I think I understand it, but I need a couple of try push to confirm it's gone.

Comment 62

[Alexandre Poirot [:ochameau]]

Attachment: cubic bezier patch v5

Fixed browser_cubic-bezier-01.js (was asserting against P1/P2 IDs).

Comment 63

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45242c5e86d4

Comment 64

[Alexandre Poirot [:ochameau]]

Attachment: Fix filter widget race

Here is a fix to prevent races during Filter widget destruction.
getPresets is async, and the widget might be destroying while it is still processing...

Comment 65

[Alexandre Poirot [:ochameau]]

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

Comment 66

[Julian Descottes [:jdescottes]]

Comment on attachment 8768810 [details] [diff] [review]
Fix filter widget race

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

Looks reasonable. 
And since we no longer have to yield on the widget element, the new race conditions in the tests make sense.

Comment 67

[Alexandre Poirot [:ochameau]]

Attachment: Merged patch

One last try on other platform.

Comment 68

[Alexandre Poirot [:ochameau]]

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

Comment 69

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92bd335dd910

Comment 70

[Alexandre Poirot [:ochameau]]

Attachment: Merged patch v2

One test wasn't run on linux and required a tweak...

Comment 71

[Alexandre Poirot [:ochameau]]

Was devtools/client/inspector/shared/test/browser_styleinspector_tooltip-size.js
And it looks like it can be enabled on all platform and e10s. Let's see what try says.

Comment 72

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/f9737fc94471de4c5fce8e47757f69febbf6fcd6
Bug 1267414 - Apply scoped stylesheet to HTMLTooltip container. r=jdescottes

https://hg.mozilla.org/integration/fx-team/rev/76d556ef9180969ee8d690ed069732faded934a2
Bug 1267414 - Convert color picker, cubic bezier and filter widgets to HTML Tooltip. r=jdescottes

Comment 73

[Alexandre Poirot [:ochameau]]

Attachment: Landed patch

With the eslint fix.

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9737fc94471
https://hg.mozilla.org/mozilla-central/rev/76d556ef9180

Comment 75

[Ada [:adalucinet]]

Verified using latest Nightly 50.0a1, across platforms [1]. 
Found 1 issue - bug 1286553. Since this new report is tracked separately, marking here accordingly.

[1] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit