Closed Bug 895076 Opened 8 years ago Closed 8 years ago

domUtils.getCSSValuesForProperty does not return non keyword values like auto/normal/none/all

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Optimizer, Assigned: almasry.mina)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

So things like display: none is possible, but background-image: none is not possible, same for letter-spacing: normal and  transition-property: all.

Not sure what other cases are missing..
Basically, we need to handle VARIANT_NONE, VARIANT_ALL, VARIANT_NORMAL, VARIANT_AUTO.

Mina, want to do this?
Flags: needinfo?(almasry.mina)
Also, you can use margin as a testcase for "auto".
I would love to.

But are none, all, and normal all we want, though? I'm happy to do that, but I have an idea that might make us tackle a decent chunk of the VARIANT_* bits, and without code duplication (sorry if I'm out of my depth here):

in nsCSSProps.h we could define an array, or hash table:
  
  nsTArray<nsString> identifiersOf[VARIANT_*];

It takes a VARIANT_* and returns an array of strings, that are the identifiers associated with that VARIANT.

Then, in ParseVariant, we can change all instances of:

if (((aVariantMask & VARIANT_COUNTER) != 0) &&
    (eCSSToken_Function == tk->mType) &&
    (tk->mIdent.LowerCaseEqualsLiteral("counter") ||
     tk->mIdent.LowerCaseEqualsLiteral("counters"))) {
    return ParseCounter(aValue);
}

to:

if (((aVariantMask & VARIANT_COUNTER) != 0) &&
      (eCSSToken_Function == tk->mType) &&
      (tk->mIdent.LowerCaseEqualsAnyOfLiterals(identifiersOf[VARIANT_COUNTER]) {
    return ParseCounter(aValue);
}

For this to work, we would have to add LowerCaseEqualsAnyOfLiterals, which compares mIdent to all the strings in the array of strings you pass it.

Then, in GetCSSValuesForProperty, we can look at identifiersOf and return them along with the colors and keywords, and there would be no code duplication. This shouldn't be significantly slower, or take much more memory than the way things are done now too, no?

Would this be a solution? If not, I am happy to keep it simple, and deal with none, all, normal and auto for now.
Flags: needinfo?(almasry.mina)
Assignee: nobody → almasry.mina
> But are none, all, and normal all we want, though? 

And "auto".  But yes, if we want to include stuff like counters and calc and whatnot that would be pretty cool too.  And note that VARIANT_COLOR can include named colors...

I'm not sure about adding LowerCaseEqualsAnyOfLiterals on all strings, but we could have a static function for it in the CSS parser.
Attached patch patch (obsolete) — Splinter Review
Okay I took care of the four mentioned plus a handful more: calc, -moz-calc, -moz-element, -moz-element-rect, rgb, hsl, -moz-rgba, -moz-hsla, rgba, hsla, cubic-bezer, and steps.

I've updated the tests, too.
Attachment #777596 - Flags: review?(bzbarsky)
Attachment #777596 - Attachment is patch: true
Attachment #777596 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 777596 [details] [diff] [review]
patch

>+static void InsertNoDuplicates(nsTArray<nsString>& aArray,

I'm a little worried about how this will behave with the long color list.  Given that that list is sorted (is it?), would it be worth putting in colors _first_ (via just appends) before doing keywords?  Either way.

>+static void GetVariants(const uint32_t aVariant,

Maybe GetOtherValuesForProperty?  And aParserVariant?

The commit message should say what the change does, not just be a description of the problem.  ;)

r=me
Attachment #777596 - Flags: review?(bzbarsky) → review+
Attached patch simple-css-autocomplete.patch (obsolete) — Splinter Review
Made changes and marking checkin needed.

If you have something in mind I should work on, please link :-)

Thanks!
Attachment #777596 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 778221 [details] [diff] [review]
simple-css-autocomplete.patch

>+static void GetOtherValuesForProperty(const uint32_t aParserVariant,
>+                           nsTArray<nsString>& aArray)

Fix the indent?
Attached patch patchSplinter Review
Yessir.
Attachment #778221 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5fae2f62da66
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.