1472942 - Drop HTMLTooltip.setContent and provide another means for specifying content sizing

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

See bug 1461522 comment 106.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

For example, in the image tooltip of the inspector, creating tooltip and showing tooltip is different timing.[1] 
This component will specify the width and height from calculated result of the image, so this component might need to have the width and height information into its object if we move these parameters to show() function.

[1] https://dxr.mozilla.org/mozilla-central/rev/9c13dbdf4cc9baf98881b4e2374363587fb017b7/devtools/client/inspector/markup/views/element-container.js#130,204

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Many tooltips expect to be enable setting the child panel content before showing the panel. Especially, the tooltip which is using the TooltipToggle.js[1] will create the content before calling the HTMLTooltip.show().

[1] https://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/tooltip/TooltipToggle.js

WIP patch:
https://hg.mozilla.org/try/rev/25303070e04f02dd5b3166884a341b9ea7353355

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Many tooltips expect to be enable setting the child panel content before
> showing the panel. Especially, the tooltip which is using the
> TooltipToggle.js[1] will create the content before calling the
> HTMLTooltip.show().
> 
> [1]
> https://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/
> tooltip/TooltipToggle.js
> 
> WIP patch:
> https://hg.mozilla.org/try/rev/25303070e04f02dd5b3166884a341b9ea7353355

Oops, I should change the tooltips-overlay as well.
Patch: https://hg.mozilla.org/try/rev/03ea4a56e39b7abae3ab91ad55c00ecb7b7fc970#l31.44

Brian,
I think that we might need to leave this setContent code for legacy implementation. So we had better to add the parameter of show function, then leaving the setContent. How do you think about this?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

What do you think of Julian's original suggestion:

> And maybe file a follow up to make this the only supported behavior of the tooltip: every caller would set the content via tooltip.panel.appendChild and would set { width, height } either via another method or via additional parameters in show()?

i.e. add a separate method for specifying the width/height?

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Thanks.

I add tootlip.panel.appendChild each caller, (note that some tooltip will reuse the tooltip panel, so we should remove the content when showing the panel) and I add the width/height parameter into HTMLTooltip.show(). An ImageTooltip calculate an own content width and height when creating the content, so this width/height should be propagated to HTMLTooltip.show().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79c75d8f600133879fbf1607f05665cb33dbbce&group_state=expanded

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: bug1472942_use_appendchild.patch

This patch will remove the HTMLTooltip.setContent function, and it will add width/height parameter into HTMLTooltip.show function.

The image tooltip might be hard to use this API a bit since the image tooltip calculates the content size when setting own panel content. However, each caller of image tooltip can propagate the tooltip content size to HTMLTooltip.show().  (For detail, see the TooltipToggle.js of this patch.)

So I think that this API is far from hard to use.

Birtles,
How do you feel this modification of API?

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 9012083 [details] [diff] [review]
bug1472942_use_appendchild.patch

Review of attachment 9012083 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed, it's probably simpler and safer to just add a method for setting the size (e.g. repurposing setContent as setSize or something like that).

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1472942 - Part 1. Remove unused paramter of HTMLTooltip.show from SwatchBaseEditorTooltip. r?jdescottes

SwatchBaseEditorTooltip specify the "topcentor bototmleft" to second parameter
of HTMLTooltip.show(). However, this parameter will ignore since show() function
require the second parameter as the object, not the string.

Bug 1307481 comment 42 has pointed out this parameter.

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1472942 - Part 2. Add setContentSize() instead of setContent() in HTMLTooltip. r?birtles

This patch will remove setContest(), and change the following things instead of this:

 * Use HTMLTooltip.panel.appendChild() instead of HTMLTooltip.setContent().
 * Add HTMLTooltip.setContetnSize() to specify the panel size if need this.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 9012476 [details]
Bug 1472942 - Part 2. Add setContentSize() instead of setContent() in HTMLTooltip. r?birtles

Brian Birtles (:birtles) has approved the revision.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 9012475 [details]
Bug 1472942 - Part 1. Remove unused paramter of HTMLTooltip.show from SwatchBaseEditorTooltip. r?jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ba6db4fd39b
Part 1. Remove unused paramter of HTMLTooltip.show from SwatchBaseEditorTooltip. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2430fbe8fef6
Part 2. Add setContentSize() instead of setContent() in HTMLTooltip. r=birtles

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4ba6db4fd39b
https://hg.mozilla.org/mozilla-central/rev/2430fbe8fef6