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

Jump to bottom

Text color is not updated when a class is toggled #7814

Closed
paulrouget opened this issue Oct 1, 2015 · 8 comments
Closed

Text color is not updated when a class is toggled #7814

paulrouget opened this issue Oct 1, 2015 · 8 comments
Assignees
@mbrubeck
Labels
A-layout/uncategorized I-wrong

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 1, 2015 

<style>
  span {
    font-size: 30px;
    color: red;
  }

  span.selected {
    color: green;
  }
</style>

<body>
  <span class='selected'>A</span>
  <span>B</span>
  <span>C</span>
  <span>D</span>
  <pre></pre>
</body>

<script>
  var i = 0;
  setInterval(() => {
    document.querySelector('.selected').classList.remove('selected');
    var span = document.querySelectorAll('span')[i];
    span.classList.add('selected');
    document.querySelector('pre').textContent = span.textContent;
    i = (i + 1) % 4;
  }, 500);
</script>

In Firefox, text color changes from green to red. In Servo, nothing happen.
@paulrouget paulrouget mentioned this issue Oct 1, 2015
Closed
62 of 92 tasks complete
@jdm
Copy link
Member

@jdm jdm commented Oct 1, 2015

Out of curiosity, does it update if you resize the window or move the mouse over the text?
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Oct 1, 2015

If I do something like :hover{font-size: something}, it updates the color.
@jdm
Copy link
Member

@jdm jdm commented Oct 1, 2015

I suspect this occurs because modifying the class attribute through the DOMSettableToken list doesn't end up marking the node as dirty. Modifying the class attribute directly goes through this path instead, which would mark it dirty.
@jdm jdm added the A-content/dom label Oct 1, 2015
@jdm
Copy link
Member

@jdm jdm commented Oct 1, 2015

I suspect the DOMSettableToken methods that mutate attributes should explicitly mark the relevant element as dirty.
@mbrubeck mbrubeck self-assigned this Oct 1, 2015
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 1, 2015

That makes no sense. The attribute changing code should handle this; there's even code in Element::attribute_mutated that should.
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 1, 2015

Yes, I verified that the node is correctly marked dirty. Also, changing to use .setAttribute("class", ...) or .className = ... does not fix the behavior. The behavior depends on which style property is changed. color and background don't work, but font-size does. Maybe the problem is that we're not triggering the correct sort of incremental layout, or the incremental layout logic is buggy for these properties.
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 1, 2015

Debugging shows that style recalc runs as expected, and repair_if_possible is called, and correctly repairs the text fragments, but the display list still ends up with the old colors for some reason.

background-color updates correctly on block elements, but not on inline elements. Maybe this is related to Flow::repair_style which is a no-op for InlineFlow.

Disabling incremental layout makes the test case work correctly.
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 1, 2015

The problem is that repair_if_possible repairs the original UnscannedText fragments from the node's ConstructionResult, rather than the InlineFlow and its ScannedText fragments.

I think this case should just return false; because we can't repair the correct fragment(s) here.
mbrubeck added a commit to mbrubeck/servo that referenced this issue Oct 1, 2015
@mbrubeck
Incremental layout: Don't try to repair text fragment styles
59029be 
Fixes servo#7814.
@mbrubeck mbrubeck mentioned this issue Oct 1, 2015
Merged
mbrubeck added a commit to mbrubeck/servo that referenced this issue Oct 1, 2015
@mbrubeck
Incremental layout: Don't try to repair text fragment styles
6426f71 
Fixes servo#7814.
bors-servo pushed a commit that referenced this issue Oct 1, 2015
bors-servo
Auto merge of #7821 - mbrubeck:class-damage, r=pcwalton
bad3f04 
Incremental layout: Don't try to repair text fragment styles

Fixes #7814. r? @pcwalton

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7821)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 2, 2015
bors-servo
Auto merge of #7821 - mbrubeck:class-damage, r=pcwalton
Loading status checks…
69db834 
Incremental layout: Don't try to repair text fragment styles

Fixes #7814. r? @pcwalton

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7821)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mbrubeck mbrubeck

Labels
A-layout/uncategorized I-wrong
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@mbrubeck @jdm @Ms2ger @paulrouget
You can’t perform that action at this time.