Closed Bug 1467722 Opened 6 years ago Closed 6 years ago

Don't return null from getComputedStyle when there's no presentation.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr60 - wontfix
firefox62 --- fixed

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.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> Instead, return the empty string and length = 0.

... from getPropertyValue.
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.
Flags: needinfo?(emilio)
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.
Flags: needinfo?(emilio)
Blocks: 1467328
Priority: -- → P3
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?
Attachment #8984382 - Flags: review?(cam) → 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.
Attachment #8984383 - Flags: review?(cam) → 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.
Attachment #8984384 - Flags: review?(cam) → review+
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
Blocks: 1464011
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.
https://hg.mozilla.org/mozilla-central/rev/cb86f60d2639
https://hg.mozilla.org/mozilla-central/rev/7a18800b893d
https://hg.mozilla.org/mozilla-central/rev/c2b039dd2ace
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1471231
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.
Flags: needinfo?(emilio)
I commented in the PR, thanks!
Flags: needinfo?(emilio)
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
Blocks: 1490688

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.

Flags: needinfo?(emilio)

(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.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio)

[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.

Flags: needinfo?(emilio)

I'm not sure why you ni?d me? If you need something from me please let me know.

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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
Flags: needinfo?(emilio)
Attachment #8984384 - Flags: approval-mozilla-esr60?

(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?

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 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 :(
Attachment #8984384 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: