Last Comment Bug 660238 - Add pseudo-class to access optimal, sub-optimal and sub-sub-optimal <meter> elements
: Add pseudo-class to access optimal, sub-optimal and sub-sub-optimal <meter> e...
Status: RESOLVED FIXED
[mentor=volkmar]
: dev-doc-needed, student-project
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: dularylaurent
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 657953
Blocks: 555985 663490 662251
  Show dependency treegraph
 
Reported: 2011-05-27 08:07 PDT by Vincent LAMOTTE
Modified: 2012-08-15 10:42 PDT (History)
8 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Content Patch v1.0 (5.66 KB, patch)
2011-05-31 02:02 PDT, dularylaurent
mounir: feedback-
Details | Diff | Splinter Review
Modification of getters (10.79 KB, patch)
2011-06-01 04:37 PDT, dularylaurent
mounir: feedback+
Details | Diff | Splinter Review
Creation of IntrinsticState (4.83 KB, patch)
2011-06-01 04:40 PDT, dularylaurent
no flags Details | Diff | Splinter Review
Creation of IntrinsticState (4.83 KB, patch)
2011-06-01 04:45 PDT, dularylaurent
no flags Details | Diff | Splinter Review
Modification of getters v1.1 (10.77 KB, patch)
2011-06-01 07:42 PDT, dularylaurent
bugs: review+
Details | Diff | Splinter Review
Creation of IntrinsticState v1.1 (5.98 KB, patch)
2011-06-01 07:44 PDT, dularylaurent
mounir: feedback+
Details | Diff | Splinter Review
Content Tests v1.0 (6.43 KB, patch)
2011-06-01 07:50 PDT, dularylaurent
bugs: review+
mounir: feedback+
Details | Diff | Splinter Review
Creation of IntrinsticState v1.2 (6.01 KB, patch)
2011-06-01 08:26 PDT, dularylaurent
bugs: review+
Details | Diff | Splinter Review

Description Vincent LAMOTTE 2011-05-27 08:07:47 PDT
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
Comment 1 Mounir Lamouri (:mounir) 2011-05-27 08:20:28 PDT
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.
Comment 2 dularylaurent 2011-05-31 02:02:13 PDT
Created attachment 536255 [details] [diff] [review]
Content Patch v1.0

Content changes to add states for optimal, sub-optimal and sub-sub-optimal <meter> elements.
Comment 3 Mounir Lamouri (:mounir) 2011-05-31 04:18:51 PDT
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;
Comment 4 Mounir Lamouri (:mounir) 2011-05-31 08:12:38 PDT
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();|.
Comment 5 dularylaurent 2011-06-01 04:37:52 PDT
Created attachment 536566 [details] [diff] [review]
Modification of getters

I've created const getters which can be called by IntrinsticState and by the current getters.
Comment 6 dularylaurent 2011-06-01 04:40:24 PDT
Created attachment 536568 [details] [diff] [review]
Creation of IntrinsticState

Realisation of the IntrinsticState function with add of NS_EVENT_STATE_{,SUB_,SUB_SUB_}OPTIMUM states.
Comment 7 dularylaurent 2011-06-01 04:45:57 PDT
Created attachment 536569 [details] [diff] [review]
Creation of IntrinsticState

Correction of coding-style errors.
Comment 8 Mounir Lamouri (:mounir) 2011-06-01 06:16:45 PDT
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
Comment 9 Mounir Lamouri (:mounir) 2011-06-01 06:25:50 PDT
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|.
Comment 10 dularylaurent 2011-06-01 07:42:45 PDT
Created attachment 536614 [details] [diff] [review]
Modification of getters v1.1
Comment 11 dularylaurent 2011-06-01 07:44:22 PDT
Created attachment 536615 [details] [diff] [review]
Creation of IntrinsticState v1.1
Comment 12 dularylaurent 2011-06-01 07:50:26 PDT
Created attachment 536617 [details] [diff] [review]
Content Tests v1.0
Comment 13 Mounir Lamouri (:mounir) 2011-06-01 07:55:33 PDT
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 ;)
Comment 14 Mounir Lamouri (:mounir) 2011-06-01 08:03:21 PDT
Comment on attachment 536617 [details] [diff] [review]
Content Tests v1.0

Review of attachment 536617 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 15 dularylaurent 2011-06-01 08:26:22 PDT
Created attachment 536627 [details] [diff] [review]
Creation of IntrinsticState v1.2
Comment 17 Jean-Yves Perrier [:teoli] 2012-07-22 09:55:12 PDT
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 Mounir Lamouri (:mounir) 2012-07-22 11:17:06 PDT
Nothing is planned for the moment. You can point to what webkit does though:
http://trac.webkit.org/wiki/Styling%20Form%20Controls#meter

Note You need to log in before you can comment on or make changes to this bug.