Skip to content

/ servo

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

Never blockify display: none #15056

Merged
merged 1 commit into from Jan 17, 2017
Merged

Never blockify display: none #15056

merged 1 commit into from Jan 17, 2017

Conversation

@upsuper
Copy link
Member

upsuper commented Jan 17, 2017 •
edited by larsbergstrom

This change is Reviewable
@upsuper
Never blockify display: none
Loading status checks…
5d2bb99
@highfive
Copy link

highfive commented Jan 17, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/properties.mako.rs
  • @emilio: components/style/properties/properties.mako.rs
@highfive
Copy link

highfive commented Jan 17, 2017

warning Warning warning

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

emilio commented Jan 17, 2017

Oh, wow, that's nasty.

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

📌 Commit 5d2bb99 has been approved by emilio
@emilio
Copy link
Member

emilio commented Jan 17, 2017

@bors-servo r-,

Actually, could you write a simple test for this?
@upsuper
Copy link
Member Author

upsuper commented Jan 17, 2017

A "simple" test for this single case is easy, but I guess we really want a more complete test to cover all blockify cases and all display values, which is harder :/
@upsuper
Copy link
Member Author

upsuper commented Jan 17, 2017

Gecko has "layout/style/test/test_flexbox_child_display_values.xhtml" for blockifying behavior inside flexbox, which can probably be migrated to wpt. But there are other cases, e.g. floating, absolutely-positioning, which also need blockifying.
@emilio
Copy link
Member

emilio commented Jan 17, 2017

I guess... Probably there are some tests for this already? I added some for display: contents on absolutely positioned elements and similar stuff to CSSWG, but of course we don't support them yet so that's kinda useless...

Let's see:

@bors-servo try

If it's blocking you hard or something, feel free to r=me directly, and file an issue for the test. Shouldn't be hard and could be a good easy issue?
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

⌛ Trying commit 5d2bb99 with merge cb7c875...
bors-servo added a commit that referenced this pull request Jan 17, 2017
@bors-servo
Auto merge of #15056 - upsuper:blockify-none, r=<try>
Loading status checks…
cb7c875 
Never blockify display: none

<!-- 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/15056)
<!-- Reviewable:end -->
@upsuper
Copy link
Member Author

upsuper commented Jan 17, 2017 •
edited

It's not blocking me. I just feel it's silly to file an issue for this trivial code change and ask other people to fix, given I've found how to fix it. But well, I didn't consider the cost of adding test... so I probably should have filed an issue instead of a pr :)
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

⌛ Testing commit 5d2bb99 with merge 7c11124...
bors-servo added a commit that referenced this pull request Jan 17, 2017
@bors-servo
Auto merge of #15056 - upsuper:blockify-none, r=emilio
Loading status checks…
7c11124 
Never blockify display: none

<!-- 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/15056)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
@bors-servo bors-servo merged commit 5d2bb99 into servo:master Jan 17, 2017
3 checks passed
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
@upsuper
Copy link
Member Author

upsuper commented Jan 17, 2017

Looks like @bors-servo doesn't recognize r- after r+... or there is some bug.
@upsuper upsuper mentioned this pull request Jan 17, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@upsuper @highfive @emilio @bors-servo @Manishearth
You can’t perform that action at this time.