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

Add support for having two separate parent styles. Fixes gecko bug 1382806. #17875

Merged
merged 3 commits into from Jul 26, 2017

Conversation

Projects
None yet
5 participants
@bzbarsky
Copy link
Contributor

commented Jul 26, 2017

This is needed for ::first-line support. See https://drafts.csswg.org/css-pseudo-4/#first-line-inheritance

This PR doesn't quite implement what the CSS spec draft says right now. It implements what Gecko does, which is what an earlier draft said.


  • There are tests for these changes OR
  • These changes do not require tests because servo doesn't support ::first-line yet

This change is Reviewable

bzbarsky added some commits Jul 26, 2017

Remove the inherited_style getter from StyleBuilder.
The concept of inherited style is about to get a bit more complicated, and this
will prevent consumers from doing it wrong.

Part 1 of Gecko bug1382806.  r=emilio
Give StyleBuilder separate inherited styles for inherited and non-inh…
…erited structs.

This is needed for ::first-line, which causes its kids to inherit different properties from different places.

Part 2 of Gecko bug 1382806.  r=emilio
@highfive

This comment has been minimized.

Copy link

commented Jul 26, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/style_resolver.rs, components/style/animation.rs, components/style/properties/longhand/font.mako.rs, components/style/values/computed/mod.rs and 6 more
  • @canaltinova: components/style/properties/longhand/inherited_text.mako.rs, components/style/style_resolver.rs, components/style/animation.rs, components/style/properties/longhand/font.mako.rs, components/style/values/computed/mod.rs and 6 more
  • @emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/style_resolver.rs, components/style/animation.rs, components/style/properties/longhand/font.mako.rs, components/style/values/computed/mod.rs and 6 more
@highfive

This comment has been minimized.

Copy link

commented Jul 26, 2017

warning Warning warning

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

emilio approved these changes Jul 26, 2017

Copy link
Member

left a comment

Looks fine. I think you may want to add the assertion in Gecko, fwiw.

r=me with the comments addressed.

@@ -2516,8 +2522,17 @@ impl<'a> StyleBuilder<'a> {
flags: ComputedValueFlags,
visited_style: Option<Arc<ComputedValues>>,
) -> Self {
debug_assert!(parent_style.is_some() == parent_style_ignoring_first_line.is_some());

This comment has been minimized.

Copy link
@emilio

emilio Jul 26, 2017

Member

nit: debug_assert_eq!.

debug_assert!(parent_style.is_some() == parent_style_ignoring_first_line.is_some());
// FIXME(bz): I wish I could assert that either
// parent_style == parent_style_ignoring_first_line or parent_style is
// a ::first-line style... But ComputedValues doesn't know its pseudo.

This comment has been minimized.

Copy link
@emilio

emilio Jul 26, 2017

Member

We could track it. The Servo one doesn't because layout doesn't use it, but there's no reason you can't poke at the ServoStyleContexts mPseudo from here actually... You could add a cfg'd is_first_line_style method on ComputedValues, if you think it's worth the effort.

@@ -2560,6 +2576,10 @@ impl<'a> StyleBuilder<'a> {
device,
parent_style,
inherited_style,
// None of our callers pass in ::first-line parent styles.

This comment has been minimized.

Copy link
@emilio

emilio Jul 26, 2017

Member

Hmm, I wonder if this is true when you animate an element inside a ::first-line...

This comment has been minimized.

Copy link
@bzbarsky

bzbarsky Jul 26, 2017

Author Contributor

Well, we always pass in the parent element style.

Whether this does the right thing when first-line is involved, I'm not sure. Will need to test.

/// have things like "box" vs "inherited_box" as struct names. Do the
/// next-best thing and call them `parent_${style_struct.name_lower}`
/// instead.
pub fn get_parent_${style_struct.name_lower}(&self) -> &style_structs::${style_struct.name} {

This comment has been minimized.

Copy link
@emilio

emilio Jul 26, 2017

Member

nit: Maybe inherited_${struct_name}, to be consistent with the other accessors you added?

(though the lack of get_ would be inconsistent with the other struct accessors...)

This comment has been minimized.

Copy link
@bzbarsky

bzbarsky Jul 26, 2017

Author Contributor

As discussed on irc, the naming is purposefully not using "inherited_", per the comments.

@bzbarsky bzbarsky force-pushed the bzbarsky:first-line-dual-inheritance branch from 7622b9b to 9ffbd71 Jul 26, 2017

Make it possible to construct StyleBuilder with two different inherit…
…ed styles.

Part 3 of Gecko bug 1382806.  r=emilio

@bzbarsky bzbarsky force-pushed the bzbarsky:first-line-dual-inheritance branch from 9ffbd71 to 048044f Jul 26, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

📌 Commit 048044f has been approved by emilio

@highfive highfive assigned emilio and unassigned mbrubeck Jul 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

⌛️ Testing commit 048044f with merge 020188f...

bors-servo added a commit that referenced this pull request Jul 26, 2017

Auto merge of #17875 - bzbarsky:first-line-dual-inheritance, r=emilio
Add support for having two separate parent styles.  Fixes gecko bug 1382806.

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

This is needed for ::first-line support.  See https://drafts.csswg.org/css-pseudo-4/#first-line-inheritance

This PR doesn't quite implement what the CSS spec draft says right now.  It implements what Gecko does, which is what an earlier draft said.

---
<!-- 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 https://bugzilla.mozilla.org/show_bug.cgi?id=1382806

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because servo doesn't support ::first-line yet

<!-- 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/17875)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

@bors-servo bors-servo merged commit 048044f into servo:master Jul 26, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.