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)
Core
CSS Parsing and Computation
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)
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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
Reporter | ||
Comment 3•9 years ago
|
||
(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
Assignee | ||
Comment 4•9 years ago
|
||
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.
Reporter | ||
Comment 5•9 years ago
|
||
(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
Assignee | ||
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
I fixed those nits, thanks.
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Reporter | ||
Comment 12•9 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•