Fixing issue #213 (text-align: justify) and issues with non-left alignment on width_overflown lines

[FremyCompany]

I guess some changes will be necessary but the functionality itself looks ok. I uploaded test cases here: http://sdrv.ms/1b4QAvZ

[jdm]

Much more elegant: self.iter_slices_for_range(range).all(|(glyphs, _, _)| glyphs.is_whitespace())

[jdm]

_ => false

[jdm]

These should probably be bitfields if we expect to create lots of LineBoxes.

[jdm]

No need for (), spaces around >=

[jdm]

whitespace_width.to_f64().unwrap_or(1.0)

[jdm]

I don't think the f64 in the name adds any useful information.

[jdm]

Space around +

[jdm]

No need for ().

[jdm]

This is better an Option value instead of using a magic constant, I think.

[FremyCompany]

Well, this is only a temporary fix until I get how to loop in the reverse order, because then you can just use "let mut could_meet_leading_whitespace = true" and set it to false as soon as you see a non-whitespace box (because then you can do this loop and the next one in one single pass).

[jdm]

I think it would be better to create named temporary variables and pass those instead of using these comments.

[FremyCompany]

I think I should just create another instace function to Box, which would be split_asap(trim_whitespace_separators) which would provide those values as a default, and rename the "split_to_width_forced" method into "split_to_width_with_params" (and maybe mark it private).

[jdm]

That sounds reasonable to me.

[jdm]

I think we tend to discourage one-line things like this.

[jdm]

Maybe just

if !should_continue && (!trim_whitespace_separator || !glyphs.is_whitespace()) {
    break;
}

[jdm]

There are various constructs like

if ... {

    ...something

}

where the extra newlines should be compressed. I don't think they tend to help readability much.

[jdm]

+= new_size.width

[FremyCompany]

I didn't write this line :-)

[FremyCompany]

just figured out some lines were also commited here:
2d23073

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

[highfive]

Warning

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

[hoppipolla-critic-bot]

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

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.

[larsbergstrom]

@FremyCompany Thanks for this PR! I've commented in critic in detail on the issues to address. I'm excited to see this, though, and am very impressed you managed to jump into this part of the code for your first PR :-)

[FremyCompany]

Can we use the Ahem font for ref tests? I find it hard to write good ref tests without it.

[SimonSapin]

@FremyCompany I filed #1685 about making Ahem a requirement. In the meantime, go ahead and use it in your tests.

[FremyCompany]

I took some time to write an exhaustive test case, and found one remaining issue. The issue is not a regression and is not about "text-align: jusitify" but about "text-align: center" (an maybe "text-align: right" in some cases).

Indeed, it seems the default box splitting algorithm accept to break just after a whitespace, generating a box that contains but is not considered as a whitespace, and which is then considered in the alignment.

I think the solution would be to add "trim_whitespace_leaders" and "trim_whitespace_traillers" methods to ScannedTextBox and call them once the line alignment has been done (and deduce the size reduction of the line).

Does that seem good to you?

[FremyCompany]

I followed the approach state above and got "text-align: right/center" working properly. The work on the more general refactoring is still pending though.

[larsbergstrom]

@SimonSapin can you look at https://critic.hoppipolla.co.uk/showcomment?chain=2851 (the stylesheet change)?

[metajack]

@SimonSapin ping!

[SimonSapin]

@metajack I had already replied a while ago to the issue Lars liked to, but I just filed #2362 about <br>. As far as I can tell there is only two minor issues left, and this will need a rebase.

[jdm]

I filed #5008 about #1638 (comment). Since #4610 merged, I think the rest of this PR is now duplicated effort, unfortunately.