Closed Bug 766032 Opened 12 years ago Closed 10 years ago

API for helping implement CSS editing autocompletion

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: sebo, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

Unfinished proposed patch
14.76 KB, patch
Details | Diff | Splinter Review 
Currently it's pretty unclear for extensions like Firebug which CSS properties Firefox supports.
The use cases for Firebug are auto-completion and validation on editing.
In order to achieve this we currently have to manually maintain a list of properties and values they accept. Though this is error-prone, costs a lot of time, isn't always up-to-date, bloats the code size and can't match different browser versions correctly.

So we would like to have an API, which allows to return a list of CSS properties the CSS engine supports including their value types.

Example XPCOM interfaces:
nsICSS
--------------------------------------------------
nsICSSPropertyList getAvailableCSSProperties();

Returns all supported CSS properties.

nsICSSProperty getCSSProperty(in AString name);

Returns a specific CSS property.


nsICSSProperty
--------------------------------------------------
Methods:
nsISimpleEnumerator getAvailableValues()

Returns a nsISimpleEnumerator of nsICSSPropertyValue entries available for the property.

Attributes:
name   AString   Name of the property


nsICSSPropertyValue
--------------------------------------------------
Methods:
nsISimpleEnumerator getSubTypes()

Returns a nsISimpleEnumerator of nsICSSPropertyValue entries available as sub-types for a property value. E.g. for a "gradient" this will be "linear-gradient", "radial-gradient", "repeating-linear-gradient", "repeating-radial-gradient".

Attributes:
type   AString   CSS data type of the property value (e.g. "color", "angle", "integer", etc.), "constant", "function" or "property"
name   AString   Name of the constant, function or property; otherwise empty string



This API is just a quick shot and may be changed/enhanced.
It doesn't describe methods to access other parts of CSS like pseudo-classes, pseudo-elements, @-rules and selectors because properties are the main use case for Firebug. Though nsICSS allows to be extended by new functions to cover them.
Also it doesn't cover value lists (like in "background-image"), value variations (like in "margin") and whether there's a specific order of values required (like in "font").

Sebastian
> Currently it's pretty unclear for extensions like Firebug which CSS properties Firefox
> supports.

Enumerating the properties of a CSSStyleDeclaration would pretty much give you that list, in somewhat-munged DOM form...  Though I agree that it would be simpler to just expose a list, since we do more or less have one available.

> nsICSSPropertyValue

The information this is looking for isn't really available anywhere sane at the moment.  All that would happen here is a hardcoded list being added which would get out of sync in exactly the way you describe.  Not to mention the other issues you mentioned...

What are you _really_ looking for here?
> Though I agree that it would be simpler to just expose a list, since we do 
> more or less have one available.
Good.

> The information this is looking for isn't really available anywhere sane at 
> the moment.
I already thought so. So this sounds like the code around that needs to be tightened up first.

> What are you _really_ looking for here?
To replace [1] by something nice and maintainable, which reflects the capabilities of the CSS engine.

Sebastian

[1] https://github.com/firebug/firebug/blob/d45c9fae92d20f38f7dbb7ed4c861dad3f1565db/extension/content/firebug/lib/css.js#L771
> So this sounds like the code around that needs to be tightened up

It's not a matter of tightened up, necessarily.  It's a matter of CSS properties having wildly different syntactic behavior...

> To replace [1] by something nice and maintainable

OK.  I don't know how (or even whether) some of the things in there (thickness, borderCollapse!, etc) map to either the spec or to our implementation.  Assuming they do at all.

What we probably _can_ do pretty easily is expose the information contained in the "how to parse" annotations in nsCSSPropList.h.  See http://hg.mozilla.org/mozilla-central/file/19bfe36cace8/layout/style/nsCSSParser.cpp#l56 through line 130 or so.  And for things including VARIANT_KEYWORD the set of keywords can be exposed.  I'm not sure that would be useful to you, necessarily, since it doesn't have higher-level concepts like "thickness" (which is a set of specific keywords and all nonnegative lengths, presumably).
> It's not a matter of tightened up, necessarily.  It's a matter of CSS 
> properties having wildly different syntactic behavior...
Though they all share that they are CSS properties. So they should be registered through a general API or be inherited from a basic ancestor. Anyway, how you do that internally doesn't matter for me.

> I don't know how (or even whether) some of the things in there (thickness, 
> borderCollapse!, etc) map to either the spec or to our implementation.  
> Assuming they do at all.
They currently do not. They are just a help for us for auto-completing properties and values.

> What we probably _can_ do pretty easily is expose the information contained in 
> the "how to parse" annotations in nsCSSPropList.h
This in combination with nsCSSKeywordList.h looks like the information we need. Again, the perfect solution would be to be able to get a list of all CSS properties, @-rules, pseudo-classes, pseudo-elements and selectors Gecko knows about including all possible values and constraints for them.
Though the most important part for us right now are CSS properties.

I assume this info could also be valuable for the built-in dev tools in Firefox. Therefore I added Mike as CC.

Sebastian
I was going to implement this in Firebug some time back.

Here is how we get the property list in the style panel's rule view:
createStyleViews: function CssHtmlTree_createStyleViews()
  {
    if (CssHtmlTree.propertyNames) {
      return;
    }

    CssHtmlTree.propertyNames = [];

    // Here we build and cache a list of css properties supported by the browser
    // We could use any element but let's use the main document's root element
    let styles = this.styleWin.contentWindow.getComputedStyle(this.styleDocument.documentElement);
    let mozProps = [];
    for (let i = 0, numStyles = styles.length; i < numStyles; i++) {
      let prop = styles.item(i);

      if (prop.charAt(0) == "-") {
        mozProps.push(prop);
      } else {
        CssHtmlTree.propertyNames.push(prop);
      }
    }

    CssHtmlTree.propertyNames.sort();
    CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames,
      mozProps.sort());
  },

There is a limited list of possible property values:
"top"
"middle"
"bottom"
"bold"
<Number>
<Color>
<string>
etc.

To detect which property values are valid for each property you could always loop through property values for each property (e.g. after let prop = styles.item(i) above to see if they are valid. This would be O(n^2) with roughly 560 property values.

You could validate each using the following:

_validate: function _validate(aName, aValue)
  {
    let style = this.doc.createElementNS(HTML_NS, "div").style;

    style.setProperty(aName, aValue, null);

    return !!style.getPropertyValue(aName);
  },

At a minimum you would need the keyword list to make this future proof. Maybe you don't need all of the values at once ... you could grab the ones you need whenever autocomplete is triggered.

Of course, an API to simplify this would be useful for extension developers.
Thanks Mike. CssHtmlTree_createStyleViews() sounds basically like what Boris suggested. That's fine for gettting all longhand properties, but this list doesn't include shorthand properties.

_validate() works, though it is obviously more of a workaround.

> At a minimum you would need the keyword list to make this future proof.
Yes.

> Maybe you don't need all of the values at once ... you could grab the ones you 
> need whenever autocomplete is triggered.
This was the intention behind my getAvailableValues() function.

Is there anything that speaks against a clear API like I suggested? It looks more future-proof to me than having different separated functions, which just return unconnected parts of information.

Sebastian
(In reply to Sebastian Zartner from comment #6)
> Is there anything that speaks against a clear API like I suggested?

Nothing as far as I can tell, in fact a few teams have agreed to help us with exactly this kind of API for use in our developer tools. Under the hood it could do pretty much what I described in my previous comment, caching the results.
> That's fine for gettting all longhand properties, but this list doesn't include
> shorthand properties.

Again, exposing the list of all properties, including shorthands, from the core code is easy.  I'm happy to do that; might be worth spinning it off into a separate bug.

> There is a limited list of possible property values:

  border-width: calc(10% - 5px);

Just saying.

I'm still trying to understand what the actual goal of the value API is here.  What is it supposed to be used for?
For auto-completion of CSS property values. e.g. white-space: would list completion options of normal, pre, nowrap, pre-wrap, pre-line and inherit. Ideally we would also have multiple lists for shorthand properties.

So we would need a way to get a list of all property names, optionally including shorthand and a way to get the possible property values for a given property name.
Ah, I see.

So as I see it, exposing the following from core code is pretty easy:

1)  A list of all CSS property names, shorthand and longhand.
2)  A list of all longhand properties.
3)  For a CSS property name, a list of all its subproperties if it's a shorthand (an
    empty list if it's not a shorthand).  Though this would be the list of subproperties
    it resets, iirc, not the list of subproperties it can set...
4)  For longhand properties that can have keyword values, the set of allowed keywords.
5)  For longhand properties some sort of indication of what sort of values it accepts
    (keyword, length, number, etc).  I'm not sure what the optimal way to expose this to
    JS is; in the core it's just a bitmask.
6)  Restrictions on the values (e.g. if it's allowed to only be a nonnegative number).

This still leaves the issue of things like ordering dependencies or random delimiter characters (e.g. in "font")....
Summary: API for available CSS properties → API for helping implement CSS editing autocompletion
> 1)  A list of all CSS property names, shorthand and longhand.
> 2)  A list of all longhand properties.
Good.

> 3)  For a CSS property name, a list of all its subproperties if it's a 
> shorthand (an empty list if it's not a shorthand). Though this would be the 
> list of subproperties it resets, iirc, not the list of subproperties it can 
> set...
Auto-completion needs a list of subproperties it can set.

> 4)  For longhand properties that can have keyword values, the set of allowed 
> keywords.
Good.

> 5)  For longhand properties some sort of indication of what sort of values it 
> accepts (keyword, length, number, etc).  I'm not sure what the optimal way to 
> expose this to JS is; in the core it's just a bitmask.
A bitmask would be ok I think. Though this would still require maintaining function names on the extension side, like e.g. linear-gradient() as part of the <image> type. So exposing them is also needed.

> 6)  Restrictions on the values (e.g. if it's allowed to only be a nonnegative 
> number).
Good.

> This still leaves the issue of things like ordering dependencies or random 
> delimiter characters (e.g. in "font")....
This is not something needed by us in the first place. This may be put into a separate issue.

Sebastian
> Auto-completion needs a list of subproperties it can set.

Right, but we don't really treat this internally in any sort of nice way is the point.  Hence not "pretty easy".  ;)

Anyone want to swag up some IDL here?
The discussion about this proceeded that nicely before, so I wonder why it got stuck now.

> Anyone want to swag up some IDL here?
I assume you asked this to your team. Though I assume the IDL I described at comment #0 could be adjusted in the manner to cover the functions you described in comment 10, no?

Sebastian
> I assume you asked this to your team.

I asked anyone with the interest in doing the work.

I'm happy to guide people who are interested in working on this.  I don't have time to work on it myself.
Whiteboard: [mentor=bz]
Would it be reasonable to twiddle the format of all the *List.h files so that we could just auto-generate a JS module containing the tables that firebug/css.js currently hand-maintains?  Said *List.h files probably couldn't just be #include'd anymore, so there'd be some cost there.  But really, cpp-able files are a bad place for keeping information anyway.
We have existing code that takes nsCSSPropList file and munges it in python, after first piping it through cpp with appropriate #defines.  See http://mxr.mozilla.org/mozilla-central/source/dom/bindings/GenerateCSS2PropertiesWebIDL.py?force=1 and http://mxr.mozilla.org/mozilla-central/source/dom/webidl/CSS2PropertiesProps.h and http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Makefile.in?force=1#98

It's a bit of a complicated setup, though.

Note that if we had some other more parseable format we could just generate the .h files from it at build time.  So if someone does want to do that sort of thing, I'd be fine with it.  David?
ni'ing David for a response to comment 16.
Flags: needinfo?(dbaron)
I think I'd like to hear what said format would look like and how things would work before saying I'd be ok with it, but I imagine I'd be ok with reasonable solutions that don't make adding or modifying an entry any harder than it is now.
Flags: needinfo?(dbaron)
Assignee: nobody → fayearthur
Hi I'm new here and I would like to take a shot at this. Can you assign me this bug? (Or is that not how it works?)

Thanks,
Probably worth checking with Heather to see what her plans are.
Flags: needinfo?(fayearthur)
I had no immediate plans to work on this. Go for it, it would be great to have this implemented.
Assignee: fayearthur → nobody
Flags: needinfo?(fayearthur)
Assignee: nobody → almasry.mina
Hi Boris,

(sorry for the late followup.)

I've been doing a bit of reading to wrap my head around the code. I'll tell you what I have so far (correct me if I'm wrong) and I have a few questions.

So far I've figured what we need is a bunch of WebIDL's like in [1] corresponding roughly to what was mentioned in the original comment + comment #10. The .idl files go in dom/webidl but where do the corresponding .cpp files go?

I'd like to start with nsICSSProperty and nsICSS. (There is an nsCSSProperty enum defined in the code.. will the similarity in names confuse future coders?)

I see that the code (gecko?) represents CSS properties by an enum nsCSSProperty. The data we need for this interface is already at a few places in the code, me thinks. Here [2] there is a global variable gPropertyTable that is a mapping between nsCSSProperty enums and the property name (string), here [3] there is an global array, kCSSRawProperties, that holds the property names (char[]'s) only.

I can't seem to figure out how the code knows which values are valid for which properties (help?) but this line [4] seems to be what the code needs to parse the value. Is that correct? That line gets it's data from the array that stores CSS Property flags, kFlagsTable.

[2], [3], and kFlagsTable all pull their data from the same file, [5].

Now I am at a xroads of what to do. In the nsICSS interface, I turn kFlagsTable into a global (give it external linkage) and use the data in [2], [3], and [4]; or I could pull the data from the source, [5].

The first approach is less memory allocations, but relies on external arrays, and the second makes the nsICSS modular, and independent from the rest of the code, but is more allocations and some duplications of code necessary. Am I on the right track? What would you recommend?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
[2] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSProps.cpp#l47
[3] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSProps.cpp#l33
[4] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSProps.h#l238
[5] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSPropList.h
No problem at all; this stuff is still somewhat half-baked....

So we _could_ use WebIDL for the CSSProperty/CSSPropertyValue/CSSPropertyList, I guess.  We'd need to mark the [NoInterfaceObject] to avoid web content being able to see the interface objects or something.

The IDL in comment 0 is XPIDL, not WebIDL, so it's not directly usable if you do WebIDL.  But I suspect doing WebIDL here might in fact be better, certainly in terms of the API JS sees.

> where do the corresponding .cpp files go

Wherever we want to, actually.  

I guess layout/inspector/src would be the best place.

> will the similarity in names confuse future coders

I think we should call the class MozCSSProperty, probably.

> I can't seem to figure out how the code knows which values are valid for which
> properties 

Generally speaking, "it's complicated".  Hence the much more restrictive wording in comment 10.

That said, nsCSSProps::ParserVariant will return some information about how a property can be parsed in terms of the "variant" defines in the CSS parser.  Those could be exposed in some header and used.  And nsCSSProps::ValueRestrictions exposes more information on valid values.  See CSSParserImpl::ParseSingleValueProperty for how those end up being used.

And yes, PropertyParseType returns more info as well, as used by CSSParserImpl::ParseProperty.

Note that if the type is PARSE_FUNCTION then you're sort of out of luck for a simple description of the valid values...

You don't need to do anything with kFlagsTable; you can just call the nsCSSProps accessors on it.  There's no value in making this stuff not depend on nsCSSProps, in my opinion.
Okay this is what I have so far. I have added one function to inIDOMUtils.idl and added two WebIDLs that we need and the corresponding .h and .cpp files. This patch builds (obj/dom and obj/layout incremental builds complete with no errors).

I need a few things to continue:

- Is this okay so far? Am I on the right track? What's wrong/very wrong?
- I needed to declare a couple of methods in the Webidls [Creator] because of the return type. Is that okay?
- I haven't actually run or tested or written tests for this yet. I would like if possible write code that instances these classes, print values to a console, and maybe step through my functions with a debugger to see what's going on. What's the best way to do that?
- Should I write tests? Do I need compiled-code tests or mochitests, or both? 

Thanks. Hope this is okay so far :)
Attachment #739321 - Flags: review?(bzbarsky)
Comment on attachment 739321 [details]
First (unfinished) proposed iteration of patch

OK, so...

If the objects can be returned without creating a new one each time, then they can't be wrapperCache:False.  So they need to inherit from wrappercache and implement cycle collection.

inDOMUtils::GetCSSPropertyObject needs to heap-allocate the object; returning a pointer to something on the stack is going to be bad.

I think what you have as CSSObject should probably be CSSPropertyInfo and the other should be CSSPropertyInfoList.  And both should be [NoInterfaceObject], I think.
Attachment #739321 - Flags: review?(bzbarsky) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #739321 - Attachment is obsolete: true
Attached patch Proposed Patch v2 (obsolete) — — Splinter Review 
>+    NS_DECL_CYCLE_COLLECTION_CLASS(CSSPropertyInfo)
>you want:
>NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CSSPropertyInfo)

Okay all fixed and added to Makefile.in and it builds again. Are you sure it's NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CSSPropertyInfo) though? Both my classes have a member in them so I thought it would be NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(CSSPropertyInfo, <member_name>)

>Just declare getCSSPropertyInfo as returning an nsISupports and return it like >a normal COM object, and xpconnect should do the right thing.

Done too. Did I get it right? I am returning an nsISupports* from the method. Or did you want me to return a nsCOMPtr<nsISupports> instead?

>So I think I see why this built for you as-is.  You need to add your new .cpp >files to the list of files to compile at http://mxr.mozilla.org/mozilla-central/source/layout/inspector/src/Makefile.in#17

Done too. I seem to be making lots of little mistakes like this (and still pretty clueless with nsISupports, nsWrapperCache, implementing refcounting and cycle collection etc). Is this typical experience for newcomers, or am I missing some guides I should be reading (please point me to them) or are these project conventions that I'll get used to by experience, or am I supposed to be able to read the source code and understand what it does (I'm trying; sometimes it's hard) ?

What's next?
Attachment #740046 - Flags: review?(bzbarsky)
Comment on attachment 740046 [details] [diff] [review]
Proposed Patch v2

> Both my classes have a member in them so I thought it would be
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(CSSPropertyInfo, <member_name>)

It depends on the member.  The only member in CSSPropertyInfo is an integer, so there's no way to form a reference cycle through it.

For CSSPropertyInfoList you do want to cycle-collect the mCSSPropertyInfos, though.

> Did I get it right?

Not quite.

What I mean is that inIDOMUtils.idl should have:

  nsISupports GetCSSPropertyInfo(in DOMString aPropertyName);

and then the C++ would look sort of like this:

  NS_IMETHODIMP
  inDOMUtils::GetCSSPropertyInfo(const nsAString& aPropString,
                                 nsISupports** aPropertyInfo)
  {
    NS_ADDREF(*aPropertyInfo = new CSSPropertyInfo(aPropString);
    return NS_OK;
  }

with no changes needed to inDOMUtils.h.  I'm guessing you didn't do a full-tree build on this patch, since it should have ended up with link errors as-is, right?

> Is this typical experience for newcomers,

To some extent.

The cycle collection bits are something pretty much everyone does via some combination of past experience and copy/paste.  I know that's what I did in this case.  ;)

Same thing for wrapper cache, unfortunately.

In terms of refcounting and nsISupports, if you haven't seen https://developer.mozilla.org/en-US/docs/Creating_XPCOM_Components/An_Overview_of_XPCOM it's worth looking at.  And also maybe http://support.microsoft.com/kb/104138 or http://www.ibm.com/developerworks/webservices/library/co-xpcom2/index.html which both talk about the general reference counting setup...

In the end you end up picking up on patterns based on experience, mostly, I think....

As far as the actual patch goes, some issues in addition to the above:

ParserVariant can only be called on non-shorthand properties that we actually implement.  ValueRestrictions and PropertyParseType can only be called on properties we actually implement. So you want to do something where you avoid calling those methods for the wrong sort of properties.  Probably just throwing for longhands for ParserVariant and for unknown properties from GetCSSPropertyInfo is fine.

Alternately, instead of throwing for longhands, we could put the parservariant bit on a sub-interface or something.  Worth checking with the devtools folks on what would be the best API fit for them there.

You don't need to call SetIsDOMBinding in destructors.

Your WrapObject method declarations should be virtual now that you're wrappercached, and should have a " MOZ_OVERRIDE" between the ')' and the ';'.

The "TODO implement this" bits obviously need addressing.  ;)

Apart from that, this is looking much better!
Attachment #740046 - Flags: review?(bzbarsky)
Comment on attachment 740046 [details] [diff] [review]
Proposed Patch v2

Asking for some API feedback.
Attachment #740046 - Flags: feedback?(mratcliffe)
Attachment #740046 - Flags: feedback?(fayearthur)
(In reply to Boris Zbarsky (:bz) from comment #27)
Okay all changes done.

> Not quite.
Of course not :P

> with no changes needed to inDOMUtils.h.  I'm guessing you didn't do a
> full-tree build on this patch
I've never done a full build if that's what you mean. It takes hours on my computer. I do objdir/layout and objdir/dom incremental builds only. Do I have to do more? God I hope not...
 
> Probably just throwing for longhands for ParserVariant and for unknown
> properties from GetCSSPropertyInfo is fine.
Done. Right now I throw for all errors detected. Let me know if that's not what you want.
I didn't know what to do in the constructor when I detect an attempt to construct an invalid object. Normally I would throw an exception but I couldn't here...?

> You don't need to call SetIsDOMBinding in destructors.
Oops. Fixed. Are my destructors okay though? I get this scary warning:  warning: deleting object of polymorphic class type ‘mozilla::dom::CSSPropertyInfo’ which has non-virtual 
destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

I figure we're okay until someone tries to subclass the types, but should I change it anyway?
 
> The "TODO implement this" bits obviously need addressing.  ;)
Done. I am not too sure I got AllowedKeywords() right. You might want me to test that (and everything else for that matter). How do I write tests/run the code to check all the edge cases? Or is that not how it works, all we need is the review?

What's next? :)
Attachment #740046 - Attachment is obsolete: true
Attachment #740046 - Flags: feedback?(mratcliffe)
Attachment #740046 - Flags: feedback?(fayearthur)
Attachment #740067 - Flags: review?
> Do I have to do more?

At least toolkit/library in addition to those two.

> I didn't know what to do in the constructor when I detect an attempt to construct an
> invalid object.

The options are to either check before calling the constructor, to give the constructor an out param that indicates failure, or to move initialization with the value from the constructor to an Init() method that can return something to indicate failure.

> I get this scary warning:

Oh, right.  Mark that class as MOZ_FINAL; that will tell the compiler no one is allowed to subclass it and it'll stop warning (and give a compile error if someone _does_ try to subclass it).

> How do I write tests/run the code to check all the edge cases?

See the existing files in layout/inspector/tests.  test_isinheritableproperty.html for example.

As far as running them, see https://developer.mozilla.org/en-US/docs/Mochitest#Running_select_tests if you didn't do anything special with custom mozconfig files.  If you did, see https://developer.mozilla.org/en-US/docs/Developer_Guide/mach#mach_and_mozconfigs
Flags: needinfo?(mratcliffe)
Flags: needinfo?(fayearthur)
Attachment #740067 - Flags: review? → review?(bzbarsky)
I'll look at the patch in a bit; I'd really like to hear back from Mike and Heather about the API...
Well, even when I wasn't asked, I want to give some feedback. First of all looking at the code it seems already good in regard of the info we can get.

I hope the constants that make up the bitmasks returned by GetParserVariant(), GetValueRestrictions() and GetParseType() will also be exposed.

Also I assume AllowedKeywords() doesn't include function names like "linear-gradient" for the "background-image" property, right?

Sebastian
So some thoughts on my side in terms of API.

It might make sense to have a CSSPropertyInfo that has something to indicate shorthand vs longhand and then have subclasses for shorthands and longhands that do different things (e.g. shorthands have the list of subproperty infos, while longhands have the longhand-specific parsing info).

allowedKeywords would include whatever is meant by "keywords", but yes "linear-gradient(...)" is not a keyword.
> It might make sense to have a CSSPropertyInfo that has something to indicate shorthand vs longhand ...
Agree.

> allowedKeywords would include whatever is meant by "keywords", but yes "linear-gradient(...)" is not a keyword.
We also need them for a complete coverage of available things for auto-completion and validation. And they should also offer different parsing info.

Sebastian
> We also need them for a complete coverage of available things for
> auto-completion and validation. And they should also offer different parsing
> info.

I think this is doable... if you look at [1], the function queries the variant mask and &'s it to bit masks like VARIENT_GRADIENT, and from there it figures that the token should be of some form (like tk->mIdent.LowerCaseEqualsLiteral("-moz-element"))

What I could do I guess is add a method that returns all the keywords, plus the possible strings deduced from the variant bit mask. This looks like it will be duplicated code though... I will have to look at how [1] does it, and simply copy the strings like "-moz-element". If the strings change there they should also be changed in CSSPropertyInfo... is that okay with you? 

[1] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSParser.cpp#l4820
As I said before our use case is to get all the information we're currently maintaining manually (see the link in comment 2) through an API.
For the auto-completion it is necessary that the API provides a way to get all available keywords and function names.

> What I could do I guess is add a method that returns all the keywords, plus the possible strings deduced from the variant bit mask. This looks like it will be duplicated code though... is that okay with you?
If you mean that allowedKeywords() will be kept and the new function duplicates this information, I'd say it's better to join them into one. That was my idea behind getAvailableValues() in comment 0. If that's not possible for whatever reason, then the new function should just return the information about available CSS functions.

Sebastian
Sebastian's feedback is spot on. Including things like linear gradient is something that the API should provide.
Flags: needinfo?(mratcliffe)
The UI can maybe include linear gradient, but it can't provide all parsing information in general, because some of that information is not present as data, only as code.

So we come back to the original question: what exactly is this API supposed to do?  I thought the goal was to expose the data-drive bits of the CSS backend so that code duplication can be avoided, right?  Seems to me like code duplication between the API implementation and the CSS parser is no better than code duplication between the API consumer and the CSS parser...
I'm a bit concerned that some of the things being exposed aren't things that we're really able to give any reasonable promise of stability or completeness for, especially in their current state.  If that's clearly documented and the consumers are ok with it (including ok with their tests potentially needing to be changed or removed as a result of internal CSS parser changes), I suppose that's acceptable.
(In reply to Boris Zbarsky (:bz) from comment #38)
> The UI can maybe include linear gradient, but it can't provide all parsing
> information in general, because some of that information is not present as
> data, only as code.
> 
> So we come back to the original question: what exactly is this API supposed
> to do?  I thought the goal was to expose the data-drive bits of the CSS
> backend so that code duplication can be avoided, right?  Seems to me like
> code duplication between the API implementation and the CSS parser is no
> better than code duplication between the API consumer and the CSS parser...

You are right, for special cases we can always work around things on the JS side.
> what exactly is this API supposed to do?
Maybe it becomes clearer with some test cases. The test cases require that you have installed Firebug[1].

Test case 1: Auto-completion
1. Open Firebug on https://getfirebug.com/tests/manual/console/joes-original/test.html
2. Switch to the HTML panel and there to the Style side panel
3. Double-click into a free space inside the "body" rule
4. Type "ba"
   => The auto-completion will complete to "background".
5. Press Right and type "-i"
   => The auto-completion will complete to "background-image".
6. Press Tab
   => The inline editor will jump to the value of the newly created property.
7. Press Down three times
   => The auto-completion will suggest "none", then "inherit", then "url()".

I.e. the auto-completion needs to know about the available property names. This we can already get via the computed styles as Mike described in comment 5.
Also the auto-completion needs to know about the keywords and function names available for a specific property and it must be able to differenciate them.

Test case 2: validation
1. Open Firebug on https://getfirebug.com/tests/manual/console/joes-original/test.html
2. Switch to the HTML panel and there to the Style side panel
3. Double-click into a free space inside the "body" rule
4. Type "p" and press Tab
   => The inline editor will jump to the value of the newly created "padding" property.
5. Type "abc"
   => The inline editor will be surrounded by a red border indicating that "abc" is not a valid value for the "padding" property.

The validation needs to know, which values are allowed for a property.

> Seems to me like code duplication between the API implementation and the CSS 
> parser is no better than code duplication between the API consumer and the CSS 
> parser...
Yes, that's why duplication needs to be avoided. I.e. the CSS parser should provide all information the API implementation needs.

Sebastian

[1] https://addons.mozilla.org/firefox/addon/firebug/
> This we can already get via the computed styles as Mike described in comment 5.

Or you can use the API we specifically added for this in bug 839443.

> I.e. the CSS parser should provide all information the API implementation needs.

That's not a reasonable requirement.
Comment on attachment 740067 [details] [diff] [review]
Unfinished proposed patch

I would really like us to be clear on the API before I spend the time to read through this again...
Attachment #740067 - Flags: review?(bzbarsky)
> That's not a reasonable requirement.

To expand on this, the CSS "parser" is never involved.

What the patch is doing is taking some data tables that we have that the CSS parser is using and exposing those.

That does not expose information that is only contained in the form of code in the CSS parser.  "Exposing" it by adding duplicated code to the CSS parser to expose it is no better than duplicating the code in the inspector utils or in devtools.  In my opinion.

Obviously one can argue that duplication in the CSS parser and inspector utils is better than duplication in the parser, the Firefox devtools, and Firebug.
(In reply to Boris Zbarsky (:bz) from comment #44)
> > That's not a reasonable requirement.
> 
> To expand on this, the CSS "parser" is never involved.
> 
> What the patch is doing is taking some data tables that we have that the CSS
> parser is using and exposing those.
> 
> That does not expose information that is only contained in the form of code
> in the CSS parser.  "Exposing" it by adding duplicated code to the CSS
> parser to expose it is no better than duplicating the code in the inspector
> utils or in devtools.  In my opinion.
> 
> Obviously one can argue that duplication in the CSS parser and inspector
> utils is better than duplication in the parser, the Firefox devtools, and
> Firebug.

So Boris something that I could do is go through the CSS parser, extract all the strings we need (like "linear-gradient") and turn them all into macros, and define those macros in nsCSSParser.h. Anyone can then use those strings via the macros in nsCSSParser.h and nsCSSProps.h. It's a bit of grunt work, but grep tells me there are only maybe 100-200 strings that are useful. If it's useful to you I don't mind doing it I guess. does that solve your code duplication problem?
I don't think it does, since the duplication is all about which things apply to which properties.

For linear-gradient in particular, there is no code duplication issue, as you noted: VARIANT_GRADIENT gives us the information we need, and the kinds of gradients can just be hardcoded, I think.  Similar for VARIANT_IMAGE_RECT and VARIANT_ELEMENT and VARIANT_COLOR and VARIANT_COUNTER and so forth.

But we shouldn't pretend in the API like "counter" is a keyword for some property.  "content: counter" is not valid, which it would be if it were a keyword.  Furthermore, autocompletion would presumably want to offer "counter(", not "counter", so exposing it as a separate thing (function values valid for the property) makes sense to me.  That's what Sebastian said in comment 41 as well, I think.

This will work fine for all the properties parsed via ParseVariant, since those are all driven via the table.

It will fail for the things that are CSS_PROPERTY_VALUE_PARSER_FUNCTION or CSS_PROPERTY_PARSE_FUNCTION, though many of the latter are shorthands.
My bad; my last suggestion was way off.

So, so far we have all properties covered except for those declared CSS_PROPERTY_PARSER_FUNCTION and CSS_PROPERTY_PARSE_FUNCTION

all CSS_PROPRETY_PARSER_FUNCTION parsing functions [1] (except ParseTextOverFlow) either:

(a) delegate to a single call of ParseVariant, passing in a variant mask, or
(b) accept integer and string values.
(c) accept keywords

We have (c) covered by grabbing the keywords, and (b) is not useful for autocompletion properties anyway, no? What we could do for (a) is define a table that has all the variant masks for all the CSS_PROPERTY_PARSER_FUNCTION properties, and have the calls  ParserVariant query that table instead of be hard coded. Then we use that table in CSSPropertyInfo.

If this helps then that leaves CSS_PROPERTY_PARSE_FUNCTION properties only... Does it help? Very short reply expected.

[1] http://dxr.mozilla.org/mozilla-central/layout/style/nsCSSParser.cpp#l6348
If the calls devolve to a single call with a variant mask, then putting those in a table somewhere makes sense.

I assume they're not using ParseVariant directly because they need to do additional validation or something?
Yes they each have a custom validation/fallback after a single and only call to ParseVariant. The only exception is ParseTextOverflow, which branches to different calls of ParseVariant with VARIANT_(INHERIT|KEYWORD|STRING).
This has been dormant for more than a month now. Are we waiting for fayearthur@gmail.com need info? Is there a decision to be made here?
I don't think we're waiting on Heather per se.

Someone needs to clearly write down what use cases this API will need to address and then design the API...
I think that we only need two methods:
1. getCSSPropertyNames([startsWith]) should return an array of matching property names including shortcuts and aliases (if they are available in HTML). If startsWith is not provided then all property names should be returned.
2. getCSSSubPropertiesFor(propertyName): Returns an array of possible sub-property values.

e.g.
- getCSSPropertyNames("back") should return ["backface-visibility", "background", "background-attachment", "background-clip", "background-color", "background-image", "background-origin", "background-position", "background-repeat", "background-size"]

- getCSSSubPropertiesFor("display") should return ["none", "inline", "block", "list-item", "inline-block", "inline-table", "table", "table-cell", "table-column", "table-column-group", "table-footer-group", "table-header-group", "table-row", "table-row-group", "flex", "inline-flex", "grid", "inline-grid", "run-in", "inherit"]

- getCSSSubPropertiesFor("color") should return the list of user colors e.g. ["red", "green", "blue"...]
So we already have a getCSSPropertyNames that returns the array of supported property names (controllable by some flags, etc).  The startsWith bit can be done purely in JS with .filter(), right?  Should be no slower than doing it in C++, and more flexible...

getCSSSubPropertiesFor doesn't sound right: subproperties are what the longhand parts of a shorthand are called.  But ignoring the name for the moment, that sounds like what this bug is really about.  So what should it include?  Seems like it should be:

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?

What else, if anything?
And I guess the question is this: if we do just what comment 53 lists, is that a useful thing to do on its own and then add more things later as needed?
> 2. getCSSSubPropertiesFor(propertyName): Returns an array of possible 
> sub-property values.
So that already goes into the direction of getAvailableValues() I described in comment 0.

> What else, if anything?
What about points 3), 5) and 6) of comment 10 and functions as values?

> is that a useful thing to do on its own and then add more things later as needed?
Points 3) and 5) and allowed values being functions are essentially for us.
I could live with the rest being implemented afterwards.

Sebastian
> What about points 3), 5) and 6) of comment 10

Not in the list from comment 52, so I'm not worrying about it for now.

> and functions as values?

That would be an addition to my list from comment 53, yes.  It' may be rather hard to do sanely, hence my question in comment 54.

> Points 3) and 5) and allowed values being functions are essentially for us.

Which "us" if I might ask?

Basically, I want to avoid the perfect becoming the enemy of the good here, but I also want to avoid shipping an API that absolutely no one can use for anything.... So in the interests of some forward progress, what is the smallest API that's actually useful?
>> What about points 3), 5) and 6) of comment 10
> Not in the list from comment 52, so I'm not worrying about it for now.
Sorry, but I am the reporter, not Mike.

>> Points 3) and 5) and allowed values being functions are essentially for us.
> Which "us" if I might ask?
The Firebug Working Group.

> what is the smallest API that's actually useful?
The points listed in comment 53 with point 3) of comment 10 (but with the list of properties that can be set, not the ones it resets) and the functions as values I'd say.

Sebastian
OK.  Mike, would you mind filing a separate bug on your stuff, since clearly there are two very different visions of what needs to happen here?
Flags: needinfo?(mratcliffe)
Whiteboard: [mentor=bz]
(In reply to Boris Zbarsky (:bz) from comment #58)
> OK.  Mike, would you mind filing a separate bug on your stuff, since clearly
> there are two very different visions of what needs to happen here?

Sure, I have logged bug 877690.
Flags: needinfo?(mratcliffe)
Bug 877690 is almost done, with this patch:

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

Also all the property names should be available from inIDOMUtils.idl here:

http://dxr.mozilla.org/mozilla-central/source/layout/inspector/public/inIDOMUtils.idl?from=inIDOMUtils.idl#l50

Does that make ends meet a little better? If something else is needed, and someone is willing to mentor me on making the needed changes, I'd be happy to help.
It certainly helps a bit! Seems like it's missing a few more generic keywords though, like "none" for "background" (which is a VARIANT_).

The main other thing we need is support for CSS functions (most importantly, {rgb,hsl}a?(), url() and calc()). Actual function names would be excellent, but if it's too hard, then just getting access to (a JS equivalent of) the VARIANT_ bitmask would be great too.

(I'm also from the Firebug team.)
Bug 895076 moves it into the "mostly usable" state, so thanks for that! If you're interested, here's a list of what we will have to do on the Firebug side to be able to use the new APIs:
- Hard-code a list of function names, so we can append "()" to them.
- Hard-code a list of property names accepting "url()", gradient functions, and transform functions.
- Hard-code a list of values of -moz-font-feature-settings.
- Hard-code a list of values and common fonts names for font/font-family. (For some reason, getCSSValuesForProperty("font-family") doesn't return e.g. "status-bar", but ("font") does?)
- Filter out "logical", "physical" for margin/padding (AFAIK they are non-standard and can't be set through the shorthands).
- Filter out "-moz-rgba", "-moz-hsla", "-moz-calc", probably others, because we don't want people to use them.
- Filter out "inherit" etc., and instead add them manually to the list only when there is nothing else in the value.
- For returned lists containing "calc()" (or for some hard-coded list of property names - whatever heuristic appears to work best), skipping auto-completion for a single "-" (because popping up auto-completion when trying to type a negative number is annoying).
- Manually special-case some shorthand properties. The reason is two-fold:
  a) Values appear to be pretty wrong. E.g. "animation" gets the same list as "display".
  b) We care about how they split into subproperties. For example when pressing down when the cursor is on the | mark within "border: 1px sol|id red", we want to cycle only through border-style values, and when completing "border: 1px solid s" we don't want to complete that to "solid" because it can't appear twice. And "font" parsing is special.

It would be nice if Gecko could help with at least the first two points.
Flags: needinfo?(fayearthur)
So out of this list,

> - Hard-code a list of property names accepting "url()", gradient functions,
> and transform functions.

These have been done I believe.

> - Hard-code a list of function names, so we can append "()" to them.
> - Hard-code a list of values of -moz-font-feature-settings.
> - Hard-code a list of values and common fonts names for font/font-family.

bz is the expert, but I don't think we have all this information in one place in the parser that we could pull from. The values for this seem to be hard coded in the parser. The way to move forward with this is to needinfo bz on that.

> (For some reason, getCSSValuesForProperty("font-family") doesn't return e.g.
> "status-bar", but ("font") does?)

This might be a bug in the parser, since that's where we are pulling the info. Maybe file a separate bug for this?

> - Manually special-case some shorthand properties. The reason is two-fold:
>   a) Values appear to be pretty wrong. E.g. "animation" gets the same list
> as "display".

This also sounds it might be a bug in the parser, since that's where we're pulling the information from. Maybe file a separate bug? This also might be fixed from one of the followup bugs.

>   b) We care about how they split into subproperties. For example when
> pressing down when the cursor is on the | mark within "border: 1px sol|id
> red", we want to cycle only through border-style values, and when completing
> "border: 1px solid s" we don't want to complete that to "solid" because it
> can't appear twice. And "font" parsing is special.

I think to do this we'll need the specific cases to address, maybe in a separate bug.
Assignee: malmasry → nobody
We finally adopted the API from bug 877690 in https://code.google.com/p/fbug/issues/detail?id=6841, so I'm wontfixing this bug.

(In reply to Mina Almasry from comment #63)
> > - Hard-code a list of property names accepting "url()", gradient functions,
> > and transform functions.
>
> These have been done I believe.

Gradient and transform functions are still missing. I filed bug 973345 for the former; I don't know what to do with "transform" given that it has custom parsing.

> > - Hard-code a list of function names, so we can append "()" to them.
> > - Hard-code a list of values of -moz-font-feature-settings.
> > - Hard-code a list of values and common fonts names for font/font-family.
> 
> bz is the expert, but I don't think we have all this information in one
> place in the parser that we could pull from. The values for this seem to be
> hard coded in the parser. The way to move forward with this is to needinfo
> bz on that.

We're fine with hard-coding this. (Though, it still would be nice to have differentiation between functions and values.)

> > (For some reason, getCSSValuesForProperty("font-family") doesn't return e.g.
> > "status-bar", but ("font") does?)
> 
> This might be a bug in the parser, since that's where we are pulling the
> info. Maybe file a separate bug for this?

It disappeared from "font" a while ago, so I don't know if this bug is still worth filing.

> > - Manually special-case some shorthand properties. The reason is two-fold:
> >   a) Values appear to be pretty wrong. E.g. "animation" gets the same list
> > as "display".
> 
> This also sounds it might be a bug in the parser, since that's where we're
> pulling the information from. Maybe file a separate bug? This also might be
> fixed from one of the followup bugs.

"animation" getting the same list as "display" was fixed in bug 907816. But generally, shorthand properties seem to be getting the values from the properties they reset, not from the ones that are settable by them. So we've had to hard-code "font" (and "font-family"), "border", "border-image", "background", and "text-decoration" (last one is bug 930665). But as mentioned, this doesn't matter so much for us because we want to treat shorthands specially anyway.

> 
> >   b) We care about how they split into subproperties. For example when
> > pressing down when the cursor is on the | mark within "border: 1px sol|id
> > red", we want to cycle only through border-style values, and when completing
> > "border: 1px solid s" we don't want to complete that to "solid" because it
> > can't appear twice. And "font" parsing is special.
> 
> I think to do this we'll need the specific cases to address, maybe in a
> separate bug.

This is specific to our implementation, we can handle this. I just mentioned it for completeness.

--

There are a few properties for which the API doesn't return any results: https://github.com/firebug/firebug/blob/f23815f595e0f416ff94a3d847a267f57512fc12/extension/content/firebug/lib/css.js#L1049-L1083

Which ones of those do you think are reasonable to file bugs for?

Also mentionable is that we're missing all Mozilla-specific colors (listed in "extraColors" in that same file), like ActiveBorder and -moz-ButtonDefault. Being non-standard I can understand that you don't want to expose them, though.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
See Also: → 1254949
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: