1355683 - Consider removing getDefaultComputedStyle

Status: NEW

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

Description

[Xidorn Quan [:xidorn] UTC+10]

It was added in bug 800983, and it was added, renamed, and eventually removed from the spec.

It seems that this API is only used by jQuery. jQuery used this for getting original "display" value of a given kind of element. jQuery uses feature detection rather than UA-detection for it since it added the usage, and it has a reasonable fallback path which calls getComputedStyle with a newly inserted element. And jQuery caches the result rather than calling it every time.

I did some investigation on BigQuery with github_repos.sample_contents table, and in that table, only two kinds of things show up, one is jQuery, the other is that shadowDOM adds kind of polyfill for this API unconditionally.

Given this, I think we may want to unship this API rather than implementing it in stylo.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

FWIW, this is the query I used on BigQuery:
SELECT
  id,
  sample_repo_name,
  sample_path
FROM
  `bigquery-public-data.github_repos.sample_contents`
WHERE
  content LIKE '%getDefaultComputedStyle%'
  AND NOT REGEXP_CONTAINS(content, r',\s*[a-zA-Z]\s*=\s*[a-zA-Z]\.getDefaultComputedStyle\s*(&&|\?\s*[a-zA-Z]\.getDefaultComputedStyle)')
  AND NOT REGEXP_CONTAINS(content, r'\.getDefaultComputedStyle\s*=\s*function\(')
  AND NOT REGEXP_CONTAINS(content, r'\.prototype\.getDefaultComputedStyle\s*=\s*function\s*\(')
  AND NOT REGEXP_CONTAINS(content, r'display\s*=\s*window\.getDefaultComputedStyle\s*(&&|\?)')
  AND NOT REGEXP_CONTAINS(content, r'[a-zA-Z]%3D[a-zA-Z]%2EgetDefaultComputedStyle(%26%26|%3F[a-zA-Z]%2E)')
  AND NOT content LIKE '%Remove use of getDefaultComputedStyle%'
  AND NOT content LIKE '{"version":%'
LIMIT
  1000

Comment 2

[Boris Zbarsky [:bzbarsky]]

The fallback has different semantics if the document has a type selector setting display for that element...

But it's probably OK to remove this and take the same codepath as other UAs anyway.  Epecially since it's been removed from jQuery starting with version 2.2.0 (and 1.12.0), as far as I can tell.  It was added in 2.1.0 and 1.11.0 respectively.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Relevant: https://github.com/jquery/jquery/pull/1647

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1355683 - Remove Window.getDefaultComputedStyle.

Review commit: https://reviewboard.mozilla.org/r/129886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129886/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8857857 [details]
Bug 1355683 - Remove Window.getDefaultComputedStyle.

https://reviewboard.mozilla.org/r/129886/#review132614

::: dom/base/nsGlobalWindow.h:1728
(Diff revision 1)
>  public:
>    // Outer windows only.
>    nsDOMWindowList* GetWindowList();
>  
>  protected:
> -  // Helper for getComputedStyle and getDefaultComputedStyle
> +  // Helper for getComputedStyle

This should just be called GetComputedStyleOuter, if that's all it's doing.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8857857 [details]
Bug 1355683 - Remove Window.getDefaultComputedStyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129886/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b5ea8bed5c
Remove Window.getDefaultComputedStyle. r=bz

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/94b5ea8bed5c

Comment 9

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/getdefaultcomputedstyle-has-been-removed/

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should bug 548397 comment 38 make us reconsider the compatibility risk here?

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

I don't quite understand the risk there.

So this is the code in jQuery 1.11: https://github.com/jquery/jquery/blob/1.11.0/src/css/defaultDisplay.js

It seems that it first tries to getDefaultComputedStyle, or it calls into jQuery.css, and if they both returns null, it tries create an invisible (via width=0 height=0) iframe and get info from that. 

If the window is inside a display:none iframe, both getComputedStyle and getDefaultComputedStyle returns null, so there shouldn't be anything different. And the iframe it creates isn't display:none, so how can removing getDefaultComputedStyle itself cause problem?

Comment 12

[Thomas Wisniewski [:twisniewski]]

I've updated the webcompat issue with a reduced test-case to show the problem. Basically the issue is that the getComputedStyles fails on the iframe, because the overall document has this CSS at the time: html { display:none; }

When that CSS isn't applied, things work out just fine. I'm not sure how many sites would run into this combination, since they seem to be doing this to fade in the page content with jquery animations.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> I don't quite understand the risk there.
> 
> So this is the code in jQuery 1.11:
> https://github.com/jquery/jquery/blob/1.11.0/src/css/defaultDisplay.js
> 
> It seems that it first tries to getDefaultComputedStyle, or it calls into
> jQuery.css, and if they both returns null, it tries create an invisible (via
> width=0 height=0) iframe and get info from that. 
> 
> If the window is inside a display:none iframe, both getComputedStyle and
> getDefaultComputedStyle returns null, so there shouldn't be anything
> different. And the iframe it creates isn't display:none, so how can removing
> getDefaultComputedStyle itself cause problem?

The key part (AIUI) is that getDefaultComputedStyle doesn't get called on the iframe, but on the parent window, which presumably does have a presshell.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

OK I understand the problem now...

That only happens if the <html> has display:none or there is a rule in the page says "iframe { display: none }" or something like that. It is not clear how widely it could be, but it certainly reveals some compat risk that we didn't think about.

I guess there are three options:
1. assuming this is rare and do nothing
2. prioritize bug 548397 given it's orthogonal to stylo and itself has caused lots of compat issues
3. back out this and try implementing getDefaultComputedStyle in Stylo

The last one probably has the lowest risk. I guess implementing getDefaultComputedStyle for Stylo may not be terribly hard, although I'm not sure.

The second one would be the best for long term, since I don't think we want to keep getDefaultComputedStyle anyway in long term given the lack of users, spec, and interests from other browsers.

hmmm...

Comment 15

[Thomas Wisniewski [:twisniewski]]

My two cents are that if we deem that fixing bug 548397 is beyond our means anytime soon, then I would strongly consider standardizing getDefaultComputedStyle instead. After all, if libraries like jQuery are using obscure, unreliable iframe hacks simply to show/hide elements, then it might be best to make a well-specced option instead.

Comment 16

[Boris Zbarsky [:bzbarsky]]

In terms of timeframes, fixing bug 548397 is not something that will happen in the next 1-2 months, is my guess.

If there's an actual site where we're hitting compat problems, we should just restore getDefaultComputedStyle for now.  I guess we removed it because it's hard to implement with servo?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to twisniewski from comment #15)
> My two cents are that if we deem that fixing bug 548397 is beyond our means
> anytime soon, then I would strongly consider standardizing
> getDefaultComputedStyle instead. After all, if libraries like jQuery are
> using obscure, unreliable iframe hacks simply to show/hide elements, then it
> might be best to make a well-specced option instead.

I think CSSWG wants to address this use case by adding a new property to CSS Display which can suppress box generation but doesn't affect the display value. And if getDefaultComputedStyle only has this scene, it really isn't worth standardizing.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> In terms of timeframes, fixing bug 548397 is not something that will happen
> in the next 1-2 months, is my guess.
> 
> If there's an actual site where we're hitting compat problems, we should
> just restore getDefaultComputedStyle for now.  I guess we removed it because
> it's hard to implement with servo?

It was not because it's hard to implement, but because I thought it is easier to remove it than doing the other way. But I guess I was wrong.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

OK, I'll handle backing it out and open another bug against stylo to track implementing it.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d61e130cca13f3b689929ab1f3ac48462c220b93

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Backed out at https://hg.mozilla.org/integration/autoland/rev/d48f5e67773d

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Also opened bug 1366157 for stylo to implement this.

We probably want to remove it eventually, but not now.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/d48f5e67773d

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Untrack from stylo. This has been implemented for Stylo.

Comment 24

[Karl Dubost💡 :karlcow]

Maybe this should be closed as WONTFIX if there is dependency.
Or add it to the compat spec.
Maybe, we should add telemetry.