Contact list entries don't adapt their height to the actual font size

RESOLVED FIXED in Instantbird 49

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: aleth)

Tracking

Instantbird 49
x86
All

Details

(Whiteboard: [1.6-wanted])

Attachments

(5 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
*** Original post on bio 935 by Rob McAuley <rmcauley AT rainwave.cc> at 2011-07-20 16:17:00 UTC ***

I occasionally set the DPI scaling on my laptop to 125% "zoom" in Windows 7's control panel, since my laptop has a very high DPI display.  This causes Instantbird's fonts to start clipping.

It's bearable but it certainly doesn't look pretty.  If I encounter more font issues I shall file them under this same bug.

This may be an upstream issue with XUL - Firefox doesn't do DPI scaling at all.
(Reporter)

Comment 1

5 years ago
Posted image Poorly scaled UI.
*** Original post on bio 935 as attmnt 752 by rmcauley AT rainwave.cc at 2011-07-20 16:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 935 at 2011-08-30 20:53:27 UTC ***

As I wrote in bug 954440 (bio 1006):
The cause of the bug is that sizes in pixels (for the icon sizes) and sizes
proportional to the default font's size are used together, and they probably
got mixed up badly somewhere. Producing a good fix would require spending a
significant amount of time checking where things can vary in size and where
they can't, and changing the sizes in our stylesheets appropriately.

It's however possible for individual users to work around the issue with a userChrome.css file, for example with:
contact { min-height: 25px; }

Thanks for reporting the issue! :)
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 954440
(Assignee)

Comment 4

5 years ago
*** Original post on bio 935 at 2012-04-17 22:08:35 UTC ***

As per comment 2 and bug 954818 (bio 1383), this does not just affect Windows, but any OS where you can change the default font size.
OS: Windows 7 → All
(Assignee)

Updated

5 years ago
Duplicate of this bug: 954818
(Assignee)

Comment 6

5 years ago
*** Original post on bio 935 at 2012-04-17 22:24:55 UTC ***

Changing bug name to make this more findable.
Summary: User Interface Problems With Scaled DPI Settings → em and pt usage in CSS currently causes size/alignment errors when changing font size or DPI
(Assignee)

Comment 7

5 years ago
*** Original post on bio 935 at 2012-04-22 22:07:10 UTC ***

Since this now breaks the look of IB on gnome 3 with default settings, as well as Windows with large fonts setting (not that unusual either), this bug has become more important to fix.
Severity: minor → normal
Summary: em and pt usage in CSS currently causes size/alignment errors when changing font size or DPI → em and pt usage in CSS currently causes size/alignment errors for certain font sizes and DPI settings
(Assignee)

Updated

5 years ago
Summary: em and pt usage in CSS currently causes size/alignment errors for certain font sizes and DPI settings → em and pt usage in CSS causes size/alignment errors for certain font sizes and DPI settings
(Assignee)

Updated

5 years ago
Whiteboard: [1.3-wanted]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 955051
(Assignee)

Comment 9

5 years ago
*** Original post on bio 935 at 2012-08-07 21:26:46 UTC ***

Also for Windows with "medium font" settings...
(Assignee)

Updated

5 years ago
Blocks: 954135
*** Original post on bio 935 at 2012-11-02 00:12:47 UTC ***

Too late to work on this for 1.3, replacing 1.3-wanted by 1.4-wanted in the whiteboard.
Whiteboard: [1.3-wanted] → [1.4-wanted]
(Assignee)

Updated

5 years ago
Whiteboard: [1.4-wanted]
Duplicate of this bug: 955544
(Reporter)

Comment 12

5 years ago
*** Original post on bio 935 by Kirill Kirillov [:kkirill] <kirill.kirillov AT yahoo.com> at 2013-10-04 21:31:30 UTC ***

(In reply to comment #2)
> It's however possible for individual users to work around the issue with a
> userChrome.css file, for example with:
> contact { min-height: 25px; }

This will make impossible to expand the contact when it is selected in the list. So the workaround breaks more than it fixes
(Reporter)

Comment 13

5 years ago
*** Original post on bio 935 by Kirill Kirillov [:kkirill] <kirill.kirillov AT yahoo.com> at 2013-10-10 22:48:10 UTC ***

I've ended up with the following userChrome.css file as the workaround:

contact:not([open]):not([selected=true]) {
 min-height: 25px;
}

contact:not([open])[selected=true] {
 min-height: 50px;
}

conv {
 min-height: 25px;
}
(Assignee)

Updated

4 years ago
Whiteboard: [1.6-wanted]
(Assignee)

Updated

3 years ago
Assignee: nobody → durwasa.chakraborty
Status: NEW → ASSIGNED
It seems there is no general solution to the problem as the heights of different fonts are different. With repeated calibration we do can reach to a brute-force solution, but it can't be a generic solution. As mentioned in the [comment] (https://bugzilla.mozilla.org/show_bug.cgi?id=954368#c2) can be a possible solution but its just a hack.

I was wondering if we could implement a more generic algorithm such as the [Fitt's Law] (https://en.wikipedia.org/wiki/Fitts%27s_law) so that we don't have to work around every time with a different font setting. Is it worth a shot ? or I am missing something ?
(Assignee)

Comment 15

3 years ago
(In reply to Durwasa Chakraborty from comment #14)
> It seems there is no general solution to the problem as the heights of
> different fonts are different.

Is this really the issue? We specify the font size in pixels. What goes wrong, exactly?

If it's a DPI issue, does CSS <resolution> help?
Posted patch blist.css (obsolete) — Splinter Review
I decided that the container should be fixed size and the text should move relative to the container (in this case all three conv,contact,group).**min-height** to the rescue as I wanted that the dpi of a system should atleast 25px and rest of the content will scale accordingly.
Attachment #8731431 - Flags: review?(aleth)
Attachment #8731431 - Flags: feedback+
(Assignee)

Comment 17

3 years ago
(In reply to Durwasa Chakraborty from comment #16)
> Created attachment 8731431 [details] [diff] [review]
> blist.css

This is not a valid patch file, and so bugzilla can't display it.
Please take a look at these instructions:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

> I wanted that the dpi of
> a system should atleast 25px

I don't understand this sentence, dots-per-inch aren't measured in pixels.
(Assignee)

Updated

3 years ago
Attachment #8731431 - Flags: review?(aleth)
Attachment #8731431 - Flags: feedback+
(Assignee)

Comment 18

3 years ago
In order to get some actual data in here (at least for gnome): 
What is the actual on-screen height (in pixels) of the text you get for font-size: 16px ? 
And the on-screen height for font-size 16px and bold weight?
What is the computed box model size of such text?
What happens when you change the system font to another font (for example, Open Sans)? 
What are your OS dpi settings?
(In reply to aleth [:aleth] from comment #18)

> What is the actual on-screen height (in pixels) of the text you get for
> font-size: 16px ? 
Screen Resolution 1366 x 768 and display aspect ratio is: 1.78 ; CSS 1" 
> And the on-screen height for font-size 16px and bold weight?
I can find 14 px. 
> What is the computed box model size of such text?
I have just changed the weight of the font. Rest all the numeric of box model size remains the same.
> What happens when you change the system font to another font (for example,
> Open Sans)?
Nothing happens with open sans but yes there are other fonts for which the similar thing is happening.
> What are your OS dpi settings?
resolution:    96x96 dots per inch
Xft.dpi:	96
(Assignee)

Updated

3 years ago
Attachment #8731431 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Posted image Screenshot of attachment 8733693 (obsolete) —
(Assignee)

Comment 22

3 years ago
Comment on attachment 8733693 [details] [diff] [review]
Changed the parent box to static and the children as relative.

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

As you can see on the screenshot, this breaks the currently correct appearance on OS X.

Also, when you select a contact, it shrinks.

I don't think you're approaching this the right way. You should change the CSS so that the vertical height of the elements adjusts to the size of the existing font (whatever it is on the user's system), not hard code sizes.

::: im/themes/blist.css
@@ +118,5 @@
>  
>  conv,
>  contact,
>  buddy {
> +  min-height: 25px; /* 21+2+2px for the protocol icon, padding top and bottom */

Where is the 21 coming from?

@@ +137,5 @@
>  #buddylistbox:focus > group[selected="true"],
>  #convlistbox:focus > conv[selected="true"] {
>    background: -moz-Dialog;
>    color: -moz-DialogText;
> +  min-height: 3.2em;

Why 3.2 em?

@@ +158,5 @@
>  .contactStatusText,
>  .convStatusText,
>  .buddyStatusText {
>    color: GrayText;
> +  font-size: 0.5em;

Why change this font size?

@@ +168,5 @@
>  .convStatusText,
>  .buddyDisplayName,
>  .buddyStatusText {
>    margin: 0;
> +  font-size: 1em;

What does this do? Is the font too small or too large on your system?

@@ +332,4 @@
>  }
>  
>  #displayName {
> +  font-size: 1em;

As you can see on the screenshot, this shrinks the font on OS X.
Attachment #8733693 - Flags: review-
(Assignee)

Updated

3 years ago
Status: ASSIGNED → NEW
Summary: em and pt usage in CSS causes size/alignment errors for certain font sizes and DPI settings → Contact list entries don't adapt their height to the actual font size
(Assignee)

Comment 23

3 years ago
After all the recent discussion, it's time to fix this. To get the right height, it's enough to use height:auto after removing the fixed height constraint coming from the protocol icon by putting it into a vbox. The problem is that height:auto breaks the CSS transitions, so I then had to work around that by reading the correct height off a dummy contact entry.
(Assignee)

Updated

3 years ago
Assignee: durwasa.chakraborty → aleth
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8733798 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Comment on attachment 8736278 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

Hi Durwasa, it would be great if you could check that this patch works for you on Linux, as I could only check it by manually setting a larger font size.
Attachment #8736278 - Flags: feedback?(durwasa.chakraborty)
(Assignee)

Updated

3 years ago
Attachment #8733693 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
I made the changes according to @aleth sir's new patch and I encountered a different error in which the contact name which is clicked becomes transparent.PFA
(Assignee)

Comment 27

3 years ago
(In reply to Durwasa Chakraborty from comment #26)
> I made the changes according to @aleth sir's new patch and I encountered a
> different error in which the contact name which is clicked becomes
> transparent.PFA

How did you apply the patch? Did you use hg import?
I am sorry I didn't run the `hg import` command rather made the changes manually. It works great for all font weights :: bold,italic and till font size 18 (as per the gnome-tweak-tool) the contact list entries do not trim out. But I guess that is gnome issue rather than an IB issue. The attachment I have attached is for the font weight bold and works fine with normal font weight as well.
Attachment #8736576 - Flags: feedback+
(Assignee)

Comment 29

3 years ago
Abdelrhman confirmed this patch works on ubuntu unity (which uses gnome behind the scenes), so I suspect the problem in attachment 8736384 [details] is due to an incompletely applied patch.
(Assignee)

Updated

3 years ago
Attachment #8736278 - Attachment is obsolete: true
Attachment #8736278 - Flags: feedback?(durwasa.chakraborty)
(Assignee)

Comment 31

3 years ago
https://hg.mozilla.org/comm-central/rev/6486df8056f66ec2a59a8141eacba596e4389e3a
Bug 954368 - Contact list entries should adapt their height to the actual font size. r=florian
(Assignee)

Comment 32

3 years ago
https://hg.mozilla.org/comm-central/rev/135fa160a32ae82095e5285f1ade5132246ffb74
Backed out changeset 6486df8056f6 (bug 954368), erroneously included in the last push. a=aleth
(Assignee)

Comment 33

3 years ago
Comment on attachment 8737326 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

clokep: If you feel OK reviewing this, it would be great if you could take a look.
Attachment #8737326 - Flags: review?(clokep)
Comment on attachment 8737326 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

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

I don't feel comfortable reviewing this.
Attachment #8737326 - Flags: review?(clokep)
Comment on attachment 8737326 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

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

Thanks for tackling this long standing issue!

::: im/content/blist.css
@@ +92,5 @@
>    display: none;
>  }
>  
> +#dummylistbox {
> +  visibility: hidden;

Is this enough to ensure screen readers won't read that item?

@@ -131,5 @@
>  }
>  
>  conv {
>    -moz-binding: url("chrome://instantbird/content/conv.xml#conv");
> -  -moz-box-align: center;

Is this related to the patch here? (not implying the change should be reverted if not; just curious)

::: im/content/blist.js
@@ +823,5 @@
> +    // Find the correct height of a contact list item. This can vary depending
> +    // on the platform font and font size.
> +    let dummyContact = document.getElementById("dummyContact");
> +    // Force height value calculation.
> +    dummyContact.style.height = "auto";

Is there a reason why this isn't in the css file?

::: im/content/contact.xml
@@ +15,5 @@
>            xmlns:html="http://www.w3.org/1999/xhtml">
>  
>    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>      <content>
> +      <xul:vbox mousethrough="always">

Why do we need a vbox that contains a single child?

@@ +535,1 @@
>          <xul:vbox flex="1" class="contact-vbox" mousethrough="always">

The indent can't be correct here.
Attachment #8737326 - Flags: review?(florian) → feedback+
(Assignee)

Comment 36

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> > +#dummylistbox {
> > +  visibility: hidden;
> 
> Is this enough to ensure screen readers won't read that item?

Yes

> > -  -moz-box-align: center;
> 
> Is this related to the patch here? (not implying the change should be
> reverted if not; just curious)

Yes, I moved the center align to the XUL file for consistency with the other centered elements.

> ::: im/content/blist.js
> @@ +823,5 @@
> > +    // Find the correct height of a contact list item. This can vary depending
> > +    // on the platform font and font size.
> > +    let dummyContact = document.getElementById("dummyContact");
> > +    // Force height value calculation.
> > +    dummyContact.style.height = "auto";
> 
> Is there a reason why this isn't in the css file?

I could move it to a #dummyContact rule if you prefer, but I wanted to make it explicit that this is overriding the usual contact style.

> @@ +15,5 @@
> >            xmlns:html="http://www.w3.org/1999/xhtml">
> >  
> >    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> >      <content>
> > +      <xul:vbox mousethrough="always">
> 
> Why do we need a vbox that contains a single child?

Because the stack it contains has a fixed height.
(Assignee)

Updated

3 years ago
Attachment #8737326 - Attachment is obsolete: true
Comment on attachment 8750929 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

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

(In reply to aleth [:aleth] from comment #36)

> > @@ +15,5 @@
> > >            xmlns:html="http://www.w3.org/1999/xhtml">
> > >  
> > >    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> > >      <content>
> > > +      <xul:vbox mousethrough="always">
> > 
> > Why do we need a vbox that contains a single child?
> 
> Because the stack it contains has a fixed height.

I still don't understand this. Probably easier to discuss over IRC than in bugzilla comments.

::: im/content/contact.xml
@@ +551,5 @@
> +        </xul:hbox>
> +
> +        <xul:hbox mousethrough="always">
> +          <xul:vbox>
> +            <xul:image class="prplBuddyIcon"/>

Why do we have an additional xul:image after the patch?
(Assignee)

Comment 39

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> ::: im/content/contact.xml
> @@ +551,5 @@
> > +        </xul:hbox>
> > +
> > +        <xul:hbox mousethrough="always">
> > +          <xul:vbox>
> > +            <xul:image class="prplBuddyIcon"/>
> 
> Why do we have an additional xul:image after the patch?

It's needed as a spacer for the second row of the expanded contact. Previously, there was just one image and it was aligned with the top of the contact. Now, as each row has a height possibly different from the image, the image needs to be centered vertically with respect to the row. Once that's done, you need a second image in the second row to keep the text aligned horizontally.
(Assignee)

Comment 40

3 years ago
This one manages without the vbox around the stacks.
Attachment #8752472 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8750929 - Attachment is obsolete: true
Attachment #8750929 - Flags: review?(florian)
(Assignee)

Comment 41

3 years ago
Comment on attachment 8752472 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

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

::: im/themes/blist.css
@@ +170,5 @@
> +/* Used for the second line of a contact-big to add space on the left
> +   where the .prplBuddyIcon sits on the first line. */
> +.contactIconSpace {
> +  /* .prplBuddyIcon width+margins (16+3+3) + .contactStatusText margin (2) */
> +  -moz-margin-start: 24px;

I did it via CSS in this patch. It saves an element, but you can see why I dislike this approach. It involves magic constants and doesn't automatically adapt should anyone ever change .prplBuddyIcon.
Comment on attachment 8752472 [details] [diff] [review]
Contact list entries should adapt their height to the actual font size

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

Looks good to me. The 2 following comments are more food for thoughts that things that have to be addressed before landing.

::: im/content/contact.xml
@@ +15,5 @@
>            xmlns:html="http://www.w3.org/1999/xhtml">
>  
>    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>      <content>
> +      <xul:hbox flex="1" class="contact-hbox" mousethrough="always" align="center">

Is this hbox really needed if it now contains the whole content or the binding? Could we instead just move the attributes to the <content> tag?

@@ +528,5 @@
>    </binding>
>  
>    <binding id="contact-big" extends="chrome://instantbird/content/contact.xml#contact">
>      <content>
> +      <xul:vbox flex="1" class="contact-vbox" mousethrough="always">

I'm kinda wondering the same thing for this vbox. It seems more useful here because we use it to set the height through CSS... but couldn't the height also be set on the two hbox it contains?
Attachment #8752472 - Flags: review?(florian) → review+
(Assignee)

Comment 43

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> ::: im/content/contact.xml
> @@ +15,5 @@
> >            xmlns:html="http://www.w3.org/1999/xhtml">
> >  
> >    <binding id="contact" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> >      <content>
> > +      <xul:hbox flex="1" class="contact-hbox" mousethrough="always" align="center">
> 
> Is this hbox really needed if it now contains the whole content or the
> binding? Could we instead just move the attributes to the <content> tag?
> 
> @@ +528,5 @@
> >    </binding>
> >  
> >    <binding id="contact-big" extends="chrome://instantbird/content/contact.xml#contact">
> >      <content>
> > +      <xul:vbox flex="1" class="contact-vbox" mousethrough="always">
> 
> I'm kinda wondering the same thing for this vbox. It seems more useful here
> because we use it to set the height through CSS... but couldn't the height
> also be set on the two hbox it contains?

I spent about 5 minutes experimenting with getting it to work by setting orient=... class=... etc on the <content>. It sort of works for the normal contact but the vertical orientation fails for the expanded contact. I am unfortunately not very motivated to spend any more time on this.
(Assignee)

Comment 44

3 years ago
https://hg.mozilla.org/comm-central/rev/1783ef06e72b01e7f25bd0806b323f5e66c5fa2b
Bug 954368 - Contact list entries should adapt their height to the actual font size. r=florian
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 49
You need to log in before you can comment on or make changes to this bug.