Closed Bug 925694 Opened 11 years ago Closed 6 years ago

Pseudo-elements' getComputedStyle().width and .height return "auto" instead of the used value

Categories

(Core :: DOM: CSS Object Model, defect)

24 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1355351

People

(Reporter: martin.bouladour, Unassigned, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130917102605

Steps to reproduce:

Consider a block pseudo-element for which the CSS width (or height) value has been set to auto. For instance, defined by:

> #element::before {
>   content: "foo";
>   display: block;
> }

Then get the width (or height) value using:

> getComputedStyle(element, '::before').width


Actual results:

The value returned by `getComputedStyle` is "auto".


Expected results:

It should return the actual used value in pixels.

The W3C CSSOM (draft) specification [1] states that `getComputedStyle` returns the "resolved value". For the `width` and `height` properties, it defines:

> If the property applies to the element or pseudo-element and the resolved value
> of the 'display' property is not 'none', the resolved value is the used value.
> Otherwise the resolved value is the computed value.

In the case discussed here, the `width` and `height` property do apply to the pseudo-element (block), so the resolved value is the used value, i.e. the actual dimension in pixels.

[1] http://dev.w3.org/csswg/cssom/#resolved-values
Note this is especially problematic considering the fact that pseudo-elements can't be manipulated like regular elements using JavaScript. The `getBoundingClientRect`, `offsetWidth` and the other properties that can be used to get an element's dimensions can't be applied to a pseudo-element.
Yep, this is because the computed style code doesn't actually have access to the box for the pseudo-element right now...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=c++][mentor=bz]
M ready to solve this bug. Being new to Mozilla can any one please guide me for this bug.

Thanks,
Jayesh
Here is JSFiddle http://jsfiddle.net/3FecG/, which probably shows the same bug.

Actual results:

The value returned by `getComputedStyle` is "10%".


Expected results:

It should return the actual used value in pixels.
The code that needs to be fixed is in the file layout/style/nsComputedDOMStyle.cpp, in the functions  nsComputedDOMStyle::GetPropertyCSSValue and nsComputedDOMStyle::GetPropertyCSSValueInternal, where mInnerFrame and mOuterFrame are initialized.  They need to be modified to get the appropriate frame for the pseudo-element (see the mPseudo member variable), which needs custom code for each pseudo-element, and probably should be done only for :before and :after for now.  The functions nsLayoutUtils::GetBeforeFrame and nsLayoutUtils::GetAfterFrame should be useful.
And it's possible other code in the file will need to be adjusted because it makes assumptions that will be broken by such a change.  So you should look through that code.

Finally, an automated test (probably best in testharness.js format, or otherwise a regular mochitest) should be added to test the fix.
This is a temporary solution http://jsfiddle.net/3FecG/3/
This is a WIP, it fixes the issue with :before and :after, but I'm not sure if I covered all the cases.
Attachment #828096 - Flags: feedback?
Attachment #828096 - Flags: feedback? → feedback?(bzbarsky)
Comment on attachment 828096 [details] [diff] [review]
WIP patch: Fix for :before and :after elements.

>+     mOuterFrame = nsLayoutUtils::GetBeforeFrame(mContent->GetPrimaryFrame());

This will crash if mContent->GetPrimaryFrame() is null (e.g. if the node has display:none).  Please add a test for that?  Similar for the ePseudo_after case.

Also, convention in this code is to brace single-line if/else bodies.

Apart from that, this looks like the right approach.  Did you do any of the auditing of the other code in this file that dbaron suggested yet?
Attachment #828096 - Flags: feedback?(bzbarsky) → feedback+
Attachment #828096 - Attachment is obsolete: true
Attachment #828382 - Flags: feedback?(dbaron)
Thank you for the review.

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 828096 [details] [diff] [review]
> WIP patch: Fix for :before and :after elements.
> 
> >+     mOuterFrame = nsLayoutUtils::GetBeforeFrame(mContent->GetPrimaryFrame());
> 
> This will crash if mContent->GetPrimaryFrame() is null (e.g. if the node has
> display:none).  Please add a test for that?  Similar for the ePseudo_after
> case.
Fixed.

> Also, convention in this code is to brace single-line if/else bodies.
Fixed.
> 
> Apart from that, this looks like the right approach.  Did you do any of the
> auditing of the other code in this file that dbaron suggested yet?

I have. The change impacts the style resolve from the following block:
> if (!mStyleContextHolder || mStyleContextHolder->HasPseudoElementData()) {.

It is not clear to me why a style resolve is needed if the element is nested inside a pseudo element (->HsPseudoElementData() == true).

There are a bunch of property getters that check mInnerFrame/mOuterFrame which would now pass for pseudo elements. I need to have another look at that.

I'm sorry for the messy approach, this is my very first mozilla patch. :)
> It is not clear to me why a style resolve is needed if the element is nested inside a
> pseudo element 

That's to handle a case like this:

  <style>
    div { color: yellow; width: 0; }
    div::first-line { color: blue; }
  </style>
  <div><span>Some text that wraps lines</span></div>
  <script>alert(getComputedStyle(document.querySelector("span")).color);</script>

The correct color to alert is the yellow.  But the primary frame for the span is inside the first-line, so actually has a blue color.  So we can't use its style context for the computed style.
I am interested in working on this bug. So please can you assign it to me?
Cătălin, are you still interested in working on this?

David, do you know when you'll get a chance to look at Cătălin's patch?
Flags: needinfo?(dbaron)
Flags: needinfo?(catalin.badea392)
(In reply to Boris Zbarsky [:bz] from comment #14)
> Cătălin, are you still interested in working on this?
> 
> David, do you know when you'll get a chance to look at Cătălin's patch?

Yes, I would like to finish it myself.
Flags: needinfo?(catalin.badea392)
Anup, sorry, this bug already has an owner, but just wasn't marked as such...

Cătălin, I'm sorry for the delay on this patch.  I'll try to make sure it gets looked at ASAP.
Assignee: nobody → catalin.badea392
Flags: needinfo?(bzbarsky)
Comment on attachment 828382 [details] [diff] [review]
pseudo-elements.patch

Please add MOZ_ASSERT()s at the beginning of
nsComputedDOMStyle::UpdateCurrentStyleSources that mOuterFrame
and mInnerFrame are both null (just like mStyleContextHolder), given
that you're depending on that behavior.

>+    if (mStyleContextHolder && !mPseudo) {

This change isn't quite right.  You need to make a slightly deeper
adjustment to this code.

The purpose of this code is that when all or part of an element is
inside a pseudo-element like ::first-line or ::first-letter, we don't
return styles that come from the ::first-line or ::first-letter.

So instead of the change you make, you need to change the four
occurrences of mStyleContextHolder in this code:

>  if (!mStyleContextHolder || mStyleContextHolder->HasPseudoElementData()) {
>#ifdef DEBUG
>    if (mStyleContextHolder) {
>      // We want to check that going through this path because of
>      // HasPseudoElementData is rare, because it slows us down a good
>      // bit.  So check that we're really inside something associated
>      // with a pseudo-element that contains elements.
>      nsStyleContext *topWithPseudoElementData = mStyleContextHolder;

to, instead of using mStyleContextHolder, starting from
mStyleContextHolder->GetParent() if mPseudo is true (since the mParent
will be the style of the element, outside of the ::before or ::after
pseudo).

Your current code causes us to unnecessarily use the slow path for all
the ::before and ::after styles.

It would probably be good to also add a test for the case of a ::before
style inside a ::first-line returning a style not influenced by the
::first-line.

Also, could you call your test file
test_computed_style_on_pseudo_elements.html instead of
test_bug925694.html.


r=dbaron with those changes, but marking as review- because I'd like to
look at the revised patch

Sorry for dropping this request on the floor for so long.
Attachment #828382 - Flags: review-
Attachment #828382 - Flags: feedback?(dbaron)
Attachment #828382 - Flags: feedback+
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)
Mentor: bzbarsky
Whiteboard: [lang=c++][mentor=bz] → [lang=c++]
Any news on when the patch for this should be applied?
When there is a patch here that's actually correct...
I a interested to work on this. Can I take it up?
Yes, thanks!
Assignee: catalin.badea392 → nobody
(In reply to Boris Zbarsky [:bz] (TPAC) from comment #19)
> When there is a patch here that's actually correct...

Looks like a lot has been changed in the meanwhile. I tried the jsfiddle code: http://jsfiddle.net/3FecG/3/
It seemed to work fine for me on Firefox.
Could you please point out must be done?

Thanks!
That jsfiddle isn't testing what this bug is about, precisely because it has a workaround for it, no?

The testcase you want is like so:

  <style>
    div { width: 200px; }
    div::before { display: block; content: "hey" }
  </style>
  <div></div>
  <script>
    alert(getComputedStyle(document.querySelector("div"), "::before").width);
  </script>

and the alert should say "200px", not "auto".  See https://jsfiddle.net/8baxh0w0/ for this testcase on jsfiddle.

> Could you please point out must be done?

Address the review comments from comment 17?
This got fixed in bug 1355351.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: