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)
Tracking
()
RESOLVED
DUPLICATE
of bug 1355351
People
(Reporter: martin.bouladour, Unassigned, Mentored)
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 1 obsolete file)
5.73 KB,
patch
|
dbaron
:
review-
dbaron
:
feedback+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•11 years ago
|
||
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.
Comment 2•11 years ago
|
||
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
Comment 4•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
This is a temporary solution http://jsfiddle.net/3FecG/3/
Comment 8•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #828096 -
Flags: feedback? → feedback?(bzbarsky)
Comment 9•11 years ago
|
||
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+
Comment 10•11 years ago
|
||
Attachment #828096 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #828382 -
Flags: feedback?(dbaron)
Comment 11•11 years ago
|
||
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. :)
Comment 12•11 years ago
|
||
> 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.
Comment 13•10 years ago
|
||
I am interested in working on this bug. So please can you assign it to me?
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
(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)
Comment 16•10 years ago
|
||
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
Updated•10 years ago
|
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)
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Mentor: bzbarsky
Whiteboard: [lang=c++][mentor=bz] → [lang=c++]
Comment 18•8 years ago
|
||
Any news on when the patch for this should be applied?
Comment 19•8 years ago
|
||
When there is a patch here that's actually correct...
Comment 20•8 years ago
|
||
I a interested to work on this. Can I take it up?
Comment 22•8 years ago
|
||
(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!
Comment 23•8 years ago
|
||
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?
Comment 24•6 years ago
|
||
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.
Description
•