Closed Bug 1246101 Opened 9 years ago Closed 9 years ago

inIDOMUtils.getCSSValuesForProperty() doesn't return values for align-* and justify-* anymore

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: sebo, Assigned: MatsPalmgren_bugz)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

According to mozregression https://hg.mozilla.org/mozilla-central/rev/56aebef6afdf related to bug 1151214 caused inIDOMUtils.getCSSValuesForProperty() to not return the specific values for the align-* and justify-* properties anymore. Test case (to execute in Scratchpad): var domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils); domUtils.getCSSValuesForProperty("align-items"); This should return an array containing values like 'baseline', 'center', 'flex-end', etc. Sebastian
That regression range seems unlikely given that it only changes layout code. Testing m-c Nightly builds I get this range: 2015-11-03-03-02-48 -- 2015-11-04-03-04-37 https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb4d614a0b09bcb9738c151dccfcd9b3857a6a7c&tochange=6077f51254c69a1e14e1b61acba4af451bf1783e which includes the style system changes in bug 1176782, which I think is the culprit.
Blocks: 1176782
No longer blocks: 1151214
I suspect this is caused by going from CSS_PROPERTY_PARSE_VALUE to CSS_PROPERTY_PARSE_FUNCTION for these properties: https://bugzilla.mozilla.org/attachment.cgi?id=8677583&action=diff#a/layout/style/nsCSSPropList.h_sec2 The problem is that the syntax is more complex now than a simple enumeration of values, so it's not really possible to revert that change. See for example: https://drafts.csswg.org/css-align-3/#propdef-justify-content I guess we could try to make domUtils.getCSSValuesForProperty return an enumeration of all possible combinations of keywords, but that'd be a rather long list so I'm not sure that's useful in the devtools UI. And it probably means we need to hard code the list of possible values somewhere, which is kind of ugly. I'm leaning towards WONTFIX.
Priority: -- → P4
(In reply to Mats Palmgren (:mats) from comment #1) > That regression range seems unlikely given that it only changes layout code. I see, so mozregression-gui may have been wrong here. (In reply to Mats Palmgren (:mats) from comment #2) > I suspect this is caused by going from CSS_PROPERTY_PARSE_VALUE to > CSS_PROPERTY_PARSE_FUNCTION for these properties: > https://bugzilla.mozilla.org/attachment.cgi?id=8677583&action=diff#a/layout/ > style/nsCSSPropList.h_sec2 > > The problem is that the syntax is more complex now than a simple enumeration > of values, so it's not really possible to revert that change. See for > example: > https://drafts.csswg.org/css-align-3/#propdef-justify-content I am not asking to revert the change, I'm asking to fix the auto-completion. > I guess we could try to make domUtils.getCSSValuesForProperty return an > enumeration of > all possible combinations of keywords, but that'd be a rather long list so > I'm not sure > that's useful in the devtools UI. The devtools always list the single keywords, never combinations of them. And the lists of keywords may be long as in case of 'color' or '-moz-appearance', for example. > And it probably means we need to hard > code the list of > possible values somewhere, which is kind of ugly. It should be done the same way as for the properties mentioned above. Sebastian
The problem is that the value for these properties used to be single keyword, but the spec changed and the syntax is more complex now. That rules out implementing them the same way as 'color' or '-moz-appearance' (which are both using CSS_PROPERTY_PARSE_VALUE). As far as I know, there is no mechanism in the style system for providing an enumeration of possible values for CSS_PROPERTY_PARSE_FUNCTION properties that the devtools completion could use. I suspect we have the same problem for all the other CSS_PROPERTY_PARSE_FUNCTION properties in this list: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h Let me know if you see one where completion works and I can try to mimic that.
(In reply to Mats Palmgren (:mats) from comment #4) > The problem is that the value for these properties used to be single keyword, > but the spec changed and the syntax is more complex now. That rules out > implementing them the same way as 'color' or '-moz-appearance' (which are > both using CSS_PROPERTY_PARSE_VALUE). > > As far as I know, there is no mechanism in the style system for providing > an enumeration of possible values for CSS_PROPERTY_PARSE_FUNCTION properties > that the devtools completion could use. > > I suspect we have the same problem for all the other > CSS_PROPERTY_PARSE_FUNCTION > properties in this list: > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h > Let me know if you see one where completion works and I can try to mimic > that. E.g. it works for: transition-property marker background-repeat grid-gap font-variant Most of them are shorthand properties, though there are also some longhands having CSS_PROPERTY_PARSE_FUNCTION. I'm not good in C++, though it seems like you can refer to a keywords table in the seventh parameter of the macros. And the keyword tables are defined at http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp#720. FWIW the related API was introduced in http://hg.mozilla.org/mozilla-central/rev/0cedd434dddb. (Btw. the auto-completion is generally pretty incomplete at the moment, though that's another point.) Sebastian
Oh, I wasn't aware that table was used only for auto-completion, but the commit message seems pretty clear about that. Good, I'll add a table so we get the single keyword values listed.
Assignee: nobody → mats
Attached patch fix (deleted) — Splinter Review
This restores keyword tables for auto-completion for align-/justify-* so we get at least auto-completion for the single keyword values. (I always thought these entries in the property list were tied to CSS_PROPERTY_PARSE_VALUE and had wider use than just auto-completion, but it's just that we re-use the same keyword tables in parsing etc for some properties.)
Attachment #8716759 - Flags: review?(dholbert)
Comment on attachment 8716759 [details] [diff] [review] fix Review of attachment 8716759 [details] [diff] [review]: ----------------------------------------------------------------- r=me, one optional nit: ::: layout/style/nsCSSProps.cpp @@ +1414,5 @@ > + { eCSSKeyword_right, NS_STYLE_ALIGN_RIGHT }, > + { eCSSKeyword_UNKNOWN, -1 } > +}; > + > +const KTableEntry nsCSSProps::kAutoCompletionAlignItems[] = { I'd suggesting swapping the order of the last 2 arrays here. In particular, this order would make more sense to me: kAutoCompletionAlignJustifySelf kAutoCompletionAlignItems kAutoCompletionAlignJustifyContent ..because the "-self" and "-items" properties are directly related (one influencing the other) and take almost the exact same list of values. So it makes sense to have their arrays side by side, for easy comparison. Not a big deal, though. (But, if you do reorder these, make sure to also reorder their decls in the header-file, too.) ::: layout/style/nsCSSProps.h @@ +733,5 @@ > static const KTableEntry kAlignNormalStretchBaseline[]; // 'normal/stretch/baseline/last-baseline' > static const KTableEntry kAlignNormalBaseline[]; // 'normal/baseline/last-baseline' > static const KTableEntry kAlignContentDistribution[]; // <content-distribution> > static const KTableEntry kAlignContentPosition[]; // <content-position> > + // -- tables for auto-completion of the align-/justify-content/items/self properties -- Perhaps a clearer way to express the list of properties here: {align,justify}-{content,items,self}
Attachment #8716759 - Flags: review?(dholbert) → review+
I fixed those nits, thanks.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Works again. Thank you for the quick fix! I think I'll go over all properties at some point and check whether they are missing their keywords. Let me know if you believe that's not worth to do. Sebastian
Status: RESOLVED → VERIFIED
Blocks: 1254949
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: