1191392 - Set font-size on html elements and recalculate any necessary rem sizes

Status: RESOLVED WONTFIX

(Hello (Loop) :: Client, defect, P2)

Comment 1

[Andrei Oprea [:andreio]]

Attachment: Set a common fixed font size

Comment 2

[Andrei Oprea [:andreio]]

Comment on attachment 8646473 [details] [diff] [review]
Set a common fixed font size

Would make sense to pair review this. These were all the font size changes that made sense to me. Doesn't seem to affect the UI showcase because fixed html font size was in place in the panel.css and contacts.css files.

Comment 4

[Andrei Oprea [:andreio]]

https://www.dropbox.com/sh/sx62vqrp0y2vg9h/AAAFsTiC88_j0ROrdERybG_Da?dl=0

Could you please review those screenshots? Font size, margin and padding should be correct for all elements.

Comment 5

[Andrei Oprea [:andreio]]

Attachment: Fix font size for Loop client

Comment 6

[Andrei Oprea [:andreio]]

The scope of the bug was making sure base font size is at 10 px. We used things like `font: menu` to ensure the font family is the same as the system but this also set font size to 11px. We also had a lot of dimensions specified in ems and things shifted when I changed size.

Comment 7

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #4)
> https://www.dropbox.com/sh/sx62vqrp0y2vg9h/AAAFsTiC88_j0ROrdERybG_Da?dl=0
> 
> Could you please review those screenshots? Font size, margin and padding
> should be correct for all elements.

Just commented on IRC about top padding on the conversations panel. The rest looks pretty good. THanks!

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8657008 [details] [diff] [review]
Fix font size for Loop client

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

I've not looked at the patch in detail, however, there's some initial issues I see:

- The "my contacts" and "Recent conversations" titles have been removed, but I don't understand why - they are still in the mock-ups but I've not seen any conversation about removing them.
- Contact avatars now overlap the bottom of the search box, and scroll on top of the bottom of the white part of the search box.
- On my Mac display, the text for the conversation window buttons looks different from your screenshot which is weird, and as a result there's virtually no border around the text.
- Something has broken the mute buttons on the conversation window.
-- With your patch, hovering over the audio mute shows a white indent on its left side.
-- Without your patch, hovering over the audio mute keeps showing the grey line.
- The "Welcome to ..." on the standalone is the wrong size
- "Join the conversation" button also seems to be wrong/worse than before
- Context hovers have lost their 2px border (should have border + highlight)
- "Let's talk about" seems to have got bigger?

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Fix font size for Loop client

Comment 10

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #8)
> - Something has broken the mute buttons on the conversation window.
> -- With your patch, hovering over the audio mute shows a white indent on its
> left side.
> -- Without your patch, hovering over the audio mute keeps showing the grey
> line.

This might have something to do with backgrounds, not sure how your patch affects it though. You can see this issue on the ui-showcase if you look at just the "ConversationToolbar" images. The line is white. However, over remote video with backgrounds, it is grey.

I'm not sure why your patch would be affecting the background colours though, so I don't really know what's happening here.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: First try merge; has at least one sizing problem

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Here's a patch merged to fx-team after the landing of bug 1192372.  There's at least one problem with it, and it may be worth redo-ing the merge.  However, if running the screenshot diffs shows that the problems aren't bad, it might be easier to fixup by hand.

Andrei, we're hoping to get this bug in sooner rather than later so that the UX team can start a holistic review on the panel.  I'm happy to pick this up and run with it, if you wish.  What's your current thinking?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

After getting the screenshotting up and going, and I think the next step is to re-do that merge.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP set font-size on root element and convert units to rems

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

In my most recent patch, merged on top of today's fx-team (i.e. including the fixed size changes), here are my thoughts on Mark's comments:

(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 8657008 [details] [diff] [review]
> Fix font size for Loop client
> 
> Review of attachment 8657008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've not looked at the patch in detail, however, there's some initial issues
> I see:
> 
> - The "my contacts" and "Recent conversations" titles have been removed, but
> I don't understand why - they are still in the mock-ups but I've not seen
> any conversation about removing them.

I put both of these back, because in addition to this, even if we want to do these changes, I think they want to happen in another bug, to avoid unnecessary complexity during the screenshot/compare process.

> - Contact avatars now overlap the bottom of the search box, and scroll on
> top of the bottom of the white part of the search box.

I'm not seeing this, probably due to the fixed-size changes.

> - On my Mac display, the text for the conversation window buttons looks
> different from your screenshot which is weird, and as a result there's
> virtually no border around the text.

I'm not seeing this, but I may not be looking in the right place.  Which buttons specifically?

I'll respond to more of the comments tomorrow.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

I'm going to grab this bug now, as I'm assuming that Andrei has gotten pulled back into non-Mozilla life, and we want to land this as soon as we can for holistic UI review to start.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

For the next little bit, I'll be iterating at <https://github.com/dmose/gecko-dev/pull/13> so that I can keep the various commits separate from one another for easier analysis.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

I'm having significant false-positive problems with the screenshot comparison tool, as well as various issues with the patch itself.

I believe this wants to land in multiple chunks and (hopefully) with a fixed screenshot comparison tool, so that we can have a high degree of confidence that we're not regressing stuff.

The most critical piece of this bug, having the font-size set on the root elements to a standard thing (10px) is already done in the tree, albeit not in the cleanest way ever.

So I think we can drop this to a P2 so that it doesn't block other stuff landing in 43.

I'll split this out into multiple smaller user stories for the font/rem cleanup bits; setting needinfo to keep it on my radar.

Comment 19

[Ian Bicking (:ianbicking)]

This has been done sufficiently for now (the font sizes no longer are causing problems).