Julian Descottes [:jdescottes] Assignee
	Description • 8 years ago

When migrating the EventDetails tooltip to use the HTMLTooltip, I changed the HTMLTooltip behaviour in order to make the preferred height specified by the user act a max-height.

This is useful when the height of the tooltip might vary and you want to allow the tooltip content to expand dynamically as much as possible.

However when you are using fixed height tooltips (such as the ruleview editors and the MDN docs previews) it is easier if the height of the tooltip is set.

To change this behaviour we have two options:

- using {height: Infinity} makes the preferred height act as max-height, any other value acts as a static height
- add an additional parameter to the options object expected by HTMLTooltip::setContent {variableHeight:true/false}

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 8 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 8 years ago

Comment on attachment 8767956 [details]
Bug 1284461 - HTMLTooltip: use variable height only if height==Infinity;

Switching to feedback for now to start the discussion around the best solution here.

	Brian Grinstead [:bgrins]
	Comment 3 • 8 years ago

https://reviewboard.mozilla.org/r/62358/#review59118

I like the height: Infinity solution since it avoids adding a new option. So I guess if a tooltip wants to do 'min-height' it should be done with CSS on the container?

::: devtools/client/themes/tooltips.css:138
(Diff revision 1)
>  }
>  
> +/* Tooltip : flexible height styles */
> +
> +.tooltip-flexible-height .tooltip-panel {
> +  /* In flexible  mode the tooltip panel should only grow according to its content. */

Nit: extra space

::: devtools/client/themes/tooltips.css:143
(Diff revision 1)
> +  /* In flexible  mode the tooltip panel should only grow according to its content. */
> +  flex-grow: 0;
> +}
> +
> +.tooltip-flexible-height .tooltip-filler {
> +  /* In flexible  mode the filler should grow as much as possible. */

Ditto

	Brian Grinstead [:bgrins]
	Comment 4 • 8 years ago

Consider that an r+, if you are happy with the patch also

	Julian Descottes [:jdescottes] Assignee
	Comment 5 • 8 years ago

Comment on attachment 8767956 [details]
Bug 1284461 - HTMLTooltip: use variable height only if height==Infinity;

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

	Brian Grinstead [:bgrins]
	Comment 6 • 8 years ago

Comment on attachment 8767956 [details]
Bug 1284461 - HTMLTooltip: use variable height only if height==Infinity;

https://reviewboard.mozilla.org/r/62358/#review59152

	Julian Descottes [:jdescottes] Assignee
	Comment 7 • 8 years ago

Thanks for the review! Happy with this approach too, so let's go ahead. Fixed the extra spaces and rebased.

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c8a2ef35bda353cf70f9fc0c55a792d7361253

	Julian Descottes [:jdescottes] Assignee
	Comment 8 • 8 years ago

This bug will not have any visible impact, setting qe-verify-.
Blocking on Bug 1267414.

	Julian Descottes [:jdescottes] Assignee
	Comment 9 • 8 years ago

Comment on attachment 8767956 [details]
Bug 1284461 - HTMLTooltip: use variable height only if height==Infinity;

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

	Julian Descottes [:jdescottes] Assignee
	Comment 10 • 8 years ago

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=215a6d072c7c&selectedJob=23663369 

Landing.

	Pulsebot
	Comment 11 • 8 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/489d075c7676
HTMLTooltip: use variable height only if height==Infinity;r=bgrins

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/489d075c7676