1458224 - Font size too big on Linux for breadcrumbs and search inputs

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Julian Descottes [:jdescottes]]

Attachment: inspector-linux.png

The font size of some inspector elements is too big on Linux. See attachment

Comment 1

[Florens Verschelde :fvsch]

This affects most of the DevTools (and I wouldn't be surprised if there are 10 duplicates of this bug in bugzilla), with affected text being:

- Anything that doesn’t have a `font-size` declared.
- Buttons and some other elements which have a UA style other than `font-size:inherit`.

Elements whose font-size may be set by UA styles may be using `font: -moz-button;`, `font: message-box`, or maybe `-moz-user-system-font`.

These seem to resolve to 11px on macOS and, as far as I know, on Windows as well, but 14.66667px on Linux.
This sounds like on Linux they are set to 11pt instead of 11px, which at a theoretical 96dpi resolution (the one used in the CSS spec for pt<->px equivalence) means 11pt === 14.666667px.

1. Can we get these default sized fixed for Linux? Or is the variance expected?
2. If not, or for a quicker fix, we can set our own font sizes, for example:

:root { font-size: 11px }
button, input { font-size: inherit }

Comment 2

[Florens Verschelde :fvsch]

Related: I created a UX issue on specifying font sizes in DevTools
https://github.com/devtools-html/ux/issues/2

Comment 3

[Florens Verschelde :fvsch]

Attachment: inspector-linux-with-highlighted-issues.png

Comment 4

[Florens Verschelde :fvsch]

Seen in `devtools/client/application/src/components/App.css`:

:root[platform="linux"],
:root[platform="linux"] button {
  font-size: 11px;
}

So someone did notice the issue ^^

Comment 5

[Florens Verschelde :fvsch]

Attachment: console-before-after.png

Showing some of the issues fixed in the Console (on Linux):
- alignment of the "filter" icon
- filter label
- filter buttons
- "Raw headers" button (in request widget)

Comment 6

[Florens Verschelde :fvsch]

Attachment: devtools-linux-fontsize-screenshots.zip

I made a bunch of screenshots to compare Firefox stable (but Nightly is similar for most things) to my fixes.

Fair warning: we might get a few bug reports from Linux users who suddenly feel that the text got too small, since they were used to 14.6px text in many places.

Comment 7

[Florens Verschelde :fvsch]

Attachment: Bug 1458224 - Fix Linux root font size in DevTools; r=pbro

Comment 8

[Florens Verschelde :fvsch]

Turns out it's a rather simple fix.

We have to look out for two things:

- The document's root font-size is 14.6667px instead of 11px on Linux.
- Every time we use `font: message-box` or something similar, it resets the font-size to the default one (again, too big on Linux).

In my patch, I tried this approach:

1. Create variables for font sizes (`--theme-body-font-size` and `--theme-code-font-size`). Both are set at 11px. In the future I would like to create one or two more variables for headings and labels, so that we can have a typographic scale:
- code = 11px
- sans-serif = 11, 12 and maybe 14px
This would take a bit of a refactor effort, but could allow things such as: a) bumping all font sizes 1-2px to follow evolutions in screen density or b) offer a "bigger fonts" option.
Happy to hear any idea or opinion about this topic. :)

2. Apply a 11px font-size to :root in common.css

3. Apply font-size: inherit for buttons and inputs in common.css

4. Remove a few of Linux-specific fixes that lowered the font-size for some elements, since they're now redundant or (in some cases) result in text that is too small.

Comment 9

[Florens Verschelde :fvsch]

Tested on macOS: no visible change.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9009965 [details]
Bug 1458224 - Fix Linux root font size in DevTools; r=pbro

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Patrick, could you pull the patch and test it on windows to make sure it has no impact ? Thanks !

Comment 12

[Patrick Brosset <:pbro>]

This seems to work fine for me on Windows 10.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Patrick Brosset <:pbro> from comment #12)
> This seems to work fine for me on Windows 10.

Great !

Florens, could you push to TRY (if you haven't already) to see if there's no failure ? If TRY is green, we'll land that patch.

Comment 14

[Florens Verschelde :fvsch]

Try on linux, linux64, win64 and macosx64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e8dbb144be35dc21d323f12598ac3ea2ef108f

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Looks like there are 2 tests failing on Windows only: https://treeherder.mozilla.org/testview.html?repo=try&revision=d4e8dbb144be35dc21d323f12598ac3ea2ef108f

Patrick, could you help us checking what's going on here ?

Comment 16

[Patrick Brosset <:pbro>]

Who would have thought that subtly changing a font size would cause an inspector event bubble test to fail?

Well, turns out that it does. My intuition so far is that the font size change is causing a click to fall right on the icon that opens the debugger.
Indeed, in test devtools\client\inspector\markup\test\browser_markup_events_source_map.js we use test helper devtools\client\inspector\markup\test\helper_events_test_runner.js (in particular the checkEventsForNode function). That function attempts to click on the event bubble header to expand it. However with the patch applied, that click actually opens the debugger instead. And that makes the test time out because it keeps on waiting for the bubble to expand, and it never does.

I took a screen capture of the 2 bubbles (with and without the patch), and sure enough, with the patch the font is a tiny bit smaller on that header, and it causes the debugger icon to be right in the middle of the header.
And because the helper function we use does EventUtils.synthesizeMouseAtCenter(header, ...) then the debugger icon gets clicked.
So, you could say that we were lucky up until now!

Therefore, we'll need to amend this patch and remove the use of synthesizeMouseAtCenter, and instead click at some specific coordinates (somewhere near the top left of the header).

In particular, replacing this line of the function:
EventUtils.synthesizeMouseAtCenter(header, {}, type.ownerGlobal);

with this one:
EventUtils.synthesizeMouse(header, 2, 2, {}, type.ownerGlobal);

seems to make the test pass on windows.

Comment 17

[Patrick Brosset <:pbro>]

As for the other test: devtools\client\inspector\test\browser_inspector_textbox-menu.js
I've not investigated this yet, mostly because it didn't fail for me locally.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bubble-nobug.png

For reference, here's the screenshot of the event bubble header, without the patch applied.
The debugger icon is a little bit right of the center of that header.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bubble-bug.png

And here is the screenshot of the event bubble with the patch applied. The font is a bit smaller, so the text is shorter, and therefore the debugger icon is right in the middle of the header, where the click is being synthesized.

Comment 21

[Florens Verschelde :fvsch]

Aside from the freak "debugger button in the middle of event header" incident, I wonder if we don't have small visual regressions in some places on macOS and Windows because of this:

/* Bug 1458224: buttons use a wrong default font-size on Linux */
html|button, html|select, html|input {
  font-size: inherit;
}

Especially for buttons, since those elements tended to have a 11px font-size on macOS and Windows, if we inherit a different size (smaller or larger) from the parent it's going to result in a bigger or smaller button, marginally shifting things around.

It might be better to use:

html|button, html|select {
  font-size: var(--theme-body-font-size);
}

(I think the input selector might not be needed either. Need to test that.)

Comment 22

[Florens Verschelde :fvsch]

Looks like I removed the whiteboard tag by mistake. Trying to restore. :P

Comment 23

[Florens Verschelde :fvsch]

Attachment: linux-meatball-menu.png

For the record, even with this patch the "three dots" menu items are:

- Label: 13px on macOS, 14.6667px on Linux
- Shortcut text: 11px on macOS, 14.6667px on Linux

Not sure if this need a fix. It seems there are differences in font-size for the main Firefox menu between mac and linux already, so maybe this is partly wanted. Leaving it as is for now.

Comment 24

[Florens Verschelde :fvsch]

Pushed an update with:

- Patrick's fix for devtools\client\inspector\markup\test\helper_events_test_runner.js#checkEventsForNode
- 11px font-size for buttons instead of font-size:inherit
- Fixes for big font sizes on Linux in specific places such as Request editing textareas (found by looking at uses of `font: message-box`)

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd3a551a66874561cf392346910d78366f4db06

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

So, looks like helper_events_test_runner is fixed :)
We still have the issue on browser_inspector_textbox-menu.js. From the logs (https://treeherder.mozilla.org/logviewer.html#?job_id=200212635&repo=try&lineNumber=8623), it looks like the issue is on those lines: https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/inspector/test/browser_inspector_textbox-menu.js#49,87-88 .
The popup never seems to open, as the screenshot suggests https://taskcluster-artifacts.net/c4l0LFJPRv20JVyc1GWyIQ/0/public/test_info/mozilla-test-fail-screenshot_ytwjcm.png .

Comment 26

[Florens Verschelde :fvsch]

Thanks for investigating.
I’m not sure which elements the `cssRuleEditor.selectorText` refers to in the UI, or if the first click or the right click is failing (or something else).

Comment 27

[Patrick Brosset <:pbro>]

(In reply to Florens Verschelde from comment #26)
> Thanks for investigating.
> I’m not sure which elements the `cssRuleEditor.selectorText` refers to in
> the UI, or if the first click or the right click is failing (or something
> else).
cssRuleEditor.selectorText is the h1 selector in the screenshot. 
Line 48 in the test:
EventUtils.synthesizeMouse(cssRuleEditor.selectorText, 0, 0, {}, inspector.panelWin);
is supposed to click on it, and when that happens, the selector turns to an input field (so users can edit the selector value).
Looking at the screenshot, that never seems to happen. So the subsequent right-click attempt on line 87 doesn't open the context menu, and the test fails.

Comment 28

[Florens Verschelde :fvsch]

Just checked and the test failure does seem to come from this patch (green try on parent changeset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=802c72318108913e87b3f8977419ed796d75f441).

But I don't understand what changed visually on Windows with this patch on this part of the UI.

Comment 29

[Florens Verschelde :fvsch]

Attachment: insector-selector-input.png

I was looking at what elements may appear on top of the selector text/input.

It looks like the draggable splitter takes the 4 first pixels on the left of the Rules panel (visualized here with red at 50% opacity). Clicking the splitter does not open a context menu.

But I’m not sure this is responsible for the failing text, a) because I don't see the relation with my changes and b) it looks like the test should _just_ pass.

EventUtils.synthesizeMouse(cssRuleEditor.selectorText, 0, 0, {}, inspector.panelWin);

-> This clicks the top left corner of (`.ruleview-selectorcontainer`), which _touches_ the splitter area but does not go below it.


Then we have:

EventUtils.synthesizeMouse(textBox, 2, 2, {type: "contextmenu", button: 2},
                           textBox.ownerDocument.defaultView);

-> This clicks the top left of (`.styleinspector-propertyeditor`) with a 2px offset, and that element does appear below the splitter but just by one pixel (see screenshot).

Styles for the text input:

.styleinspector-propertyeditor {
  border: 1px solid #CCC;
  padding: 0;
  margin: -1px -3px -1px -1px;
}

Theoretically both clicks should work, even if barely.

Comment 30

[Florens Verschelde :fvsch]

I did a try after changing the first synthesizeMouse to 2,2 (instead of 0,0), and it seems to pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e8332811c6fa6a7f6f5453fe151faac96718b97

Should I include that in my patch, or do we need a better fix?

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Florens Verschelde from comment #30)
> I did a try after changing the first synthesizeMouse to 2,2 (instead of
> 0,0), and it seems to pass:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7e8332811c6fa6a7f6f5453fe151faac96718b97
> 
> Should I include that in my patch, or do we need a better fix?

I think this is fine, with maybe a comment explaining the `2,2`, or you could use synthesizeMouseAtCenter so it's more obvious what we want to do. I don't feel strongly about this. so either the coords change or the function change is fine to me :)

Comment 32

[Nicolas Chevobbe [:nchevobbe]]

Florens, are you blocked on something for this ?

Comment 33

[Florens Verschelde :fvsch]

Nothing blocking, just a computer change and relative lack of free time. I'll try to update my patch in the next few days. Good news is I'll now be working on Linux more often, so I'll be able to spot more small problems. :D

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Good to know :)
I'll be on PTO for the next 2 weeks, so if you want to have a patch reviewed during this timespan, feel free to redirect to someone else (I think pbro can do it :) )

Comment 35

[Florens Verschelde :fvsch]

Updated on Phabricator (using synthesizeMouseAtCenter to fix the remaining breaking test):
https://phabricator.services.mozilla.com/D6155

New try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4004784c9a4b87f1b69f04b6f28f21bb711d58c4

Fingers crossed :D
In any case, I've now tested this change on two Linux distros with a bunch of different configurations, it seems pretty solid.

Comment 36

[Florens Verschelde :fvsch]

Patrick, latest try (previous comment) shows intermittent failures in win64/dt2 and linux64/dt7. I restarted both, and they don't seem to repeat with the same failures, so maybe they're true intermittents? Not sure how to interpret the results.

The two previous failures we had seem to be fixed.

I've also added you as a reviewer on https://phabricator.services.mozilla.com/D6155

Comment 37

[Patrick Brosset <:pbro>]

Comment on attachment 9009965 [details]
Bug 1458224 - Fix Linux root font size in DevTools; r=pbro

Patrick Brosset <:pbro> has approved the revision.

Comment 38

[Patrick Brosset <:pbro>]

I've retriggered a bunch of new test runs on those failures. From what I can see, I don't think they are related to your change. But I wanted to make sure, hence the re-triggers. Let's wait a bit and see.

Comment 39

[Patrick Brosset <:pbro>]

We can ignore the browser_animation_pause-resume-button_end-time.js failures.
But I'm afraid browser_dbg_variables-view-popup-*.js seem to fail a bit more frequently, judging by the robot comments on the corresponding bugs.
Now, this is a test for the old debugger. And the old debugger is going away very shortly. David Walsh is working on this at this very moment over in bug 1314057.
So I think we should ignore those as well.

Comment 40

[Florens Verschelde :fvsch]

I updated the comment that explains why we set a font-size on `:root`.
We should be ready to land, unless we're waiting for bug 1314057 to land first.

I don't have the permission level required to use Lando, so I'll leave it to you if that's okay.

Comment 41

[Florens Verschelde :fvsch]

Green try on top of latest mozilla-central (with bug 1314057 checked in):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a76a3b2faa88180a481e9622958dcda19f6209

Comment 42

[Florens Verschelde :fvsch]

Checkin needed for: https://phabricator.services.mozilla.com/D6155

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6091f5c6320b
Fix Linux root font size in DevTools; r=pbro,nchevobbe

Comment 44

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6091f5c6320b