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

Use the new style system #1109

Merged
merged 1 commit into from Oct 23, 2013
Merged

Use the new style system #1109

merged 1 commit into from Oct 23, 2013

Conversation

@sanxiyn
Copy link
Contributor

sanxiyn commented Oct 23, 2013

Regressions are:

  • Incremental layout is broken
  • :link is broken
  • Source URL is not passed to CSS parser
  • text-decoration propagation for block containers establishing an inline formatting context is not handled

This also does not remove NetSurf libcss from the build. That can be done in a followup.

This was a team effort. Credits to Deokjin Kim, Ilyong Cho, Jaeman Park, Junyoung Cho, Ryan Choi, Sangeun Kim, Yongjin Kim, Youngmin Yoo, Youngsoo Son.

Credits to:
    Deokjin Kim
    Ilyong Cho
    Jaeman Park
    Junyoung Cho
    Ryan Choi
    Sangeun Kim
    Yongjin Kim
    Youngmin Yoo
    Youngsoo Son
@yichoi
Copy link
Contributor

yichoi commented Oct 23, 2013

this can be a good starting point to transit new css. thanks all

@@ -356,6 +359,7 @@ fn parse_qualified_name(iter: &mut Iter, allow_universal: bool, namespaces: &Nam
}
},
Some(&Delim('|')) => explicit_namespace(iter, allow_universal, Some(~"")),
Some(&IDHash(*)) => default_namespace(namespaces, None),

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 23, 2013

Member

This seems wrong: an ID selector would be parsed as an universal selector?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 23, 2013

Member

Apparently this is a work-around for a larger bug in selector parsing. Let’s take this, and I’ll work on the underlying bug next.

Some(&Comma) => (),
Some(&Comma) => {
iter.next();
}

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 23, 2013

Member

I don’t understand this. Shouldn’t the peeked comma be consumed so that it’s not part of the next selector?

@SimonSapin

This comment has been minimized.

Copy link

SimonSapin commented on b243191 Oct 23, 2013

This looks good to me except for the changes in src/components/style/selectors.rs, see inline comments.

This comment has been minimized.

Copy link

SimonSapin replied Oct 23, 2013

r+

This comment has been minimized.

Copy link

metajack replied Oct 23, 2013

r=SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on b243191 Oct 23, 2013

saw approval from SimonSapin
at sanxiyn@b243191

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 23, 2013

merging sanxiyn/servo/new-style = b243191 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 23, 2013

sanxiyn/servo/new-style = b243191 merged ok, testing candidate = 9b25df1

This comment has been minimized.

Copy link
Contributor

bors-servo replied Oct 23, 2013

fast-forwarding master to auto = 9b25df1

bors-servo pushed a commit that referenced this pull request Oct 23, 2013
Regressions are:
* Incremental layout is broken
* `:link` is broken
* Source URL is not passed to CSS parser
* `text-decoration` propagation for block containers establishing an inline formatting context is not handled

This also does not remove NetSurf libcss from the build. That can be done in a followup.

This was a team effort. Credits to Deokjin Kim, Ilyong Cho, Jaeman Park, Junyoung Cho, Ryan Choi, Sangeun Kim, Yongjin Kim, Youngmin Yoo, Youngsoo Son.
@bors-servo bors-servo merged commit b243191 into servo:master Oct 23, 2013
1 check passed
1 check passed
default all tests passed
SimonSapin added a commit to SimonSapin/servo that referenced this pull request Oct 23, 2013
I have no idea how to test it,
but this code builds and is close enough to what it was befor servo#1109.

Review much needed.
bors-servo pushed a commit that referenced this pull request Oct 23, 2013
Following up on #1109. These three commits are independent.
@SimonSapin
Copy link
Member

SimonSapin commented Oct 23, 2013

Incremental layout is broken

I think I fixed it in #1112, but I don’t know how to test it. Could you take a look?

:link is broken

:not() is the only pseudo-class supported by the new selector matching at the moment. More can be added easily. Where can I find out what selectors we used to support with netsurfcss?

Source URL is not passed to CSS parser

Right now the parser does not parse anything that involves URLs. When it does, we’ll need to make sure the base URL is available. We’re using extra::url::Url, but do you know how we handle relative URLs? extra::url doesn’t seem to do it.

text-decoration propagation for block containers establishing an inline formatting context is not handled

I don’t understand what this means. What part of the code is responsible?

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

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