Implement Inline Tooltip widget

RESOLVED FIXED in Firefox 55

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: gl, Assigned: sroddick)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 14 obsolete attachments)

24.62 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
6.64 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
42.17 KB, patch
Details | Diff | Splinter Review
24.11 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
This purpose of this bug is to add a new tooltip widget that would allow us to embed our swatch based editor tooltips onto the rule view by specifying an option. It is assumed that there will be additional bugs on top of this to make the existing tooltip widgets to work fully as expected in the inline mode. This bug will only provide the framework to add inline tooltips.
Reporter

Updated

3 years ago
Priority: -- → P3
Reporter

Updated

3 years ago
Blocks: 1258390
Comment on attachment 8797691 [details] [diff] [review]
Part 1: Move Tooltip.js and HTMLTooltip.js into widgets/tooltip/ [1.0]

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

Works for me if try is green! Thanks :)

Just 1 minor comment:
nit: there are still comments referencing devtools/client/shared/widgets/Tooltip.js in 3 CSS files, let's clean that up
Attachment #8797691 - Flags: review?(jdescottes) → review+
Reporter

Updated

3 years ago
Keywords: leave-open
Reporter

Comment 4

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/d3366b063f5231867f2cea79ae229ac4b4e2c63b
Bug 1307481 - Part 1: Move Tooltip.js and HTMLTooltip.js into widgets/tooltip/ r=jdescottes
Reporter

Comment 6

3 years ago
We are moving the OptionsStore into a tooltip/util/OptionsStore.js so that it can be reused in InlineTooltip.js and Tooltip.js
Attachment #8798294 - Flags: review?(jdescottes)
Reporter

Comment 7

3 years ago
Since InlineTooltip will attach to the panel document instead of the toolbox document, we should ideally just pass the document that each tooltip will be attaching to instead of just passing in the toolbox.
Attachment #8798295 - Flags: review?(jdescottes)
Reporter

Comment 8

3 years ago
WIP for reference of how Part 2 and 3 fits into InlineTooltip.js
Comment on attachment 8798295 [details] [diff] [review]
Part 3: HTMLTooltip should receive the document that it should be attached to instead of the toolbox [1.0]

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

The reason we are asking for the toolbox is to make sure that we have access to the top-level devtools document.
I prefer asking for the toolbox rather than asking for a document and writing "please provide the toolbox document" in a JSDoc nobody reads :)

It's not as if we could use the same document for an inline editor and for an html tooltip.
From your WIP patch, when I see:

> SwatchBasedEditorTooltip.call(this, document, stylesheet, true);

It feels like I can just flip true to false and have a tooltip instead of an inline editor. But then the tooltip will be constrained to the document provided, which probably won't be XUL, so won't be able to use a XUL wrapper etc...

I would prefer keeping the toolbox parameter for the HTML Tooltip and discuss alternatives.
The SwatchBasedEditorTooltip constructor could maybe use an object to support document/toolbox being optional parameters: 
> SwatchBasedEditorTooltip({toolbox, document, inline, stylesheet})

We could throw if (!inline && toolbox) or if (inline && !document).
Going a step further, we could let the ColorPickerTooltip (& siblings) build the tooltip and provide it to the SwatchBasedEditorTooltip.

>  // with HTML tooltip
>  function SwatchColorPickerTooltip(inspector) {
>    this.inspector = inspector;
>    let tooltip = new HTMLTooltip(inspector.toolbox, {
>      type: "arrow",
>      consumeOutsideClicks: true,
>      useXulWrapper: true,
>      stylesheet: "chrome://devtools/content/shared/widgets/spectrum.css"
>    });
>    SwatchBasedEditorTooltip.call(this, tooltip);
>    // ...
>  }
>  
>  // with inline tooltip
>  function SwatchColorPickerTooltip(inspector) {
>    this.inspector = inspector;
>    let tooltip = new InlineTooltip(inspector.panelDoc, {});
>    SwatchBasedEditorTooltip.call(this, tooltip);
>    // ...
>  }

(or with a SwatchColorPickerTooltip({toolbox, document}) if we don't want to bind the editors to the inspector too much)

Feel free to ping me to discuss this :)
Attachment #8798295 - Flags: review?(jdescottes)
Comment on attachment 8798294 [details] [diff] [review]
Part 2: Move OptionsStore into a tooltip utils [1.0]

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

Code looks ok to me, but I can't see why this is needed over ES6 default parameters?
Any specific use case in mind?

[quick comment because I looked at your WIP patch to see how this was used. you are reusing the Tooltip.js constructor signature and just so you know, the HTMLTooltip.js signature is a bit different (some options are gone, options names have changed etc...)]
Attachment #8798294 - Flags: review?(jdescottes)
Reporter

Comment 12

3 years ago
You made a good point about simply using ES6 default parameters. So, I went ahead with making changes to Tooltip.js to make it look similar to what we already have in HTMLTooltip.js. This was probably unnecessary for this bug, but it's done.
Attachment #8798294 - Attachment is obsolete: true
Attachment #8798571 - Flags: review?(jdescottes)
Reporter

Updated

3 years ago
Attachment #8798571 - Flags: review?(jdescottes)
Reporter

Updated

3 years ago
Attachment #8798576 - Flags: review?(jdescottes)
Reporter

Updated

3 years ago
Attachment #8798576 - Flags: review?(jdescottes)
Reporter

Updated

3 years ago
Attachment #8798296 - Attachment is obsolete: true
Comment on attachment 8798698 [details] [diff] [review]
Part 2: Remove OptionsStore to use ES6 default parameters and PanelFactory in Tooltip.js [3.0]

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

Great, thanks for doing this!
Attachment #8798698 - Flags: review?(jdescottes) → review+
Comment on attachment 8798576 [details] [diff] [review]
Part 3: HTMLTooltip should receive the document that it should be attached to instead of the toolbox [2.0]

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

Thanks.

::: devtools/client/inspector/shared/style-inspector-overlays.js
@@ +288,5 @@
>  
> +    // Initializing the different tooltips that are used in the inspector.
> +    // These tooltips are attached to the toolbox document if they require a popup panel.
> +    // Otherwise, it is attached to the inspector panel document if it is an inline
> +    // tooltip.

"inline tooltip" seems wrong (if it's inline, it's not really a tooltip right? What about inline editor?

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js
@@ +14,5 @@
>   *
> + * @param {Document} document
> + *        The document to attach the SwatchBasedEditorTooltip. This is either the toolbox
> + *        document if the tooltip is a popup tooltip or the panel's document if it is an
> + *        inline tooltip.

Same comment about inline tooltip / inline editor.

::: devtools/client/webconsole/jsterm.js
@@ +255,5 @@
>  
>      let toolbox = gDevTools.getToolbox(this.hud.owner.target);
>      if (!toolbox) {
>        // In some cases (e.g. Browser Console), there is no toolbox.
>        toolbox = { doc };

no need to create this fake toolbox object anymore.
Attachment #8798576 - Flags: review?(jdescottes) → review+
Reporter

Comment 19

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/a9f9f561127275de84173d0f536e6d74a6000730
Bug 1307481 - Part 2: Remove OptionsStore to use ES6 default parameters and PanelFactory in Tooltip.js r=jdescottes

https://hg.mozilla.org/integration/fx-team/rev/91c6a72e5d0d7779aae8fd514c1e18d5e083c8e4
Bug 1307481 - Part 3: HTMLTooltip should receive the document that it should be attached to instead of the toolbox r=jdescottes
Reporter

Comment 24

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/9a1730c2fdd5a27f0caa0023b0b1205644efc378
Bug 1307481 - Part 2: Remove OptionsStore to use ES6 default parameters and PanelFactory in Tooltip.js r=jdescottes

https://hg.mozilla.org/integration/fx-team/rev/bc270f96c358000a96bafb743a5f1e74c0d36c81
Bug 1307481 - Part 3: HTMLTooltip should receive the document that it should be attached to instead of the toolbox r=jdescottes
Reporter

Updated

3 years ago
Attachment #8797691 - Flags: checkin+
Reporter

Updated

3 years ago
Attachment #8798698 - Flags: checkin+
Reporter

Updated

3 years ago
Attachment #8799094 - Flags: checkin+
Reporter

Comment 26

2 years ago
Posted patch 1307481-4.patch (obsolete) — Splinter Review
Reporter

Updated

2 years ago
Assignee: gl → sheldon.roddick
Assignee

Comment 27

2 years ago
Posted patch Bug1307481 v1 (obsolete) — Splinter Review
Attachment #8837391 - Flags: review?(gl)
Reporter

Updated

2 years ago
Attachment #8829148 - Attachment is obsolete: true
Reporter

Comment 28

2 years ago
Posted patch 1307481 v1 Rebased (obsolete) — Splinter Review
Reporter

Comment 29

2 years ago
Comment on attachment 8837391 [details] [diff] [review]
Bug1307481 v1

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

::: devtools/client/shared/widgets/Spectrum.js
@@ -72,5 @@
>    this.dragger = this.element.querySelector(".spectrum-color");
>    this.dragHelper = this.element.querySelector(".spectrum-dragger");
>    Spectrum.draggable(this.dragger, this.onDraggerMove.bind(this));
>  
> -  this.alphaSlider = this.element.querySelector(".spectrum-alpha");

I don't think we should be making any changes to Spectrum.js
Attachment #8837391 - Flags: review?(gl)
Assignee

Comment 30

2 years ago
Posted patch Bug-1307481.patch v2 (obsolete) — Splinter Review
Attachment #8837391 - Attachment is obsolete: true
Attachment #8840317 - Flags: review?(gl)
Reporter

Updated

2 years ago
Attachment #8840317 - Flags: review?(gl) → review?(jdescottes)
Reporter

Updated

2 years ago
Attachment #8840257 - Attachment is obsolete: true
Comment on attachment 8840317 [details] [diff] [review]
Bug-1307481.patch v2

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

Thanks for the patch, it's in the good direction. 

I'll let you look through my comments, ping me if you need to discuss anything.
Most importantly we need to cleanup the hide/show logic of the InlineTooltip. 

Some STRs that fail right now:

STR1
- open data:text/html;charset=utf-8,<body style="background:gold;color:blue">hi
- open inspector
- click on swatch next to gold
- press ESCAPE
- click on swatch next to blue
=> Widget is displayed below gold, but should be displayed below blue.

STR2
- open data:text/html;charset=utf-8,<body style="background:gold;color:blue">hi
- open inspector
- click on swatch next to gold
- select HTML element in markup view
- select back BODY element in markup view
- click on swatch next to gold
=> Nothing happens 

The patch also needs to be rebased.

::: devtools/client/inspector/shared/tooltips-overlay.js
@@ +88,5 @@
>      this.cssDocs = new CssDocsTooltip(toolbox.doc);
>  
>      if (this.isRuleView) {
>        // Color picker tooltip
> +      this.colorPicker = new SwatchColorPickerTooltip(panelDoc,

It's mandatory to pass the toolbox document, because the HTML tooltip needs it. You can still get the panel document later on, in SwatchColorPickerTooltip, when you know which widget you will use.

::: devtools/client/shared/widgets/ColorWidget.js
@@ +441,5 @@
>        rgbNoAlpha + ")";
>      this.alphaSliderInner.style.background = alphaGradient;
>    },
>  
> +  _initializeColorWidget: function () {

We are not using the `_` prefix anywhere in this file, so let's rename this to initializeColorWidget and move it before the show() method.

::: devtools/client/shared/widgets/color-widget.css
@@ +42,5 @@
>  }
>  
>  .colorwidget-container {
>    position: relative;
> +  display: block;

You don't need to change this, and the expected display here is "inline-block" anyway. It is forced when we call show() on the ColorWidget, by adding the `colorwidget-show` classname.

In the long run I think we should get rid of the colorwidget-show class, since showing/hiding the widget is handled by the InlineTooltip. But for now let's leave it as it is.

::: devtools/client/shared/widgets/tooltip/InlineTooltip.js
@@ +5,5 @@
> +"use strict";
> +
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
> +function InlineTooltip(toolboxDoc, {

Add JSDoc

toolboxDoc -> doc

Here you most likely won't have a "toolbox" document.
A toolbox document means the document of the top-level toolbox iframe. 
It's useful for the regular HTML Tooltip because it allows us to use as much viewport as possible.

@@ +6,5 @@
> +
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
> +function InlineTooltip(toolboxDoc, {
> +    closeOnEvents = []

remove unused argument.

@@ +11,5 @@
> +  } = {}) {
> +  EventEmitter.decorate(this);
> +
> +  this.doc = toolboxDoc;
> +  this.closeOnEvents = closeOnEvents;

remove unused property.

@@ +14,5 @@
> +  this.doc = toolboxDoc;
> +  this.closeOnEvents = closeOnEvents;
> +
> +  this.panel = this.doc.createElement("div");
> +  this.panelContent = this.panel.firstChild;

This is always null, but I think we should get rid of panelContent anyway (see next comments)

@@ +15,5 @@
> +  this.closeOnEvents = closeOnEvents;
> +
> +  this.panel = this.doc.createElement("div");
> +  this.panelContent = this.panel.firstChild;
> +  this.clear();

No need to clear() here, this.panel is already empty.

@@ +17,5 @@
> +  this.panel = this.doc.createElement("div");
> +  this.panelContent = this.panel.firstChild;
> +  this.clear();
> +
> +  // // Listen to custom emitters' events to close the tooltip

Cleanup commented out code

@@ +34,5 @@
> +InlineTooltip.prototype = {
> +  /**
> +   * Show the tooltip. It might be wise to append some content first if you
> +   * don't want the tooltip to be empty. You may access the content of the
> +   * tooltip by setting a XUL node to t.content.

There is no XUL involved here, looks like you took the comment from Tooltip.js. 
The implementation and technical details are a bit different. Not even sure what
t.content is supposed to be.

@@ +37,5 @@
> +   * don't want the tooltip to be empty. You may access the content of the
> +   * tooltip by setting a XUL node to t.content.
> +   *
> +   * @param {DOMNode} anchor
> +   *        Which node should the tooltip be shown on

This isn't really accurate for the inline tooltip, it is display "inline", "below" the anchor, but not "on" it

@@ +41,5 @@
> +   *        Which node should the tooltip be shown on
> +   */
> +  show(anchor) {
> +    if (this.isVisible()) {
> +      return;

If the anchor is different from the current one, we should move the inline tooltip. Even if the tooltip is already visible.

@@ +46,5 @@
> +    }
> +
> +    this.panel.appendChild(this.panelContent);
> +
> +    let propertyContainer = anchor.closest(".ruleview-propertycontainer");

This logic should be moved to the caller and the inline tooltip should directly use the provided anchor.

.ruleview-propertycontainer is a classname specific to the inspector ruleview.
The inline tooltip is a widget and could be used in other panels.

@@ +51,5 @@
> +
> +    // Insert the inline editor next to the property
> +    if (!this.panel.parentNode) {
> +      propertyContainer.parentNode.insertBefore(this.panel,
> +          propertyContainer.nextSibling);

The show and hide actions should mirror each other.

If calling show() appends this.panel to a container, then hide should remove this.panel from the container.
There is no reason to do this only the first time.

I would modify the current implementation as follows:
- get rid of the panelContent property
- show() adds this.panel in its container
- hide() remove this.panel from its container and no longer calls clear()
- clear() simply does this.panel.innerHTML = "";
- isVisible() checks this.panel.parentNode
- setContent() calls clear() and this.panel.appendChild(content)

@@ +74,5 @@
> +
> +  /**
> +   * Check if the tooltip is currently displayed.
> +   *
> +   * @return {Boolean} true if the tooltip is not empty

Should be "true if the tooltip is visible", being empty or not is a different problem.
A tooltip can have a content set, but still be hidden.

@@ +77,5 @@
> +   *
> +   * @return {Boolean} true if the tooltip is not empty
> +   */
> +  isVisible() {
> +    return this.panel.firstChild != null && this.panel.firstChild != undefined;

it's usually bad practice to compare to undefined.
  typeof this.panel.firstChild !== "undefined" && this.panel.firstChild !== null;

@@ +80,5 @@
> +  isVisible() {
> +    return this.panel.firstChild != null && this.panel.firstChild != undefined;
> +  },
> +
> +  clear() {

JSDoc

@@ +94,5 @@
> +   * append the new content element.
> +   * Consider using one of the set<type>Content() functions instead.
> +   *
> +   * @param {DOMNode} content
> +   *        A node that can be appended in the tooltip XUL element

no XUL involved.

@@ +97,5 @@
> +   * @param {DOMNode} content
> +   *        A node that can be appended in the tooltip XUL element
> +   */
> +  setContent(content) {
> +    if (this.content == content) {

I don't think we need to guard against that.

@@ +113,5 @@
> +    return this.panel.firstChild;
> +  },
> +
> +  destroy() {
> +    // let closeOnEvents = this.options.get("closeOnEvents");

cleanup commented out code.

@@ +123,5 @@
> +    //     }
> +    //   }
> +    // }
> +
> +    this.panel.parentNode.removeChild(this.panel);

If you implement my suggestions, just call hide() instead.

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js
@@ +17,5 @@
>   *        The document to attach the SwatchBasedEditorTooltip. This is either the toolbox
>   *        document if the tooltip is a popup tooltip or the panel's document if it is an
>   *        inline editor.
>   */
> +function SwatchBasedEditorTooltip(document, stylesheet, useInline) {

Update the JS Doc here. 

Will need to add the missing doc for stylesheet (it is a String, the URL of the stylesheet to use if useInline is false).

@@ +38,5 @@
>  
>    // By default, swatch-based editor tooltips revert value change on <esc> and
>    // commit value change on <enter>
>    this.shortcuts = new KeyShortcuts({
> +    window: useInline ? this.tooltip.doc : this.tooltip.topWindow

this.tooltip.doc is not a window element. Maybe you could add a getter to the InlineTooltip that would return this.doc.defaultView? 

This way we won't need a ternary operator here.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +38,5 @@
>  function SwatchColorPickerTooltip(document,
>                                    inspector,
>                                    {supportsCssColor4ColorFunction}) {
>    let stylesheet = NEW_COLOR_WIDGET ?
>      "chrome://devtools/content/shared/widgets/color-widget.css" :

As it turns out, "stylesheet" will only be used if we are using the regular HTMLTooltip color picker.

Either always pass "chrome://devtools/content/shared/widgets/spectrum.css", or pass null when NEW_COLOR_WIDGET is true.

@@ +40,5 @@
>                                    {supportsCssColor4ColorFunction}) {
>    let stylesheet = NEW_COLOR_WIDGET ?
>      "chrome://devtools/content/shared/widgets/color-widget.css" :
>      "chrome://devtools/content/shared/widgets/spectrum.css";
> +  SwatchBasedEditorTooltip.call(this, document, stylesheet, NEW_COLOR_WIDGET);

Here "document" should be the the toolbox document (cf previous comment).

You could do let tooltipDocument = NEW_COLOR_WIDGET ? inspector.view.panelDoc ? document.

The HTMLTooltip used to expect a toolbox argument instead of a document, which made this really explicit. Changing it to a `document` was a mistake in my opinion.

This means that the current jsDoc comment for the `document` argument is inaccurate. Since the decision between inline or regular tooltip is made here, the document passed to this function should always be the toolbox document. Can you update the jsdoc accordingly?

I think it would also be nice to rename the document argument to toolboxDocument, to make it more explicit.

@@ +58,2 @@
>     * Fill the tooltip with a new instance of the spectrum color picker widget
>     * initialized with the given color, and return the instance of spectrum

JS Doc should be updated since the method can return both a Spectrum or ColorWidget instance.

I know this wasn't introduced by your changeset but since we're updating the method, it would be nice to fix this :)
Attachment #8840317 - Flags: review?(jdescottes) → feedback+
Assignee

Comment 32

2 years ago
Posted patch Bug-1307481.patch v3 (obsolete) — Splinter Review
Attachment #8840317 - Attachment is obsolete: true
Attachment #8843680 - Flags: review?(jdescottes)
Assignee

Comment 33

2 years ago
Posted patch Bug-1307481.patch v3b (obsolete) — Splinter Review
Attachment #8843680 - Attachment is obsolete: true
Attachment #8843680 - Flags: review?(jdescottes)
Attachment #8843755 - Flags: review?(jdescottes)
Attachment #8843755 - Flags: review?(gl)
Reporter

Updated

2 years ago
Attachment #8843755 - Flags: review?(gl) → feedback?(gl)
Assignee

Comment 34

2 years ago
Posted patch Bug-1307481.patch v3c (obsolete) — Splinter Review
Removed outdated JDoc and commented code
Attachment #8843755 - Attachment is obsolete: true
Attachment #8843755 - Flags: review?(jdescottes)
Attachment #8843755 - Flags: feedback?(gl)
Attachment #8843757 - Flags: review?(jdescottes)
Attachment #8843757 - Flags: feedback?(gl)
Reporter

Updated

2 years ago
Attachment #8843757 - Flags: feedback?(gl)
Comment on attachment 8843757 [details] [diff] [review]
Bug-1307481.patch v3c

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

Thanks for the update, getting close to landing! 

I will let you address my last comments.

Also I see you have a few eslint failures, let me know if you need help to run eslint locally. 
(https://wiki.mozilla.org/DevTools/CodingStandards#JS_linting_with_ESLint)

::: devtools/client/shared/widgets/tooltip/InlineTooltip.js
@@ +12,5 @@
> + *
> + * @param {Document} doc
> + *        The toolbox document to attach the InlineTooltip container.
> + */
> +function InlineTooltip(doc) {

eslint: The whole file contains CRLF (windows) line endings. 
Configure your editor to use LF line endings.

@@ +18,5 @@
> +
> +  this.doc = doc;
> +
> +  this.panel = this.doc.createElement("div");
> +  this.doc.body.appendChild(this.panel);

No need to append here.

@@ +45,5 @@
> +    if (!this.isVisible()) {
> +      return;
> +    }
> +
> +    this.panel.parentNode.removeChild(this.panel);

eslint: replace this by "this.panel.remove();"

@@ +69,5 @@
> +
> +  /**
> +   * Set the content of this tooltip. Will first clear the tooltip and then
> +   * append the new content element.
> +   * Consider using one of the set<type>Content() functions instead.

This line is not relevant for the InlineTooltip.

@@ +85,5 @@
> +    return this.panel.firstChild;
> +  },
> +
> +  _getTopWindow: function () {
> +    return this.doc.defaultView;

nit: this is only used in one spot, maybe no need to create an extra function. 
We can just do this.topWindow = this.doc.defaultView in the constructor.

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js
@@ +15,5 @@
>   *
>   * @param {Document} document
>   *        The document to attach the SwatchBasedEditorTooltip. This is either the toolbox
>   *        document if the tooltip is a popup tooltip or the panel's document if it is an
>   *        inline editor.

Please add the missing arguments to the JSDoc here.

@@ +80,5 @@
>     * @return {Promise} a promise that resolves once the editor tooltip is displayed, or
>     *         immediately if there is no currently active swatch.
>     */
>    show: function () {
> +    let tooltipAnchor = this.useInline ? this.activeSwatch.closest(".ruleview-propertycontainer") : this.activeSwatch;

eslint: line too long (90chars max)

It would be nice to introduce a new class unrelated to the ruleview to identify the inline tooltip container. 

The ruleview property container is created at http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/devtools/client/inspector/rules/views/text-property-editor.js#111 

There you could add a second class "inline-tooltip-container" and rely on this new class here. This way, when we want to use the inline tooltip in other panels than the rule view, we can also reuse "inline-tooltip-container", which will have no styles attached.

Some optional improvements that you could add here: 
- the "inline-tooltip-container" string could be defined as a const in the beginning of the SwatchBasedEditorTooltip file
- log an error when useInline is true but activeSwatch.closest(".inline-tooltip-container") is false
Attachment #8843757 - Flags: review?(jdescottes) → feedback+
Assignee

Comment 36

2 years ago
Posted patch Bug-1307481.patch v4 (obsolete) — Splinter Review
Attachment #8843757 - Attachment is obsolete: true
Attachment #8844805 - Flags: review?(jdescottes)
Attachment #8844805 - Flags: feedback?(gl)
Comment on attachment 8844805 [details] [diff] [review]
Bug-1307481.patch v4

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

Thanks for addressing my comments!
Patch needs rebasing, can you upload a version rebased on the latest mozilla central?

Pushed to try in the meantime: https://treeherder.mozilla.org/#/jobs?repo=try&revision=deee32642a1983f77f31f9e92936efd9a861fb63
Attachment #8844805 - Flags: review?(jdescottes) → review+
Reporter

Updated

2 years ago
Attachment #8844805 - Flags: feedback?(gl)
Reporter

Comment 39

2 years ago
Comment on attachment 8844805 [details] [diff] [review]
Bug-1307481.patch v4

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

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js
@@ +7,5 @@
>  const EventEmitter = require("devtools/shared/event-emitter");
>  const KeyShortcuts = require("devtools/client/shared/key-shortcuts");
>  const {HTMLTooltip} = require("devtools/client/shared/widgets/tooltip/HTMLTooltip");
> +const InlineTooltip = require("devtools/client/shared/widgets/tooltip/InlineTooltip");
> +const INLINE_TOOLTIP_CLASS = "inline-tooltip-container";

Add a new line to separate requires and constant values.

@@ +94,2 @@
>        let onShown = this.tooltip.once("shown");
> +      this.tooltip.show(tooltipAnchor, "topcenter bottomleft");

I think we can simply make this a default value in the show() function in HTMLTooltip.js unless this is changed elsewhere. What do you think Julian?

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +66,5 @@
>      container.id = "spectrum-tooltip";
> +
> +    let widget;
> +    let node = doc.createElementNS(XHTML_NS, "div");
> +    if (NEW_COLOR_WIDGET) {

Add a new line before the if statement.

@@ +85,5 @@
>       */
>      eyedropper.style.pointerEvents = "auto";
>      container.appendChild(eyedropper);
>  
> +    this.tooltip.setContent(container, { width: 218, height: 224 });

This seems wrong for the NEW_COLOR_WIDGET
Attachment #8844805 - Flags: feedback+
Assignee

Comment 40

2 years ago
Attachment #8844805 - Attachment is obsolete: true
Attachment #8846523 - Flags: review?(jdescottes)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #39)
> Comment on attachment 8844805 [details] [diff] [review]
> Bug-1307481.patch v4
> 
> @@ +94,2 @@
> >        let onShown = this.tooltip.once("shown");
> > +      this.tooltip.show(tooltipAnchor, "topcenter bottomleft");
> 
> I think we can simply make this a default value in the show() function in
> HTMLTooltip.js unless this is changed elsewhere. What do you think Julian?

Interestingly, this is actually not used in the HTMLTooltip show method. We can simply drop this argument
Comment on attachment 8846523 [details] [diff] [review]
Bug-1307481.patch v5

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

Thanks for the update! 
Pushing to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88b5030f50d56e16ce675649cf5fb8ef50c5a0ff 

Let's just wait for results here and land this patch. 

By the way, you normally don't have to re-request for review as soon as you have been granted an r+.
You can simply upload a new version of your patch and set r+ directly on it.

::: devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js
@@ +95,2 @@
>        let onShown = this.tooltip.once("shown");
> +      this.tooltip.show(tooltipAnchor, "topcenter bottomleft");

Sorry to ask for an additional change, but after checking this extra parameter is not needed in either the HTMLTooltip or the InlineTooltip so we can simply drop it.

No need to block landing on this, I will log a follow up.
Attachment #8846523 - Flags: review?(jdescottes) → review+

Comment 43

2 years ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda394bed282
Part 4: Implements the Inline Tooltip widget. r=jdescottes
Reporter

Updated

2 years ago
Keywords: leave-open

Comment 44

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fda394bed282
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.