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

Enable the internal -x-lang CSS property #31723

Closed
Loirooriol opened this issue Mar 17, 2024 · 18 comments · Fixed by #31737
Closed

Enable the internal -x-lang CSS property #31723

Loirooriol opened this issue Mar 17, 2024 · 18 comments · Fixed by #31737
Labels
A-stylo C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.

Comments

@Loirooriol
Copy link
Contributor

The behavior of text-transform can be affected by the language of the text, e.g.

<div style="text-transform: uppercase">
  <span lang="tr">i</span> <!-- İ -->
  <span lang="en">i</span> <!-- I -->
</div>

Therefore, we need some way to track the language.

Gecko uses an internal -x-lang CSS property for that, so it seems a good idea to do the same (especially since we use the same style engine).

To do that,

  1. Clone a fork of https://github.com/servo/stylo/
  2. Enable -x-lang not just for gecko but also servo.
    https://github.com/servo/stylo/blob/7dd884031d6b97d801d50f8fcdff69bbeb292ae0/style/properties/longhands/font.mako.rs#L257-L266
  3. Run cargo build --features servo to ensure it builds fine
  4. Open a PR at https://github.com/servo/stylo/
  5. Here in Servo, update Cargo.toml to take the various style-related crates from your PR. Compile, this will automatically update Cargo.lock. See 871e36f for reference.
  6. In this case I don't expect anything to break, but otherwise you would need to do the changes necessary so that it still compiles fine.
  7. Open a PR with the Servo changes, so that we can run the tests and ensure everything is fine. -x-lang is an internal property, so I don't expect any test to complain.
  8. Then we can land the PR for Stylo
  9. Then update the PR for Servo, reverting the changes from Cargo.toml, but bumping Cargo.lock to point to the new upstream rev. See 0935bf5 for reference.
  10. Finally we can land the PR for Servo
@Loirooriol Loirooriol added E-less-complex Straightforward. Recommended for a new contributor. A-stylo labels Mar 17, 2024
@servo-highfive
Copy link

cc @emilio

@MunishMummadi
Copy link
Contributor

@servo-highfive assign me

@servo-highfive
Copy link

Hey @MunishMummadi! Thanks for your interest in working on this issue. It's now assigned to you!

@servo-highfive servo-highfive added the C-assigned There is someone working on resolving the issue label Mar 18, 2024
@MunishMummadi
Copy link
Contributor

Hello @Loirooriol,

I encountered an error while trying to build:

munish@Zoro:~/servo$ cargo build --features servo
error: none of the selected packages contains these features: servo
munish@Zoro:~/servo$

I'm wondering if I've missed something. I added the necessary local paths of my stylo repo to cargo.toml and uncommented the patch.crates-io for stylo. Here's the change I made to stylo:

${helpers.predefined_type(
    "-x-lang",
    "XLang",
    engines="gecko",
    initial_value="computed::XLang::get_initial_value()",
    animation_value_type="none",
    enabled_in="servo",
    has_effect_on_gecko_scrollbars=False,
    spec="Internal (not web-exposed)",
)}

I greatly appreciate your time and would be grateful for any guidance you could provide.

@sandeepB3
Copy link
Contributor

I think you have to replace engines="gecko" with engines="gecko servo", similar to other predefined_type like "font-family" to enable -x-lang for servo. But I think before that you may have to make some other modifications too, to properly support -x-lang for servo.

@MunishMummadi
Copy link
Contributor

Thanks for input @sandeepB3 . I will consider your suggestions and I will try to modify them.

@mrobinson
Copy link
Member

This is part of the Outreachy project, so please don't work on this one yet. Thank you!

@Rhea-Eve
Copy link
Contributor

Opened PR for step 7. See mentions above.

@Loirooriol
Copy link
Contributor Author

@MunishMummadi The cargo build --features servo should be run on the Stylo repository, not on Servo.

@MunishMummadi
Copy link
Contributor

MunishMummadi commented Mar 18, 2024

@mrobinson I am an outreachy applicant. @Loirooriol I did the things and everything is working fine. Can I push a PR.

@MunishMummadi
Copy link
Contributor

@Rhea-Eve Thank you for working on an issue assigned to me. I literally sat overnight and failed.

@Loirooriol
Copy link
Contributor Author

@MunishMummadi Martin meant to defer the bulk of the Outreachy project work for the actual internship after the contribution phase. But for something small like this I guess it's fine to do it now.

Anyways you were assigned first but @Rhea-Eve already posted the PRs, so maybe one of you can land the Stylo patch and the other the Servo patch?

@Rhea-Eve
Copy link
Contributor

Sorry! @Loirooriol linked me to this issue on Zulip, and I misread Martin's comment to say that this was now open for others. Would be happy to work with you on this. Let me know if you have any questions about my code. :).

@MunishMummadi
Copy link
Contributor

@Loirooriol thanks for the suggestion. Hey @Rhea-Eve can we discuss the details over Zulip. I can't find you in Zulip. can you ping me there!!!

@Rhea-Eve
Copy link
Contributor

Rhea-Eve commented Mar 19, 2024

@MunishMummadi How about you do the Stylo PR? As this seems to be the part that you have already started on. Then after that gets merged, I can update the PR for Servo.

If you need help with fixing the issues in your PR, you can look at the Stylo PR I did (servo/stylo#23).

@MunishMummadi
Copy link
Contributor

@Rhea-Eve Thanks for the heads up. I will be on it.

@MunishMummadi
Copy link
Contributor

hey @Rhea-Eve . I made the PR based on your suggestion. can u review it.

@Rhea-Eve
Copy link
Contributor

Rhea-Eve commented Mar 19, 2024

@MunishMummadi it seems like Oriol requested some changes. Once you make those, it looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stylo C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants