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

Merge adjacent text nodes while appending #5829

Closed
wants to merge 2 commits into from

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 24, 2015

Fixes #5828. r? @jdm or @kmcallister

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 24, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4788

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 24, 2015

Note: This only handles the append case. I'll file a follow-up issue to merge text nodes in append_before_sibling too. That will let us get rid of get_or_create completely.

Fixes #5828.
@jdm
Copy link
Member

jdm commented Apr 24, 2015

Reviewed files:

  • components/script/parse/html.rs @ r1

components/script/parse/html.rs, line 159 [r1] (raw file):
You can cast directly from Node->CharacterData instead



Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 24, 2015

@jdm
Copy link
Member

jdm commented Apr 24, 2015

Reviewed files:

  • components/script/parse/html.rs @ r2

Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Apr 24, 2015

+S-needs-squash -S-awaiting-review


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 25, 2015

This breaks the following reftests

  * `acid2_noscroll.html == acid2_ref_broken.html`
  * `filter_inline_a.html == filter_inline_ref.html`
  * `text_align_complex_a.html == text_align_complex_ref.html`

filter_inline_a.html is broken because of a comment in a <style> element. Servo doesn't re-parse the <style> contents when an existing Text child is modified (#976).

I haven't determined yet whether the other two tests are broken for similar reasons or different ones.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 25, 2015

mbrubeck/servo@7f7ecc2 is a sketch of a solution to the filter_inline_a breakage. It does not fix the other two failures, though. :(

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 25, 2015

And commented on critic

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 25, 2015

text_align_complex_a is broken because html5ever calls append_text separately for each '\r'-separated line of text in the <style> element, and even if we re-parse after each call, we end up processing the CSS rules in a bad order (since we can't yet unload the results of previous parses).

@bors-servo
Copy link
Contributor

bors-servo commented Jun 19, 2015

The latest upstream changes (presumably #6150) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm added the S-needs-rebase label Jun 21, 2015
@metajack
Copy link
Contributor

metajack commented Jul 29, 2015

@mbrubeck What's the plan for this?

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jul 29, 2015

I think this is blocked until other improvements are made to stylesheet handling, like the ability to remove or modify an existing stylesheet (#976). Closing this for now.

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.

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