WIP: Fix CSS, update screenshots, and raise threshold #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some of our subtitle layout screenshot tests were producing confusing and inconsistent results across different machines or environments. After some analysis, I discovered some odd things in our CSS that may explain some or all of this. The smaller thing is that font-family settings always started with "Roboto-Regular" before "Roboto", but we don't load "Roboto-Regular". It may exist already on some systems, but we don't explicitly load it. We do explicitly load "Roboto", so that should be the only Roboto-family font in the list. This issue has been present since the very first introduction of our UI. The bigger issue is more subtle. Font settings were applied to ".shaka-video-container *", all descendents of .shaka-video-container. This meant that the font setting inheritance intended in our UI text displayer was broken, because those descendents effectively had their own settings and didn't inherit them from their parents in the subtitle DOM hierarchy. We fix this by breaking down the CSS and applying those font settings directly to the top level .shaka-video-container, to be inherited by any descendent without their own setting. These two changes seem to fix issues with inconsistent bold display in the tests. Some devices that displayed bold as normal are now bold. This fixes inconsistency in results between Safari in our lab and Safari on GitHub Actions VMs. This PR does not update any screenshots, because it would be difficult to read. This will be followed by a PR to update all screenshots.
This is a follow-up to "Fix CSS inconsistencies in fonts and
subtitles" to update the screenshots. This PR contains only
screenshot updates, to keep the other PR more readable.
Changes marked with a `*` are likely old changes that didn't break our
similarity threshold and therefore were not updated automatically
before now.
- Edge on Mac:
- Most native renders backgrounds now hug the text as they already
did on other platforms
- Most UI renders have corrected font weight, no longer default
bold
- Chrome on Mac:
- Most UI renders have corrected font weight, no longer default
bold
- `*` Very small alignment changes (less than one space) on some
layouts
- `*` Flipped order of "end-time-edge-case"
- Safari:
- Most UI renders have corrected font weight, no longer default
bold
- `*` Very small alignment changes (less than one space) on some
layouts
- Firefox on Mac:
- Most UI renders have corrected font weight, no longer default
bold
- Edge on Linux and Windows; Chrome on Android, Linux, and Windows:
- `*` Very small alignment changes (less than one space) on some
layouts
- Firefox on Windows:
- `*` Bold render is slightly different than before
The screenshots with native text rendering don't always trigger our threshold even when they should. That's because the native text tends to be much smaller. Therefore the matching threshold should be higher to make these smaller regions of text more sensitive to change. This raises the threshold from 95% (still used for DOM-based) to 97% for native text. I believe this could have caught some of the changes that I only caught later with manual review of screenshots.
joeyparrish
changed the title
|
Incremental code coverage: No instrumented code was changed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.