1267401 - Create "arrow" tooltip style for HTML tooltips

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

XUL panel support several display types. Devtools only use the default one or arrow.

The goal of this bug is to implement the arrow type tooltip based on the default HTML tooltip implemented in Bug 1259834.

The initial style should be similar to the current XUL arrow tooltip style.
The arrow should always point to the anchor element.

Comment 1

[Julian Descottes [:jdescottes]]

Note: the position of the arrow should remain correct in RTL setups (see https://bugzilla.mozilla.org/show_bug.cgi?id=1178145 & https://bug1178145.bmoattachments.org/attachment.cgi?id=8627067)

Comment 2

[Julian Descottes [:jdescottes]]

Helen: I think you have mockups for new tooltips designs? 
Could you share them here so that we can have an idea of the gap between the current "arrow" XUL panel's style and the new ones?

Comment 3

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

Attachment: popup-spec-sheet.png

Here's the spec—the bug where this info was originally posted was Bug 1258365.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

Rename document -> doc for consistency and parent to panel
for test compatibility.

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

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

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

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

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

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

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

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: html-tooltips-demo.gif

I initially implemented the new tooltip design from Helen. Then I had both the old and new designs supported as 2 different tooltip styles. 

But I think we should first focus on landing these changes and then we can think about using the new style. 

Except for the limitations linked to the HTMLTooltip implementation (the tooltip has to fit in the toolbox), I tried to remain as close as possible to the existing UI.

There is an ongoing try build with the patches from this Bug and from Bug 1266448. Feel free to retrieve the builds and to try out the image preview tooltips in the markup view.

Comment 9

[Julian Descottes [:jdescottes]]

try build mentioned above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51eee685a85c

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

https://reviewboard.mozilla.org/r/52812/#review50158

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

https://reviewboard.mozilla.org/r/52814/#review50162

Seems fine - why is this needed?

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8752838 [details]
> MozReview Request: Bug 1267401 - part2: include common.css in html tooltip
> tests;r=bgrins
> 
> https://reviewboard.mozilla.org/r/52814/#review50162
> 
> Seems fine - why is this needed?

I could have merged this with the part3. I started using a classname in order to switch the visibility of the tooltip. Originally this was because I was experimenting with tooltips relying on display:flex rather than display:block and it was more convenient to do this in CSS.

Comment 13

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/52816/#review51262

I'm getting this error when running the tests locally: 21 INFO TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_html_tooltip_arrow-02.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/shared/test/helper_html_tooltip.js:57 - TypeError: document is undefined

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

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

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

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

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

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

Comment 17

[Julian Descottes [:jdescottes]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> https://reviewboard.mozilla.org/r/52816/#review51262
> 
> I'm getting this error when running the tests locally: 21 INFO
> TEST-UNEXPECTED-FAIL |
> devtools/client/shared/test/browser_html_tooltip_arrow-02.js | Uncaught
> exception - at
> chrome://mochitests/content/browser/devtools/client/shared/test/
> helper_html_tooltip.js:57 - TypeError: document is undefined

Indeed, needed rebasing and fixing. Updated the review requests accordingly.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

https://reviewboard.mozilla.org/r/52816/#review51514

Nice job, this looks good

::: devtools/client/shared/test/browser_html_tooltip_arrow-01.js:31
(Diff revision 3)
> +  <window class="theme-light"
> +          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +          xmlns:html="http://www.w3.org/1999/xhtml"
> +          title="Tooltip test">
> +    <vbox flex="1" style="position: relative">
> +      ${getAnchor("top: 0; left: 0;")}

I noticed the arrow doesn't quite make it to the anchor when it's all the way to the right or left.  I guess that's the blocked by viewport case.

It's not a big deal - we could work on alternate styling for that case later on if needed

Comment 19

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/52816/#review51514

Thanks for the review, will land after resubmitting to try!

> I noticed the arrow doesn't quite make it to the anchor when it's all the way to the right or left.  I guess that's the blocked by viewport case.
> 
> It's not a big deal - we could work on alternate styling for that case later on if needed

I think this limitation is acceptable for now.

As you said, we need to use a different arrow style if we want to match anchors very close to the edges of the viewport.
I will open a follow up for this.

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

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

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

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

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

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

Comment 23

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/52816/#review51514

> I think this limitation is acceptable for now.
> 
> As you said, we need to use a different arrow style if we want to match anchors very close to the edges of the viewport.
> I will open a follow up for this.

Opened Bug 1275408, dropping the issue here.

Comment 24

[Julian Descottes [:jdescottes]]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=09bfb15b3305

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f8d7e7040b88
https://hg.mozilla.org/integration/fx-team/rev/5a9b084f41a5
https://hg.mozilla.org/integration/fx-team/rev/b060a42710df

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8d7e7040b88
https://hg.mozilla.org/mozilla-central/rev/5a9b084f41a5
https://hg.mozilla.org/mozilla-central/rev/b060a42710df

Comment 27

[Ada [:adalucinet]]

The tooltip is properly displayed with latest Nightly 49.0a1 en-US and ar (RTL) builds, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.10.5.