Closed
Bug 1458224
Opened 7 years ago
Closed 6 years ago
Font size too big on Linux for breadcrumbs and search inputs
Categories
(DevTools :: Inspector, defect, P3)
Tracking
(firefox64 fixed)
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: jdescottes, Assigned: fvsch)
References
Details
(Whiteboard: [dt-q])
Attachments
(9 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
text/x-phabricator-request
|
nchevobbe
:
review+
pbro
:
review+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
The font size of some inspector elements is too big on Linux. See attachment
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Comment 1•6 years ago
|
||
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 }
Assignee | ||
Comment 2•6 years ago
|
||
Related: I created a UX issue on specifying font sizes in DevTools
https://github.com/devtools-html/ux/issues/2
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
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 ^^
Assignee | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
Tested on macOS: no visible change.
Comment 10•6 years ago
|
||
Comment on attachment 9009965 [details]
Bug 1458224 - Fix Linux root font size in DevTools; r=pbro
Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9009965 -
Flags: review+
Comment 11•6 years ago
|
||
Patrick, could you pull the patch and test it on windows to make sure it has no impact ? Thanks !
Flags: needinfo?(pbrosset)
Comment 13•6 years ago
|
||
(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.
Assignee | ||
Comment 14•6 years ago
|
||
Try on linux, linux64, win64 and macosx64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e8dbb144be35dc21d323f12598ac3ea2ef108f
Comment 15•6 years ago
|
||
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 ?
Flags: needinfo?(pbrosset)
Comment 16•6 years ago
|
||
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.
Flags: needinfo?(pbrosset)
Comment 17•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee: nobody → florens
Status: NEW → ASSIGNED
Updated•6 years ago
|
Whiteboard: [dt-q]
Assignee | ||
Comment 21•6 years ago
|
||
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.)
Whiteboard: [dt-q]
Assignee | ||
Comment 22•6 years ago
|
||
Looks like I removed the whiteboard tag by mistake. Trying to restore. :P
Whiteboard: [dt-q]
Assignee | ||
Comment 23•6 years ago
|
||
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.
Assignee | ||
Comment 24•6 years ago
|
||
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•6 years ago
|
||
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 .
Assignee | ||
Comment 26•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 28•6 years ago
|
||
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.
Assignee | ||
Comment 29•6 years ago
|
||
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.
Assignee | ||
Comment 30•6 years ago
|
||
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?
Flags: needinfo?(nchevobbe)
Comment 31•6 years ago
|
||
(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 :)
Flags: needinfo?(nchevobbe)
Assignee | ||
Comment 33•6 years ago
|
||
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
Flags: needinfo?(florens)
Comment 34•6 years ago
|
||
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 :) )
Updated•6 years ago
|
Attachment #9009965 -
Attachment description: Bug 1458224 - Fix Linux root font size in DevTools; r=nchevobbe → Bug 1458224 - Fix Linux root font size in DevTools; r=pbro
Assignee | ||
Comment 35•6 years ago
|
||
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.
Assignee | ||
Comment 36•6 years ago
|
||
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
Flags: needinfo?(pbrosset)
Comment 37•6 years ago
|
||
Comment on attachment 9009965 [details]
Bug 1458224 - Fix Linux root font size in DevTools; r=pbro
Patrick Brosset <:pbro> has approved the revision.
Attachment #9009965 -
Flags: review+
Comment 38•6 years ago
|
||
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.
Flags: needinfo?(pbrosset)
Comment 39•6 years ago
|
||
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.
Assignee | ||
Comment 40•6 years ago
|
||
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.
Assignee | ||
Comment 41•6 years ago
|
||
Green try on top of latest mozilla-central (with bug 1314057 checked in):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a76a3b2faa88180a481e9622958dcda19f6209
Assignee | ||
Comment 42•6 years ago
|
||
Checkin needed for: https://phabricator.services.mozilla.com/D6155
Keywords: checkin-needed
Comment 43•6 years ago
|
||
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6091f5c6320b
Fix Linux root font size in DevTools; r=pbro,nchevobbe
Keywords: checkin-needed
Comment 44•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•