Don't return null from getComputedStyle when there's no presentation.
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(3 files)
Instead, return the empty string and length = 0. This is closer to the CSSOM spec. We will still return cross-doc styles if available, but when not available we won't throw an exception, but return the empty string, the same way WebKit / Blink do for out-of-document elements. This is a preliminar change for bug 548397.
Assignee | ||
Comment 1•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #0) > Instead, return the empty string and length = 0. ... from getPropertyValue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=726accc5c119f73d669faf1897eab515bff90abf
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
What was the result of Blink's attempt to do this mentioned in bug 548397 comment 42? I'm wondering whether we were waiting for those results to come in before changing our behaviour.
Assignee | ||
Comment 11•6 years ago
|
||
This is not quite related to it. That Blink's intent is about returning an empty style in all display: none iframes. They already return an empty style for disconnected nodes for example. We want to wait for them to start unconditionally returning empty styles for display: none iframes, but that's unrelated to this change, as I said this change doesn't make us return anything less useful.
Updated•6 years ago
|
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8984382 [details] Bug 1467722: Don't return null for getComputedStyle when there's no pres shell. https://reviewboard.mozilla.org/r/250198/#review257806 ::: layout/style/nsComputedDOMStyle.h:83 (Diff revision 2) > eAll // Includes all stylesheets > }; > > nsComputedDOMStyle(mozilla::dom::Element* aElement, > const nsAString& aPseudoElt, > - nsIPresShell* aPresShell, > + nsIDocument* aPresShell, aDocument ::: layout/style/nsComputedDOMStyle.cpp:326 (Diff revision 2) > , mStyleType(aStyleType) > , mComputedStyleGeneration(0) > , mExposeVisitedStyle(false) > , mResolvedComputedStyle(false) > { > - MOZ_ASSERT(aElement && aPresShell); > + MOZ_ASSERT(aElement); Assert aDocument is non-null too?
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8984383 [details] Bug 1467722: Make nsComputedDOMStyle store an actual Element. https://reviewboard.mozilla.org/r/250200/#review257808 ::: layout/style/nsComputedDOMStyle.h:726 (Diff revision 2) > > // We don't really have a good immutable representation of "presentation". > // Given the way GetComputedStyle is currently used, we should just grab the > - // 0th presshell, if any, from the document. > + // presshell, if any, from the document. > nsWeakPtr mDocumentWeak; > - nsCOMPtr<nsIContent> mContent; > + nsCOMPtr<mozilla::dom::Element> mElement; RefPtr for Element.
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8984384 [details] Bug 1467722: Do not throw when we don't have a style, but just return an empty style. https://reviewboard.mozilla.org/r/250202/#review257810 ::: layout/style/nsComputedDOMStyle.cpp:453 (Diff revision 3) > if (!mComputedStyle) { > - return NS_ERROR_NOT_AVAILABLE; > + return NS_OK; > } For consistency, let's do this in GetCSSImageURLs too.
Comment 15•6 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb86f60d2639 Don't return null for getComputedStyle when there's no pres shell. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/7a18800b893d Make nsComputedDOMStyle store an actual Element. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b039dd2ace Do not throw when we don't have a style, but just return an empty style. r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11573 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb86f60d2639 https://hg.mozilla.org/mozilla-central/rev/7a18800b893d https://hg.mozilla.org/mozilla-central/rev/c2b039dd2ace
Upstream PR merged
Updated•6 years ago
|
Comment 22•6 years ago
|
||
For the docs, I've tried to note this here: https://github.com/mdn/browser-compat-data/pull/2419. Please let me know if it's correct.
Comment 24•6 years ago
|
||
Thanks for the reviews :) I've added a note to the compat data: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle#Browser_compatibility Due to a caching experiment MDN is currently running, you might need to add a parameter to the URL to see the changes: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle?some-bogus-parameter#Browser_compatibility
Comment 25•6 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/getcomputedstyle-no-longer-returns-null-when-style-can-t-be-retrieved/
Comment 26•5 years ago
|
||
Hi All,
I am facing this issue in Firefox 65.0.5esr release , While I am having no trouble , when i ran same code on Firefox 65.
As i just want to know that whether this fix pushed into Firefox esr release or not ,please let me know.
If not pushed into esr , when it will get push into Firefox esr release , if you know any timeline , please let me know.
Thanks.
Assignee | ||
Comment 27•5 years ago
|
||
(In reply to ankit_k_chaniyara from comment #26)
I am facing this issue in Firefox 65.0.5esr release , While I am having no trouble , when i ran same code on Firefox 65.
You mean Firefox 60.5esr, right?
As i just want to know that whether this fix pushed into Firefox esr release or not ,please let me know.
If not pushed into esr , when it will get push into Firefox esr release , if you know any timeline , please let me know.
I don't think there's any particular plan to push it to ESR, but if you want to make a case for that, please do. I don't think it's a very risky patch to uplift.
Comment 28•5 years ago
|
||
Sorry for typo , Yes i mean Firefox 60.5.0esr ,
Thanks for information.
I am new in this bugzilla , so could you please give me some information about how to patch this thing into esr release and how to work with that.
Maybe it's silly question , but if you give some lead on that , it will be very helpful.
Assignee | ||
Comment 29•5 years ago
|
||
If you go to the patch "Details" page: https://bugzilla.mozilla.org/attachment.cgi?id=8984384&action=edit
You can see a approval-mozilla-esr flag, you can change it to "?", and that gives you an approval request form which you need to fill in order to get the release managers to look at it.
Comment 30•5 years ago
|
||
[Tracking Requested - why for this release]: I ran project in firefox 65 , which works very fine , but it got break in firefox 60.5.0esr , our project used by a customer who uses esr release of firefox to run our project , and we are using GWT as our UI component , so it is not possible for us to change structure [ entire code base is in java] , please release this patch for Firefox-esr60 release also.
Assignee | ||
Comment 31•5 years ago
|
||
I'm not sure why you ni?d me? If you need something from me please let me know.
Comment 32•5 years ago
|
||
Sorry for that , but i did not find "approval-mozilla-esr" flag , for that may be i need an account with more privileges , so that's why i just googled it and found out that you can do in this way.
Sorry for bothering you again , but this is very serious issue for me as a product itself is not working because of this issue , and we do not have other alternative to solve this , until and unless this patch will get release in next esr release.
Need --> please , i just want to have this patch in esr release.
please help me out for this.
Thanks,
Ankit
Assignee | ||
Comment 33•5 years ago
|
||
Comment on attachment 8984384 [details]
Bug 1467722: Do not throw when we don't have a style, but just return an empty style.
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Filing on behalf of comment 30, see that comment.
- User impact if declined: See comment 30
- Fix Landed on Version: 62
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): It's not a too hard patch to uplift.
- String or UUID changes made by this patch: none
Assignee | ||
Comment 34•5 years ago
|
||
(In reply to ankit_k_chaniyara from comment #32)
Sorry for that , but i did not find "approval-mozilla-esr" flag , for that may be i need an account with more privileges , so that's why i just googled it and found out that you can do in this way.
I filed the request, see above. Not sure if people will take it though.
Sorry for bothering you again , but this is very serious issue for me as a product itself is not working because of this issue , and we do not have other alternative to solve this , until and unless this patch will get release in next esr release.
Why isn't using a try { } catch () {} block in an appropriate place not an acceptable solution?
Comment 35•5 years ago
|
||
I filed the request, see above. Not sure if people will take it though.
Thanks a lot for that.
Why isn't using a try { } catch () {} block in an appropriate place not an acceptable solution?
Because we are using GWT , where in a lot of places we have used that , almost every page , and more of it will convert Java into generated Javascript , and it is throwing exception. so we can not put that code in there.
Comment 36•5 years ago
|
||
Comment on attachment 8984384 [details] Bug 1467722: Do not throw when we don't have a style, but just return an empty style. Sorry, but while I know this is causing pain for the internal site in question, these patches don't meet the criteria for ESR uplifts: https://wiki.mozilla.org/Release_Management/ESR_Landing_Process#What_should_land_on_mozilla-esr60.2Fmozilla-esr68 > Security and some major stability fixes when they're landed/merged onto mozilla-beta, or fixes for regressions specific to the ESR. In the first few cycles of ESR we may be more flexible on these criteria. As the versions progress we should limit this to the most critical security fixes. This is a longstanding issue from what I can see, and we're already more than halfway through the current ESR cycle. As noted earlier, this fix is available on release versions of Firefox and will also be in the ESR68 release coming in early July. I apologize for the inconvenience :(
Updated•5 years ago
|
Description
•