1259834 - Create a basic HTML Tooltip API

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

As we convert more tools to HTML they will need a way to open panels that can open outside of their frame.  We don't have a way to open or modify these without using a XUL panel.

The toolbox needs to provide some way for tools to open and close these panels.

Comment 1

[Brian Grinstead [:bgrins]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1257613#c2 for the current plan for context menus, which is a similar issue.

Comment 2

[Brian Grinstead [:bgrins]]

I'd like some feedback about if having the panel expand beyond the toolbox frame is a requirement we need to keep.  It would be possible to implement this without a XUL shim if we had a restriction that the popup never gets bigger than the toolbox window and we absolutely positioned it within the toolbox.

The tradeoff is that if the toolbox is small, then popups become smaller (and may even need to scroll to show the full contents of the popup).  The good part is it would work without chrome priv and entirely without XUL.  This would affect popups throughout the tools (think inplace editor for CSS rules, popups in rule view, image previews in markup view, autocomplete in console and style editor).

I'm leaning towards building an API that opens popups in this manner, without relying at all on XUL but we should be conscious of the UX tradeoffs (and maybe other technical limitations I haven't thought of).

Comment 3

[Patrick Brosset <:pbro>]

Hard decision, but it needs to be taken.
XUL panels have served us well, and I'd really love for the web platform to provide something like this, but it doesn't, so I don't see an alternative here.

Of course, we can (like in other bugs) have an API that's independent of the way the panel is ultimately shown, so that on Firefox Desktop we can still use XUL panels, but in other hosts, we could use fake DIV-based popups constrained to the toolbox.

Now, in practice, I don't think this is too bad:
- Chrome devtools has always had this limitation, and they don't seem to suffer from it much.
- For visual editors in the rule-view: not a big deal anyway since we agreed to move to inline editors, not tooltips anymore.
- For image preview tooltips: these are preview of images only so, in some sense, not as important as other devtools features like, say, debugging javascript. Maybe we could even have the image size adapt to the toolbox size.
- For event tooltips and variableview tooltips: these are the ones that require the most space, and having them be constrained to a small toolbox size would be really bad. But then again, who would debug javascript in a small toolbox?
- For webconsole autocomplete: as soon as you enter a couple of first letters in the field, then the suggestion list is usually no longer than 10 entries anyway. So we'd have to have a max-height and a scrollbar, but in most cases we wouldn't be hiding too many entries.

I think the biggest problem is that most of the important tooltips are tall rather than wide (at least variableviews, event lists, auto-completion lists) so, while in side-docking mode that won't be much of a problem, our most used docking-mode (I believe) is bottom and so we have more chances of hiding important information. Maybe we should investigate more horizontal ways of displaying the same information?

Comment 4

[Helen V. Holmes (:helenvholmes) (:✨)]

The only thing I can think of this negatively affecting is bug 1238982 (popup toolbars), but I think that there's no reason these popups couldn't open in a direction that allowed them to remain within the devtools frame.

Patrick's right that it doesn't seem to affect Chrome devtools negatively. I think from my end it's safe to no longer have that as a requirement.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to (Unavailable until April 25) Brian Grinstead [:bgrins] from comment #2)
> I'd like some feedback about if having the panel expand beyond the toolbox
> frame is a requirement we need to keep.  It would be possible to implement
> this without a XUL shim if we had a restriction that the popup never gets
> bigger than the toolbox window and we absolutely positioned it within the
> toolbox.
> 

After reviewing the different panels used in devtools, assuming we switch to inline editors for all ruleview editors, there is still an issue with the developer toolbar. This widget can be displayed in standalone today, without the rest of the toolbox, and heavily relies on popups in order to display hints and results.

We either have to change the UX of this component, or make sure our implementation of floating panels is still able to use XUL panels if needed.

Brian: what do you think? do we have special plans for the developer toolbar when it comes to devtools.html?

Comment 6

[Julian Descottes [:jdescottes]]

-> see Bug 1258390 : replace the current ruleview editor tooltips with inline editor widgets.

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Julian Descottes [:jdescottes] from comment #5)
> After reviewing the different panels used in devtools, assuming we switch to
> inline editors for all ruleview editors, there is still an issue with the
> developer toolbar. This widget can be displayed in standalone today, without
> the rest of the toolbox, and heavily relies on popups in order to display
> hints and results.

If that's the case, could this possible end up affecting CSS autocomplete, or no?

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #7)
> (In reply to Julian Descottes [:jdescottes] from comment #5)
> > After reviewing the different panels used in devtools, assuming we switch to
> > inline editors for all ruleview editors, there is still an issue with the
> > developer toolbar. This widget can be displayed in standalone today, without
> > the rest of the toolbox, and heavily relies on popups in order to display
> > hints and results.
> 
> If that's the case, could this possible end up affecting CSS autocomplete,
> or no?

As discussed on IRC, the CSS autocomplete of the rule view is not impacted by the same issue.

The problem with the gcli is that we can display it on its own, with almost no height available to host a tooltip/popup.

Comment 9

[Julian Descottes [:jdescottes]]

(clearing the ni? for Brian as we discussed it in a meeting. gcli might not be ported to devtools.html, but keeping the possibility of having a XUL wrapper panel would be nice to have) 

Listed all the XUL panels used in devtools today https://docs.google.com/document/d/1cFbpZyO8YOsVZyNvfZ3llP_-smvofuvD_Gyl1g7waPU/

There are two main entry points for creating panels in the devtools:
- directly use a XUL panel (declared in a XUL file or created dynamically)
- use the Tooltip.js helper (devtools/client/shared/widgets/Tooltip.js)

Tooltip.js acts as a wrapper on top of the XUL panel. It provides the same (renamed) events, similar APIs, as well as helpers to create the more complex popups : image preview, event details, variables bubble, ruleview widgets.

Comment 10

[Julian Descottes [:jdescottes]]

In this bug we should implement a basic HTML Tooltip class.

It should provide an API similar to the existing Tooltip.js class:
- events : shown, showing, hidden, hiding
- show
- hide
- setContent
- getState/isHidden/isDisplayed/isEmpty

Some devtools panels use the type "arrow" (displays an arrow pointing to the panel's anchor). The HTML tooltip should support a similar "arrow" style.

The HTML tooltip should be positioned either in the topmost document. 
If this is a XUL document, the tooltip could be wrapped in a <panel> element. This way, tooltips could still overlap with the content page (helpful for small toolboxes).
If this is a HTML document, the tooltip will be wrapped in a HTML element.

The HTML tooltip should support the same positioning capabilities as the XUL panel. We need to be able to specify:
- anchor element
- preferred position(s?)
- x,y offsets

If the specified position&offset cannot be respected, the tooltip should try to flip to other positions (see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/panel.flip). If no position can support the tooltip, it will ultimately be resized.

The tooltip should not try to stick to its anchor in case of scrolling, panel resizing etc... Instead the tooltip should be closed.

Other bugs have been filed to create replacements for the advanced tooltips provided today by Tooltip.js as well as for the autocomplete popup, which is shared by many components. Other tooltips are either pretty limited (simple text/layout) and can be migrated when their respective module is migrated (bug 1263337).

Comment 11

[Julian Descottes [:jdescottes]]

Doing some further splitting.

In this first bug I would like to focus on the pure HTML implementation.
New bugs created : 
- Bug 1267401 to implement the arrow style tooltip type 
- Bug 1267403 to take advantage of XUL panels if possible

Decided to split after spending some time prototyping HTML tooltip solutions. The fact that our toolbox will still rely on iframes to isolate modules/sub-modules/widgets (...) implies an increased complexity for:
- positioning the popup: there is no API to compute the position of an element relative to a viewport other than its own
- events are not propagated between windows: most tooltips are closed when clicking outside of the tooltip element. But click events on frame F are not propagated to the parent window/document of frame F.
- same issue for tooltips closed on certain keys

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: bug1259834.proto.patch

Brian: Not sure what is the best way to wrap our HTML tooltip content in a XUL document. For now, I am using an iframe to isolate the tooltip, but maybe simply inserting a floating "html:div" element would be enough?

Very basic implementation. The API is currently very close to the one we have for Tooltip.js (setContent, show, hide).

Current limitations: only supports TOP/BOTTOM positions, no arrow styling, no offset supported. Missing helpers such as "toggleOnHover".
Current issues: Did not manage to catch when the window is losing focus, so the tooltips remain displayed if you alt+tab.

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Review commit: https://reviewboard.mozilla.org/r/49817/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49817/

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8747106 [details] [diff] [review]
bug1259834.proto.patch

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

::: devtools/client/shared/widgets/HTMLTooltip.js
@@ +265,5 @@
> +    return this.document.documentElement.namespaceURI === XUL_NS;
> +  },
> +
> +  _getTopmostWindow: function () {
> +    let win = this.document.defaultView;

Could you do win.top instead?

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

As discussed during standup I added a test case (which is really more a playground right now)

-> devtools/client/shared/test/browser_html_tooltip-01.js

It listens to clicks, and displays a tooltip, using the event target as anchor. 
I had to add a temporary "ready" event to notify the test that the iframe container is loaded and ready to be used, but I'll try to find a cleaner way to deal with this ; it was just to get the test case working.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49817/diff/1-2/

Comment 17

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/49817/#review46803

::: devtools/client/shared/widgets/HTMLTooltip.js:116
(Diff revision 2)
> +   *          If layout permits, the tooltip will be displayed on top/bottom
> +   *          of the anchor. If ommitted, the tooltip will be displayed where
> +   *          more space is available.
> +   */
> +  show: function (anchor, {position} = {}) {
> +    this.emit("popupshowing");

Are the popupshowing / popupshown events here for compat with existing code / tests?

Curious, because there are features on top of this that we may not want to deal with (like being able to preventDefault() on the popupshowing event to cancel a show - or even constructing an Event object to pass in that may be used by other code / tests where they will be expecting certain elements to be set as target, etc).

So if we end up not supporting these things we may want to remove popupshowing and rename popupshown to 'shown' or similar to prevent any confusion about what it is doing.

::: devtools/client/shared/widgets/HTMLTooltip.js:124
(Diff revision 2)
> +
> +      if (this._isXUL()) {
> +        this.container.setAttribute("width", width);
> +        this.container.setAttribute("height", height);
> +      } else {
> +        this.container.style.width = width + "px";

We should think about how we are going to handle overflow.  Should we scroll if height is < preferredHeight?

::: devtools/client/shared/widgets/HTMLTooltip.js:187
(Diff revision 2)
> +      container.setAttribute("transparent", true);
> +      container.setAttribute("flex", "1");
> +      container.style.border = "none";
> +      this.document.querySelector("window").appendChild(container);
> +    } else {
> +      container = this.document.createElementNS(XHTML_NS, "div");

These should probably have .theme-body applied so it automatically looks OK in both light and dark theme (same in the iframe that is loaded for XUL docs)

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Looks good, I'm interested to see a 'real' consumer of this API to see what sorts of changes (if any) we'll need to make when migrating to this from the panel.

Comment 19

[Julian Descottes [:jdescottes]]

Thanks for the feedback Brian! Comments inline below.

(In reply to Brian Grinstead [:bgrins] from comment #17)
> https://reviewboard.mozilla.org/r/49817/#review46803
> 
> ::: devtools/client/shared/widgets/HTMLTooltip.js:116
> (Diff revision 2)
> > +   *          If layout permits, the tooltip will be displayed on top/bottom
> > +   *          of the anchor. If ommitted, the tooltip will be displayed where
> > +   *          more space is available.
> > +   */
> > +  show: function (anchor, {position} = {}) {
> > +    this.emit("popupshowing");
> 
> Are the popupshowing / popupshown events here for compat with existing code
> / tests?
> 
> Curious, because there are features on top of this that we may not want to
> deal with (like being able to preventDefault() on the popupshowing event to
> cancel a show - or even constructing an Event object to pass in that may be
> used by other code / tests where they will be expecting certain elements to
> be set as target, etc).
> 
> So if we end up not supporting these things we may want to remove
> popupshowing and rename popupshown to 'shown' or similar to prevent any
> confusion about what it is doing.

Yes I only kept them for backward compat. Let's remove them for now, rename 
"popupshown" to "shown" and "popuphidden" to "hidden".

Other than for tests, only the debugger seems to be really using the 
"popuphiding" event for the variables popup. We can decide whether or not we 
reintroduce a similar event when we will be migrating this popup.  

> 
> ::: devtools/client/shared/widgets/HTMLTooltip.js:124
> (Diff revision 2)
> > +
> > +      if (this._isXUL()) {
> > +        this.container.setAttribute("width", width);
> > +        this.container.setAttribute("height", height);
> > +      } else {
> > +        this.container.style.width = width + "px";
> 
> We should think about how we are going to handle overflow.  Should we scroll
> if height is < preferredHeight?
> 

I don't think the tooltip container should force the scroll. 
I would say overflow: hidden + defined height and width on the tooltip 
container. Then the tooltip content is free to either adapt to the container's 
dimensions or not.

> ::: devtools/client/shared/widgets/HTMLTooltip.js:187
> (Diff revision 2)
> > +      container.setAttribute("transparent", true);
> > +      container.setAttribute("flex", "1");
> > +      container.style.border = "none";
> > +      this.document.querySelector("window").appendChild(container);
> > +    } else {
> > +      container = this.document.createElementNS(XHTML_NS, "div");
> 
> These should probably have .theme-body applied so it automatically looks OK
> in both light and dark theme (same in the iframe that is loaded for XUL docs)

Will do, thanks.

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8747299 [details]
> MozReview Request: Bug 1259834 - Create basic HTML tooltip API
> 
> Looks good, I'm interested to see a 'real' consumer of this API to see what
> sorts of changes (if any) we'll need to make when migrating to this from the
> panel.

I think the easiest way to assert this is to try migrating the image tooltips of
the markup view. With the current implementation the main pain point for the
migrations is the fact that preferredWidth/Height are mandatory. 

We should open a follow up bug to make preferredWidth/Height optional. For most 
tooltips the information can be measured before showing the tooltip, no need 
extract the information by measuring the content node before displaying it.

On a sidenote, for panels that can change size after being displayed (events tooltip,
variables bubble etc...) the implementation will also need to be improved to
no longer rely on a constant height the tooltip container.

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1259834 - example: Use HTML tooltips to display markupview image previews;f=bgrins

Review commit: https://reviewboard.mozilla.org/r/50421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50421/

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49817/diff/2-3/

Comment 22

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/49817/#review46803

> We should think about how we are going to handle overflow.  Should we scroll if height is < preferredHeight?

I've thought a bit more about that and I think we should consider two kinds of tooltips.
- tooltips which can shrink and adapt to the height. For those, the tooltip container does not need to adapt
- tooltips which can not shrink. For those, the tooltip container should allow scrolling and should add extra width to accommodate the scrollbar.

Also, in case we have height <  preferredHeight:
- if the tooltip is displayed ABOVE the anchor, the content should initially be scrolled. The bottom of the tooltip content should be visible
- if the tooltip is displayed BELOW the anchor, the content should not be scrolled. The top of the tooltip content should be visible

(Maybe that's what you actually meant here by "should we scroll" ?)

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Brian: I addressed your comments, and attached an additional patch which uses the HTML tooltip in the markup view to display image previews. It displays the previews on "click" and not on "hover" here, was simpler for this exercise.

Comment 24

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/49817/#review47293

I haven't reviewed _findBestPosition / _getRelativeRect yet, but everything I've checked out looks really good

::: devtools/client/shared/test/browser_html_tooltip-01.js:3
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Please include a summary of what each test is covering in a comment at the top

::: devtools/client/shared/test/browser_html_tooltip-02.js:62
(Diff revision 3)
> +  is(tooltip.isVisible(), true, "Tooltip is still visible");
> +
> +  tooltip.destroy();
> +}
> +
> +function* testConsumeOutsideClicksFalse(doc) {

Isn't this asserting pretty much the same thing as testTooltipClosesOnOutsideClick?  Could they be merged together?

::: devtools/client/shared/widgets/HTMLTooltip.js:201
(Diff revision 3)
> +      e.preventDefault();
> +      e.stopPropagation();
> +    }
> +  },
> +
> +  _isInTooltipContainer: function (node) {

I wonder if this will give incorrect results if an iframe is injected in the tooltip content (i.e. the colorpicker).  We can cross that bridge when we get there if it's a problem.

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49817/diff/3-4/

Comment 26

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/49817/#review47293

> Please include a summary of what each test is covering in a comment at the top

Done, thanks!

> Isn't this asserting pretty much the same thing as testTooltipClosesOnOutsideClick?  Could they be merged together?

Yes it's just a superset of testTooltipClosesOnOutsideClick, I guess we can remove the first one.

> I wonder if this will give incorrect results if an iframe is injected in the tooltip content (i.e. the colorpicker).  We can cross that bridge when we get there if it's a problem.

Good point, I have a new implementation for this method that should take care of this.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

https://reviewboard.mozilla.org/r/49817/#review47775

Looks great, let's land this and deal with any changes needed in follow ups / implementation bug

::: devtools/client/shared/test/helper_html_tooltip.js:54
(Diff revision 4)
> + *        The name of the preference to updated
> + * @param {} value
> + *        The preference value, type can vary
> + * @return {Promise} resolves when the preferences have been updated
> + */
> +function pushPref(preferenceName, value) {

Can this helper function be moved to shared-head.js (either here or in a follow up bug)?

::: devtools/client/shared/widgets/HTMLTooltip.js:208
(Diff revision 4)
> +
> +  _isInTooltipContainer: function (node) {
> +    let contentWindow = this.parent.ownerDocument.defaultView;
> +    let win = node.ownerDocument.defaultView;
> +
> +    if (win == contentWindow) {

Nit: use === throughout the file to be consistent

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8747299 [details]
MozReview Request: Bug 1259834 - Create basic HTML tooltip API;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49817/diff/4-5/

Comment 29

[Julian Descottes [:jdescottes]]

Thanks for the review Brian! Updated the patch with the fixes you pointed at and pushed the latest revision to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e456780892b

Comment 30

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/49817/#review47775

> Can this helper function be moved to shared-head.js (either here or in a follow up bug)?

Moved!

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ea89f71fc38e

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea89f71fc38e