Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows servo UI missing elements #15255

Closed
jonathandturner opened this issue Jan 26, 2017 · 20 comments
Closed

Windows servo UI missing elements #15255

jonathandturner opened this issue Jan 26, 2017 · 20 comments
Labels

Comments

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Jan 26, 2017

When I open a recent Servo for Windows nightly, the UI shows this at the top:

servo_ui_chrome

Instead of the expected browser.html chrome.

@emilio
Copy link
Member

@emilio emilio commented Jan 26, 2017

Seems that this may be related with the font stuff?

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Jan 26, 2017

It's possible these are related, but the elements here aren't font elements but other UI elements.

@jdm jdm added the P-windows label Jan 26, 2017
@larsbergstrom larsbergstrom mentioned this issue Feb 15, 2017
11 of 11 tasks complete
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 13, 2017

Can you add detail to the bug please? I can't reproduce with neither the nightly nor compiling servo myself. By the way I am running Windows 10.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Mar 13, 2017

I can confirm that it also displays this on my Windows 10 laptop. My Linux version of the nightly works fine though.

@atouchet
Copy link
Contributor

@atouchet atouchet commented Mar 13, 2017

I just tested the latest nightly build with Windows 10 and I am still seeing UI issues.

Windows UI

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Mar 13, 2017

Right - I'm seeing the same as @KiChjang and @atouchet.

Other details:

  • Running Windows 10
  • Running inside of VMware Fusion on a macOS
    • Using the "VMware SVGA 3D" driver
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 13, 2017

Thanks, about your previous comment are you sure the symbol or the UI aren't made of "characters" because they look like missing characters that I have on other pages (for example, the Chinese character ones at the bottom of the servo web page). On my home PC I can reproduce the issue but it is weird because the source code does not even display those characters in Chrome and Firefox. So maybe it is just some weird characters that are not in the font packaged with Windows by default?

EDIT : On my home computer they display nicely in Firefox and Chrome but not in Servo. So the bug is still there but it looks like a font issue to me. I confirmed this in Chrome, because if I toggle off the style 'font-family : FontAwesome' then it render a square like Servo does.

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Mar 13, 2017

Leaving notes here as I look around.

I noticed that if I install FontAwesome as a system TTF font, it worked fine. I reconverted it to woff, and the result was a little bigger than the one currently in the repo. Sure enough, when I tried the new woff, that also worked for me. Maybe the one that's there now was missing some sigils?

Update is here: browserhtml/browserhtml#1272

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 13, 2017

Would you mind checking how it renders on FF or Chrome while not having FontAwesome installed system wide? On my home computer the browser.html page render fine in Chrome and Firefox but not in Servo. Then, if it some missing sigils why it works on FF & Chrome and Linux and MacOS and not servo?

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Mar 13, 2017

@codec-abc - like I said, I don't know why updating the woff fixes the issue on Windows. I don't understand the code well enough. I was just experimenting to see if I could fix it.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 14, 2017

I opened the 2 woff files (the original one and the one repackaged by you) with fontforge and both contains the glyphs used by the Servo's UI. I have difficulties finding the code that does the work of reading and parsing the woff file.

EDIT: After quite a few searchs I found that the problem seems to be located here. It looks like that the original version cannot be loaded for some reason.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 15, 2017

I tried to reproduce the issue by creating a small repo containing the code that I think cause the problem. Basically, I read a woff file as a Vec pass it through fontsan (a font sanitizer library) and feed the result to dwrote (a DirectWrite wrapper). Strangely enough it managed to read both the original woff file and the a "fixed" one. Yet yesterday a breakpoint was only hit here with the original font file. I am starting to run out of idea to diagnose further this issue so if anyone has some tips on the subject I would gladly take them.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 16, 2017

Some update: My previous message was plain wrong because of a breakpoint hit in Visual studio while the underlying code was not run. That would be funny if only I didn't lost so much time chasing a bug in place where the code actually works. Using good old println! instruction I find the place where the code differ when using the original font vs the "fixed" one. It is right here and it make much more sense as a comment indicate that some external crate only support some "things" on mac. Commenting the two if in the methods make the original font works. But that does not seems the right things to do. So now I have to understand the purpose of those 2 if and the intended behavior and how to fix it for windows.

@codec-abc
Copy link

@codec-abc codec-abc commented Mar 16, 2017

I have understood the problem quite well. Those 2 checks (here and here) prevent to create a FontInfo structure where we wouldn't be able to return a font family name and a font face name. This is because the underlying library called truetype is not able to parse all the records properly. The proper things to do would be to add the missing piece in truetype and then remove the checks. But please note that the checks seems to not work properly even now. With the modified font file by @jonathandturner that pass the checks and display the missing UI elements I have a font family name and a font face strings of < unsupported > which is the default value for things that the library is not able to parse.

EDIT: The modified woff file is able to pass the check because when it was regenerated a "macintosh english" record was added.

@jonathandturner jonathandturner mentioned this issue Mar 20, 2017
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Mar 21, 2017
Fix Windows UI and font squishing

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes #15255)
* updates the Windows font metric calculation (fixes #15698, based on codec-abc #15937 (comment))
* may address #15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16034)
<!-- Reviewable:end -->
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 21, 2017

I think the PR made to remove this from the windows blockers is great but the underlying issue is still there. As such, I think it would be better if we would leave this open and remove this issue from the windows blockers. Do you agree?

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Mar 21, 2017

@codec-abc yes. Actually, do you want to open a new issue that's specifically that we can't load the old woff file? That way we can track it as a woff loading issue instead.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 21, 2017
…ndturner:fix_font); r=metajack

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes servo/servo#15255)
* updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment))
* may address servo/servo#15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : e9654e401876e04082919212e16f185c2992cdc9
@codec-abc
Copy link

@codec-abc codec-abc commented Mar 22, 2017

@jonathandturner I created #16078. This one can remain closed.

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue Mar 28, 2017
…ndturner:fix_font); r=metajack

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes servo/servo#15255)
* updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment))
* may address servo/servo#15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1
@atouchet atouchet reopened this Oct 26, 2017
@atouchet
Copy link
Contributor

@atouchet atouchet commented Oct 26, 2017

This is an issue again with the latest nightly build.

@atouchet
Copy link
Contributor

@atouchet atouchet commented Oct 26, 2017

@jonathandturner I updated Servo's version of Browser.html and it seems to have re-broken this in the process. Do you have an idea of whether this would be related to the Font Awesome WOFF file change you made or something else?

Edit: I verified that the WOFF file got reverted and is the problem here. I made browserhtml/browserhtml#1308 to see if re-applying the file change fixes things.

@atouchet
Copy link
Contributor

@atouchet atouchet commented Oct 26, 2017

Whoops, should probably close this after it's fixed in Servo itself.

@atouchet atouchet reopened this Oct 26, 2017
@atouchet atouchet mentioned this issue Oct 26, 2017
1 of 5 tasks complete
bors-servo added a commit that referenced this issue Oct 26, 2017
Update Browser.html again

Update Browser.html again to latest version to pick up fontawseome woff file change that got lost in the process. Should fix the missing interface icons on Windows.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #15255 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19025)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…ndturner:fix_font); r=metajack

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes servo/servo#15255)
* updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment))
* may address servo/servo#15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1

UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…ndturner:fix_font); r=metajack

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes servo/servo#15255)
* updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment))
* may address servo/servo#15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1

UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…ndturner:fix_font); r=metajack

<!-- Please describe your changes on the following line: -->

This PR:

* updates the browserhtml dep (fixes servo/servo#15255)
* updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment))
* may address servo/servo#15933.  With this PR, I was not able to repro

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because change are to UI/visual results of font drawing

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1

UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.