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

Implement white-space property(pre) #1507

Merged
merged 1 commit into from Jan 23, 2014
Merged

Conversation

deokjinkim
Copy link
Contributor

In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)

@hoppipolla-critic-bot
Copy link

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

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.

@highfive
Copy link

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
  • @deokjinkim, please confirm that src/test/html/acid1.html and your favourite wikipedia page still render correctly!

@deokjinkim
Copy link
Contributor Author

@pcwalton r?

@metajack
Copy link
Contributor

I pointed out some nits and code duplication, but I think this is a sub-optimal solution.

Creating line break boxes looks like it works, but really we should not create these fake boxes which are only used to signal the line breaks. Instead, I think scan_for_lines should handle this logic.

Basically the idea is that the the white-space: normal mode tries to append each box to the line box. In white-space: pre mode, it should instead just make each box into its own line box.

When white-space: pre-wrap and other modes are added later, scan_for_lines can do a mixture of the things.

This seems like it makes the logic much simpler.

Also, I think it's ok that we compute newline locations in the text transforming function, but I'm less sure about where we should actually break the text runs. I think what you're doing now is probably useful, since we know it will need to be broken in those spots in all pre modes. Those might get broken more later.

@metajack metajack mentioned this pull request Jan 17, 2014
@deokjinkim
Copy link
Contributor Author

@metajack, r?

Thank you for your kind review~!!
I revised code for white-space(pre).
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)

@deokjinkim
Copy link
Contributor Author

I modified comment(main/layout/inline.rs).
@metajack r?

bors-servo pushed a commit that referenced this pull request Jan 23, 2014
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
bors-servo pushed a commit that referenced this pull request Jan 23, 2014
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
@deokjinkim
Copy link
Contributor Author

@metajack I have same problem with aydinkim([servo] fix option order of make servo (#1539)). I don't know why this build error is occurred.

bors-servo pushed a commit that referenced this pull request Jan 23, 2014
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
bors-servo pushed a commit that referenced this pull request Jan 23, 2014
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
bors-servo pushed a commit that referenced this pull request Jan 23, 2014
In order to support line-break by new-line character,
Fist, calculate new-line character's position.(fn flush_clump_to_list)
Second, split box(which includes new-line character) and do line-break.(fn scan_for_lines)
@bors-servo bors-servo merged commit 3d94141 into servo:master Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants