919594 - domUtils.getCSSValuesForProperty fails on mix-blend-mode, text-combine-horizontal, text-orientation & writing-mode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

domUtils.getCSSValuesForProperty fails on:
- mix-blend-mode
- text-combine-horizontal
- text-orientation
- writing-mode.

Here is a simple scratchpad that demonstrates the problem:
let DOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
                 .getService(Ci.inIDOMUtils);

let target = content.document.body;
let names = content.window.getComputedStyle(target);

for (let name of names) {
  try {
    DOMUtils.getCSSValuesForProperty(name);
  } catch(e) {
    dump("Cannot get values for " + name + "\n");
  }
}

Comment 1

[Boris Zbarsky [:bzbarsky]]

These properties are all disabled by default (as in, do not exist, as far as Gecko is concerned).

Did you test in a build in which you enabled them?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I tested on fx-team so I guess so.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Why would they be enabled on fx-team, exactly?

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I don't know. All I know is that using:
content.window.getComputedStyle(content.document.body);

returns properties including mix-blend-mode, text-combine-horizontal, text-orientation & writing-mode on fx-team

Comment 5

[Boris Zbarsky [:bzbarsky]]

"returns" in what sense?  What actual code are you using here?  Certainly |"mixBlendMode" in content.window.getComputedStyle(content.document.body)| should be returning false...

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So run the following in scratchpad:

let target = content.document.body;
let properties = content.window.getComputedStyle(target);

for(let name of properties) {
  if (name === "mix-blend-mode" ||
      name === "text-combine-horizontal" ||
      name === "text-orientation" ||
      name === "writing-mode") {
    dump("name: '" + name + "'\n");
  }
}

//////////////////

The problem is that in order to get a full list of properties for the Computed CSS View we use getComputedStyle() to get all available property names (they are all displayed when browser styles is checked).

Iterating over the CSS2Properties object using a for of loop iterates through mix-blend-mode etc. and because the value is undefined we are left with blank lines in the computed view.

If these properties are disabled then can they be removed from the CSS2Properties object?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> for(let name of properties) {

Ah, the indexed props!  Those are just buggy, looks like.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Though fixing this while keeping the prefs live during a brower session but not regressing performance is actually pretty darned hard.

David, how ok would you be with nsComputedDOMStyle::GetQueryablePropertyMap reflecting the pref state of the very first time it's called?

Comment 9

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

Hmmm.  Maybe bug 264517 will get around the problem?  Or does the issue have something to do with the way the bindings code constructs a prototype?

I guess if this is per-element or per-page it's probably ok, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The issue is that nsComputedDOMStyle code has a big static array of property names.  When you ask it for its .length, it returns the length of that array.  When you ask it for [n] for some integer n it indexes into that array.

The "right" way to handle preferences, in theory, would be to store the enabled state in the array too.  Then .length would walk the array, counting the enabled props.  And [n] would walk the array looking for the n-th enabled prop.  But that of course makes .length O(N) and iteration O(N^2).

The cheap option is to construct the array once, then go through memmoving things in it to get rid of disabled stuff.  But that's not live if prefs change during runtime.  This is basically what I was proposing in comment 8.

I guess we could stop using a static array and hang an array off the document.  Or even off the computed style object itself, I suppose...  That feels dangerous in terms of memory use and iteration performance if people keep getting new computed style objects, though.

Comment 11

[Cameron McCormack (:heycam)]

Is the order that the properties are exposed important?  You could swap disabled ones to the end of the array, rather than shifting everything across, if not.

Another option would be to have a static array that maps from indexes-excluding-disabled-properties to indexes-including-disabled-properties which gets updated when a pref changes.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is the order that the properties are exposed important?

Technically, it'll be standardized at some point.  And I think people kinda expect them in alphabetical order.

> Another option would be to have a static array that maps from
> indexes-excluding-disabled-properties to indexes-including-disabled-properties which
> gets updated when a pref changes.

Ah, yes.  I like this.  This shouldn't be too bad to set up.  So cache the length and the index-to-index mapping, then update the cache in the unlikely case that the pref changes.

Want to do it?  ;)

Comment 13

[Cameron McCormack (:heycam)]

Sure, I'll take a look.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 1: Move computed style map entries to a preprocessor included file.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 2: Encapsulate the computed style map and make it take disabled properties into account.

Comment 16

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=f042f10a5eb1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 812530 [details] [diff] [review]
Part 1: Move computed style map entries to a preprocessor included file.

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 812531 [details] [diff] [review]
Part 2: Encapsulate the computed style map and make it take disabled properties into account.

Beautiful.  r=me

Add a test?

Comment 19

[Cameron McCormack (:heycam)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/67d83e429b29
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f5e68fc1f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/512d5d35712e

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/67d83e429b29
https://hg.mozilla.org/mozilla-central/rev/a8f5e68fc1f6
https://hg.mozilla.org/mozilla-central/rev/512d5d35712e