Adding sync method to update atrr from inline style updates

[craftytrickster]

#9307

I am not terribly confident in this PR, but getAttribute("style") does update correctly now. If the implementation here is more or less "correct", I will add unit tests to confirm that the attr for style is updated correctly. If it is completely off, I will gladly keep trying, but I also understand if someone else with more experience with Rust and this area of the application wants to do it.

There are a few things I do not like here, such as:

• Manually forming the string new_style in sync_property_with_attrs_style
• In the fn update_inline_style, I needed to coerce the value to a string slice, so I did &format!("{}", &property_decl.name(), but I am sure there is a better way to do this.

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[jdm]

r? @nox

[paulrouget]

You're trying to update the attribute, keeping the previous value. This won't work.

For example, this is valid:

elt.setAttribute("style", "meh:42");
console.log(elt.getAttribute("style")); // prints "meh:42"
elt.style.color = "red"; //
console.log(elt.getAttribute("style")); // prints "color:red"

With your code, i'd expect it to print: meh:42; color: red.

sync_property_with_attrs_style should not take a property and a value, but just empty the DOM style attribute, and replace it with the content of self.style_attribute().

Does it make sense?

[nox]

As stated before, you need to set the attribute value from the serialisation of the CSSStyleDeclaration instance.

[craftytrickster]

I think I understand what you are looking for. When I have time (hopefully tonight) to make the change, I will push it here.

[paulrouget]

Just some comments:

• Don't forget to remove the calls to document.content_changed in cssstyledeclaration.rs. Now that the style attribute mutates, content_changed should be called automatically.
• In https://github.com/servo/servo/pull/9378/files (PR that is not necessary anymore, thanks to your work), there's a test that I think should be added to this PR
• This doesn't use shorthands (border:2px solid red is expanded into border-left-style: solid; border-left-color: red; border-bottom-width: 2px;...)
• !important is missing

[nox]

You want to implement this algorithm.

[craftytrickster]

Thanks for comments, I will make the changes and implement algorithm as well. I should have done it beforehand.

[craftytrickster]

I know that @paulrouget mentioned that having issue #9307 solved would help in debugging. I have been extremely busy the past few weeks, it has been hard to find time and I feel bad that I am holding up this issue.

I am looking into serializing this correctly, but I have not solved it yet. In the meantime, it might provide value to integrate the current changes. Then someone (perhaps me if I am able to complete it on time) can simple replace the implementation of serialize in a different PR perhaps (without closing the current issue). This is a good reference for the implementation - https://github.com/AngleSharp/AngleSharp/blob/36e94f9492a1ee309af8a8bf0be50333d107b914/AngleSharp/Dom/Css/CssStyleDeclaration.cs#L2315

Please let me know what you think.

[craftytrickster]

I found some time to look at this tonight, I have a better idea of what to do now with the serialize piece, maybe I can finish this weekend.

[paulrouget]

I know that @paulrouget mentioned that having issue #9307 solved would help in debugging. I have been extremely busy the past few weeks, it has been hard to find time and I feel bad that I am holding up this issue.

Don't worry about that. Fixing this was important for #9378, but we landed a temporary fix.

So no hurry.

[craftytrickster]

That's good to hear, thanks.

[craftytrickster]

I would like some assistance looking at my latest commits if possible. During the serialize method, I do a lot of iter().cloned() that intuitively seems wrong. However, I am not sure of another way to do it.

Also, it seems that the pre-existing method serialize_shorthand (whose location I moved) is serializing incorrectly, with the following input: margin: 10px; margin-bottom: 13px it outputs - margin: 10px 10px 10px 13px;. It also does not take into account whether or not the attribute is important.

Thank you for the help and patience.

[KiChjang]

It'd be nice if you can put in some annotations about which step of the algorithm that the code is following. Just a // Step 1. above step 1 would be enough, for example. // Substep 1. for substeps, and so on.

[craftytrickster]

I will add that now.

[KiChjang]

What you probably want is a slice.

[craftytrickster]

How can you turn an iterator result (for example, after performing a filter) into a slice? I do not see any documentation that demonstrates that.

[KiChjang]

.collect::<[T]>()

[craftytrickster]

If I try .collect::<&[PropertyDeclaration]>(), I get the following message:

error: expected `<`, found `&`
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28520                     
.collect::&[PropertyDeclaration]>();

[KiChjang]

Yeah, sorry, you're not supposed to add a & there. Also, you're missing a <.

[KiChjang]

Hmm... this means that PropertyDeclaration doesn't have a compile-time known size... is it possible to #[derive(Sized)] for PropertyDeclaration? Otherwise, we'd have to do .collect::<Vec<PropertyDeclaration>>() and then form a slice from the vector afterwards.

[craftytrickster]

It still complains that the trait is not implemented for the type, even with the derive.

[KiChjang]

Oh, it's complaining about the [PropertyDeclaration] type in particular (note the square brackets). Are there any hints/notes that get printed after the error?

[craftytrickster]

These are all the errors. The Array remove one makes sense, since you cannot remove from a fixed array:

18:22: 28518:29 error: the trait `core::marker::Sized` is not implemented for the type `[properties::PropertyDeclaration]` [E0277]
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518                     .collect::<[PropertyDeclaration]>();
                                                                                                              ^~~~~~~
/home/divad/Rust/servo/components/style/lib.rs:75:5: 75:58 note: in this expansion of include!
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518:22: 28518:29 help: run `rustc --explain E0277` to see a detailed explanation
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518:22: 28518:29 note: `[properties::PropertyDeclaration]` does not have a constant size known at compile-time
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518:22: 28518:29 error: the trait `core::iter::FromIterator<properties::PropertyDeclaration>` is not implemented for the type `[properties::PropertyDeclaration]` [E0277]
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518                     .collect::<[PropertyDeclaration]>();
                                                                                                              ^~~~~~~
/home/divad/Rust/servo/components/style/lib.rs:75:5: 75:58 note: in this expansion of include!
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518:22: 28518:29 help: run `rustc --explain E0277` to see a detailed explanation
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28518:22: 28518:29 note: a collection of type `[properties::PropertyDeclaration]` cannot be built from an iterator over elements of type `properties::PropertyDeclaration`
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28516:21: 28516:34 error: the trait `core::marker::Sized` is not implemented for the type `[properties::PropertyDeclaration]` [E0277]
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28516                 let mut longhands = self.normal.iter().chain(self.important.iter()).cloned()
                                                                                                             ^~~~~~~~~~~~~
/home/divad/Rust/servo/components/style/lib.rs:75:5: 75:58 note: in this expansion of include!
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28516:21: 28516:34 help: run `rustc --explain E0277` to see a detailed explanation
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28516:21: 28516:34 note: `[properties::PropertyDeclaration]` does not have a constant size known at compile-time
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28516:21: 28516:34 note: all local variables must have a statically known size
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28578:39: 28578:45 error: no method named `remove` found for type `[properties::PropertyDeclaration]` in the current scope
/home/divad/Rust/servo/target/debug/build/style-f9fb073aea07cada/out/properties.rs:28578                             longhands.remove(index);

[KiChjang]

Yeah, we'll have to make a Vec<PropertyDeclaration>, which you already did in your latest commit.

[bors-servo]

☔ The latest upstream changes (presumably #9510) made this pull request unmergeable. Please resolve the merge conflicts.

[craftytrickster]

The wpt test for tests/wpt/mozilla/tests/css/removeproperty.html was failing due to the fact that node.dirty was not called during property removals. After my latest commit, this is now passing. I still see some wpt-test failures locally (such as viewport_percentage_vmin_vmax_a.html), but they seem intermittent/unrelated to the changes I made here.

Please let me know if anything else is missing here.

[bors-servo]

☔ The latest upstream changes (presumably #11192) made this pull request unmergeable. Please resolve the merge conflicts.

[craftytrickster]

With the latest rebase and changes, the unit test and wpt-test results in this branch are the same as the results from master branch.

@SimonSapin Is this able to be merged, or is there still something I am missing?

[bors-servo]

☔ The latest upstream changes (presumably #11326) made this pull request unmergeable. Please resolve the merge conflicts.

[SimonSapin]

I think this is good to go. I’ve rebased it and sent it to CI in #11377.

[craftytrickster]

Thank you everyone for your help and patience on this issue. I learned a quite a bit about Rust from doing this one (although it did take a little longer than expected).

[SimonSapin]

Thank you for keeping at it all this time!