Set a more usable default font size #228

Merged
Rosuav merged 1 commit from set-default-font-size into master 2021-01-16 18:23:17 -06:00
Rosuav commented 2020-05-08 06:26:55 -05:00 (Migrated from github.com)

Description

Set a default font size of 16px to ensure that text is readable when unstyled.

Motivation and Context

The default font size in most browsers is 16px, but the default here appears sometimes to be 1px. This results in unreadable text (see eg the confusion in #226 which ultimately was caused by this), and is inconsistent.

How Has This Been Tested?

Fired up several pages. Browser plugin was built against CEF 75.1.14+gc81164e+chromium-75.0.3770.100 - didn't test with other CEF versions. It might be that this patch only benefits older browser builds, but that's still worthwhile as 3770 is the one named in the OBS installation instructions.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
<!--- Please fill out the following template, which will help other contributors review your Pull Request. --> <!--- Make sure you’ve read the contribution guidelines here: https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst --> ### Description Set a default font size of 16px to ensure that text is readable when unstyled. ### Motivation and Context The default font size in most browsers is 16px, but the default here appears sometimes to be 1px. This results in unreadable text (see eg the confusion in #226 which ultimately was caused by this), and is inconsistent. ### How Has This Been Tested? Fired up several pages. Browser plugin was built against CEF 75.1.14+gc81164e+chromium-75.0.3770.100 - didn't test with other CEF versions. It might be that this patch only benefits older browser builds, but that's still worthwhile as 3770 is the one named in the OBS installation instructions. ### Types of changes <!--- What types of changes does your PR introduce? Uncomment all that apply --> - Bug fix (non-breaking change which fixes an issue) <!--- - New feature (non-breaking change which adds functionality) --> <!--- - Performance enhancement (non-breaking change which improves efficiency) --> <!--- - Code cleanup (non-breaking change which makes code smaller or more readable) --> <!--- - Breaking change (fix or feature that would cause existing functionality to change) --> <!--- - Documentation (a change to documentation pages) --> ### Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate.
WizardCM commented 2020-05-08 06:31:47 -05:00 (Migrated from github.com)

In testing, this is exclusive to 3770 and only on Linux as far as we could tell. Is it common for apps using CEF to manually set the font size?

In testing, this is exclusive to 3770 and only on Linux as far as we could tell. Is it common for apps using CEF to manually set the font size?
Rosuav commented 2020-05-08 06:35:22 -05:00 (Migrated from github.com)

I'm not saying the bug is exclusive to 3770/Linux - just that that's all I have access to test it on. I'm currently trying to build other CEFs for testing, but that's its own job and may or may not succeed.

I'm not saying the *bug* is exclusive to 3770/Linux - just that that's all I have access to test it on. I'm currently trying to build other CEFs for testing, but that's its own job and may or may not succeed.
Rosuav commented 2020-05-08 06:35:59 -05:00 (Migrated from github.com)

No idea how common it is for apps to manually set the font size, but given that I'm setting it to the most commonly-used default, it's going to do nothing if it wasn't a problem.

No idea how common it is for apps to manually set the font size, but given that I'm setting it to the most commonly-used default, it's going to do nothing if it wasn't a problem.
WizardCM commented 2020-05-08 06:37:30 -05:00 (Migrated from github.com)

No I know, I mean when we were preparing 25.x builds with the browser source, we ran into the same issue, and the reason we ended up jumping to 3809 on our official build on Linux is because the issue didn't occur there. We originally thought it was a rendering bug, but turned out to be font-size related.

Personally I think this change is fine if other browsers also use it as default, as long as we can verify that.

No I know, I mean when we were preparing 25.x builds with the browser source, we ran into the same issue, and the reason we ended up jumping to 3809 on our official build on Linux is because the issue didn't occur there. We originally thought it was a rendering bug, but turned out to be font-size related. Personally I think this change is fine if other browsers also use it as default, as long as we can verify that.
Rosuav commented 2020-05-08 06:40:57 -05:00 (Migrated from github.com)

Actually, can confirm that this does not happen on 3809, so this is a fix to ensure that the version listed in the installation instructions does work. Alternatively, changing the installation instructions (and including a fully-built CEF for download) would resolve this issue.

Actually, can confirm that this does not happen on 3809, so this is a fix to ensure that the version listed in the installation instructions does work. Alternatively, changing the installation instructions (and including a fully-built CEF for download) would resolve this issue.
aerogus commented 2020-05-11 09:20:53 -05:00 (Migrated from github.com)

Great idea this default font size. It may resolve the bug I have with "em" or "rem" units in my CSS. I had to switch to "px" units.

Great idea this default font size. It may resolve the bug I have with "em" or "rem" units in my CSS. I had to switch to "px" units.
Rosuav commented 2020-05-11 09:55:37 -05:00 (Migrated from github.com)

Yes, it should resolve that (based on my testing).

Yes, it should resolve that (based on my testing).
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!228
No description provided.