Closed Bug 1266450 Opened 8 years ago Closed 8 years ago

Create a HTML Tooltip to display event details

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox48 affected, firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Iteration:
50.1 - Jun 20
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox50 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [btpp-fix-later] [devtools-html])

Attachments

(8 files)

Bug 1266450 - part1: move devtools tooltip styles to dedicated file;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1266450 - part2: remove iframe container for HTML tooltip;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1266450 - part3: fix helper to check if click occurred in tooltip;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1266450 - part4: allow tooltips to have a variable height;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1266450 - part6: migrate EventDetails tooltip;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
event-details-tooltip.gif
1.73 MB, image/gif
hholmes
: ui-review+
Details
Bug 1266450 - part5: move event details tooltip css to tooltips.css;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1266450 - part7: fix html tooltip autofocus behavior;
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
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.
Depends on: 1259834
Whiteboard: [btpp-fix-later] [devtools-html]
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Blocks: 1195587
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.
Blocks: 1194363
Blocks: 1197202
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Iteration: 49.2 - May 23 → 49.3 - Jun 6
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/
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/
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 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/
Attachment #8758034 - Attachment description: MozReview Request: Bug 1266450 - part1: move devtools tooltip styles to dedicated file → MozReview Request: Bug 1266450 - part1: move devtools tooltip styles to dedicated file;r?bgrins
Attachment #8758035 - Attachment description: MozReview Request: Bug 1266450 - part2: remove iframe container for HTML tooltip → MozReview Request: Bug 1266450 - part2: remove iframe container for HTML tooltip;r?bgrins
Attachment #8758036 - Attachment description: MozReview Request: Bug 1266450 - part3: fix helper to check if click occurred in tooltip → MozReview Request: Bug 1266450 - part3: fix helper to check if click occurred in tooltip;r?bgrins
Attachment #8758037 - Attachment description: MozReview Request: Bug 1266450 - part4: allow tooltips to have a variable height → MozReview Request: Bug 1266450 - part4: allow tooltips to have a variable height;r?bgrins
Attachment #8758038 - Attachment description: MozReview Request: Bug 1266450 - part5: migrate EventDetails tooltip → MozReview Request: Bug 1266450 - part5: migrate EventDetails tooltip;r?bgrins
Attachment #8758034 - Flags: review?(bgrinstead)
Attachment #8758035 - Flags: review?(bgrinstead)
Attachment #8758036 - Flags: review?(bgrinstead)
Attachment #8758037 - Flags: review?(bgrinstead)
Attachment #8758038 - Flags: review?(bgrinstead)
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 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 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 on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56398/diff/3-4/
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 )
Attachment #8758647 - Flags: ui-review?(hholmes)
(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?
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
(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.
Attachment #8758647 - Flags: ui-review?(hholmes) → ui-review+
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
Attachment #8758034 - Flags: review?(bgrinstead) → review+
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
Attachment #8758035 - Flags: review?(bgrinstead) → review+
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
Attachment #8758036 - Flags: review?(bgrinstead)
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
Attachment #8758037 - Flags: review?(bgrinstead) → review+
Comment on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

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

Nice
Attachment #8758038 - Flags: review?(bgrinstead) → review+
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/
Attachment #8758034 - Attachment description: MozReview Request: Bug 1266450 - part1: move devtools tooltip styles to dedicated file;r?bgrins → Bug 1266450 - part1: move devtools tooltip styles to dedicated file;
Attachment #8758035 - Attachment description: MozReview Request: Bug 1266450 - part2: remove iframe container for HTML tooltip;r?bgrins → Bug 1266450 - part2: remove iframe container for HTML tooltip;
Attachment #8758036 - Attachment description: MozReview Request: Bug 1266450 - part3: fix helper to check if click occurred in tooltip;r?bgrins → Bug 1266450 - part3: fix helper to check if click occurred in tooltip;
Attachment #8758037 - Attachment description: MozReview Request: Bug 1266450 - part4: allow tooltips to have a variable height;r?bgrins → Bug 1266450 - part4: allow tooltips to have a variable height;
Attachment #8758038 - Attachment description: MozReview Request: Bug 1266450 - part5: migrate EventDetails tooltip;r?bgrins → Bug 1266450 - part5: migrate EventDetails tooltip;
Attachment #8758036 - Flags: review?(bgrinstead)
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 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 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 on attachment 8758038 [details]
Bug 1266450 - part6: migrate EventDetails tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56398/diff/4-5/
Review commit: https://reviewboard.mozilla.org/r/57578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57578/
Attachment #8758038 - Attachment description: Bug 1266450 - part5: migrate EventDetails tooltip; → Bug 1266450 - part6: migrate EventDetails tooltip;
Attachment #8759639 - Flags: review?(bgrinstead)
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/
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!
https://reviewboard.mozilla.org/r/56394/#review54066

Works perfectly, and thanks for pointing me at frameElement, I didn't know this one.
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.
Attachment #8758036 - Flags: review?(bgrinstead) → review+
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 on attachment 8759639 [details]
Bug 1266450 - part5: move event details tooltip css to tooltips.css;

https://reviewboard.mozilla.org/r/57578/#review54354
Attachment #8759639 - Flags: review?(bgrinstead) → review+
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/
Attachment #8760153 - Flags: review?(bgrinstead)
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=jdescottes@mozilla.com
Blocks: 1278181
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?
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.
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
Attachment #8760153 - Flags: review?(bgrinstead) → review+
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?
Iteration: 49.3 - Jun 6 → 50.1
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 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 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 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 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 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 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/
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.
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
Pushed by jdescottes@mozilla.com:
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
Depends on: 1279400
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
status-firefox50: fixed → verified
Flags: qe-verify+
Depends on: 1279763
Depends on: 1287090
Depends on: 1326875
Depends on: 1327997
Depends on: 1344504
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: