Pseudo element ":before" #1158

parkjaeman · 2013-10-31T05:39:05Z

This pr is regarding to implementation of pseudo element.
To make pseudo element work normally, we invalidated a assert which is "assert!(style_attribute..." in selector_matching.rs. From the assertion, servo crashes on all the test case with inline style.

Credit with ILyaon.

pcwalton · 2013-10-31T05:48:44Z

You can't add managed boxes to LayoutData. It will cause shutdown crashes as the script task tries to destroy boxes belonging to the layout task. That's why the function that is not called was added: to prevent this from happening. Use Arc instead.

SimonSapin · 2013-10-31T23:06:08Z

src/components/style/selectors.rs

@@ -24,6 +24,7 @@ pub enum PseudoElement {
    After,
    FirstLine,
    FirstLetter,
+    none,


This should not be necessary, as we’re using Option<PseudoElement> everywhere.

SimonSapin · 2013-10-31T23:07:56Z

src/components/style/selector_matching.rs

@@ -267,7 +278,7 @@ fn matches_first_child<N: TreeNode<T>, T: TreeNodeRefAsElement<N, E>, E: Element
 }

 #[inline]
-fn match_attribute<N: TreeNode<T>, T: TreeNodeRefAsElement<N, E>, E: ElementLike>(
+fn match_attribute<N: TreeNode<T>, T: TreeNodeRefAsElement<N, E, PseudoElement>, E: ElementLike>(


Sigh. This is getting out of hand. (Not a criticism of this PR, but we’ll need to get rid of these traits somehow.)

SimonSapin · 2013-10-31T23:10:03Z

src/components/style/properties.rs.mako


+    <%self:raw_longhand name="content" inherited="False">
+            pub use to_computed_value = super::computed_as_specified;
+            pub type SpecifiedValue = ~str;


Even if no type of value other than is supported yet, this should really be an enum of normal, none, or a list of values.

SimonSapin · 2013-10-31T23:11:18Z

src/components/style/properties.rs.mako

+            #[inline] pub fn get_initial_value() -> computed_value::T  { ~"" }
+            pub fn parse_specified(input: &[ComponentValue]) -> Option<DeclaredValue<SpecifiedValue>> {
+                let mut iter = input.skip_whitespace();
+                match iter.next() {


Do not leave the rest of this iterator un-consumed. Properties that only take one value should reject input with more, but in this case this really is a list.

SimonSapin · 2013-10-31T23:14:23Z

src/components/style/selector_matching.rs

-            &self, element: &T, style_attribute: Option<&PropertyDeclarationBlock>,
-            pseudo_element: Option<PseudoElement>) -> ~[Arc<~[PropertyDeclaration]>] {
+    pub fn get_applicable_declarations<N: TreeNode<T>, T: TreeNodeRefAsElement<N, E, PseudoElement>, E: ElementLike>(
+            &self, element: &T, style_attribute: Option<&PropertyDeclarationBlock>) -> (~[Arc<~[PropertyDeclaration]>], ~[Arc<~[PropertyDeclaration]>]) {


I disagree with this change of function signature, it doesn’t work as soon as you implement any other pseudo-element. Or do you want the function then to return a bigger data structure with all of them? I’ll need more convincing on why the complexity is better than having this call for each pseudo-element (or none).

SimonSapin · 2013-10-31T23:15:36Z

src/components/util/tree.rs

@@ -243,8 +244,10 @@ pub trait TreeNodeRef<Node>: Clone {
    fn is_root(&self) -> bool;
 }

-pub trait TreeNodeRefAsElement<Node, E: ElementLike>: TreeNodeRef<Node> {
+pub trait TreeNodeRefAsElement<Node, E: ElementLike, PseudoElement>: TreeNodeRef<Node> {


Is this generic to work around circular dependencies? I’d rather move the PseudoElement enum to libutil.

SimonSapin · 2013-10-31T23:19:07Z

src/components/script/dom/node.rs

+    pseudo_style: Option<ComputedValues>,
+
+    /// The enum value of pseudo element
+    pseudo_element: Option<PseudoElement>,


I don’t think that "having a pseudo-element" (and only one!) should be a property of DOM nodes. It’s box/frame generation that checks whether a box should be created for any pseudo-element.

Do you mean that LayoutData of Node should not have the data which decides whether a box for pseudo-element is needed or not?
If so, where do you think the data is included?

Now I am trying to move pseudo node generation from push_node() to construct_recursively() because pseudo element seems to have its own box generator to cover pseudo element with display = block. In current design, pseudo element with display = inline is only considered.

I moved pseudo element creation from original location of render box creation to location of box generator creation. Then, current design can cover pseudo element of which display value is block as well as inline.

SimonSapin · 2013-10-31T23:24:31Z

Sorry, but I believe that this patch in 9450e0d has not just minor issues to fix, but the whole design should be revised. In particular, a DOM not does not have a single optional pseudo-elements, but box generation may be affected by a number of pseudo-elements on each element.

Also, I’m not convinced that the style system should handle all declarations for an element and its pseudo-elements at the same time, rather than having the caller request styles for a given pseudo-element (or none for the element itself.)


        Impl 2.1 Section 12 - Generated content


        Implementation in CSS parsing/selector-matching regarding to pseudo e…

…lement 'before'.


        Generate a new render box for pseudo element


        Transfer pseudo_parent_element & pseudo_text from LayoutData to Node


        Move PseudoElement from libstyle to libtree, restore the trait TreeNo…

…deRefAsElement to its original state and remove unnecessary value in enum PseudoElement


        Move pseudo element creation from location of render box creation to …

…location of box generator creation

yichoi · 2013-11-08T05:33:39Z

please rebase it

parkjaeman · 2013-11-08T08:00:28Z

We are doing rebase now


        Modify content property according to CSS 2.1

deokjinkim · 2013-11-08T11:03:57Z

We finished rebasing our code.

jdm · 2013-11-08T12:04:56Z

Is it possible for script to access pseudo elements/text? These nodes won't have JS wrappers right now, which sounds bad to me.

parkjaeman · 2013-11-08T23:44:59Z

@jdm In current design, script does not access to pseudo element. As we know, such design is conforming with CSS 2.1 specification. Please check the sentence "Neither pseudo-elements nor pseudo-classes appear in the document source or document tree." in 5.10 Pseudo-elements and pseudo-classes.

SimonSapin · 2013-11-09T03:40:33Z

@jdm getComputedStyle takes an optional argument to access style of pseudo-elements, but there is no DOM node for pseudo-elements. So there is access to the "content" property, but I don’t know a way to access the actual generated text.

parkjaeman · 2013-11-10T23:51:41Z

r? @SimonSapin

SimonSapin · 2013-11-11T00:23:04Z

@parkjaeman

In style/properties.rs.mako, the parser for the 'content' property should support the 'none' and 'normal' keywords (only by themselves), and should return None when encountering unsupported values instead of ignoring them silently.
In style/selector_matching.rs, same comment as earlier: The get_applicable_declarations function used to take an optional pseudo_element parameter, indicating which styles should be returned. You change it to not take that parameter, and have it return both "normal" and pseudo-element style. Is this only for the ::before pseudo-element? How will this work when more pseudo-elements are supported, return a more complex data structure? How is this better than the previous design?
In DOM nodes, same as above: Is .pseudo_style() only for ::before? How will this be extended for more pseudo-elements?
I don’t understand what the pseudo_element and related fields are in LayoutData. Can you explain what the node tree looks like when pseudo-elements are involved?

Also, I’m less familiar with box generation code and I would appreciate someone else to review that part :)

jdm · 2013-11-11T05:30:29Z

I am worried about the layout-created nodes sitting inside the script-created nodes. When the script task GCs the script nodes, we will end up freeing layout boxes in the script task, which has caused crashes before.

SimonSapin · 2013-11-11T06:02:48Z

I’m not convinced that pseudo-elements should be DOM nodes at all. My mental model is that one element (DOM node) creates more than one CSS box.

parkjaeman · 2013-11-12T00:22:39Z

@SimonSapin
Regarding to the following comment,

"In style/selector_matching.rs, same comment as earlier: The get_applicable_declarations function used to take an optional pseudo_element parameter, indicating which styles should be returned. You change it to not take that parameter, and have it return both "normal" and pseudo-element style. Is this only for the ::before pseudo-element? How will this work when more pseudo-elements are supported, return a more complex data structure? How is this better than the previous design?"

Do you think that get_applicable_declarations() has to be called in main/css/matching.rs:match_node() for normal style and all kinds of pseudo element. In that case, we have to call get_applicable-declarations() 5 times for each element, if we consider 4 pseudo elements(first-line, first-letter, before and after), although most element does not include pseudo element. Then, matches_selector() in get_applicable_declarations() has to call 5 times than current design on my pull request where matches_selector() is called just one time for each element & for each rule. Hence, I propose following two approaches,

First, the function get_applicable_declarations() is modified to return a vector of applicable_declarations where length of the vector is same with pseudo element's number plus 1.

Second, we transfer match_node() into style/selector_matching.rs and merge the function with get_applicable_declarations().

How do you think about those approach?

jdm · 2013-11-12T15:29:37Z

I asked Boris for his thoughts on using actual DOM elements:

<bz> So we tried doing it without creating DOM nodes in Gecko
<bz> We stopped
<bz> because it required so much special-casing in various code
<bz> So it might not be _necessary_
<bz> but it was fairly hard to make it behave sanely otherwise
<bz> lemme find the relevant bug
* bz waits for hg blame
<bz> I guess at least in part it depends on how servo's code for this stuff is structured
<bz> "this stuff" being "layout"
<bz> and what it does and doesn't assume about a DOM
<bz> Ah
[<bz> So we did this when we started wanting to do things like float and position generated content
[<bz> https://bugzilla.mozilla.org/show_bug.cgi?id=238072
<firebot> Bug 238072 enh, P3, Future, roc, RESO FIXED, [GC] support positioned and floated generated content for non-replaced elements
<bz> Because otherwise we had to basically duplicate all the box-construction logic for it
<bz> Whereas this way we just have an element, we send it in through the normal logic, and off we go
<bz> Basically, whatever the abstraction layout sees for "element" should probably be associated with ::before/::after
<bz> Whether that involves an actual DOM element is a separate question, I guess

SimonSapin · 2013-11-20T16:28:37Z

@parkjaeman

For the parsing of the 'content' property, please use SimonSapin@b64bfbe . In addition to whitespace and naming it parses the "normal" and "none" values, and returns None on unsupported values.

Regarding selector matching:

Do you think that get_applicable_declarations() has to be called in main/css/matching.rs:match_node() for normal style and all kinds of pseudo element. In that case, we have to call get_applicable-declarations() 5 times for each element, if we consider 4 pseudo elements(first-line, first-letter, before and after), although most element does not include pseudo element.

Yes, that is the design I had in mind.

Note that before your changes, the first thing matches_selector does is check the pseudo element. == on a C-like enum is very cheap. If that doesn’t match, the rest of the selector is not even looked at. (&& is "short-circuit").

Then, matches_selector() in get_applicable_declarations() has to call 5 times than current design on my pull request where matches_selector() is called just one time for each element & for each rule. Hence, I propose following two approaches,

First, the function get_applicable_declarations() is modified to return a vector of applicable_declarations where length of the vector is same with pseudo element's number plus 1.

If you’re worried about the cost of doing multiple stylesheet traversal per element (ever though the cost of each step is small), I could agree with your idea of having get_applicable_declarations returning a bigger data structure for declarations of the element and all of its pseudo-elements. That data structure should be extensible for more pseudo-elements than just ::before.

I am very much against using a vector where each index has a special meaning. Please use a struct with fields like element, before_pseudo_element, etc.

Second, we transfer match_node() into style/selector_matching.rs and merge the function with get_applicable_declarations().

With the current crate dependency graph, that would mean adding a lot to the traits for the DOM tree in the util trait, and probably worsening the get_applicable_declarations<N: TreeNode<T>, T: TreeNodeRefAsElement<N, E>, E: ElementLike> kind of signature that we have to work around the lack of associated types in traits.

What would be the benefit?

SimonSapin · 2013-11-20T16:29:02Z

Also, I would like someone else to review the DOM and box generation parts of this.


        Fix parsing of the 'content' property.

jdm · 2013-12-10T04:36:17Z

src/components/script/dom/node.rs

@@ -175,9 +181,13 @@ impl<View> TreeNodeRefAsElement<Node<View>, Element> for AbstractNode<View> {
    fn with_imm_element_like<R>(&self, f: &fn(&Element) -> R) -> R {
        self.with_imm_element(f)
    }
+    fn set_pseudo_element(&mut self, pseudo_element: Option<PseudoElement>) {


This should be implemented on the Node<LayoutView> type; script should not be able to acces the layout data.

jdm · 2013-12-10T04:36:47Z

src/components/script/dom/node.rs

@@ -484,6 +494,12 @@ impl AbstractNode<ScriptView> {
    }
 }

+impl<View> AbstractNode<View> {


Likewise, this should be LayoutView, not a generic View type.

pcwalton · 2013-12-10T06:16:53Z

Just saw this.

<bz> Basically, whatever the abstraction layout sees for "element" should probably be associated with ::before/::after
<bz> Whether that involves an actual DOM element is a separate question, I guess

It should be relatively easy to refactor flow construction to handle both "real" nodes and generated content, BTW. Traits for the win :)

larsbergstrom · 2013-12-10T17:29:53Z

Related to issue #805

SimonSapin · 2014-01-02T11:48:08Z

For matching, another option would be to separate the selectors into "buckets" (eg. different Stylist objects) by pseudo-element (or lack-thereof) when parsing stylesheets. Then, when matching against a given element, we can ask for the styles of a given pseudo-element (or none) and only loop on relevant selectors.

This or the PR’s current approach of returning a big data structure both work, so I suggest doing whatever is more convenient for layout code.

bzbarsky · 2014-01-02T18:11:01Z

For what it's worth, what Gecko does is to have a separate RuleHash per pseudo-element.

parkjaeman · 2014-01-03T06:31:59Z

@SimonSapin
Thanks for your comment. I can not make sense what you mean exactly. Hence, I want to ask a question to understand your intention correctly. Which is your recommendation?

First, all pseudo-elements have its own stylist like follows.

struct LayoutTask {
...
stylist:RWArc,
before_stylist:RWArc,
after_stylist:RWArc,
...
}

Second, all pseudo-elements have one new stylist which covers all pseudo-elements like follows.

struct LayoutTask {
...
stylist:RWArc,
pseudo_stylist:RWArc,
...
}

Third, all pseudo-elements share the existing stylist which need to be modified to cover pseudo element like follows.

struct LayoutTask {
...
stylist:RWArc,
...
}

where

struct PerOriginSelectorMap {
normal: PseudoElementSelectorMap,
important: PseudoElementSelectorMap,
}

struct PseudoElementSelectroMap {
element: SelectorMap,
before: SelectorMap,
after: SelectorMap
}

Happy new year :)

SimonSapin · 2014-01-03T10:04:00Z

@parkjaeman Yes, I believe we understand each other :) The PR currently does something like your third, and I suggest it could do something like your first if that’s more convenient for layout code.

SimonSapin · 2014-01-10T09:52:57Z

The Selector matching and cascading parts of this have been merged in #1464. What’s remaining is box generation.

jdm · 2014-01-24T01:09:13Z

Has this been superseded by #1158?

parkjaeman · 2014-01-25T01:14:23Z

@jdm This has been superseded by #1496. We need to close this PR after finishing #1496.

jdm · 2014-01-25T01:20:02Z

Is there any reason to leave this one open right now?

parkjaeman · 2014-02-13T23:56:56Z

@jdm It is OK to close this PR.


        Merge pull request servo#1158 from Ms2ger/getElementsByName-null-undef

Cleanup document.getElementsByName-null-undef.{html,xhtml}.

SimonSapin reviewed Oct 31, 2013
View changes

src/components/style/selectors.rs Outdated

@@ -24,6 +24,7 @@ pub enum PseudoElement {

After,

FirstLine,

FirstLetter,

none,

This comment has been minimized.

Sign in to view

SimonSapin Oct 31, 2013
Member

This should not be necessary, as we’re using Option<PseudoElement> everywhere.

SimonSapin reviewed Oct 31, 2013
View changes

recrack and others added 6 commits Oct 25, 2013

Impl 2.1 Section 12 - Generated content

5a25a6c

Implementation in CSS parsing/selector-matching regarding to pseudo e…

dc540a4

…lement 'before'.

Generate a new render box for pseudo element

ee54f63

Transfer pseudo_parent_element & pseudo_text from LayoutData to Node

f21fe8e

Move PseudoElement from libstyle to libtree, restore the trait TreeNo…

40bc38a

…deRefAsElement to its original state and remove unnecessary value in enum PseudoElement

Move pseudo element creation from location of render box creation to …

56f5a57

…location of box generator creation

Modify content property according to CSS 2.1

da09ac1

Fix parsing of the 'content' property.

b905ff1

jdm reviewed Dec 10, 2013
View changes

src/components/script/dom/node.rs

@@ -484,6 +494,12 @@ impl AbstractNode<ScriptView> {

}

}

impl<View> AbstractNode<View> {

This comment has been minimized.

Sign in to view

jdm Dec 10, 2013
Member

Likewise, this should be LayoutView, not a generic View type.

SimonSapin mentioned this pull request Jan 7, 2014

Pseudo element selectors matching #1464

Merged

This was referenced Jan 10, 2014

fix line-height, implement <br> #1475

Closed

support :before and :after pseudo-elements #1511

Closed

parkjaeman closed this Feb 13, 2014

SimonSapin mentioned this pull request Mar 24, 2014

Implement pseudo-element(before and after) #1974

Closed

ChrisParis pushed a commit to ChrisParis/servo that referenced this pull request Sep 7, 2014

Merge pull request servo#1158 from Ms2ger/getElementsByName-null-undef

3f62995

Cleanup document.getElementsByName-null-undef.{html,xhtml}.

servo / servo

Join GitHub today

Pseudo element ":before" #1158

Pseudo element ":before" #1158

Conversation

parkjaeman commented Oct 31, 2013

pcwalton commented Oct 31, 2013

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 Oct 31, 2013

yichoi commented Nov 8, 2013

parkjaeman commented Nov 8, 2013

deokjinkim commented Nov 8, 2013

This comment has been minimized.

jdm commented on src/components/script/dom/node.rs in ee54f63 Nov 8, 2013

parkjaeman commented Nov 8, 2013

SimonSapin commented Nov 9, 2013

parkjaeman commented Nov 10, 2013

SimonSapin commented Nov 11, 2013

jdm commented Nov 11, 2013

SimonSapin commented Nov 11, 2013

parkjaeman commented Nov 12, 2013

jdm commented Nov 12, 2013

SimonSapin commented Nov 20, 2013

SimonSapin commented Nov 20, 2013

This comment has been minimized.

This comment has been minimized.

pcwalton commented Dec 10, 2013

larsbergstrom commented Dec 10, 2013

SimonSapin commented Jan 2, 2014

bzbarsky commented Jan 2, 2014

parkjaeman commented Jan 3, 2014

SimonSapin commented Jan 3, 2014

SimonSapin commented Jan 10, 2014

jdm commented Jan 24, 2014

parkjaeman commented Jan 25, 2014

jdm commented Jan 25, 2014

parkjaeman commented Feb 13, 2014

jdm commented on `src/components/script/dom/node.rs` in `ee54f63` Nov 8, 2013