Closed Bug 660238 Opened 13 years ago Closed 12 years ago

Add pseudo-class to access optimal, sub-optimal and sub-sub-optimal <meter> elements

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

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)

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
Blocks: 555985
Depends on: 657953
OS: Windows 7 → All
Version: unspecified → Trunk
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
Attached patch Content Patch v1.0 (obsolete) — Splinter Review
Content changes to add states for optimal, sub-optimal and sub-sub-optimal <meter> elements.
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-
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();|.
Attached patch Modification of getters (obsolete) — Splinter Review
I've created const getters which can be called by IntrinsticState and by the current getters.
Attached patch Creation of IntrinsticState (obsolete) — Splinter Review
Realisation of the IntrinsticState function with add of NS_EVENT_STATE_{,SUB_,SUB_SUB_}OPTIMUM states.
Attachment #536255 - Attachment is obsolete: true
Attached patch Creation of IntrinsticState (obsolete) — Splinter Review
Correction of coding-style errors.
Attachment #536568 - Attachment is obsolete: true
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 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|.
Assignee: nobody → dularylaurent
Keywords: student-project
Whiteboard: [mentor=volkmar]
Status: NEW → ASSIGNED
Attachment #536566 - Attachment is obsolete: true
Attachment #536566 - Flags: review?(Olli.Pettay)
Attached patch Creation of IntrinsticState v1.1 (obsolete) — Splinter Review
Attachment #536569 - Attachment is obsolete: true
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+
Attachment #536614 - Flags: review?(Olli.Pettay)
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+
Attachment #536615 - Attachment is obsolete: true
Attachment #536615 - Flags: review?(Olli.Pettay)
Attachment #536627 - Flags: review?(Olli.Pettay)
Blocks: 662251
Attachment #536614 - Flags: review?(Olli.Pettay) → review+
Attachment #536627 - Flags: review?(Olli.Pettay) → review+
Attachment #536617 - Flags: review?(Olli.Pettay) → review+
Blocks: 663490
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: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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)
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.