From ed86c822c065faeb39705d159dfd240c4e9926bc Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Fri, 28 Feb 2020 15:08:22 +0000 Subject: [PATCH] Bug 1616847 - When xulWrapper is visible move it instead of trying to show it again. r=jdescottes. If the XUL wrapper was opened and a consumer called show again, the panel wouldn't update its position. This can be fixed by checking the state of the element, and if it's open move the panel using `_moveXulWrapperTo`. With this change in place, we can remove the updateContainerBounds methods, as it has the same arguments and is only used in one place. Differential Revision: https://phabricator.services.mozilla.com/D63488 --HG-- extra : moz-landing-system : lando --- .../shared/components/menu/MenuButton.js | 2 +- .../test/browser_html_tooltip_resize.js | 2 +- .../client/shared/test/helper_html_tooltip.js | 11 ++- .../shared/widgets/tooltip/HTMLTooltip.js | 72 ++++++++++--------- 4 files changed, 45 insertions(+), 42 deletions(-) diff --git a/devtools/client/shared/components/menu/MenuButton.js b/devtools/client/shared/components/menu/MenuButton.js index a6ceb582bd6f..ed25f733d20e 100644 --- a/devtools/client/shared/components/menu/MenuButton.js +++ b/devtools/client/shared/components/menu/MenuButton.js @@ -227,7 +227,7 @@ class MenuButton extends PureComponent { return; } - this.tooltip.updateContainerBounds(this.buttonRef.current, { + this.tooltip.show(this.buttonRef.current, { position: this.props.menuPosition, y: this.props.menuOffset, }); diff --git a/devtools/client/shared/test/browser_html_tooltip_resize.js b/devtools/client/shared/test/browser_html_tooltip_resize.js index 9cb0ede3743a..c969100ced74 100644 --- a/devtools/client/shared/test/browser_html_tooltip_resize.js +++ b/devtools/client/shared/test/browser_html_tooltip_resize.js @@ -48,7 +48,7 @@ add_task(async function() { // Resize the content div.style.cssText = "width: 200px; height: 30px"; - tooltip.updateContainerBounds(box1, { position: "top" }); + tooltip.show(box1, { position: "top" }); // The panel should have moved 100px to the left and 10px down const updatedPanelBounds = tooltip.panel diff --git a/devtools/client/shared/test/helper_html_tooltip.js b/devtools/client/shared/test/helper_html_tooltip.js index 4b8601131711..d5e038e00547 100644 --- a/devtools/client/shared/test/helper_html_tooltip.js +++ b/devtools/client/shared/test/helper_html_tooltip.js @@ -9,21 +9,18 @@ */ /** - * Display an existing HTMLTooltip on an anchor. After the tooltip "shown" - * event has been fired a reflow will be triggered. + * Display an existing HTMLTooltip on an anchor and properly wait for the popup to be + * repainted. * * @param {HTMLTooltip} tooltip * The tooltip instance to display * @param {Node} anchor * The anchor that should be used to display the tooltip * @param {Object} see HTMLTooltip:show documentation - * @return {Promise} promise that resolves when "shown" has been fired, reflow - * and repaint done. + * @return {Promise} promise that resolves when reflow and repaint are done. */ async function showTooltip(tooltip, anchor, { position, x, y } = {}) { - const onShown = tooltip.once("shown"); - tooltip.show(anchor, { position, x, y }); - await onShown; + await tooltip.show(anchor, { position, x, y }); return waitForReflow(tooltip); } diff --git a/devtools/client/shared/widgets/tooltip/HTMLTooltip.js b/devtools/client/shared/widgets/tooltip/HTMLTooltip.js index ac57cc02b754..1611e4055ada 100644 --- a/devtools/client/shared/widgets/tooltip/HTMLTooltip.js +++ b/devtools/client/shared/widgets/tooltip/HTMLTooltip.js @@ -464,8 +464,9 @@ HTMLTooltip.prototype = { }, /** - * Show the tooltip next to the provided anchor element. A preferred position - * can be set. The event "shown" will be fired after the tooltip is displayed. + * Show the tooltip next to the provided anchor element, or update the tooltip position + * if it was already visible. A preferred position can be set. + * The event "shown" will be fired after the tooltip is displayed. * * @param {Element} anchor * The reference element with which the tooltip should be aligned @@ -483,27 +484,50 @@ HTMLTooltip.prototype = { */ async show(anchor, options) { const { left, top } = this._updateContainerBounds(anchor, options); + const isTooltipVisible = this.isVisible(); if (this.useXulWrapper) { - await this._showXulWrapperAt(left, top); + if (!isTooltipVisible) { + await this._showXulWrapperAt(left, top); + } else { + this._moveXulWrapperTo(left, top); + } } else { this.container.style.left = left + "px"; this.container.style.top = top + "px"; } + if (isTooltipVisible) { + return; + } + this.container.classList.add("tooltip-visible"); // Keep a pointer on the focused element to refocus it when hiding the tooltip. this._focusedElement = this.doc.activeElement; - this.doc.defaultView.clearTimeout(this.attachEventsTimer); - this.attachEventsTimer = this.doc.defaultView.setTimeout(() => { - // Update the top window reference each time in case the host changes. - this.topWindow = this._getTopWindow(); - this.topWindow.addEventListener("click", this._onClick, true); - this.topWindow.addEventListener("mouseup", this._onMouseup, true); - this.emit("shown"); - }, 0); + if (this.doc.defaultView) { + if (this.attachEventsTimer) { + this.doc.defaultView.clearTimeout(this.attachEventsTimer); + } + + // On Windows and Linux, if the tooltip is shown on mousedown/click (which is the + // case for the MenuButton component for example), attaching the events listeners + // on the window right away would trigger the callbacks; which means the tooltip + // would be instantly hidden. To prevent such thing, the event listeners are set + // on the next tick. + await new Promise(resolve => { + this.attachEventsTimer = this.doc.defaultView.setTimeout(() => { + // Update the top window reference each time in case the host changes. + this.topWindow = this._getTopWindow(); + this.topWindow.addEventListener("click", this._onClick, true); + this.topWindow.addEventListener("mouseup", this._onMouseup, true); + resolve(); + }, 0); + }); + } + + this.emit("shown"); }, startTogglingOnHover(baseNode, targetNodeCb, options) { @@ -514,27 +538,6 @@ HTMLTooltip.prototype = { this.toggle.stop(); }, - /** - * Recalculate the dimensions and position of the tooltip in response to - * changes to its content. - * - * Parameters are identical to show(). - */ - updateContainerBounds(anchor, options) { - if (!this.isVisible()) { - return; - } - - const { left, top } = this._updateContainerBounds(anchor, options); - - if (this.useXulWrapper) { - this._moveXulWrapperTo(left, top); - } else { - this.container.style.left = left + "px"; - this.container.style.top = top + "px"; - } - }, - _updateContainerBounds(anchor, { position, x = 0, y = 0 } = {}) { // Get anchor geometry let anchorRect = getRelativeRect(anchor, this.doc); @@ -794,7 +797,10 @@ HTMLTooltip.prototype = { return; } - this.doc.defaultView.clearTimeout(this.attachEventsTimer); + if (this.doc && this.doc.defaultView) { + this.doc.defaultView.clearTimeout(this.attachEventsTimer); + } + // If the tooltip is hidden from a mouseup event, wait for a potential click event // to be consumed before removing event listeners. if (fromMouseup) {