1248506 - Use calc() to avoid hard-coded height of Synced Tabs panel

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

In bug 1236372, the SyncedTabs CSS has a work-around for bug 1224412, in which we use a hard-coded min-height declaration to prevent scrollbars in the panel in some cases. In bug 1236372 comment 7, Gijs suggests using calc() to avoid hardcoding px - this bug is to track doing that (but even better would be for bug 1224412 to be fixed so we can remove the workaround completely.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Gijs, if I'm understanding correctly, this would require duplicating the 180px right? Is that necessarily a good idea?

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Thom Chiovoloni [:tcsc] from comment #1)
> Gijs, if I'm understanding correctly, this would require duplicating the
> 180px right? Is that necessarily a good idea?

180px is the width, and we're dealing with the height. Is the image in question actually square?

Anyway, you don't need to duplicate the image size - you can use a CSS variable to avoid doing that.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Ah, yeah, that's right (unfortunately, I didn't look as closely as I should have). The height is 157.5px, and specifying the image size that way, and using it in the calc (with var) would work fine.

So, just to check, you're thinking something like `calc(var(--panel-ui-sync-illustration-height) + 19em)`? I'm not positive this has a huge benefit (other than moving the image size out, which is nice, I guess), since the 19em has most of the same problems as the 33em (e.g. it's dependent on the number of lines of text, is larger than necessary for some languages, etc). 

Computing it based on the number of lines seems like it would be more ideal to me, but it has the downside of requiring the line height and margin sizes be specified in the calc...

Or am I totally confused about this? If you think something like the calc expression above is what we want, I'll absolutely make a patch for it, I'm just not certain it *is* in fact what you mean.

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Thom Chiovoloni [:tcsc] from comment #3)
> Ah, yeah, that's right (unfortunately, I didn't look as closely as I should
> have). The height is 157.5px, and specifying the image size that way, and
> using it in the calc (with var) would work fine.
> 
> So, just to check, you're thinking something like
> `calc(var(--panel-ui-sync-illustration-height) + 19em)`? I'm not positive
> this has a huge benefit (other than moving the image size out, which is
> nice, I guess), since the 19em has most of the same problems as the 33em
> (e.g. it's dependent on the number of lines of text, is larger than
> necessary for some languages, etc). 

<snip>

> Or am I totally confused about this? If you think something like the calc
> expression above is what we want, I'll absolutely make a patch for it, I'm
> just not certain it *is* in fact what you mean.

Not totally confused, but I think you're missing something (or maybe I am). The benefit of using effectively something like:

calc(157.5px + 19em);

is that it represents the boxes that in the container: one fixed size in pixels, one (kind of) fixed size on text.

I'm not really trying to solve the "German strings are really long" problem. I'm trying to solve the "1em might be different on different machines" problem.

Specifically, if you were to test a German build and used the Windows font settings to make text quite small or quite large, we will mis-size this panel. calc() with a combination of the px-fixed and em-fixed values separately helps avoid that problem, where both a fixed-in-px value (doesn't work with large text) and a fixed-in-em value (doesn't work with small text) would break.

Does that help?

(FWIW, it's true that we don't know the number of lines of text, and can't do without some help from JS or something... I'd like to avoid going down that rabbithole for now unless it becomes necessary. One thing we've done in the past is making the size in question be a localized string that localizers can adjust for their locale, but to a certain degree it's not very fair to shift the burden that way, and it's not often possible for localizers to test all the platforms we ship on...)

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

> I'm not really trying to solve the "German strings are really long" 
> problem. I'm trying to solve the "1em might be different on different 
> machines" problem.

Ah, this makes more sense. Although, since the margin sizes are specified in px it's going to be a little more complex than just 157.5px + 19em, but it still should be doable.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: MozReview Request: Bug 1248506 - Replace hardcoded height of synced tab panel with calc expression based on content size r?gijs

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

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8738617 [details]
MozReview Request: Bug 1248506 - Replace hardcoded height of synced tab panel with calc expression based on content size r?gijs

https://reviewboard.mozilla.org/r/44611/#review41321

r=me with the extra comment. Your choice about the structure of the calc here, see below.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css:755
(Diff revision 1)
> +     paddings of the items contained in the panel (that is, of
> +     .PanelUI-remotetabs-instruction-label, .PanelUI-remotetabs-instruction-box,
> +     and .PanelUI-remotetabs-prefs-button).
> +  */
> +  min-height: calc(var(--panel-ui-sync-illustration-height) +
> +                   10px * 2 + 8px * 2 + 15px * 2 + 30px + 15px +

TBH, I would just stick all of these on one line per thing and pre-multiply:

20px /* margin of foobar */ +
16px /* padding of flopsy */ +
30px /* ... */

but I don't really care too much.

I do think it'd make sense to add a comment where we're setting the "original"s of these numbers noting that the min-height here might need updating if they get changed.

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8738617 [details]
MozReview Request: Bug 1248506 - Replace hardcoded height of synced tab panel with calc expression based on content size r?gijs

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/572875411dba

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/572875411dba