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

Invalidate node style after style property removed #9378

Merged
merged 1 commit into from Jan 27, 2016
Merged

Invalidate node style after style property removed #9378

merged 1 commit into from Jan 27, 2016

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Jan 19, 2016

Fix #9377

Review on Reviewable
@nox
Copy link
Member

nox commented Jan 19, 2016

@bors-servo r-

I'm pretty sure this is the wrong fix. You should make CSSStyleDeclaration change the style attribute of the element, and then Element::attribute_mutated will call Document::content_changed for you.
@nox nox self-assigned this Jan 19, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Jan 19, 2016

I'm pretty sure this is the wrong fix. You should make CSSStyleDeclaration change the style attribute of the element, and then Element::attribute_mutated will call Document::content_changed for you.

I just reproduced what setProperty and setPropertyPriority do.

Element::remove_inline_style_property and Element::update_inline_style apparently don't trigger attribute_mutated. Because of #9307 I guess.
@nox
Copy link
Member

nox commented Jan 23, 2016

setProperty and setPropertyPriority look wrong to me too.
@paulrouget
Copy link
Contributor Author

paulrouget commented Jan 23, 2016

setProperty and setPropertyPriority look wrong to me too.

Right. I'm expecting PR #9307 to remove all the calls to content_changed.
@paulrouget
Copy link
Contributor Author

paulrouget commented Jan 27, 2016

@nox, do you mind if we land this while waiting for PR #9410?

This is breaking browser.html HTML pretty badly, and RemoveProperty will eventually be fixed along setProperty and setPropertyPriority in PR #9410.
@KiChjang
Copy link
Member

KiChjang commented Jan 27, 2016

@bors-servo r+

@nox please don't hurt me.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

📌 Commit c6e902b has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

⌛ Testing commit c6e902b with merge 81381ea...
bors-servo added a commit that referenced this pull request Jan 27, 2016
@bors-servo
Auto merge of #9378 - paulrouget:removeProperty, r=KiChjang
81381ea 
Invalidate node style after style property removed

Fix #9377

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9378)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

💔 Test failed - mac-rel-css
@KiChjang
Copy link
Member

KiChjang commented Jan 27, 2016 

Ran 9335 tests finished in 1138.0 seconds.
  • 9333 ran as expected. 55 tests skipped.
  • 2 tests passed unexpectedly

Tests with unexpected results:
  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/perspective-containing-block-dynamic-1a.htm

  ▶ PASS [expected FAIL] /css-transforms-1_dev/html/transform-containing-block-dynamic-1a.htm

wat
@KiChjang
Copy link
Member

KiChjang commented Jan 27, 2016

Oh ok, the test failures aren't intermittents - it's a new pass after the changes made in this PR. @paulrouget you'll have to update the test expectations.
@paulrouget
Invalidate node style after style property removed
Loading status checks…
6d7dc0b
@paulrouget paulrouget force-pushed the paulrouget:removeProperty branch from c6e902b to 6d7dc0b Jan 27, 2016
@nox
Copy link
Member

nox commented Jan 27, 2016

@paulrouget @KiChjang Sounds reasonable. :)
@nox
Copy link
Member

nox commented Jan 27, 2016

@bors-servo r=KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

📌 Commit 6d7dc0b has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

⌛ Testing commit 6d7dc0b with merge 8c07362...
bors-servo added a commit that referenced this pull request Jan 27, 2016
@bors-servo
Auto merge of #9378 - paulrouget:removeProperty, r=KiChjang
Loading status checks…
8c07362 
Invalidate node style after style property removed

Fix #9377

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9378)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 6d7dc0b into servo:master Jan 27, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@paulrouget paulrouget mentioned this pull request Jan 29, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@paulrouget @nox @KiChjang @bors-servo @highfive
You can’t perform that action at this time.