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)

59 Branch
defect

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: jdescottes, Assigned: fvsch)

References

Details

(Whiteboard: [dt-q])

Attachments

(9 files)

Attached image inspector-linux.png (deleted) —
The font size of some inspector elements is too big on Linux. See attachment
Product: Firefox → DevTools
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 }
Related: I created a UX issue on specifying font sizes in DevTools https://github.com/devtools-html/ux/issues/2
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 ^^
Attached image console-before-after.png (deleted) —
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)
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.
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.
Tested on macOS: no visible change.
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+
Patrick, could you pull the patch and test it on windows to make sure it has no impact ? Thanks !
Flags: needinfo?(pbrosset)
This seems to work fine for me on Windows 10.
Flags: needinfo?(pbrosset)
(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.
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)
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)
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.
Attached image bubble-nobug.png (deleted) —
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.
Attached image bubble-bug.png (deleted) —
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.
Assignee: nobody → florens
Status: NEW → ASSIGNED
Whiteboard: [dt-q]
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]
Looks like I removed the whiteboard tag by mistake. Trying to restore. :P
Whiteboard: [dt-q]
Attached image linux-meatball-menu.png (deleted) —
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.
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
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).
(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.
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.
Attached image insector-selector-input.png (deleted) —
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.
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)
(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)
Florens, are you blocked on something for this ?
Flags: needinfo?(florens)
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)
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 :) )
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
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.
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 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+
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)
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.
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.
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: