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.

Sign up for GitHub

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

Show tabs in pre elements #5470

Merged
merged 1 commit into from
Apr 3, 2015
Merged

Show tabs in pre elements #5470

merged 1 commit into from
Apr 3, 2015

Conversation

bjwbell
Copy link
Contributor

@bjwbell bjwbell commented Mar 31, 2015

Display tab characters as 4 spaces for pre and other elements that don't
compress whitespace.

Fixes #4651

@pcwalton r?

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

mbrubeck
mbrubeck reviewed Mar 31, 2015
View reviewed changes
components/gfx/text/shaping/harfbuzz.rs
@@ -521,6 +521,9 @@ impl Shaper {
// We elect to only space the two required code points.
if character == ' ' || character == '\u{a0}' {
advance = advance + options.word_spacing
} else if character == '\u{9}' || character == '\t' {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'\u{9}' and '\t' are the same value, so this is redundant.

@mbrubeck mbrubeck added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Mar 31, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 31, 2015

Pretty sure tabs should be 8 spaces.

@mbrubeck
Copy link
Contributor

Firefox uses 8 spaces per tab stop, and a tab advances to the next tab stop. (So in "AAA\tB" the tab only advances five spaces, because it starts at column three.) But that could be saved for a follow-up bug...

@bjwbell
Copy link
Contributor Author

bjwbell commented Mar 31, 2015

@mbrubeck sry, I wasn't thinking there.
@Ms2ger fixed, I should put in a todo, since tab size is controllable by https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size

@bjwbell
Copy link
Contributor Author

bjwbell commented Mar 31, 2015

@mbrubeck I looked at the gecko code (which does that) & decided I didn't want to try & implement the whole thing in one go. Also I'm not sure if their implementation is how I'd want to do it.

@mbrubeck mbrubeck added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 31, 2015
@mbrubeck mbrubeck closed this Mar 31, 2015
@mbrubeck mbrubeck reopened this Mar 31, 2015
@hoppipolla-critic-bot
Copy link

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

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.

@metajack
Copy link
Contributor

Assigned @mbrubeck since he seems to be doing the review.

@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 1, 2015

Looks great, thanks! I put one minor issue on Critic, but this can land with r=mbrubeck as soon as that's addressed.

@mbrubeck mbrubeck added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 1, 2015
@bjwbell
Copy link
Contributor Author

bjwbell commented Apr 3, 2015

@mbrubeck I removed the f64 cast, thnx for the catch.

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 3, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 3, 2015

Thanks! r=mbrubeck, needs squash.

@mbrubeck mbrubeck added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 3, 2015
@bjwbell
Show tabs in pre elements
29852bb 
Display tab characters as 8 spaces for pre and other elements that don't
compress whitespace.
@bjwbell
Copy link
Contributor Author

bjwbell commented Apr 3, 2015

@mbrubeck Thanks! squashed

bors-servo pushed a commit that referenced this pull request Apr 3, 2015
auto merge of #5470 : bjwbell/servo/tab-in-pre, r=mbrubeck
49ff683 
Display tab characters as 4 spaces for pre and other elements that don't
compress whitespace.

Fixes #4651

@pcwalton r?
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 3, 2015

Bors appears to be stuck; closing temporarily.

@Ms2ger Ms2ger closed this Apr 3, 2015
@Ms2ger Ms2ger reopened this Apr 3, 2015
bors-servo pushed a commit that referenced this pull request Apr 3, 2015
auto merge of #5470 : bjwbell/servo/tab-in-pre, r=mbrubeck
40fab8e 
Display tab characters as 4 spaces for pre and other elements that don't
compress whitespace.

Fixes #4651

@pcwalton r?
@Ms2ger Ms2ger merged commit 29852bb into servo:master Apr 3, 2015
@bjwbell bjwbell deleted the tab-in-pre branch August 4, 2015 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
S-needs-squash Some (or all) of the commits in the PR should be combined.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tabs in <pre> not shown
8 participants
@bjwbell @highfive @Ms2ger @mbrubeck @hoppipolla-critic-bot @metajack @bors-servo @jdm