Skip to content

Commit

Permalink
style: Move unboxing to style, and handle display: contents before ot…
Browse files Browse the repository at this point in the history 
…her suppressions.

This also adopts the resolution of [1] while at it, and switches XUL to not
support display: contents until a use case appears.

This makes our behavior consistent both with the spec and also in terms of
handling dynamic changes to stuff that would otherwise get suppressed.

Also makes us consistent with both Blink and WebKit in terms of computed style.
We were the only ones respecting "behaves as display: none" without actually
computing to display: none. Will file a spec issue to get that changed.

It also makes us match Blink and WebKit in terms of respecting display: contents
before other suppressions, see the reftest which I didn't write as a WPT
(because there's no spec supporting neither that or the opposite of what we do),
where a <g> element respects display: contents even though if it had any other
kind of display value we'd suppress the frame for it and all the descendants
since it's an SVG element in a non-SVG subtree.

Also, this removes the page-break bit from the display: contents loop, which I
think is harmless.

As long as the tests under style are based in namespace id / node name /
traversal parent, this should not make style sharing go wrong in any way, since
that's the first style sharing check we do at [2].

The general idea under this change is making all nodes with computed style of
display: contents actually honor it. Otherwise there's no way of making the
setup sound except re-introducing something similar to all the state tracking
removed in bug 1303605.

[1]: w3c/csswg-drafts#2167
[2]: https://searchfox.org/mozilla-central/rev/fca4426325624fecbd493c31389721513fc49fef/servo/components/style/sharing/mod.rs#700

Bug: 1453702
Reviewed-by: mats, xidorn
MozReview-Commit-ID: JoCKnGYEleD
  • Loading branch information
@emilio @Moggers
emilio authored and Moggers committed Jun 13, 2018
1 parent d2052a2 commit 70e9395
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 16 deletions.
9 changes: 9 additions & 0 deletions components/style/dom.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,15 @@ pub trait TElement:
/// Return whether this element is an element in the HTML namespace.
fn is_html_element(&self) -> bool;

/// Return whether this element is an element in the MathML namespace.
fn is_mathml_element(&self) -> bool;

/// Return whether this element is an element in the SVG namespace.
fn is_svg_element(&self) -> bool;

/// Return whether this element is an element in the XUL namespace.
fn is_xul_element(&self) -> bool { false }

/// Return the list of slotted nodes of this node.
fn slotted_nodes(&self) -> &[Self::ConcreteNode] {
&[]
Expand Down
24 changes: 17 additions & 7 deletions components/style/gecko/wrapper.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -673,11 +673,6 @@ impl<'le> GeckoElement<'le> {
self.as_node().node_info().mInner.mNamespaceID
}

#[inline]
fn is_xul_element(&self) -> bool {
self.namespace_id() == (structs::root::kNameSpaceID_XUL as i32)
}

#[inline]
fn has_id(&self) -> bool {
self.as_node()
Expand Down Expand Up @@ -826,7 +821,7 @@ impl<'le> GeckoElement<'le> {
match self.parent_element() {
Some(e) => {
e.local_name() == &*local_name!("use") &&
e.namespace() == &*ns!("http://www.w3.org/2000/svg")
e.is_svg_element()
},
None => false,
}
Expand Down Expand Up @@ -1058,7 +1053,22 @@ impl<'le> TElement for GeckoElement<'le> {

#[inline]
fn is_html_element(&self) -> bool {
self.namespace_id() == (structs::root::kNameSpaceID_XHTML as i32)
self.namespace_id() == structs::kNameSpaceID_XHTML as i32
}

#[inline]
fn is_mathml_element(&self) -> bool {
self.namespace_id() == structs::kNameSpaceID_MathML as i32
}

#[inline]
fn is_svg_element(&self) -> bool {
self.namespace_id() == structs::kNameSpaceID_SVG as i32
}

#[inline]
fn is_xul_element(&self) -> bool {
self.namespace_id() == structs::root::kNameSpaceID_XUL as i32
}

/// Return the list of slotted nodes of this node.
Expand Down
112 changes: 103 additions & 9 deletions components/style/style_adjuster.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//! A struct to encapsulate all the style fixups and flags propagations
//! a computed style needs in order for it to adhere to the CSS spec.

use Atom;
use app_units::Au;
use dom::TElement;
use properties::{self, CascadeFlags, ComputedValues, StyleBuilder};
Expand All @@ -23,6 +24,84 @@ pub struct StyleAdjuster<'a, 'b: 'a> {
style: &'a mut StyleBuilder<'b>,
}

#[cfg(feature = "gecko")]
fn is_topmost_svg_svg_element<E>(e: E) -> bool
where
E: TElement,
{
debug_assert!(e.is_svg_element());
if e.local_name() != &*atom!("svg") {
return false;
}

let parent = match e.traversal_parent() {
Some(n) => n,
None => return true,
};

if !parent.is_svg_element() {
return true;
}

parent.local_name() == &*atom!("foreignObject")
}

// https://drafts.csswg.org/css-display/#unbox
#[cfg(feature = "gecko")]
fn is_effective_display_none_for_display_contents<E>(element: E) -> bool
where
E: TElement,
{
// FIXME(emilio): This should be an actual static.
lazy_static! {
static ref SPECIAL_HTML_ELEMENTS: [Atom; 16] = [
atom!("br"), atom!("wbr"), atom!("meter"), atom!("progress"),
atom!("canvas"), atom!("embed"), atom!("object"), atom!("audio"),
atom!("iframe"), atom!("img"), atom!("video"), atom!("frame"),
atom!("frameset"), atom!("input"), atom!("textarea"),
atom!("select"),
];
}

// https://drafts.csswg.org/css-display/#unbox-svg
//
// There's a note about "Unknown elements", but there's not a good way to
// know what that means, or to get that information from here, and no other
// UA implements this either.
lazy_static! {
static ref SPECIAL_SVG_ELEMENTS: [Atom; 6] = [
atom!("svg"), atom!("a"), atom!("g"), atom!("use"),
atom!("tspan"), atom!("textPath"),
];
}

// https://drafts.csswg.org/css-display/#unbox-html
if element.is_html_element() {
let local_name = element.local_name();
return SPECIAL_HTML_ELEMENTS.iter().any(|name| &**name == local_name);
}

// https://drafts.csswg.org/css-display/#unbox-svg
if element.is_svg_element() {
if is_topmost_svg_svg_element(element) {
return true;
}
let local_name = element.local_name();
return !SPECIAL_SVG_ELEMENTS.iter().any(|name| &**name == local_name);
}

// https://drafts.csswg.org/css-display/#unbox-mathml
//
// We always treat XUL as display: none. We don't use display:
// contents in XUL anyway, so should be fine to be consistent with
// MathML unless there's a use case for it.
if element.is_mathml_element() || element.is_xul_element() {
return true;
}

false
}

impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
/// Trivially constructs a new StyleAdjuster.
#[inline]
Expand Down Expand Up @@ -377,20 +456,35 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
}
}

/// Native anonymous content converts display:contents into display:inline.
/// Handles the relevant sections in:
///
/// https://drafts.csswg.org/css-display/#unbox-html
///
/// And forbidding display: contents in pseudo-elements, at least for now.
#[cfg(feature = "gecko")]
fn adjust_for_prohibited_display_contents(&mut self) {
// TODO: We should probably convert display:contents into display:none
// in some cases too: https://drafts.csswg.org/css-display/#unbox
//
fn adjust_for_prohibited_display_contents<E>(&mut self, element: Option<E>)
where
E: TElement,
{
if self.style.get_box().clone_display() != Display::Contents {
return;
}

// FIXME(emilio): ::before and ::after should support display: contents,
// see bug 1418138.
if self.style.pseudo.is_none() || self.style.get_box().clone_display() != Display::Contents
{
if self.style.pseudo.is_some() {
self.style.mutate_box().set_display(Display::Inline);
return;
}

self.style.mutate_box().set_display(Display::Inline);
let element = match element {
Some(e) => e,
None => return,
};

if is_effective_display_none_for_display_contents(element) {
self.style.mutate_box().set_display(Display::None);
}
}

/// If a <fieldset> has grid/flex display type, we need to inherit
Expand Down Expand Up @@ -657,7 +751,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
self.adjust_for_visited(element);
#[cfg(feature = "gecko")]
{
self.adjust_for_prohibited_display_contents();
self.adjust_for_prohibited_display_contents(element);
self.adjust_for_fieldset_content(layout_parent_style);
}
self.adjust_for_top_layer();
Expand Down

0 comments on commit 70e9395

Please sign in to comment.