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

deokjinkim commented Jan 16, 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)

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jan 16, 2014

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

highfive commented Jan 16, 2014

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

deokjinkim commented Jan 17, 2014

@metajack
Copy link
Contributor

metajack commented Jan 17, 2014

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

deokjinkim commented Jan 20, 2014

@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

deokjinkim commented Jan 23, 2014

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)
@metajack

This comment has been minimized.

Copy link

metajack commented on 3d94141 Jan 23, 2014

r+

This comment has been minimized.

Copy link

yichoi replied Jan 23, 2014

@bors: retry

This comment has been minimized.

Copy link

metajack replied Jan 23, 2014

@bors: retry

This comment has been minimized.

Copy link

metajack replied Jan 23, 2014

@bors: retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 3d94141 Jan 23, 2014

saw approval from metajack
at deokjinkim@3d94141

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

merging deokjinkim/servo/white_space = 3d94141 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

deokjinkim/servo/white_space = 3d94141 merged ok, testing candidate = b16c731

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

saw approval from metajack
at deokjinkim@3d94141

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

saw approval from metajack
at deokjinkim@3d94141

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

merging deokjinkim/servo/white_space = 3d94141 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

deokjinkim/servo/white_space = 3d94141 merged ok, testing candidate = 26fc108

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 23, 2014

fast-forwarding master to auto = 26fc108

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

deokjinkim commented Jan 23, 2014

@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
1 check passed
1 check passed
default all tests passed
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.

None yet

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