877690 - Implement an API for getting CSS property values to be used in auto-completion

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

bz asked me to log the things we need from bug 766032 in a new bug so here it is.

The API should be simple, something like getCSSValuesForProperty("display"). The resultant array should contain:

1. "initial"
2. "inherit"
3. For a longhand that takes keyword values, all the keyword values.
4. For a longhand that takes color values all named colors.
5. For a shorthand the union of the lists for all its subproperties.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Mina, do you want to take a shot at this?

Comment 2

[Simon Sapin (:SimonSapin)]

I’m puzzled by item 5. Do you really want property names mixed in an array of property values?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Item 5 is the union of the lists of the subproperty _values_.

Comment 4

[Simon Sapin (:SimonSapin)]

Note that some shorthands do not accept all keywords in the union of those accepted by their longhands. For example, css-fonts-3 adds a lot of values to 'font-variant', but only the 2.1 variant keywords are available in the 'font' shorthand:

http://www.w3.org/TR/css3-fonts/#ltfont-variant-css21gt

I think that the list you want is:

1. CSS-wide keywords. 'initial' and 'inherit' for now, css-cascade-3 adds 'default'. http://www.w3.org/TR/css3-values/#common-keywords
2. Keywords accepted by the property.

Named colors are keywords, and all propreties (longhand or shorthand) define their own syntax,the 'Value' line in specs.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's a pain to implement, actually, since anything that has that behavior will have custom parsing and hence no easy way to extract that information...  :(

Comment 6

[Simon Sapin (:SimonSapin)]

Maybe we can ignore the font/font-variant corner case (I’m not aware of another one) but don’t many if not all properties have custom parsing, shorthand or not?

Comment 7

[Boris Zbarsky [:bzbarsky]]

The vast majority of non-shorthand properties in Gecko effectively use a table-driven parser, last I checked.  To quantify, 189 parseable longhands use the table-driven setup while 38 use custom functions.

All shorthands use custom functions, of course.

Comment 8

[Mina Almasry]

Attachment: proposed patch

I'm on it! So far I think I've figured out the keywords, but I'm having trouble with colors. If I understand correctly, if a property takes colors, then we'd want to append the entire color table in [1]?

Also, I'm having trouble telling whether a property takes color values. In the parser the test is variantMask & VARIANT_COLOR, but VARIANT_COLOR is only defined in nsCSSParser.cpp. What should I do about that?

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/src/nsColor.cpp#l24

[2] http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#5295

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758817 [details] [diff] [review]
proposed patch

> +#include "nsCSSParser.h"

Shouldn't need this.

>+  if (!(keywordTable == nullptr)) {

  if (keywordTable)

>+    int i = 0;

size_t?  Or at least int32_t.

>+      aArray.AppendElement(
>+          NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStringValue(word)));

Probably faster to do:

  CopyASCIItoUTF16(nsCSSKeywords::GetStringValue(word),
                   *aArray.AppendElement());

>+  array.AppendElement(NS_ConvertUTF8toUTF16("initial"));

  NS_LITERAL_STRING("initial")

and likewise for "inherit".  Also, do we support unprefixed "initial" now?  If not, "-moz-initial"...

>+  nsCSSProperty propertyID = nsCSSProps::LookupProperty(aProperty,
>+                                                        nsCSSProps::eEnabled);

If this returns eCSSProperty_UNKNOWN you probably want to throw or something... or at least return immediately with just the initial/inherit bit.

>+  if (!ret) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

NS_Alloc is infallible, so no need.

>+    ret[i] = NS_strdup(array[i].get());

  ret[i] = ToNewUnicode(array[i]);

> VARIANT_COLOR is only defined in nsCSSParser.cpp. 

I think it should be fine to move all the VARIANT_* defines to nsCSSProps.h...

Comment 10

[Boris Zbarsky [:bzbarsky]]

David, any objection to moving the VARIANT_* bits?

Comment 11

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

I guess it's ok, though I'm somewhat hesitant to expose that stuff to this sort of API.  I guess if the API is only exposing a few of the bits (e.g., VARIANT_COLOR), I'm probably ok with it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I don't think the API would expose the bits at all; it would just use the bits in the inspector utils code to decide what values the property supports.

Comment 13

[Mina Almasry]

Attachment: proposed patch v2

How is this?

I had to add an accessor for kColorNames in nsColor to access the color names, but beyond that I think everything was straightforward. 

I have a few whitespace errors in the diff, but they're because the code in the tree has trailing whitespace, and not because I'm adding any. Seems like getting rid of the whitespace errors will be a pain. Let me know if necessary.

Comment 14

[Mina Almasry]

Attachment: proposed patch with mochitest

Hi bz, I hope all is well. 

I've worked on this patch some more. I've added a mochitest, and fixed mistakes the test reveals, and removed all the whitespace diffs. Now the patch builds, and passes the attached test. This should be a much easier review for you.

Comment 15

[Mina Almasry]

Attachment: proposed patch v2 with test

Caught a couple mistakes in my last patch and fixed them.

Comment 16

[Mina Almasry]

Attachment: patch

I don't know if you tried looking at the patch I had yet, but it was edited in a way that breaks the review tool and makes it appear wrong. I've attached a correct patch now.

Comment 17

[Mina Almasry]

Boris just a friendly reminder this patch is waiting review. I think you'll like this one, too. It has a mochitest written, it passes the test, and I think the behaviour is what you want.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 772413 [details] [diff] [review]
patch

>+++ b/gfx/src/nsColor.cpp
>+  if (kColorNames) {

kColorNames can never be null, right?  So it should be reasonable to just return the const char** directly from this function, too.

>+++ b/gfx/src/nsColor.h
>+NS_GFX_(bool) NS_AllColorNames(const char * const ** aPColorNameArray,

Document that the caller is NOT supposed to free the array?

The rest of this looks great.  r=me with the signature of NS_AllColorNames fixed up.  And sorry for the lag...

Comment 19

[Mina Almasry]

Attachment: (I think) final patch

Wooo! First granted review from you!

Here is a patch with all the changes. What is there to do now? Mark checkin-needed?

Also, if there is anything else you're mentoring that you'd like me to work on, please point me to that.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 775220 [details] [diff] [review]
(I think) final patch

r=me

If you want to give bug 888864 or bug 734861 or bug 82711 a shot maybe?

Comment 21

[Mina Almasry]

Attachment: Commiter's agreement

Here is my commiter's agreement, in case you didn't have one on file before.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9897835271b5

Comment 23

[Ed Morley [:emorley]]

Backed out for crashes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25286014&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25286098&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/41dc3cba351f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

FWIW, there was a green push prior to the backout, so this might simply be an IID rev or clobber issue.

Comment 25

[Boris Zbarsky [:bzbarsky]]

We do need to rev the iid of inIDOMUtils in this bug.

Comment 26

[Mina Almasry]

Attachment: patch - reved the iid

Reved the IID

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cedd434dddb

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb

Comment 29

[Philip Chee]

https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.22
> +  if (!array.indexOf) {
!Array.isArray(array)

https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.99
> +  ok(testValues(values, expected), "Shorthand proprety values.");
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.134
> +  // test proprety
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.148
> +  ok(testValues(values, expected), "proprety float values");
s/proprety/property/

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/024c3db2505a