1293211 - Fix devtools tooltip tests to support async XUL panel show/hide

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

Devtools tooltip tests sometimes implicitly relied on the openPopup()/hidePopup() methods of the XUL panel being synchronous.

Bug 1206133 will make the open/hide methods asynchronous, so devtools tests need to be updated to wait for the proper events.

Try on top of current fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f19e0c1744 

Try on top of the async patch from Bug 1206133 : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7f7791db7e74638315450e82bfe5891958aadfa

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1293211 - wait for xul wrapper popupshown event in HTMLTooltip;

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1293211 - fire ready event in SwatchBasedEditorTooltips after widget initialization;

Now that showing a XUL panel is asynchronous, tests need to be able to wait for the complete
initialization of the editor tooltips. Waiting for the tooltip 'shown' event is not enough
here because tooltips are also waiting for this event to start initializing their widgets.

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1293211 - wait for EditorTooltip ready event in tests;

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

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1293211 - hide all tooltips before tests ending;

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8778880 [details]
Bug 1293211 - wait for xul wrapper popupshown event in HTMLTooltip;

https://reviewboard.mozilla.org/r/70016/#review67366

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8778881 [details]
Bug 1293211 - fire ready event in SwatchBasedEditorTooltips after widget initialization;

https://reviewboard.mozilla.org/r/70018/#review67370

::: devtools/client/shared/widgets/Tooltip.js:637
(Diff revision 1)
>          }
>        });
> +
> +      return onShown;
>      }
> +    return null;

Should this return Promise.resolve() instead of null?  I know this is being used in Tasks so they handle this fine with `yield` but returning a promise is what the comment above indicates should happen

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8778882 [details]
Bug 1293211 - wait for EditorTooltip ready event in tests;

https://reviewboard.mozilla.org/r/70020/#review67384

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8778883 [details]
Bug 1293211 - hide all tooltips before tests ending;

https://reviewboard.mozilla.org/r/70022/#review67386

::: devtools/client/inspector/rules/test/head.js:183
(Diff revision 1)
>   * Use this function to close the tooltip and make sure the test waits for the
>   * ruleview-changed event.
> - * @param {Tooltip} tooltip
> + * @param {SwatchColorPickerTooltip} colorpicker tooltip
>   * @param {CSSRuleView} view
>   */
> -function* hideTooltipAndWaitForRuleViewChanged(tooltip, view) {
> +function* hideTooltipAndWaitForRuleViewChanged(colorpicker, view) {

I see this used for more than just colorpickers: https://dxr.mozilla.org/mozilla-central/search?q=hideTooltipAndWaitForRuleViewChanged&redirect=true.  Why should this be specific to color pickers?

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8778880 [details]
Bug 1293211 - wait for xul wrapper popupshown event in HTMLTooltip;

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

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8778881 [details]
Bug 1293211 - fire ready event in SwatchBasedEditorTooltips after widget initialization;

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

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8778882 [details]
Bug 1293211 - wait for EditorTooltip ready event in tests;

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

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8778883 [details]
Bug 1293211 - hide all tooltips before tests ending;

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

Comment 13

[Julian Descottes [:jdescottes]]

Thanks for the reviews! Updated the patches based on your comments and pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd0e7a7aa3b

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8778883 [details]
Bug 1293211 - hide all tooltips before tests ending;

https://reviewboard.mozilla.org/r/70022/#review67590

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b850bd6aa5a3
part1: wait for xul wrapper popupshown event in HTMLTooltip;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/a98ce7344c9a
part2: fire ready event in SwatchBasedEditorTooltips after widget initialization;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/05ec8a857037
part3: wait for EditorTooltip ready event in tests;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/a3fb4eb11fcf
part4: ide all tooltips before tests ending;r=bgrins

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b850bd6aa5a3
https://hg.mozilla.org/mozilla-central/rev/a98ce7344c9a
https://hg.mozilla.org/mozilla-central/rev/05ec8a857037
https://hg.mozilla.org/mozilla-central/rev/a3fb4eb11fcf