HTML List works #1 #1524

aydinkim · 2014-01-21T06:57:14Z

added list sequence calculation
added list style parsing & cascading
added sequence converter (toward roman, alphabet etc)
added list marker

To Do.
support more list style
support more sequence type converter kind
add some TCs
boost up performance
appearance verification

highfive · 2014-01-21T06:57:18Z

Warning

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

hoppipolla-critic-bot · 2014-01-21T06:57:19Z

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

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.

recrack · 2014-01-22T00:20:15Z

Impl #1232 #805

aydinkim · 2014-01-22T06:05:39Z

@metajack I have heard from @recrack that you suggest us to add some ref tests.
I'm not sure that what kind of references can we test via ref test?
Until now, there is no list representation(or similar) on servo.
Do you have any idea?

metajack · 2014-01-22T18:59:22Z

Several suggestions were made yesterday. Since list markers are just text usually, you can use the unicode glyphs for that. Then you can test a list against a set of <p>&circle; foo</p> and it should look the same. Some care will need to be taken with the outer indent, etc, but it seems it should be possible to mimic the list.

(Where &circle; is whatever unicode is needed for the disc marker.

aydinkim · 2014-01-23T00:43:06Z

@metajack OK. This PR is not acceptable to merge, yet? I think we can just put this base code into mainstream and then add remained stuffs step by step.

SimonSapin · 2014-01-23T02:03:45Z

Please use the same terminology as the spec:

(List) sequence -> counter
Sequence converter -> counter style

http://dev.w3.org/csswg/css-lists/#counter
http://dev.w3.org/csswg/css-counter-styles/

SimonSapin · 2014-01-23T02:12:10Z

src/components/style/properties.rs.mako

+
+    // 12.5 Lists
+    ${new_style_struct("List")}
+    ${single_keyword("list-style-type", "disc circle square decimal decimal-leading-zero lower-roman upper-roman lower-greek lower-latin upper-latin armenian georgian lower-alpha upper-alpha none", inherited=True)}


Do not include here values that are not implemented yet (especially if they trigger fail!().) Keep them in a TODO comment instead.

Yes, I think your opinion is better.

SimonSapin · 2014-01-23T02:32:58Z

src/components/style/properties.rs.mako

+            let mut style_type = None;
+            let mut style_position = None;
+            let mut style_image = None;
+            for component_value in iter {


This isn’t quite right. Parsing none values in the list-style shorthand is tricky, because you don’t always know what longhand they belong to when you see them. You have to count the number of nones you see and assign them after the fact.

Have a look at the WeasyPrint code for this

SimonSapin · 2014-01-23T02:34:21Z

src/components/style/properties.rs.mako

+                if style_type.is_none() {
+                    match list_style_type::from_component_value(component_value) {
+                        Some(s) => { style_type = Some(s); continue },
+                        None => ()


Invalid values should not be silently ignored, but should have the declaration be rejected. (Use return None to do that.)

i added return None

SimonSapin · 2014-01-23T02:40:35Z

src/components/main/layout/converter_util.rs

+	fn to_roman(&self, number: int) -> ~str;
+}
+
+impl Roman for HashMap<int, ~str> {


This does not need a hashmap and can be done with a list ordered by greater-valued "digits" first. Also, it’d be nice to separate the roman-specific data from the generic "additive system" algorithm described in the spec. Then the same algorithm can be used for other counter styles.

http://dev.w3.org/csswg/css-counter-styles/#additive-system
http://dev.w3.org/csswg/css-counter-styles/#predefined-counters

i will add it, after implement counter-style.

SimonSapin · 2014-01-23T02:47:24Z

src/components/main/layout/construct.rs

+                        };
+                        match node.with_element(|e: &LayoutElement| {e.get_attr(None, "reversed")}) {
+                            Some(s) => {
+                                if s == ~"true" {is_reversed = true;}


reversed is a boolean attribute. The spec says:

The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

Note: The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

("Not allowed" here is an author requirement, not an implementation requirement)

So you want something like is_reversed = e.get_attr(None, "reversed").is_some()

You are correct. Thanks!

SimonSapin · 2014-01-23T02:52:25Z

src/components/main/css/user-agent.css

-ol[type="A"]        { list-style-type: upper-alpha; }
-ol[type="i"]        { list-style-type: lower-roman; }
-ol[type="I"]        { list-style-type: upper-roman; }
+ol[type="1"], li[type="1"] { list-style-type: decimal; }


This is wrong. <li> elements do not have this type attribute. Only <ol> does.

@SimonSapin
this UA is from http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#decohints

ol[type=1], li[type=1] { list-style-type: decimal; } ol[type=a], li[type=a] { list-style-type: lower-alpha; } ol[type=A], li[type=A] { list-style-type: upper-alpha; } ...

My bad, type on <li> elements is in the spec, I had missed it as it’s in the Obsolete features section. This diff chunk is fine, then.

SimonSapin · 2014-01-23T19:45:52Z

I’m concerned that this uses ad-hoc Rust code specific to lists rather than CSS counters. Was there a reason to not follow the plan I suggested in #1232 (comment) ?

That plan is only my opinion and I’m happy to discuss it if you disagree and possibly change my mind, but that conversation did not happen.

aydinkim · 2014-01-23T23:38:39Z

@SimonSapin
I had discussed with @pcwalton before start about code structural flow and he had suggested me that implement freely like float stuffs. In spite of there are still a few things to do like optimization, improvement, I think our code has not huge problem except for the way to calculate list style and attributes.

What I'm trying to tell you is that this commit is just a initial work for granting structurally allowance, not perfect commit. (You can see "#1" flag at the title of this issue)
Thanks for your suggestion and we are totally agree that your opinion in terms of CSS is better.
Progress is important as well as completeness in our side.
We just think that we will do that step by step..

I think that we can remove "ad-hoc Rust code specific to lists" in construct.rs if we will complete the works you suggested.
And I can inform you that @recrack is now modifying the commit according to your snippets, but not uploaded yet.

I'll be happy if I can get your opinion.

metajack · 2014-01-24T22:33:18Z

src/components/main/layout/block.rs

@@ -488,16 +492,32 @@ impl BlockFlow {
        box_.position.set(position);
    }

+    /// wrapper of build_display_list. For switching to html list.
+    pub fn build_display_list_wrapper<E:ExtraDisplayListData>(


It seems very odd that we'd need custom display list items for list markers. I don't think this is quite right. Can you explain the design decision here?

@metajack
There are 2 resons for making "build_display_list_wrapper"

To avoid code duplication
The code below is used both block and float function.
I think it is same code. Just purpose of refactoring.

// add box that starts block context for box_ in self.box_.iter() { box_.build_display_list(builder, dirty, offset, flow, list) }

To add display item which doesn't take effect on box calculation.
To represent display list marker, we had needed a list item. I think that normal list item affects box dimension calculation. but as I checked Firefox& Chrome, list marker does not belong to its own box. It was just a display level item for showing and acts independently. @pcwalton said to me that I should not make any other flow, and use the block flow to make list. So I had added custom list maker at there for avoiding any influence to box calculation. I thought that only difference of block and list is just exist or not of marker for now. (in aspect of appearance)

If it is not proper to design of servo, please let me know.
I'll follow your advice.


        Add CSS Section 12.5 Lists


        html list implementation


        Impl Number converter in counter_style.rs


        add FIXME: glyph_count is depending on the font.


        Update user-agent.css

recrack · 2014-02-04T00:37:20Z

@SimonSapin r?

SimonSapin · 2014-02-04T01:34:23Z

@recrack Explanations of why the code does something are better as code comment than github comment, which become hard to find after the review is done.

Other than that, I’m sorry but I am not able to do a full review at the moment. @larsbergstrom agreed take over.

larsbergstrom · 2014-02-06T19:53:59Z

@aydinkim @recrack

I'd like to see this work split into two parts.

Implement CSS counters in the 'style' crate. Specification is at:

http://dev.w3.org/csswg/css-lists/#counter

This will reuse code currently in layout/counter-style.rs. The goal of this commit is to have working support for the counter-increment and counter-reset functions, which are used in the implementation of lists. Also implementing counter-set should be very little work over counter-increment, and providing the counter() and counters() methods will allow you to write a test for the functionality.

After that, change the UA stylesheet to handle the list counting via counters. An example of that is at:

http://dev.w3.org/csswg/css-lists/#ua-stylesheet

This change will remove the code in layout/construct.rs that handles incrementing or decrementing the counters explicitly, as the style system will now handle tracking the counts as well as the correct marker-style.

This change should be able to reuse the changes you made to properties.rs.mako to identify the list-style-type, etc.

To insert the marker, rather than writing your own code in the flow and display list builders, please use the approach used for pseudo-element construction in #1496, which should land very soon.

I apologize for the delay in my feedback on this PR and will be more involved with these changes going forward. Please do not hesitate to ask me questions, either in a GitHub Issue or on IRC.

aydinkim · 2014-04-07T11:14:44Z

In our opinion, this is not a suitable commit for now. We had almost completed the Acid2 works and list spec. is not significant point for that works. We had decided that this works would be abandoned if anyone doesn't mind about this. Next time we might be able to make more better code.

SimonSapin reviewed Jan 23, 2014
View changes

metajack reviewed Jan 24, 2014
View changes

recrack and others added 5 commits Jan 9, 2014

Add CSS Section 12.5 Lists

6fecc26

html list implementation

4ecbc9a

Impl Number converter in counter_style.rs

11f2a4f

add FIXME: glyph_count is depending on the font.

bad3965

Update user-agent.css

c8a610c

Ms2ger closed this Apr 7, 2014

servo / servo

HTML List works #1 #1524

HTML List works #1 #1524

aydinkim commented Jan 21, 2014

highfive commented Jan 21, 2014

hoppipolla-critic-bot commented Jan 21, 2014

recrack commented Jan 22, 2014

aydinkim commented Jan 22, 2014

metajack commented Jan 22, 2014

aydinkim commented Jan 23, 2014

SimonSapin commented Jan 23, 2014

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Jan 23, 2014

aydinkim commented Jan 23, 2014

This comment has been minimized.

This comment has been minimized.

recrack commented Feb 4, 2014

SimonSapin commented Feb 4, 2014

larsbergstrom commented Feb 6, 2014

aydinkim commented Apr 7, 2014

servo / servo

Join GitHub today

HTML List works #1 #1524

HTML List works #1 #1524

Conversation

aydinkim commented Jan 21, 2014

highfive commented Jan 21, 2014

hoppipolla-critic-bot commented Jan 21, 2014

recrack commented Jan 22, 2014

aydinkim commented Jan 22, 2014

metajack commented Jan 22, 2014

aydinkim commented Jan 23, 2014

SimonSapin commented Jan 23, 2014

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Jan 23, 2014

aydinkim commented Jan 23, 2014

This comment has been minimized.

This comment has been minimized.

recrack commented Feb 4, 2014

SimonSapin commented Feb 4, 2014

larsbergstrom commented Feb 6, 2014

aydinkim commented Apr 7, 2014