Closed Bug 1472942 Opened Last year Closed Last year

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

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Priority: -- → P3
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
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
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
(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?
Flags: needinfo?(bbirtles)
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?
Flags: needinfo?(bbirtles)
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
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?
Attachment #9012083 - Flags: feedback?(bbirtles)
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).
Attachment #9012083 - Flags: feedback?(bbirtles)
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.
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 on attachment 9012476 [details]
Bug 1472942 - Part 2. Add setContentSize() instead of setContent() in HTMLTooltip. r?birtles

Brian Birtles (:birtles) has approved the revision.
Attachment #9012476 - Flags: review+
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.
Attachment #9012475 - Flags: review+
Pushed by mantaroh@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/4ba6db4fd39b
https://hg.mozilla.org/mozilla-central/rev/2430fbe8fef6
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.