[markup-view] Set line-height to 14px for more predictable vertical rhythm and alignment

RESOLVED FIXED in Firefox 65

Status

enhancement
P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: fvsch, Assigned: fvsch)

Tracking

(Blocks 1 bug)

unspecified
Firefox 65

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 months ago
Currently the lines in the markup view have use a line-height defined like this:

.tag-line {
  min-height: 1.4em;
  line-height: 1.4em;
}

Since the font-size is fixed to 11px, this results in a computed line-height and min-height of 15.4px.

In my experience, fractional line-heights can result in imprecise rendering on 1x displays, due to pixel-fitting. A line can be visually rendered at 15px or 16px, the distance between two text baselines can be alternatively 15px or 16px, etc.

In https://github.com/devtools-html/ux/issues/3 I described how we could strive to match the Photon grid, which uses 4px steps. So a content line could be 16px or 20px (depending on the need), toolbars and headers 24px or 28px, etc.

Since we're already close to 16px, I suggest going for that. This can be implemented with more precise fractions, assuming a 11px font-size but leaving room for font-size changes in the future (if any):

.tag-line {
  min-height: 1.4545em;
  line-height: 1.4545em; /* or 1.4545 (inherited as a ratio rather than as a computed pixel value) */
}

This would enable us to try and achieve more precise and less jumpy micro-layouts for editor textareas, and more precise vertical alignment for event/grid/flex pill buttons.

Re. content density, this means that when we were showing 26 lines in a viewport, we would now be showing 25 lines instead.
(Assignee)

Comment 1

6 months ago
Patrick, do you think it's worthwhile pursuing this?
IMO it unlocks more "UI precision" improvements to the markup view.
Flags: needinfo?(pbrosset)
Yes I think it is.
Flags: needinfo?(pbrosset)
(Assignee)

Updated

6 months ago
Assignee: nobody → florens
(Assignee)

Updated

6 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 months ago
So I’ve looked a making the markup view's line height more precise, and the good news is that it's doable and it allows us to more precisely align the markup badges ("event", "grid", "flex", and "...").

On the design front, I think we have two options:

1. Left: increase the current line-height (15.4px) a tiny bit, to 16px.
2. Right: decrease the line-height from 15.4px to 14px, and use 1px padding to achieve a 16px row height. This makes rows that wrap to several lines (tags with many attributes, long text nodes) *smaller* than currently.

So the design question is: which do we like best, left or right?

For context:
- CSS property:value lines in Rules use 14px line-height
- Console rows use 14px line-height
- Debugger source code uses "normal" line-height, resulting in 13.5px line-height on some platforms/fonts (we've talked about setting it to 14px)
- font-size in all these contexts and in the markup view is 11px

So it could be interesting to go with the 14px option, for consistency's sake.
Attachment #9020540 - Flags: ui-review?(victoria)
Attachment #9020540 - Flags: feedback?(pbrosset)
(Assignee)

Comment 4

6 months ago
Set markup view lines to use a precise 14px line-height, 16px total height.
Align markup badges more precisely with vertical-align:top and margin-top,
because vertical-align:middle is imprecise and has unwanted side effects.
(Assignee)

Updated

6 months ago
Blocks: 1502003
(Assignee)

Comment 5

6 months ago
I have a tentative patch on Phabricator for the 14px option, but it can easily amended for 16px.

I've also aligned the drag-target and drop-target markers (they're 2px tall, so a top:-1px centers them), and the markup badges using vertical-align:top and margin-top (because vertical-align:middle is often just wrong, and has side effects such as increasing the line's height beyond the specified line-height).
Comment on attachment 9020540 [details]
markup-view-line-height-16px-vs-14px.png

I think I prefer a line-height of 14px with some padding to make each node be on a 16px line.
This way, each node is nicely separated from the rest, and nodes that wrap are a bit more compact.
Also because of the consistency reason you mentioned.
Attachment #9020540 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 7

6 months ago
On Linux at 1x pixel density: current Nightly (left) versus 14px line-height (right).

For the record, the markup badge is 12px tall and the "expand" (ellipsis) badge is 10px tall. We can try making them 14px and 12px if the small height is a concern, but then they look a bit bulky IMO.
Priority: -- → P3
Comment on attachment 9020540 [details]
markup-view-line-height-16px-vs-14px.png

Sorry for the delay on this! I agree, the 14px does have a nice grouping effect on the nodes, and the consistency with other panels will be great.
Attachment #9020540 - Flags: ui-review?(victoria) → ui-review+
(Assignee)

Updated

6 months ago
Summary: [markup-view] Set line-height to 16px for more predictable vertical rhythm and alignment → [markup-view] Set line-height to 14px for more predictable vertical rhythm and alignment
Attachment #9020549 - Attachment description: Bug 1501997 - Set markup-view line-height to 14px; r?pbro → Bug 1501997 - Set markup-view line-height to 14px; r=pbro

Comment 9

5 months ago
Pushed by florens@fvsch.com:
https://hg.mozilla.org/integration/autoland/rev/7a8540eb0e90
Set markup-view line-height to 14px; r=pbro

Comment 11

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a8540eb0e90
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.