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

style: Stop using PseudoElement::inherits_all. #21946

Merged
merged 2 commits into from Oct 15, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

style: Stop using PseudoElement::inherits_all.

This was done that way just because Servo didn't support the `all` property at
the time.

We should do it this way and optimize it if it's slow. Though I suspect that
most of stuff doesn't actually need to be inherited, my patch at bug 1498943
should make it much faster than what it would otherwise be.
  • Loading branch information
emilio committed Oct 15, 2018
commit 7ed3995725919e02f729a6f5ae83ff7bfba0366b
@@ -221,41 +221,9 @@ impl PseudoElement {
}
}

/// For most (but not all) anon-boxes, we inherit all values from the
/// parent, this is the hook in the style system to allow this.
///
/// FIXME(emilio): It's likely that this is broken in a variety of
/// situations, and what it really wants is just inherit some reset
/// properties... Also, I guess it just could do all: inherit on the
/// stylesheet, though chances are that'd be kinda slow if we don't cache
/// them...
/// To be removed.

This comment has been minimized.

@nox

nox Oct 15, 2018

Member

Why do we keep it then?

This comment has been minimized.

@emilio

emilio Oct 15, 2018

Author Member

Because I'm removing it in that Gecko bug I point above, which I'll sink in a few..

This comment has been minimized.

@emilio

emilio Oct 15, 2018

Author Member

sync*

pub fn inherits_all(&self) -> bool {
match *self {
PseudoElement::After |
PseudoElement::Before |
PseudoElement::Selection |
PseudoElement::DetailsContent |
PseudoElement::DetailsSummary |
// Anonymous table flows shouldn't inherit their parents properties in order
// to avoid doubling up styles such as transformations.
PseudoElement::ServoAnonymousTableCell |
PseudoElement::ServoAnonymousTableRow |
PseudoElement::ServoText |
PseudoElement::ServoInputText => false,

// For tables, we do want style to inherit, because TableWrapper is
// responsible for handling clipping and scrolling, while Table is
// responsible for creating stacking contexts.
//
// StackingContextCollectionFlags makes sure this is processed
// properly.
PseudoElement::ServoAnonymousTable |
PseudoElement::ServoAnonymousTableWrapper |
PseudoElement::ServoTableWrapper |
PseudoElement::ServoAnonymousBlock |
PseudoElement::ServoInlineBlockWrapper |
PseudoElement::ServoInlineAbsolute => true,
}
false
}

/// Covert non-canonical pseudo-element to canonical one, and keep a
@@ -585,7 +553,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> {
}
ServoInlineBlockWrapper
},
"-servo-input-absolute" => {
"-servo-inline-absolute" => {
if !self.in_user_agent_stylesheet() {
return Err(location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(name.clone())))
}
@@ -168,6 +168,33 @@ svg > * {
display: none;
}

/*
* For most (but not all) anon-boxes, we inherit all values from the
* parent.
*
* Anonymous table flows shouldn't inherit their parents properties in order
* to avoid doubling up styles such as transformations. Same for text and such.
*
* For tables, we do want style to inherit, because TableWrapper is
* responsible for handling clipping and scrolling, while Table is
* responsible for creating stacking contexts.
*
* StackingContextCollectionFlags makes sure this is processed
* properly.
*
* FIXME(emilio): inheriting all is a very strong hammer, and it's likely
* broken for stuff like table backgrounds and such. Gecko explicitly inherits
* what it wants, which seems a bit better off-hand.
*/
*|*::-servo-anonymous-table,
*|*::-servo-anonymous-table-wrapper,
*|*::-servo-table-wrapper,
*|*::-servo-anonymous-block,
*|*::-servo-inline-block-wrapper,
*|*::-servo-inline-absolute {
all: inherit;
}

/* style for text node. */
*|*::-servo-text {
margin: 0;
@@ -239,10 +266,16 @@ svg > * {
margin: 0;
}

/* The outer fragment wrapper of an inline absolute hypothetical fragment. */
/* The outer fragment wrapper of an inline absolute hypothetical fragment.
*
* FIXME(emilio): This was disabled because of a typo in the CSS parser,
* re-enable or figure out why it's not needed.
*|*::-servo-inline-absolute {
clip: auto;
border: none;
padding: 0;
margin: 0;
}
*/
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.