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

layout: Mark flex items properly during construction #14130

Merged
merged 1 commit into from Nov 9, 2016

Conversation

@stshine
Copy link
Contributor

stshine commented Nov 8, 2016

Set the flag of the fragment of children in a flex container according
to the direction of the container. The mark is done on the fragment
because flex item enstablish a stacking context when its z-index is
non-zero ,despite its `position' property.

Part of #14123.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because refactoring

r? @pcwalton


This change is Reviewable

@highfive
Copy link

highfive commented Nov 8, 2016

Heads up! This PR modifies the following files:

  • @emilio: components/layout/construct.rs, components/layout/fragment.rs, components/layout/block.rs, components/layout/flex.rs, components/layout/model.rs, components/layout/flow.rs
@highfive
Copy link

highfive commented Nov 8, 2016

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@stshine
Copy link
Contributor Author

stshine commented Nov 8, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 8, 2016

Trying commit 5781dd4 with merge 767beae...

bors-servo added a commit that referenced this pull request Nov 8, 2016
layout: Mark flex items properly during construction

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

Set the flag of the fragment of children in a flex container according
to the direction of the container. The mark is done on the fragment
because flex item enstablish a stacking context when its z-index is
non-zero ,despite its `position' property.

Part of #14123.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

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

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

r? @pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Nov 8, 2016

@@ -3111,6 +3118,16 @@ impl Overflow {
}
}

bitflags! {
pub flags FragmentFlags: u8 {
// TODO(stshine): find a beter name since these flags can also be used for grid item.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Nov 8, 2016

Contributor

typo: "beter" -> "better"

@@ -996,6 +1002,7 @@ impl Fragment {
specific: info,
inline_context: self.inline_context.clone(),
pseudo: self.pseudo.clone(),
flags: FragmentFlags::empty(),

This comment has been minimized.

Copy link
@pcwalton

pcwalton Nov 8, 2016

Contributor

Have you verified that adding these flags does not change the size of Fragment?

This comment has been minimized.

Copy link
@jdm

This comment has been minimized.

Copy link
@stshine

stshine Nov 8, 2016

Author Contributor

This flags field are just the one that was removed by #13848 .

@stshine stshine force-pushed the stshine:construct-flexbox branch from 5781dd4 to 796ceac Nov 8, 2016
// TODO(stshine): find a better name since these flags can also be used for grid item.
/// Whether this fragment represent a child in a row flex container.
const IS_INLINE_FLEX_ITEM = 0b0000_0001,
/// Whether this fragment represent a child in a column flex container.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Nov 9, 2016

Contributor

nit: should say "represents" instead of "represent", also above

panic!("called as_mut_flex() on a non-flex flow")
}


This comment has been minimized.

Copy link
@pcwalton

pcwalton Nov 9, 2016

Contributor

nit: remove extra newline

@pcwalton
Copy link
Contributor

pcwalton commented Nov 9, 2016

r=me with a couple of nits

Set the flag of the fragment of children in a flex container according
to the direction of the container. The mark is done on the fragment
because flex item enstablish a stacking context when its z-index is
non-zero ,despite its `position' property.
@stshine stshine force-pushed the stshine:construct-flexbox branch from 796ceac to 7f721e1 Nov 9, 2016
@emilio
Copy link
Member

emilio commented Nov 9, 2016

@bors-servo r=pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2016

📌 Commit 7f721e1 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2016

Testing commit 7f721e1 with merge 333c397...

bors-servo added a commit that referenced this pull request Nov 9, 2016
layout: Mark flex items properly during construction

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

Set the flag of the fragment of children in a flex container according
to the direction of the container. The mark is done on the fragment
because flex item enstablish a stacking context when its z-index is
non-zero ,despite its `position' property.

Part of #14123.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

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

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

r? @pcwalton

<!-- 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/14130)
<!-- Reviewable:end -->
@stshine
Copy link
Contributor Author

stshine commented Nov 9, 2016

Thanks @emilio :)

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2016

@bors-servo bors-servo merged commit 7f721e1 into servo:master Nov 9, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@stshine stshine deleted the stshine:construct-flexbox branch Nov 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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