Closed Bug 1283454 Opened 4 years ago Closed 4 years ago

Migrate MDN Docs tooltip to use HTML Tooltip

Categories

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

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

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.
Flags: qe-verify+
Whiteboard: [devtools-html] [triage]
QA Contact: alexandra.lucinet
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
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
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 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/
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 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/
(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?
Attachment #8769764 - Flags: review?(poirot.alex) → review+
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>
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.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/395c5a486954
migrate MDN docs tooltip to use HTML;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/395c5a486954
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1287438
Depends on: 1287464
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.