1266450 - Create a HTML Tooltip to display event details

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

Come up with a HTML based replacement for creating HTML tooltips to display image previews.

Image previews are currently created using Tooltip.js helper setEventContent().

This tooltip is currently used by the markupview.

This bug is about implementing the popup content. Displaying and positioning the popup will use the outcome of Bug 1259834.

Comment 1

[Julian Descottes [:jdescottes]]

As discussed in Bug 1195587, it looks like the consumeOutsideClicks attribute of XUL panels actually can accept 3 values:
- consume any click
- consume click if on anchor
- consume no click

The event bubble is silently relying on this, and the HTMLTooltip API might have to be updated to offer a similar API.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part1: move devtools tooltip styles to dedicated file;

Tooltip styles are scattered between common.css and panel-specific CSS
files (e.g. inspector.css). For the HTML tooltip, the panel specific CSS
files will not be applied since the tooltip container is appended to the
devtools top window.

This changeset creates a new tooltips.css file which is loaded by default
with devtools themes.

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part2: remove iframe container for HTML tooltip;

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

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

The existing helper checking if a click occurred inside or outside a
HTMLTooltip container was failing if the click occurred in an iframe.

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part4: allow tooltips to have a variable height;

With this changeset the tooltip's effective height can be smaller than
the height specified when calling setContent. If the tooltip content is
dynamic, this allows to display a small tooltip frame if the content is
collapsed, and a bigger tooltip frame when it is expanded.

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

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part6: migrate EventDetails tooltip;

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

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8758034 [details]
Bug 1266450 - part1: move devtools tooltip styles to dedicated file;

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8758035 [details]
Bug 1266450 - part2: remove iframe container for HTML tooltip;

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

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8758036 [details]
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

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

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8758037 [details]
Bug 1266450 - part4: allow tooltips to have a variable height;

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

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

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

Comment 22

[Julian Descottes [:jdescottes]]

Attachment: event-details-tooltip.gif

The event details tooltip can grow quite tall if the user clicks on nodes with a lot of attached events and starts opening code previews.
As usual, with the HTML tooltip the available height might be much smaller.

I reduced the height of the code previews from 100px to 50px, just enough to display 3 lines of code).

I think as it is now it is still usable, but maybe we should look into more compact designs for this tooltip? What do you think Helen?

(you can grab a build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b89c5c8803261d5a21db40195eb6c75904675ac2 )

Comment 23

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

(In reply to Julian Descottes [:jdescottes] from comment #22)
> Created attachment 8758647 [details]
> event-details-tooltip.gif
> 
> The event details tooltip can grow quite tall if the user clicks on nodes
> with a lot of attached events and starts opening code previews.
> As usual, with the HTML tooltip the available height might be much smaller.
> 
> I reduced the height of the code previews from 100px to 50px, just enough to
> display 3 lines of code).
> 
> I think as it is now it is still usable, but maybe we should look into more
> compact designs for this tooltip? What do you think Helen?
> 
> (you can grab a build here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b89c5c8803261d5a21db40195eb6c75904675ac2 )

I agree that the tooltip gets a little unwieldy—do you want to try and solve those issues in this bug, or do you want to land the HTML version of the tooltip and open a new bug to work on a new design?

Comment 24

[Julian Descottes [:jdescottes]]

I would prefer to land this and handle the new design in another bug, unless you feel the usability impact is too big?

Some options we can consider to improve the situation:
- remove the code preview, and maybe a more obvious "open in debugger" link
- allow tooltips to be displayed on the side of the anchor, in order to use the full toolbox's height
- wait for Bug 1267403 to be able to use the whole window

Comment 25

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

(In reply to Julian Descottes [:jdescottes] from comment #24)
> I would prefer to land this and handle the new design in another bug, unless
> you feel the usability impact is too big?
I don't think the usability impact is too big, so I'm comfortable doing that.

> Some options we can consider to improve the situation:
> - remove the code preview, and maybe a more obvious "open in debugger" link
This was going to be my suggestion and I think it might be the best of the three you've made—I'm not sure how much concrete work/inspection anyone is going to be able to do the tooltip no matter its size.

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8758034 [details]
Bug 1266450 - part1: move devtools tooltip styles to dedicated file;

https://reviewboard.mozilla.org/r/56390/#review54044

I'm asssuming the moved CSS is copy/pasted, let me know if it's not

::: devtools/client/shared/test/browser_html_tooltip-01.js:15
(Diff revision 4)
>  
>  const HTML_NS = "http://www.w3.org/1999/xhtml";
>  const TEST_URI = `data:text/xml;charset=UTF-8,<?xml version="1.0"?>
>    <?xml-stylesheet href="chrome://global/skin/global.css"?>
>    <?xml-stylesheet href="chrome://devtools/skin/common.css"?>
> +  <?xml-stylesheet href="chrome://devtools/skin/tooltips.css"?>

Doubt these tests need a reference to devtools/skin/common.css anymore

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8758035 [details]
Bug 1266450 - part2: remove iframe container for HTML tooltip;

https://reviewboard.mozilla.org/r/56392/#review54058

Removing extra complexity for XUL docs works for me.  Consumers will need to be careful not to accidentally use XUL inside the panel content, might be worth calling out in a comment for setContent

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8758036 [details]
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

https://reviewboard.mozilla.org/r/56394/#review54066

Please see the comment - I think this could be a little easier.  If it doesn't work, let me know and I'll re-review the current implementation

::: devtools/client/shared/widgets/HTMLTooltip.js:226
(Diff revision 4)
>        e.stopPropagation();
>      }
>    },
>  
> -  _isInTooltipContainer: function (node) {
> -    let contentWindow = this.panel.ownerDocument.defaultView;
> +  _isInTooltipContainer: function (e) {
> +    let x = e.pageX;

I guess it's OK, but here's a smaller change requiring less bounds checking that I think also fixes the problem:

    // Otherwise check if the node window is in the tooltip window.
    while (win.parent && win.parent != win) {
      if (win.parent === contentWindow) {
        return this.panel.contains(win.frameElement);
      }
      win = win.parent;
    }
    
Basically, prevent false positives by making sure that the window's frame is a child of the panel instead of just returning true when you get to the same window

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8758037 [details]
Bug 1266450 - part4: allow tooltips to have a variable height;

https://reviewboard.mozilla.org/r/56396/#review54068

Nice!  We can wait and see if we want 'shrinking' to be the default behavior, but I'm happy to try it and see if we find consumers that don't want this behavior before making extra options

::: devtools/client/themes/tooltips.css:154
(Diff revision 4)
>  
>  .tooltip-bottom[type="arrow"] .tooltip-panel {
>    bottom: 0;
>  }
>  
>  .tooltip-arrow {

I just realized that when clicking on the arrow the panel closes.  I think we should change `_isInTooltipContainer` so that the arrow element counts as being part of the container

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

https://reviewboard.mozilla.org/r/56398/#review54078

Nice

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8758034 [details]
Bug 1266450 - part1: move devtools tooltip styles to dedicated file;

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

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8758035 [details]
Bug 1266450 - part2: remove iframe container for HTML tooltip;

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

Comment 33

[Julian Descottes [:jdescottes]]

Comment on attachment 8758036 [details]
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

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

Comment 34

[Julian Descottes [:jdescottes]]

Comment on attachment 8758037 [details]
Bug 1266450 - part4: allow tooltips to have a variable height;

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

Comment 35

[Julian Descottes [:jdescottes]]

Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

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

Comment 36

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part5: move event details tooltip css to tooltips.css;

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

Comment 37

[Julian Descottes [:jdescottes]]

Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

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

Comment 38

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/56390/#review54044

There are a few differences worth reviewing I believe (particularly the way I handle the text ellipsis for .event-tooltip-filename)
I have split my commit: part5 is a simple copy of the CSS, you can check part 6 to see what was changed for the HTML version of the tooltip.

> Doubt these tests need a reference to devtools/skin/common.css anymore

Good point, removed!

Comment 39

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/56394/#review54066

Works perfectly, and thanks for pointing me at frameElement, I didn't know this one.

Comment 40

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/56396/#review54068

That's exactly what I thought. We can quite easily bring back the old behavior as an option if needed, ne need for the extra complexity right now.

> I just realized that when clicking on the arrow the panel closes.  I think we should change `_isInTooltipContainer` so that the arrow element counts as being part of the container

Right! Fixed this in part3.

Comment 41

[Brian Grinstead [:bgrins]]

Comment on attachment 8758036 [details]
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

https://reviewboard.mozilla.org/r/56394/#review54352

Comment 42

[Brian Grinstead [:bgrins]]

Comment on attachment 8759639 [details]
Bug 1266450 - part5: move event details tooltip css to tooltips.css;

https://reviewboard.mozilla.org/r/57578/#review54354

Comment 43

[Julian Descottes [:jdescottes]]

Attachment: Bug 1266450 - part7: fix html tooltip autofocus behavior;

For autofocus tooltips, we need to find a focusable item in order
to call focus() now that the tooltip content lives in the same
document as the toolbox. Updated the corresponding test and made
some superficial changes to HTMLTooltip.js.

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

Comment 50

[Julian Descottes [:jdescottes]]

Thanks for the reviews Brian! I just want to update the autofocus behavior after finding about Bug 1277906. We have no real consumer of the autofocus configuration for the HTML tooltip for now, which makes it harder to maintain.

I know the conditional breakpoint tooltip (debugger) will use this, so it might be a while until we get some real usage here. Let me know if you would prefer to get rid of the feature for now, I'd be fine with it as well.

(Only part7 has to be reviewed here, the rest has only been rebased on a more recent fx-team)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

Comment 51

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/57860/#review54948

::: devtools/client/shared/widgets/HTMLTooltip.js:370
(Diff revision 1)
> +  /**
> +   * If the tootlip is configured to autofocus and a focusable element can be found,
> +   * focus it.
> +   */
> +  _maybeFocusTooltip: function () {
> +    let focusableSelector = "a, button, iframe, input, select, textarea";

Where does this list come from?

Comment 52

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/57860/#review54948

> Where does this list come from?

The list here is just a simplified version of http://stackoverflow.com/questions/1599660/which-html-elements-can-receive-focus to start with something.
I would prefer to use an API to do this, but didn't find one. Looks like it would be something useful to have, if not available.

Comment 53

[Julian Descottes [:jdescottes]]

FWIW, in the chrome APIs there is nsIFocusManager::moveFocus [1], which would work fine here. But in the context of devtools-html, I assume this means we would have to reimplement it in JS to use it anyway? I see that the HTML breadcrumbs implementation is still using it so I am not sure what we should allow here or not.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl#86

Comment 54

[Brian Grinstead [:bgrins]]

Comment on attachment 8760153 [details]
Bug 1266450 - part7: fix html tooltip  autofocus behavior;

https://reviewboard.mozilla.org/r/57860/#review55142

::: devtools/client/shared/widgets/HTMLTooltip.js:370
(Diff revision 1)
> +  /**
> +   * If the tootlip is configured to autofocus and a focusable element can be found,
> +   * focus it.
> +   */
> +  _maybeFocusTooltip: function () {
> +    let focusableSelector = "a, button, iframe, input, select, textarea";

Alright, that's fine.  Please add a comment explaining.  I almost think we should just drop this feature, but we know we will need it for future consumers, right?

Comment 55

[Julian Descottes [:jdescottes]]

Comment on attachment 8758034 [details]
Bug 1266450 - part1: move devtools tooltip styles to dedicated file;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56390/diff/6-7/

Comment 56

[Julian Descottes [:jdescottes]]

Comment on attachment 8758035 [details]
Bug 1266450 - part2: remove iframe container for HTML tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56392/diff/6-7/

Comment 57

[Julian Descottes [:jdescottes]]

Comment on attachment 8758036 [details]
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56394/diff/6-7/

Comment 58

[Julian Descottes [:jdescottes]]

Comment on attachment 8758037 [details]
Bug 1266450 - part4: allow tooltips to have a variable height;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56396/diff/6-7/

Comment 59

[Julian Descottes [:jdescottes]]

Comment on attachment 8759639 [details]
Bug 1266450 - part5: move event details tooltip css to tooltips.css;

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

Comment 60

[Julian Descottes [:jdescottes]]

Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56398/diff/7-8/

Comment 61

[Julian Descottes [:jdescottes]]

Comment on attachment 8760153 [details]
Bug 1266450 - part7: fix html tooltip  autofocus behavior;

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

Comment 62

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/57860/#review55142

Thanks for the review. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cdecc27991f

> Alright, that's fine.  Please add a comment explaining.  I almost think we should just drop this feature, but we know we will need it for future consumers, right?

Added a comment. 

The consumers today are:
- debugger conditional-breakpoint tooltip
- rule-view widgets (filter, colorpicker etc...).

It is still not clear to me if the rule view widgets will use a HTMLTooltip. So this leaves us with one guaranteed consumer, but which won't be implemented in the near future. Let's discuss during standup.

Comment 63

[Julian Descottes [:jdescottes]]

Looks like I was rebased on top of a bad commit. New try push, OSX/Win7 only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c02b56527091

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/230e657f6769
part1: move devtools tooltip styles to dedicated file;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/4e4b15f80b80
part2: remove iframe container for HTML tooltip;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/ed461f8fe522
part3: fix helper to check if click occurred in tooltip;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/2ad8e1757b86
part4: allow tooltips to have a variable height;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/42e38084205e
part5: move event details tooltip css to tooltips.css;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/0109ec48bf5a
part6: migrate EventDetails tooltip;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/62ad4f6b5c34
part7: fix html tooltip  autofocus behavior;r=bgrins

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/230e657f6769
https://hg.mozilla.org/mozilla-central/rev/4e4b15f80b80
https://hg.mozilla.org/mozilla-central/rev/ed461f8fe522
https://hg.mozilla.org/mozilla-central/rev/2ad8e1757b86
https://hg.mozilla.org/mozilla-central/rev/42e38084205e
https://hg.mozilla.org/mozilla-central/rev/0109ec48bf5a
https://hg.mozilla.org/mozilla-central/rev/62ad4f6b5c34

Comment 66

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to test this bug and found a new issue that was separately (Bug 1279441).
Since the found issue was logged separate, I am marking this bug Verified Fixed.
The tests were performed on Firefox 50.0a1 (2016-06-09) and on Windows 10 x64, Mac OS X 10.11.1, Ubuntu 14.04 x64.