The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Vincent LAMOTTE, Assigned: dularylaurent)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, student-project})

Trunk
mozilla16
dev-doc-needed, student-project
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=volkmar])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
(Assignee)

Comment 2

6 years ago
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 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();|.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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.
Attachment #536255 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
Created attachment 536569 [details] [diff] [review]
Creation of IntrinsticState

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
(Assignee)

Comment 10

6 years ago
Created attachment 536614 [details] [diff] [review]
Modification of getters v1.1
Attachment #536566 - Attachment is obsolete: true
Attachment #536566 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 11

6 years ago
Created attachment 536615 [details] [diff] [review]
Creation of IntrinsticState v1.1
Attachment #536569 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 536617 [details] [diff] [review]
Content Tests v1.0
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+
(Assignee)

Comment 15

6 years ago
Created attachment 536627 [details] [diff] [review]
Creation of IntrinsticState v1.2
Attachment #536615 - Attachment is obsolete: true
Attachment #536615 - Flags: review?(Olli.Pettay)
Attachment #536627 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Blocks: 662251

Updated

6 years ago
Attachment #536614 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #536627 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #536617 - Flags: review?(Olli.Pettay) → review+
Keywords: dev-doc-needed

Updated

6 years ago
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
Last Resolved: 5 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.