1283454 - Migrate MDN Docs tooltip to use HTML Tooltip

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Julian Descottes [:jdescottes]]

The goal of this bug is to migrate the tooltip used to display the MDN documentation to use a HTML Tooltip instance.

The current implementation can be found in Tooltip.js, class CssDocsTooltip. It uses an iframe to display its content, so the migration path will probably be similar to what is being done for the ruleview editor tooltips: Bug 1267414.

Testing and verification: 
The MDN docs can be tested by going to the ruleview, right clicking on a property name and selecting "Show MDN Docs". This is a technical migration, so the goal is to ensure no regression was introduced for this feature.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1283454 - migrate MDN docs tooltip to use HTML;

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

Comment 2

[Julian Descottes [:jdescottes]]

Alex: feel free to redirect if you feel you won't have time to review this before going on vacations.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=989887603b02766af401cf712ca7d9c148236aa5

Comment 3

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/62346/#review60366

Looks good overall.
Tested it quickly and haven't found anything different except the link focus.
There is a light border around the Visit MDN Page.
I don't remember why but I also hit that forced focus while converting the other tooltips...

Also, the footer looks slightly bigger, isn't it? Could you verify?

::: devtools/client/shared/widgets/mdn-docs.css:56
(Diff revision 1)
> +  opacity: 1;
> +}
> +
> +.devtools-throbber > * {
> +  opacity: 0;
> +}

Why all these new styles?
from `.mdn-property-info::before` to `.devtools-throbbser > *`

::: devtools/client/shared/widgets/tooltip/CssDocsTooltip.js:20
(Diff revision 1)
> +const TOOLTIP_HEIGHT = 308;
> +
> +/**
> + * Tooltip for displaying docs for CSS properties from MDN.
> + *
> + * @param {XULDocument} doc

Doc is outdated.

::: devtools/client/shared/widgets/tooltip/CssDocsTooltip.js:78
(Diff revision 1)
> +    /**
> +   * Set the content of this tooltip to the MDN docs widget.
> +   *
> +   * This is called when the tooltip is first constructed.
> +   *
> +   * @return {promise} A promise which is resolved with an MdnDocsWidget.

Doesn't look like a Promise anymore

::: devtools/client/shared/widgets/tooltip/CssDocsTooltip.js:82
(Diff revision 1)
> +   *
> +   * @return {promise} A promise which is resolved with an MdnDocsWidget.
> +   *
> +   * It loads the tooltip's structure from a separate XHTML file
> +   * into an iframe. When the iframe is loaded it constructs
> +   * an MdnDocsWidget and passes that into resolve.

This also look outdated.

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8769764 [details]
Bug 1283454 - migrate MDN docs tooltip to use HTML;

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

Comment 5

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/62346/#review60366

The MdnDocsTooltip is using autofocus: "noAutoFocus: false" for the old Tooltip implementation, "autofocus: true" for the HTML tooltip. 
Before, the iframe was getting the focus. This is no longer possible, so the first focusable element gets the focus (the link here).

On OSX the footer is the same size as before, I'll check Linux.

> Why all these new styles?
> from `.mdn-property-info::before` to `.devtools-throbbser > *`

The previous implementation was not working out of the box because tooltip content was set before displaying the tooltip. The opacity transition was not taking place. 

After reading the code I realized it had a strange behavior: it was simply fading out the loading animation after 400ms, didn't matter if the content had loaded or not. I wanted to fix this and came up with the CSS you reviewed. I reverted this in order to simply migrate the existing implementation. I'll submit a separate patch to improve the loading order.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8769764 [details]
Bug 1283454 - migrate MDN docs tooltip to use HTML;

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

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #5)
> https://reviewboard.mozilla.org/r/62346/#review6036
> 
> On OSX the footer is the same size as before, I'll check Linux.
> 

Just tested Ubuntu, and the footer is the same size as before for me. Can you check on your side again and maybe add a screenshot?

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8769764 [details]
Bug 1283454 - migrate MDN docs tooltip to use HTML;

https://reviewboard.mozilla.org/r/62346/#review60972

<p>(In reply to Julian Descottes [:jdescottes] from comment #5)</p>
<blockquote>
<p></p>
<p>The MdnDocsTooltip is using autofocus: "noAutoFocus: false" for the old<br />
Tooltip implementation, "autofocus: true" for the HTML tooltip. <br />
Before, the iframe was getting the focus. This is no longer possible, so the<br />
first focusable element gets the focus (the link here).</p>
</blockquote>
<p>I imagine we need the focus for the key shortcuts?<br />
This focus border looks bad to me. For it may be a positive thing for accessiblity.</p>
<p></p>
<p></p>
<blockquote>
<p>On OSX the footer is the same size as before, I'll check Linux.</p>
</blockquote>
<p>I verified and it is just the border around the Visit MDN page which makes the footer looks bigger!</p>

Comment 9

[Julian Descottes [:jdescottes]]

Thanks for checking! Regarding the border I think we have to live with it, otherwise the link will not be accessible easily via keyboard navigation, which would be bad for accessibility.

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27777a2ee412 . Landing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/395c5a486954
migrate MDN docs tooltip to use HTML;r=ochameau

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/395c5a486954

Comment 12

[Ada [:adalucinet]]

Found 2 regressions [1] while verifying this fix with latest Nightly 50.0a1, across platforms [2].
Since both reports are filed separately, marking here accordingly.

[1] bug 1287438 & bug 1287464
[2] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit