Closed Bug 1355351 Opened 7 years ago Closed 7 years ago

pseudo-elements don't return the correct style via getComputedStyle.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Testcase
263 bytes, text/html
Details
Bug 1355351: Add a node property to access the ::before and ::after pseudo-elements.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1355351: Look for the frame for ::before and ::after pseudos.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1355351: Tests for correct resolution of style for pseudo-elements.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1355351: Clean up pseudo-element props.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1355351: Simplify nsLayoutUtils callers, and make child iterators notice display: contents pseudos.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1355351: Update stylo test expectations due to missing pseudo-element style updates.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Attached file Testcase — 
Testcase attached. I initially thought this was only for display: contents, but apparently it isn't. 

STR: Open the attached test-case.

Expected: "50px"
Got: "50%"
In particular, this is because nsComputedDOMStyle::UpdateCurrentStyleSources makes no attempt to set mInnerFrame and mOuterFrame when mPseudo is non-null.

I believe Emilio mentioned verbally that Chrome behaves differently from us, i.e., it does return layout-based computed styles for pseudo-elements.
Yup, it does.

I'm interested in adding an API to get a given generated content pseudo for an element. We need it for stylo to handle properly restyles of ::before and ::after with display: contents elements (we use the frame tree to get it for other elements, but that's a no-go for display: contents), so I'll probably give this a shot too on the way.
Confirmed that Chrome reports 50px, as does Safari.  Edge reports 50%, though.  The CSSOM spec doesn't treat ::before / ::after computed style any differently from regular elements.  So I think it makes sense to make this change.
We have the nsLayoutUtils::Get{Before,After}FrameForContent methods, which is what GeckoRestyleManager uses to find these things for restyling.  Is there a reason we can't use these here?
Flags: needinfo?(emilio+bugs)
So, the reason I thought we couldn't use it was display: contents stuff, because we didn't have a real _parent_ frame.

But looking at the frame constructor and restyle manager stuff, giving the closest ancestor parent frame will work. I find this more elegant that walking up the frame tree, but I guess I can do that too.
Flags: needinfo?(emilio+bugs)
So, I'm pushing back on this one, sorry Cameron.

There are a lot of places that assume that an element must have a frame to have pseudos, which is wrong. This patch fixes it, in a few places, and simplifies a lot of that code.

Also, right now there are only one remaining accessor of nsIFrame::GenConProperty(). I tried to remove it, but that check is hiding a bug when display: contents is used inside a fieldset, so I preferred to keep it for now and address it in a followup.

I also tried to fix nsLayoutUtils::IsGeneratedContentFor (because right now it's bogus, see bug 1251799), but that's hiding even more bugs and needs a more elaborate fix, so I'll look into it in a followup too.
See Also: → 1358575
Blocks: 1358580
This also blocks the new pseudo-element stuff, since otherwise we'll never restyle properly ::before or ::after for display: contents.
Blocks: 1331047
I think I want to land this with the two test expectation changes for animations in stylo, since it's directly related to us now using the frame style, and is only exposing the fact that we were not correctly updating the frame style in the first place, which I intend to fix in bug 1331047.
Comment on attachment 8858761 [details]
Bug 1355351: Add a node property to access the ::before and ::after pseudo-elements.

https://reviewboard.mozilla.org/r/130800/#review136068

::: dom/base/nsGkAtomList.h:74
(Diff revision 2)
>  GK_ATOM(activetitlebarcolor, "activetitlebarcolor")
>  GK_ATOM(activateontab, "activateontab")
>  GK_ATOM(actuate, "actuate")
>  GK_ATOM(address, "address")
>  GK_ATOM(after, "after")
> +GK_ATOM(afterPseudoProperty, "afterPseudoProperty")  // nsXMLElement*

Let's add these down in the "Content property names" block.
Attachment #8858761 - Flags: review?(cam) → review+
Comment on attachment 8858762 [details]
Bug 1355351: Look for the frame for ::before and ::after pseudos.

https://reviewboard.mozilla.org/r/130802/#review136078

::: layout/style/nsComputedDOMStyle.cpp:718
(Diff revision 2)
> +static bool
> +HasToReResolveStyle(const nsStyleContext* aContext)

Please add some comments in here describing what the point of these checks are.

Small nit: "MustReresolveStyle" sounds a bit better to me.  (I can see a bunch of other functions named "MustDoWhatever", but no "HaveTo"/"HasTo".)

::: layout/style/nsComputedDOMStyle.cpp:730
(Diff revision 2)
> +    return aContext->GetParent() &&
> +           aContext->GetParent()->HasPseudoElementData();

This one I don't quite get.  Why are we not just checking for ::before / ::after specifically, and returning false in that case?
Comment on attachment 8858763 [details]
Bug 1355351: Tests for correct resolution of style for pseudo-elements.

https://reviewboard.mozilla.org/r/130804/#review136082
Attachment #8858763 - Flags: review?(cam) → review+
Comment on attachment 8860328 [details]
Bug 1355351: Clean up pseudo-element props.

https://reviewboard.mozilla.org/r/132356/#review136084

::: layout/base/nsCSSFrameConstructor.cpp:8547
(Diff revision 1)
> -    for (; ancestor; ancestor = ancestor->GetParent()) {
> -      ancestorFrame = ancestor->GetPrimaryFrame();
> -      if (ancestorFrame) {
> -        break;
> -      }
> -    }
> -    if (ancestorFrame) {
> -      nsTArray<nsIContent*>* generated = ancestorFrame->GetGenConPseudos();
> +    if (nsLayoutUtils::HasPseudoStyle(aChild,
> +                                      childDisplayContentsStyle,
> +                                      CSSPseudoElementType::before,
> +                                      mPresShell->GetPresContext()) ||
> +        nsLayoutUtils::HasPseudoStyle(aChild,
> +                                      childDisplayContentsStyle,
> +                                      CSSPseudoElementType::after,
> +                                      mPresShell->GetPresContext())) {

Why are we doing these checks instead of |ancestorFrame->Properties().Get(nsIFrame::GenConProperty())|?  (Your planned followup will fix this to grab the property from the content instead, right?  If so, why change this?)

::: layout/base/nsLayoutUtils.cpp:1588
(Diff revision 1)
>  /*static*/ nsIFrame*
>  nsLayoutUtils::GetBeforeFrameForContent(nsIFrame* aFrame,
>                                          const nsIContent* aContent)

This and GetAfterFrameForContent don't need the aFrame argument any more.
Comment on attachment 8860329 [details]
Bug 1355351: Simplify nsLayoutUtils callers, and make child iterators notice display: contents pseudos.

https://reviewboard.mozilla.org/r/132358/#review136106

This does look a lot neater, thanks.

::: dom/animation/EffectCompositor.cpp:529
(Diff revision 1)
> -  return pseudoFrame->GetContent()->AsElement();
> +
> +  return nullptr;

Maybe retain the NS_NOTREACHED?

::: layout/generic/nsFrame.cpp:10368
(Diff revision 1)
>  }
>  
>  Element*
>  nsIFrame::GetPseudoElement(CSSPseudoElementType aType)
>  {
> -  nsIFrame* frame = nullptr;
> +  if (!GetContent()) {

I think it's fine to just use mContent, here and below.
Attachment #8860329 - Flags: review?(cam) → review+
Comment on attachment 8858762 [details]
Bug 1355351: Look for the frame for ::before and ::after pseudos.

https://reviewboard.mozilla.org/r/130802/#review136078

> Please add some comments in here describing what the point of these checks are.
> 
> Small nit: "MustReresolveStyle" sounds a bit better to me.  (I can see a bunch of other functions named "MustDoWhatever", but no "HaveTo"/"HasTo".)

Ok, will change the name and add a comment.

> This one I don't quite get.  Why are we not just checking for ::before / ::after specifically, and returning false in that case?

The point of that check is to re-resolve the style if the style context is inside a ::first-line frame, for example, so the computed style doesn't include ::first-line styles.

That's why we need that check. See bug 505515.
Comment on attachment 8860328 [details]
Bug 1355351: Clean up pseudo-element props.

https://reviewboard.mozilla.org/r/132356/#review136164

::: layout/base/nsCSSFrameConstructor.cpp:8547
(Diff revision 1)
> -    for (; ancestor; ancestor = ancestor->GetParent()) {
> -      ancestorFrame = ancestor->GetPrimaryFrame();
> -      if (ancestorFrame) {
> -        break;
> -      }
> -    }
> -    if (ancestorFrame) {
> -      nsTArray<nsIContent*>* generated = ancestorFrame->GetGenConPseudos();
> +    if (nsLayoutUtils::HasPseudoStyle(aChild,
> +                                      childDisplayContentsStyle,
> +                                      CSSPseudoElementType::before,
> +                                      mPresShell->GetPresContext()) ||
> +        nsLayoutUtils::HasPseudoStyle(aChild,
> +                                      childDisplayContentsStyle,
> +                                      CSSPseudoElementType::after,
> +                                      mPresShell->GetPresContext())) {

Yeah, sounds good, will revert this.

::: layout/base/nsLayoutUtils.cpp:1588
(Diff revision 1)
>  /*static*/ nsIFrame*
>  nsLayoutUtils::GetBeforeFrameForContent(nsIFrame* aFrame,
>                                          const nsIContent* aContent)

This is fixed in the next patch. Was a noisy enough patch I wanted to do it separately.
Comment on attachment 8858762 [details]
Bug 1355351: Look for the frame for ::before and ::after pseudos.

https://reviewboard.mozilla.org/r/130802/#review136208
Attachment #8858762 - Flags: review?(cam) → review+
Comment on attachment 8860328 [details]
Bug 1355351: Clean up pseudo-element props.

https://reviewboard.mozilla.org/r/132356/#review136210

r=me with the reversion to check for GenConProperty().
Attachment #8860328 - Flags: review?(cam) → review+
Comment on attachment 8861626 [details]
Bug 1355351: Update stylo test expectations due to missing pseudo-element style updates.

https://reviewboard.mozilla.org/r/133612/#review136496
Attachment #8861626 - Flags: review?(emilio+bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3dee36db1ef3
Add a node property to access the ::before and ::after pseudo-elements. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7b8b371f964a
Look for the frame for ::before and ::after pseudos. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5693d8a26b49
Tests for correct resolution of style for pseudo-elements. r=heycam
https://hg.mozilla.org/integration/autoland/rev/dba324c58046
Clean up pseudo-element props. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b081b6c44658
Simplify nsLayoutUtils callers, and make child iterators notice display: contents pseudos. r=heycam
https://hg.mozilla.org/integration/autoland/rev/73b97e528623
Update stylo test expectations due to missing pseudo-element style updates. r=emilio
Blocks: 1359656
Depends on: 1359779
Depends on: 1360157
Assignee: nobody → emilio+bugs
Depends on: 1413619
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: