Julian Descottes [:jdescottes] Assignee
	Description • 8 years ago

The goal of this bug is to take advantage of XUL panels if the topmost document hosting the devtools toolbox is a XUL panel. 

XUL panels can be displayed outside of the boundaries of their container document, which can be very useful for small toolboxes.

The XUL panel should be used only as an invisible wrapper to position the HTML Tooltip. The visible markup and style should remain the same as the ones used in the regular HTML tooltip.

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 8 years ago

Since I blocked several P1 bugs on this one, I'm having a go at this.

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 8 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 3 • 8 years ago

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

	Alexandre Poirot [:ochameau]
	Comment 4 • 8 years ago

I'm wondering if you have the same issue, or if that's specific to linux?
The tooltip are not positioned correctly vertically. It is correct horizontaly.
It is too high on top whenever the tooltip displays on top or bottom.
It seems to be shifted by about the side of the arrow or something.

Otherwise it seems to work great. I got the test passing for the color picker!

	Julian Descottes [:jdescottes] Assignee
	Comment 5 • 8 years ago

Great! Thanks for letting me know about the issue on Linux. My implementation was only working on OSX as it turns out. I have a new patch that should fix this.

I want to land parts 1-7 of Bug 1266456 first, and will send my patch on top of this, otherwise the rebasing will become too painful.

	Julian Descottes [:jdescottes] Assignee
	Comment 6 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 7 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 8 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 10 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

Switching to feedback for now, because I still have one failure on Windows VMs on try. In the meantime, can you let me know if you're ok with the overall approach?

	Alexandre Poirot [:ochameau]
	Comment 11 • 8 years ago

https://reviewboard.mozilla.org/r/60360/#review58150

It looks like you can make this xul hack less intrusive.

Otherwise, I'm not able to easily apply this patch. Is this based on some other bug or something that landed very recently?

::: devtools/client/shared/widgets/HTMLTooltip.js:299
(Diff revision 5)
> -  show: function (anchor, {position, x = 0, y = 0} = {}) {
> +  show: Task.async(function* (anchor, {position, x = 0, y = 0} = {}) {
>      // Get anchor geometry
>      let anchorRect = getRelativeRect(anchor, this.doc);
> -    // Get document geometry
> -    let docRect = this.doc.documentElement.getBoundingClientRect();
> +    if (this.useXulWrapper) {
> +      anchorRect = this._convertToScreenRect(anchorRect);
> +    }

I'm not sure I'm following exactly everything in there, but can't you pass the relative position to showXulWrapperAt and prevent having such xul specifics within show()?

::: devtools/client/shared/widgets/HTMLTooltip.js:308
(Diff revision 5)
>  
>      let themeHeight = EXTRA_HEIGHT[this.type] + 2 * EXTRA_BORDER[this.type];
>      let preferredHeight = this.preferredHeight + themeHeight;
>  
>      let {top, height, computedPosition} =
> -      calculateVerticalPosition(anchorRect, docRect, preferredHeight, position, y);
> +      calculateVerticalPosition(anchorRect, viewportSize, preferredHeight, position, y);

Then here we would still compute position relative to the doc.

::: devtools/client/shared/widgets/HTMLTooltip.js:313
(Diff revision 5)
> -      calculateVerticalPosition(anchorRect, docRect, preferredHeight, position, y);
> +      calculateVerticalPosition(anchorRect, viewportSize, preferredHeight, position, y);
> +
> +    if (this.useXulWrapper && this.preferredHeight === Infinity) {
> +      // XUL tooltips can automatically flip if not enough position is available. This
> +      // behaviour can be erratic: XUL panels sometimes flip even if mathematically there
> +      // is enough height to display the tooltip.

"not enough position"?
Did you meant "not enough space"?

::: devtools/client/shared/widgets/HTMLTooltip.js:314
(Diff revision 5)
> +
> +    if (this.useXulWrapper && this.preferredHeight === Infinity) {
> +      // XUL tooltips can automatically flip if not enough position is available. This
> +      // behaviour can be erratic: XUL panels sometimes flip even if mathematically there
> +      // is enough height to display the tooltip.
> +      height = height - 1;

Do you have a STR to reproduce this if I comment this line?
Could it be that the panel have some kind of 1px border which mess up with this?

::: devtools/client/shared/widgets/HTMLTooltip.js:542
(Diff revision 5)
> +
> +  _showXulWrapperAt: function (left, top) {
> +    let onPanelShown = listenOnce(this.xulPanelWrapper, "popupshown");
> +
> +    // Calculate screen coordinates.
> +    let {offsetLeft, offsetTop} = this._getScreenOffset();

Then here we would only append the screen offsets, but the document offsets.

::: devtools/client/shared/widgets/HTMLTooltip.js:557
(Diff revision 5)
> +
> +  _hideXulWrapper: function () {
> +    let onPanelHidden = listenOnce(this.xulPanelWrapper, "popuphidden");
> +
> +    this.xulPanelWrapper.hidePopup();
> +    this.xulPanelWrapper.setAttribute("hidden", true);

Is setting hidden to true required?

	Julian Descottes [:jdescottes] Assignee
	Comment 12 • 8 years ago

Thanks for having a look! Answers below, but feel free to ping me if you want to discuss them in more details.

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> https://reviewboard.mozilla.org/r/60360/#review58150
> 
> It looks like you can make this xul hack less intrusive.
> 
> Otherwise, I'm not able to easily apply this patch. Is this based on some
> other bug or something that landed very recently?
> 

Yes this one is based on my last patches for the autocomplete migration which did not land yet. I will reupload a version that can be applied on top of fx-team.

> ::: devtools/client/shared/widgets/HTMLTooltip.js:299
> (Diff revision 5)
> > -  show: function (anchor, {position, x = 0, y = 0} = {}) {
> > +  show: Task.async(function* (anchor, {position, x = 0, y = 0} = {}) {
> >      // Get anchor geometry
> >      let anchorRect = getRelativeRect(anchor, this.doc);
> > -    // Get document geometry
> > -    let docRect = this.doc.documentElement.getBoundingClientRect();
> > +    if (this.useXulWrapper) {
> > +      anchorRect = this._convertToScreenRect(anchorRect);
> > +    }
> 
> I'm not sure I'm following exactly everything in there, but can't you pass
> the relative position to showXulWrapperAt and prevent having such xul
> specifics within show()?

I need to have the position of the anchor relative to the screen in order to know how much height is available above / below the anchor, so that I can decide where the tooltip should be displayed. It's not only about translating coordinates. Same issue for the X-axis positioning, I need to know the position of the anchor relative to the edges of the viewport, so that I can calculate how much the arrow should be shifted.

> 
> ::: devtools/client/shared/widgets/HTMLTooltip.js:308
> (Diff revision 5)
> >  
> >      let themeHeight = EXTRA_HEIGHT[this.type] + 2 * EXTRA_BORDER[this.type];
> >      let preferredHeight = this.preferredHeight + themeHeight;
> >  
> >      let {top, height, computedPosition} =
> > -      calculateVerticalPosition(anchorRect, docRect, preferredHeight, position, y);
> > +      calculateVerticalPosition(anchorRect, viewportSize, preferredHeight, position, y);
> 
> Then here we would still compute position relative to the doc.

See previous comment.

> 
> ::: devtools/client/shared/widgets/HTMLTooltip.js:313
> (Diff revision 5)
> > -      calculateVerticalPosition(anchorRect, docRect, preferredHeight, position, y);
> > +      calculateVerticalPosition(anchorRect, viewportSize, preferredHeight, position, y);
> > +
> > +    if (this.useXulWrapper && this.preferredHeight === Infinity) {
> > +      // XUL tooltips can automatically flip if not enough position is available. This
> > +      // behaviour can be erratic: XUL panels sometimes flip even if mathematically there
> > +      // is enough height to display the tooltip.
> 
> "not enough position"?
> Did you meant "not enough space"?
> 

Ahah, yes that's probably what I meant.

> ::: devtools/client/shared/widgets/HTMLTooltip.js:314
> (Diff revision 5)
> > +
> > +    if (this.useXulWrapper && this.preferredHeight === Infinity) {
> > +      // XUL tooltips can automatically flip if not enough position is available. This
> > +      // behaviour can be erratic: XUL panels sometimes flip even if mathematically there
> > +      // is enough height to display the tooltip.
> > +      height = height - 1;
> 
> Do you have a STR to reproduce this if I comment this line?
> Could it be that the panel have some kind of 1px border which mess up with
> this?
> 

Good call! That was it. I had to apply a border-style: none for windows (which displayed a border by default) but I didn't think about testing again on Linux. Thanks!

> ::: devtools/client/shared/widgets/HTMLTooltip.js:542
> (Diff revision 5)
> > +
> > +  _showXulWrapperAt: function (left, top) {
> > +    let onPanelShown = listenOnce(this.xulPanelWrapper, "popupshown");
> > +
> > +    // Calculate screen coordinates.
> > +    let {offsetLeft, offsetTop} = this._getScreenOffset();
> 
> Then here we would only append the screen offsets, but the document offsets.
> 
> ::: devtools/client/shared/widgets/HTMLTooltip.js:557
> (Diff revision 5)
> > +
> > +  _hideXulWrapper: function () {
> > +    let onPanelHidden = listenOnce(this.xulPanelWrapper, "popuphidden");
> > +
> > +    this.xulPanelWrapper.hidePopup();
> > +    this.xulPanelWrapper.setAttribute("hidden", true);
> 
> Is setting hidden to true required?

Not sure, I was using it because it was used in Tooltip.js, I'll test without it.

	Julian Descottes [:jdescottes] Assignee
	Comment 13 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Alexandre Poirot [:ochameau]
	Comment 14 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

https://reviewboard.mozilla.org/r/60360/#review58422

Tested with color,cubic,filter tooltips works great.
Also looked at event details and images. Is there any others?

Note that while testing event details I've seen various weird exception.
But I don't think it is related to this patch:
  TypeError: can't convert undefined to object: EventTooltip.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:293:24
It happens when clicking outside the tooltip to dismiss it. It is intermittent and doesn't always throws...

::: devtools/client/shared/test/browser_html_tooltip_arrow-01.js:107
(Diff revision 6)
>        "The tooltip arrow remains inside the tooltip panel horizontally");
>  
>      yield hideTooltip(tooltip);
>    }
> -});
> +
> +  tooltip.destroy();

You explicitely destroy the tooltip in this test.
Shouldn't we do this in all tests?
We only call tooltip.hide() in most of them.

::: devtools/client/shared/test/browser_html_tooltip_xul-wrapper.js:28
(Diff revision 6)
> +  </window>`;
> +
> +const {HTMLTooltip} = require("devtools/client/shared/widgets/HTMLTooltip");
> +loadHelperScript("helper_html_tooltip.js");
> +
> +// The test toolbox will be 200px tall, the anchors are 50px tall, therefore,tThe maximum

tThe => the

::: devtools/client/shared/widgets/HTMLTooltip.js:301
(Diff revision 6)
>      let preferredHeight = this.preferredHeight + themeHeight;
>  
>      let {top, height, computedPosition} =
> -      calculateVerticalPosition(anchorRect, docRect, preferredHeight, position, y);
> +      calculateVerticalPosition(anchorRect, viewportSize, preferredHeight, position, y);
>  
> -    // Apply height and top information before measuring the content width (if "auto").
> +    this._position = computedPosition;

that _position doesn't seem to be used anywhere?

::: devtools/client/shared/widgets/HTMLTooltip.js:348
(Diff revision 6)
> +  }),
> +
> +  /**
> +   * Calculate the size of the viewport tha limits the tooltip dimensions. When using a
> +   * XUL panel wrapper, the viewport will be able to use the whole screen. Otherwise, the
> +   * viewport will be limited to the tooltip's document.

tha => that

Also, as for getScreenOffset, this isn't just the screen size. It is only the screen size, excluding OS bars on the screen edges.

::: devtools/client/shared/widgets/HTMLTooltip.js:372
(Diff revision 6)
> +    if (this.useXulWrapper && !this.isVisible()) {
> +      // Move the container out of the XUL Panel to measure it.
> +      this.doc.documentElement.appendChild(this.container);
> +    }
> +
>      this.container.classList.add("tooltip-hidden");

This isn't directly related to this patch, but I'm wondering if we have some blinking effects because of this tooltip-hidden toggle when calling tooltip.show multiple times with different anchor/size/position.

While reviewing I was wondering if isVisible() was ever true in this method. It can, but only when calling show() multiple times.

::: devtools/client/shared/widgets/HTMLTooltip.js:508
(Diff revision 6)
> +
> +  _createXulPanelWrapper: function () {
> +    let panel = this.doc.createElementNS(XUL_NS, "panel");
> +
> +    // The XUL panel is just a technical wrapper: disable all features that impact the
> +    // behavior.

I would rather say:
XUL panel is only a way to display DOM elements outside of the document viewport, so disable all features that impact the behavior.

::: devtools/client/shared/widgets/HTMLTooltip.js:523
(Diff revision 6)
> +  },
> +
> +  _showXulWrapperAt: function (left, top) {
> +    let onPanelShown = listenOnce(this.xulPanelWrapper, "popupshown");
> +
> +    // Calculate screen coordinates.

I would mention why we add this offset.
Calculate absolute screen coordinates by adding back offsets due to OS bars on the screen edges.

::: devtools/client/shared/widgets/HTMLTooltip.js:544
(Diff revision 6)
> +    return onPanelHidden;
> +  },
> +
> +  /**
> +   * Convert left/top coordinates relative the tooltip's document to coordinates relative
> +   * to the screen.

This sentence is missing some "to". It sounds more correct like this:
Convert from coordinates relative to the tooltip's document, to coordinates relative to the "available" screen. By "available" we mean the screen, excluding the OS bars display on screen edges.

::: devtools/client/shared/widgets/HTMLTooltip.js:553
(Diff revision 6)
> +    // viewport, excluding chrome UI.
> +    let {mozInnerScreenX, mozInnerScreenY} = this.doc.defaultView;
> +    let {offsetLeft, offsetTop} = this._getScreenOffset();
> +
> +    left = left + mozInnerScreenX - offsetLeft;
> +    top = top + mozInnerScreenY - offsetTop;

left += mozInnerScreenX - offsetLeft;
Same for top.

::: devtools/client/shared/widgets/HTMLTooltip.js:569
(Diff revision 6)
> +    // for applications (excluding OS taskbars, menus etc...)
> +    return {
> +      offsetLeft: this.doc.defaultView.screen.availLeft,
> +      offsetTop: this.doc.defaultView.screen.availTop
> +    };
> +  },

I was wondering if we could simplify the coordinates logic by merging getScreenOffset into getViewportSize.
Instead we would only have getViewportRect, returning all screen.avail{Left,Top,Width,Height}.

	Julian Descottes [:jdescottes] Assignee
	Comment 15 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 16 • 8 years ago

https://reviewboard.mozilla.org/r/60360/#review58422

> You explicitely destroy the tooltip in this test.
> Shouldn't we do this in all tests?
> We only call tooltip.hide() in most of them.

Good point, I will do so in a separate patch.

> that _position doesn't seem to be used anywhere?

good catch, leftover from the HTML autocomplete bug!

> This isn't directly related to this patch, but I'm wondering if we have some blinking effects because of this tooltip-hidden toggle when calling tooltip.show multiple times with different anchor/size/position.
> 
> While reviewing I was wondering if isVisible() was ever true in this method. It can, but only when calling show() multiple times.

This actually occurs for the autocomplete (we call show at every keystroke basically). I'll work on this in the autocomplete bug.

> I was wondering if we could simplify the coordinates logic by merging getScreenOffset into getViewportSize.
> Instead we would only have getViewportRect, returning all screen.avail{Left,Top,Width,Height}.

It does simplify the implementation, thanks!

And also thanks for going through the comments and suggesting fixes/updates, applied most of it.

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 8 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 18 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 19 • 8 years ago

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

	Alexandre Poirot [:ochameau]
	Comment 20 • 8 years ago

Comment on attachment 8767671 [details]
Bug 1267403 - HTMLTooltip tests: destroy tooltips at the end of tests;

https://reviewboard.mozilla.org/r/62112/#review58826

r+ assuming try is green.

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Comment on attachment 8764562 [details]
> Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL
> document;
> 
> https://reviewboard.mozilla.org/r/60360/#review58422
> 
> Tested with color,cubic,filter tooltips works great.
> Also looked at event details and images. Is there any others?

Thanks for testing! We also have the network monitor tooltips (image previews and request stacks), as well as the font preview. I tested all of those, but you're welcome to double check!

> 
> Note that while testing event details I've seen various weird exception.
> But I don't think it is related to this patch:
>   TypeError: can't convert undefined to object:
> EventTooltip.prototype.destroy@resource://gre/modules/commonjs/toolkit/
> loader.js ->
> resource://devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:293:
> 24
> It happens when clicking outside the tooltip to dismiss it. It is
> intermittent and doesn't always throws...
> 

Indeed thanks for the heads up. There is a race condition which can lead to erase the tooltip content before having destroyed the previous tooltip. Comes from two reasons: 
- calling "hide" on a Tooltip not yet visible is not triggering the "hidden" event (early return in the hide() method)
- in markup.js, on event bubble click handler, we hide() the tooltip, then fetch the event info asynchronously and then show() the tooltip. We should hide() right before calling show() otherwise we are exposed to race conditions

Opened a separate bug here: Bug 1284259.

	Alexandre Poirot [:ochameau]
	Comment 22 • 8 years ago

https://reviewboard.mozilla.org/r/60360/#review58828

::: devtools/client/shared/widgets/HTMLTooltip.js:55
(Diff revision 8)
>   * prevents this.
>   *
>   * @param {DOMRect} anchorRect
>   *        Bounding rectangle for the anchor, relative to the tooltip document.
> - * @param {DOMRect} docRect
> - *        Bounding rectange for the tooltip document owner.
> + * @param {DOMRect} viewportRect
> + *        Viewport dimensions object {width, height}.

this comment need updates. Makes it so that it helps understanding the following lines:
  anchorTop -= viewportRect.top;
  top += viewportRect.top;

	Julian Descottes [:jdescottes] Assignee
	Comment 23 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 24 • 8 years ago

Comment on attachment 8767671 [details]
Bug 1267403 - HTMLTooltip tests: destroy tooltips at the end of tests;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 25 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

ochemeau: Switching to r? so that you can have another look at the comments. Let me know where you feel more details are needed.

Thanks!

	Alexandre Poirot [:ochameau]
	Comment 26 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

https://reviewboard.mozilla.org/r/60360/#review59342

Ship it!

::: devtools/client/shared/widgets/HTMLTooltip.js:74
(Diff revision 9)
>  
>    let {top: anchorTop, height: anchorHeight} = anchorRect;
> -  let {bottom: docBottom} = docRect;
>  
> +  // Translate to the available viewport space before calculating dimensions and position.
> +  anchorTop -= viewportRect.top;

It might be clearer to apply this `-viewPortRect.top` only to availableTop:
`let availableTop = anchorTop - viewportRect.top;`
And then replace anchorTop occurences by `availableTop`.
It think it makes sense to do that, right?
If you think it doesn't help following this code, let it as-is.

::: devtools/client/shared/widgets/HTMLTooltip.js:99
(Diff revision 9)
>    height = Math.floor(height);
>  
>    // Calculate TOP.
>    let top = pos === TOP ? anchorTop - height - offset : anchorTop + anchorHeight + offset;
>  
> +  // Translate back to a coordinate that can be used for tooltip positioning.

Translate back to absolute coordinates by re-including viewport top margin.

	Julian Descottes [:jdescottes] Assignee
	Comment 27 • 8 years ago

Comment on attachment 8764562 [details]
Bug 1267403 - HTMLTooltip: add useXulWrapper option when displayed in a XUL document;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60360/diff/9-10/

	Julian Descottes [:jdescottes] Assignee
	Comment 28 • 8 years ago

Comment on attachment 8767671 [details]
Bug 1267403 - HTMLTooltip tests: destroy tooltips at the end of tests;

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

	Pulsebot
	Comment 29 • 8 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/eb169031125a
HTMLTooltip: add useXulWrapper option when displayed in a XUL document;r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/5dc96caed881
HTMLTooltip tests: destroy tooltips at the end of tests;r=ochameau

	Julian Descottes [:jdescottes] Assignee
	Comment 30 • 8 years ago

https://reviewboard.mozilla.org/r/60360/#review59342

> It might be clearer to apply this `-viewPortRect.top` only to availableTop:
> `let availableTop = anchorTop - viewportRect.top;`
> And then replace anchorTop occurences by `availableTop`.
> It think it makes sense to do that, right?
> If you think it doesn't help following this code, let it as-is.

I kept my initial version in the end, because it makes the vertical and horizontal calculations more similar. 
But thanks for reviewing this in depth, really appreciate it.

> Translate back to absolute coordinates by re-including viewport top margin.

Better!

	Carsten Book [:Tomcat]
	Comment 31 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/eb169031125a
https://hg.mozilla.org/mozilla-central/rev/5dc96caed881

	Julian Descottes [:jdescottes] Assignee
	Comment 32 • 8 years ago

Some details for verifying this bug.

Before testing, make sure that Bug 1285206 has been integrated in the version you are testing (as it enables the XUL panel wrapper implemented here for a handful of tooltips). Looks like it is not in nightly at the moment, might need to wait for the next version.

The XUL panel is now used for the following tooltips:
- image and font preview in the rule view
- image preview in the markup view
- colorpicker, bezier and filter editors in the rule view (Bug 1267414, should be in nightly)
- mdn docs in the rule view (Bug 1283454, not in nightly yet)

What should be tested here:
- simple non regression on the tooltips mentioned above, particularly checking that the tooltip position is correct, with the arrow pointing to the anchor
- tooltips should now be able to extend outside of the firefox window -> verifying that Bug 1279189 has been fixed would be a good test for instance. All the tooltips mentioned above should be displayed "full size" and no longer be resized to fit the toolbox dimensions

	Ada [:adalucinet]
	Comment 33 • 8 years ago

Verified fixed with latest Nightly 50.0a1, across platforms [1], using the guidance from comment 32.

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