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

RESOLVED WONTFIX

Status

Hello (Loop)
Client
P2
normal
Rank:
27
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

unspecified
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

User Story

As a developer, the font-size for the root <html> element for each view should be set to 10px.

== Tech Checklist ==
* likely to want to force font size on html element
* (timebox) see if it's simple to use imagediff or pdiff in the verification process
* recalculate any necessary rem sizes and verifying that they correspond to the mockups

Attachments

(5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Rank: 15
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [visual refresh]
(Assignee)

Updated

3 years ago
Blocks: 1190734
(Assignee)

Updated

3 years ago
Summary: set fonts on html elements and recalculate any necessary rem sizes → set font-size on html elements and recalculate any necessary rem sizes
(Assignee)

Updated

3 years ago
Blocks: 1191398
Blocks: 1190738
Assignee: nobody → andrei.br92
Summary: set font-size on html elements and recalculate any necessary rem sizes → Set font-size on html elements and recalculate any necessary rem sizes

Updated

3 years ago
Iteration: --- → 43.1 - Aug 24
Created attachment 8646473 [details] [diff] [review]
Set a common fixed font size
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.
Attachment #8646473 - Flags: review?(dmose)
No longer blocks: 1190734
(Assignee)

Updated

3 years ago
Attachment #8646473 - Flags: review?(dmose)

Updated

3 years ago
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7

Updated

3 years ago
Duplicate of this bug: 1191398
Depends on: 1192372
Whiteboard: [visual refresh] → [visual refresh][waiting for bug 1192372 to land to make visual diffs easier]
Blocks: 1179193
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.
Flags: needinfo?(vpg)
Created attachment 8657008 [details] [diff] [review]
Fix font size for Loop client
Attachment #8646473 - Attachment is obsolete: true
Attachment #8657008 - Flags: review?(standard8)
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.
(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!
Flags: needinfo?(vpg)
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?
Attachment #8657008 - Flags: review?(standard8) → review-
Created attachment 8657384 [details] [diff] [review]
Fix font size for Loop client
Attachment #8657008 - Attachment is obsolete: true
(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.

Updated

3 years ago
Rank: 15 → 6

Updated

3 years ago
Iteration: 43.2 - Sep 7 → 43.3 - Sep 21
(Assignee)

Comment 11

3 years ago
Created attachment 8658976 [details] [diff] [review]
First try merge; has at least one sizing problem
(Assignee)

Comment 12

3 years ago
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?
Flags: needinfo?(andrei.br92)
(Assignee)

Comment 13

3 years ago
After getting the screenshotting up and going, and I think the next step is to re-do that merge.
(Assignee)

Comment 14

3 years ago
Created attachment 8659555 [details] [diff] [review]
WIP set font-size on root element and convert units to rems
(Assignee)

Comment 15

3 years ago
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.
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: andrei.br92 → dmose
Flags: needinfo?(andrei.br92)
(Assignee)

Updated

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

Updated

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

Updated

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

Comment 17

3 years ago
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.
(Assignee)

Updated

3 years ago
Points: 1 → 8
(Assignee)

Updated

3 years ago
Whiteboard: [visual refresh][waiting for bug 1192372 to land to make visual diffs easier] → [visual refresh]
(Assignee)

Comment 18

3 years ago
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.
Rank: 6 → 25
Flags: needinfo?(dmose)
Priority: P1 → P2

Updated

2 years ago
Rank: 25 → 27
Whiteboard: [visual refresh] → [visual refresh, tech-debt]
Iteration: 43.3 - Sep 21 → ---
Status: NEW → ASSIGNED
Whiteboard: [visual refresh, tech-debt] → [tech-debt]
This has been done sufficiently for now (the font sizes no longer are causing problems).
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(dmose)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.