Closed
Bug 660238
Opened 14 years ago
Closed 13 years ago
Add pseudo-class to access optimal, sub-optimal and sub-sub-optimal <meter> elements
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: Vincent.Lamotte, Assigned: dularylaurent)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, student-project, Whiteboard: [mentor=volkmar])
Attachments
(3 files, 5 obsolete files)
10.77 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.43 KB,
patch
|
smaug
:
review+
mounir
:
feedback+
|
Details | Diff | Splinter Review |
6.01 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110527 Firefox/7.0a1
Build Identifier:
The meter element is basically implemented in the Bug #657953 (no optimum management).
The purpose of this bug is to add different colors for the meter bar according to the optimum and the value.
Reproducible: Always
Reporter | ||
Updated•14 years ago
|
Comment 1•14 years ago
|
||
Changing the summary given that this is going to be done trough pseudo-classes.
These names could be used:
:-moz-meter-optimum
:-moz-meter-sub-optimum
:-moz-meter-sub-sub-optimum
It's only a proposition, use other names if you feel like it.
Status: UNCONFIRMED → NEW
Component: Layout → DOM: Core & HTML
Ever confirmed: true
QA Contact: layout → general
Hardware: Other → All
Summary: Add colors for optimum related areas of the meter element → Add pseudo-class to access optimal, sub-optimal and sub-sub-optimal <meter> elements
Assignee | ||
Comment 2•14 years ago
|
||
Content changes to add states for optimal, sub-optimal and sub-sub-optimal <meter> elements.
Comment 3•14 years ago
|
||
Comment on attachment 536255 [details] [diff] [review]
Content Patch v1.0
Review of attachment 536255 [details] [diff] [review]:
-----------------------------------------------------------------
You should write tests too ;)
::: content/events/public/nsEventStates.h
@@ +265,5 @@
> #define NS_EVENT_STATE_MOZ_UI_VALID NS_DEFINE_EVENT_STATE_MACRO(33)
> +// Content is in the optimum region.
> +#define NS_EVENT_STATE_OPTIMUM NS_DEFINE_EVENT_STATE_MACRO(34)
> +// Content is in the suboptimal region.
> +#define NS_EVENT_STATE_SUBOPTIMUM NS_DEFINE_EVENT_STATE_MACRO(35)
I would prefer this name: NS_EVENT_STATE_SUB_OPTIMUM
And you should probably add this state: NS_EVENT_STATE_SUB_SUB_OPTIMUM
::: content/html/content/src/nsHTMLMeterElement.cpp
@@ +71,5 @@
> NS_IMETHOD_(PRUint32) GetType() const { return NS_FORM_METER; }
> NS_IMETHOD Reset();
> NS_IMETHOD SubmitNamesValues(nsFormSubmission* aFormSubmission);
>
> + nsEventStates IntrinsicState();
Should be:
virtual nsEventStates IntrinsicState() const;
@@ +88,5 @@
> + * 0 if the actual value is in the sub-suboptimal region.
> + *
> + * @return the region of the actual value.
> + */
> + bool IsOptimum();
2 and 1 are going to be equivalent of 'true' here while '0' will be false. You should probably create an enum for those three states.
And make the method const and private, please. Actually, I think you could change "protected" to "private".
@@ +158,5 @@
> + state |= NS_EVENT_STATE_SUBOPTIMUM;
> + } else if (isOptimum == 0) {
> + state &= ~NS_EVENT_STATE_OPTIMUM;
> + state &= ~NS_EVENT_STATE_SUBOPTIMUM;
> + }
I don't think you need to unset the other states given that they are not going to be set by nsGenericHTMLFormElement::IntrinsicState().
Also, it looks like you want to have sub-sub-optimum equivalent to not optimum and not sub-optimum. I don't think we should do that.
@@ +441,5 @@
> + * and then the value returned is 1.
> + * if the value is in [minimum, low[,
> + * the value is in the sub-suboptimal region
> + * and then the value returned is 0.
> + */
I think this comment is too deep. You should just explain the general idea.
@@ +476,5 @@
> + return 1;
> + } else {
> + return 0;
> + }
> + }
In this function, you should use the tertiary operator "?" to make it simpler to read. In addition. when using "return", you shouldn't add "else" after.
For example:
if (foo > bar) {
return 0;
}
return 1;
Attachment #536255 -
Flags: feedback-
Comment 4•14 years ago
|
||
As said on IRC, it might be better to split this in two patches:
1. Create the needed const methods (for IntrinsincState) and have current getters calling them.
2. What should be done in this bug.
And BTW, IsOptimum() should be renamed "GetOptimumState()" instead, returning NS_EVENT_STATE{,_SUB,_SUB_SUB}_OPTIMUM so you will be able to do: |state |= GetOptimumState();|.
Assignee | ||
Comment 5•14 years ago
|
||
I've created const getters which can be called by IntrinsticState and by the current getters.
Assignee | ||
Comment 6•14 years ago
|
||
Realisation of the IntrinsticState function with add of NS_EVENT_STATE_{,SUB_,SUB_SUB_}OPTIMUM states.
Attachment #536255 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
Correction of coding-style errors.
Attachment #536568 -
Attachment is obsolete: true
Comment 8•14 years ago
|
||
Comment on attachment 536566 [details] [diff] [review]
Modification of getters
Review of attachment 536566 [details] [diff] [review]:
-----------------------------------------------------------------
f=mounir with all the nits fixed.
::: content/html/content/src/nsHTMLMeterElement.cpp
@@ +83,5 @@
> static const double kDefaultValue;
> static const double kDefaultMin;
> static const double kDefaultMax;
> +
> +private:
You can just rename protected to private. We don't need to have these static variables protected.
@@ +201,5 @@
> + * Otherwise, the maximum is the default value.
> + * If the maximum value is less than the minimum value,
> + * the maximum value is the same as the minimum value.
> + */
> + double aValue;
Please rename the variable (max or value, as you want). The reader might be confused assuming it's a parameter.
@@ +224,5 @@
> + * the actual value is the same as the minimum value.
> + * If the actual value is greater than the maximum value,
> + * the actual value is the same as the maximum value.
> + */
> + double aValue;
ditto
@@ +261,5 @@
> + if (!attrLow || attrLow->Type() != nsAttrValue::eDoubleValue) {
> + return min;
> + }
> +
> + double aValue = attrLow->GetDoubleValue();
ditto
@@ +289,5 @@
> + if (!attrHigh || attrHigh->Type() != nsAttrValue::eDoubleValue) {
> + return max;
> + }
> +
> + double aValue = attrHigh->GetDoubleValue();
ditto
@@ +322,5 @@
> + if (!attrOptimum || attrOptimum->Type() != nsAttrValue::eDoubleValue) {
> + return (min + max) / 2.0;
> + }
> +
> + double aValue = attrOptimum->GetDoubleValue();
ditto
Attachment #536566 -
Flags: review?(Olli.Pettay)
Attachment #536566 -
Flags: feedback+
Comment 9•14 years ago
|
||
Comment on attachment 536569 [details] [diff] [review]
Creation of IntrinsticState
Review of attachment 536569 [details] [diff] [review]:
-----------------------------------------------------------------
You have to add the new pseudo-classes to this file:
layout/style/nsCSSPseudoClassList.h
::: content/html/content/src/nsHTMLMeterElement.cpp
@@ +467,5 @@
> + if (value <= high) {
> + return NS_EVENT_STATE_SUB_OPTIMUM;
> + }
> + return NS_EVENT_STATE_SUB_SUB_OPTIMUM;
> + } else if (optimum <= high) {
You don't need the |else if| here. You can just put an |if| statement.
@@ +471,5 @@
> + } else if (optimum <= high) {
> + if (value >= low && value <= high) {
> + return NS_EVENT_STATE_OPTIMUM;
> + }
> + return NS_EVENT_STATE_SUB_OPTIMUM;
Could be:
return (value >= low && value <= high) ? NS_EVENT_STATE_OPTIMUM : NS_EVENT_STATE_SUB_OPTIMUM;
@@ +472,5 @@
> + if (value >= low && value <= high) {
> + return NS_EVENT_STATE_OPTIMUM;
> + }
> + return NS_EVENT_STATE_SUB_OPTIMUM;
> + } else {
Don't need the |else|.
Updated•14 years ago
|
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #536566 -
Attachment is obsolete: true
Attachment #536566 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 11•14 years ago
|
||
Attachment #536569 -
Attachment is obsolete: true
Assignee | ||
Comment 12•14 years ago
|
||
Comment 13•14 years ago
|
||
Comment on attachment 536615 [details] [diff] [review]
Creation of IntrinsticState v1.1
Review of attachment 536615 [details] [diff] [review]:
-----------------------------------------------------------------
Great :)
::: content/html/content/src/nsHTMLMeterElement.cpp
@@ +455,5 @@
> + double low = GetLow();
> +
> + double high = GetHigh();
> +
> + double optimum = GetOptimum();
You don't need a blank line between each declaration.
@@ +472,5 @@
> + return NS_EVENT_STATE_OPTIMUM;
> + }
> + return NS_EVENT_STATE_SUB_OPTIMUM;
> + }
> + if (value > high) {
A comment like: "// optimum > high" could be helpful.
Though, I would set the optimum > high case in second and optimum in [low, high] for the last case. This said, it's a detail ;)
Attachment #536615 -
Flags: review?(Olli.Pettay)
Attachment #536615 -
Flags: feedback+
Updated•14 years ago
|
Attachment #536614 -
Flags: review?(Olli.Pettay)
Comment 14•14 years ago
|
||
Comment on attachment 536617 [details] [diff] [review]
Content Tests v1.0
Review of attachment 536617 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536617 -
Flags: review?(Olli.Pettay)
Attachment #536617 -
Flags: feedback+
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #536615 -
Attachment is obsolete: true
Attachment #536615 -
Flags: review?(Olli.Pettay)
Attachment #536627 -
Flags: review?(Olli.Pettay)
Updated•14 years ago
|
Attachment #536614 -
Flags: review?(Olli.Pettay) → review+
Updated•14 years ago
|
Attachment #536627 -
Flags: review?(Olli.Pettay) → review+
Updated•14 years ago
|
Attachment #536617 -
Flags: review?(Olli.Pettay) → review+
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 16•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ca2a01618e43
https://hg.mozilla.org/mozilla-central/rev/3ffbdbac1f7f
https://hg.mozilla.org/mozilla-central/rev/25c91c36638c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment 17•12 years ago
|
||
Small question in order to help me in documenting this.
Is this in a spec in the standard track? If so in which one? If not, is this planned? (So that I can track the spec in the future)
Comment 18•12 years ago
|
||
Nothing is planned for the moment. You can point to what webkit does though:
http://trac.webkit.org/wiki/Styling%20Form%20Controls#meter
You need to log in
before you can comment on or make changes to this bug.
Description
•