servo / stylo

39 stars 11 forks source link

Added support of style & color for `text-decoration` #31

Closed MunishMummadi closed 2 months ago

MunishMummadi commented 3 months ago

Description: This PR has same core changes that of #29 . As that PR has become a lot messier with unnecessary changes and improper formatting in the code. I made this PR for better understanding. If this is necessary I will close this PR and I update the previous PR.

Changes:

  1. Updated the sub_properties attribute of the text-decoration shorthand to include text-decoration-style and text-decoration-color for the Servo engine.

  2. Modified the parse_value function to parse text-decoration-style and text-decoration-color properties when the engine is Servo. The parsed values are stored in the style and color variables, respectively.

  3. Updated the ToCss implementation to serialize text-decoration-style and text-decoration-color properties for the Servo engine.

These changes ensure that the text-decoration shorthand in Servo correctly parses and serializes the text-decoration-style and text-decoration-color properties, providing a complete implementation of the shorthand.

Resolves: #25

mrobinson commented 3 months ago

You do not need to open a new PR. Instead you can just do a force push to your existing branch. If you do open a new PR though, please close the old one. I've done that.

Loirooriol commented 3 months ago

BTW, if what confused you to open a new PR was that I force-pushed the main branch, then you need to rebase with something like

git rebase --onto upstream/2024-01-16 upstream/2023-12-01 new-text-25

I have done it now.

MunishMummadi commented 3 months ago

Thnx @Loirooriol I am familiar with rebase. But this command is new & interesting to me. @mrobinson Thnx for closing it. I thought of doing that(amend followed by force-with-lease or force push). But that PR seemed too messy.

MunishMummadi commented 2 months ago

@Loirooriol .Do u also want me to perform tests before changes. I think I have to perform tests by following .

./mach build -r
./mach test-wpt -r --log-raw /tmp/servo.log tests/wpt/tests/css/css-text-decor/
./mach update-wpt /tmp/servo.log

Just a basic observation Before changes

  • 288 ran as expected. 0 tests skipped.
  • 1 tests failed unexpectedly
  • 4 tests passed unexpectedly
  • 3 tests had unexpected subtest results

After changes

  • 287 ran as expected. 0 tests skipped.
  • 5 tests failed unexpectedly
  • 1 tests passed unexpectedly
  • 3 tests had unexpected subtest results
Loirooriol commented 2 months ago

It's strange that you are getting unexpected failures. Anyways getting unexpected results before the changes probably implies that your PC has some different settings than the bots and may not be much reliable. So you can just create a PR for Servo to run the bots. There could also be relevant tests outside of that directory.

MunishMummadi commented 2 months ago

I use wsl for development. It might be a reason. I will push a PR with updated Cargo.toml and respective files.

MunishMummadi commented 2 months ago

Hey @Loirooriol what do you think about the test results. Do u want me to rebase this branch on top of main ?.

Loirooriol commented 2 months ago

Yes please rebase. About the failures, can you investigate if they are reasonable (like an existing bug that wasn't exposed because we only supported very basic text-decoration declarations) or there is something wrong in this patch?

MunishMummadi commented 2 months ago

Hey @Loirooriol sorry for the delay. My laptop got into trouble. I know my patch failed the checks. But I am not able to understand the error related to nsstring . I understood it has something to do with path .

error: failed to load manifest for workspace member `/home/runner/work/stylo/stylo/style`

Caused by:
  failed to load manifest for dependency `style_traits`

Caused by:
  failed to load manifest for dependency `nsstring`

Caused by:
  failed to read `/home/runner/work/xpcom/rust/nsstring/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
Error: Process completed with exit code [10](https://github.com/servo/stylo/actions/runs/8767278657/job/24060344107?pr=31#step:4:11)1.
Loirooriol commented 2 months ago

You didn't rebase correctly, this PR has unrelated commits: https://github.com/servo/stylo/pull/31/commits

MunishMummadi commented 2 months ago

Hey @Loirooriol I am trying to setup my new mac . I don't even know what happened but my PR got closed.

Loirooriol commented 2 months ago

I don't know what command you are using to rebase, but it's not working well: now the PR has 0 commits, that's probably why it got closed. Please run something like

git checkout new-text-25;
git fetch upstream d3240e081133e8e7fee586333610f7ff669a61b8;
git reset --hard FETCH_HEAD;
git rebase --onto main upstream/2024-01-16 new-text-25;
git push --force;