servo / servo

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

Support 'vertical-align' and 'line-height'. #764

Closed

june0cho wants to merge 5 commits into servo:master from june0cho:verticalalign

Contributor

june0cho commented Aug 23, 2013

This is a modification of the previous PR #741 to support 'vertical-align' and 'line-height'. Added test cases for vertical-align, line-height, and mixed.

In the last commit, after applying 'vertical-align' property, the current line-box's height and following line-boxes' heights are being updated. This is because line-box's height was already assigned when the line-box is created.
But there may be a better way, though I'm not sure. In Gecko, line-boxes' heights seem to be assigned at one time after 'vertical-align'. @metajack

june0cho added 3 commits


        Implement 'vertical-align' and fix 'line-height'.

3b3f3f8


        Add a test case for vertical_align and line-height

127c7b9


        Update linebox's height which is changed by vertical-align.

672d7f1

Contributor

metajack commented Aug 23, 2013

This isn't quite correct yet, at least as far as the end result goes. The following test file:

<p>
this is a paragraph with normal line height. this is a paragraph with normal
line height. this is a paragraph with normal line height. this is a paragraph
with normal line height. this is a paragraph with normal line height. this is
a paragraph with normal line height. this is a paragraph with normal line
height. this is a paragraph with normal line height.
</p>
<p style="line-height: 2">
this is a paragraph with line height of 2. this is a paragraph with line
height of 2. this is a paragraph with line height of 2. this is a paragraph
with line height of 2. this is a paragraph with line height of 2. this is a
paragraph with line height of 2. this is a paragraph with line height of
2. this is a paragraph with line height of 2.
</p>

produces

It apears that the first line is (maybe?) correctly sized, but the vertical-align is off. The remainder of the lines have incorrect height.

metajack commented on `src/components/main/layout/inline.rs` in `3b3f3f8` Aug 23, 2013

Factoring this out is great.

BrendanEich commented Aug 23, 2013

@dbaron has been writing

https://github.com/dbaron/inlines-and-floats

It's still under construction but worth a look.

/be


        Fix the line-height error in a multi-line page

8cd9a6b

Contributor Author

june0cho commented Aug 23, 2013

@metajack fixed the error! It was a simple mistake. thanks for comment :)

Contributor

yichoi commented Aug 23, 2013

ACID1 Pass !

Contributor

metajack commented Aug 23, 2013

@yichoi not quite. The reference image and servo's image show some very minor border issues on right borders:
http://note.io/178XHNa

This is unrelated to @june0cho's work, and should be easy to fix.

Contributor

brson commented Aug 25, 2013

@BrendanEich Thanks, I added that to https://github.com/mozilla/servo/wiki/General-implementation-and-design-notes

metajack reviewed

View changes

src/components/main/layout/inline.rs

  
                                }

                                text_bounds.translate(&Point2D(text_box.base.position.origin.x, Au(0)))

                            }

                                // Find the top and bottom of the content area.

metajack Aug 27, 2013

Contributor

Is this supposed to be a TODO comment? I don't see the code actually doing this.

metajack reviewed

View changes

src/components/main/layout/inline.rs Outdated

  
      @@ -654,80 +665,206 @@ impl InlineFlowData {
    
                        }

                    };

                    // Update the line's y position before setting the box's y position

metajack Aug 27, 2013

Contributor

I don't understand this comment. Can you explain more? Why would the current linebox ever cause a previous linebox to change values?

june0cho Aug 27, 2013

Author Contributor

My comment was not enough. I meant that the start offset(uppermost) of the current linebox is set by the previous line's height. line_height_offset is updated on Line 866.

metajack reviewed

View changes

src/components/main/layout/inline.rs Outdated

  
                        let parent_text_bottom  = Au(0);

                        do cur_box.with_mut_base |base| {

                            //get parent node

                            let mut parent = base.node;

metajack Aug 27, 2013

Contributor

A better idiom here is let parent = match base.node.parent_node() { None => base.node, Some(parent) => parent, }. We should minimize use of mut.

metajack Aug 27, 2013

Contributor

Actually, even better is to use map_default here I think.

metajack reviewed

View changes

src/components/main/layout/inline.rs Outdated

  
                            }

                            let font_size = match parent.style().font_size() {

                                CSSFontSizeLength(Px(length)) => length,

                                // todo: this is based on a hard coded font size, should be the parent element's font size

metajack Aug 27, 2013

Contributor

Isn't there already a function to determine this? We have to know this information to calculate the font size elsewhere.

june0cho Aug 27, 2013

Author Contributor

I've copied Line 747 ~ Line 750 from box.rs. The font size seems not to be correctly calcuated yet elsewhere as well.

metajack reviewed

View changes

src/components/main/layout/inline.rs

  
                                CSSFontSizeLength(Px(length)) => length,

                                // todo: this is based on a hard coded font size, should be the parent element's font size

                                CSSFontSizeLength(Em(length)) => length * 16f, 

                                _ => 16f // px units

metajack Aug 27, 2013

Contributor

So this fails for % values? Again, I think the other bits that deal with this could be reused here, assuming they actually exist.

june0cho Aug 27, 2013

Author Contributor

As the above comment, the font size seems not to support % values yet.

metajack reviewed

View changes

src/components/main/layout/inline.rs

  
                                CSSFontSizeLength(Em(length)) => length * 16f, 

                                _ => 16f // px units

                            };

                            parent_text_top = Au::from_frac_px(font_size);

metajack Aug 27, 2013

Contributor

I don't understand what this is. I thought it was the top of the parent's box, but apparently it's the height of the default linebox? This seems kind of odd.

june0cho Aug 27, 2013

Author Contributor

parent_text_top is used when vertical-align is 'text-top'. parent_text_top should be the top of parent's content area and content area is defined in http://www.w3.org/TR/CSS2/visudet.html#inline-non-replaced. But since I couldn't find how to extract the em-box of the font, I assumed that the parent_text_top is font-size of parent and parent_text_bottom is 0 (Line 736).

metajack reviewed

View changes

src/components/main/layout/inline.rs Outdated

  
                            parent_text_top = Au::from_frac_px(font_size);

                        }

                        let mut no_update_flag = false;

metajack Aug 27, 2013

Contributor

This variable could use a small comment.

Contributor

metajack commented Aug 27, 2013

@june0cho I have a lot of questions about this code. I suspect perhaps I am misunderstanding the names of things. Please ping me in IRC.


        Fix and add comments. Use `map_default`.

729ac0c

Contributor Author

june0cho commented Aug 27, 2013

In the new commit, I applied your comments which are left here and talked in IRC today.
Thanks for detailed comments! :)

metajack commented on `src/components/main/layout/inline.rs` in `729ac0c` Aug 27, 2013

Typo: vertex-align should be vertical-align

Owner Author

june0cho replied Aug 28, 2013

ohhh another same typo exists :( should fix it.

metajack commented on `729ac0c` Aug 27, 2013

r+

Contributor

bors-servo commented on `729ac0c` Aug 27, 2013

saw approval from metajack
at june0cho@729ac0c

Contributor

bors-servo replied Aug 27, 2013

merging june0cho/servo/verticalalign = 729ac0c into auto

Contributor

bors-servo replied Aug 27, 2013

june0cho/servo/verticalalign = 729ac0c merged ok, testing candidate = 4e68c1a

Contributor

bors-servo replied Aug 27, 2013

all tests pass:
http://servo-buildbot.pub.build.mozilla.org/builders/linux/builds/284
http://servo-buildbot.pub.build.mozilla.org/builders/mac/builds/239

Contributor

bors-servo replied Aug 27, 2013

fast-forwarding master to auto = 4e68c1a

bors-servo pushed a commit that referenced this pull request


        auto merge of #764 : june0cho/servo/verticalalign, r=metajack

4e68c1a

This is a modification of the previous PR #741 to support 'vertical-align' and 'line-height'. Added test cases for vertical-align, line-height, and mixed.

In the last commit, after applying 'vertical-align' property, the current line-box's height and following line-boxes' heights are being updated. This is because line-box's height was already assigned when the line-box is created. 
But there may be a better way, though I'm not sure. In Gecko, line-boxes' heights seem to be assigned at one time after 'vertical-align'. @metajack

bors-servo closed this

june0cho added a commit to june0cho/servo that referenced this pull request


        Fix typos made in PR servo#764

823bbdd

june0cho mentioned this pull request

Fix typos made in PR #764 #810

Merged

bors-servo pushed a commit that referenced this pull request


        auto merge of #810 : june0cho/servo/master, r=metajack

4aaaf60

Fix typos in PR #764.
Send new PR since #764 was merged before this modification.

sanxiyn mentioned this pull request

Use 'line-height' and 'vertical-align' when setting linebox element positions #226

Open

ChrisParis pushed a commit to ChrisParis/servo that referenced this pull request


        Merge pull request servo#764 from w3c/jgraham/manifest_bugfix

776e4c1

Fix manifest JSONifier for new iteration API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.