The default bug view has changed. See this FAQ.

Implement the basic layout of the meter element

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: Vincent LAMOTTE, Assigned: Vincent LAMOTTE)

Tracking

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

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=volkmar])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

This bug is about the layout/rendering of the HTML5 meter element.

Reproducible: Always
(Assignee)

Updated

6 years ago
Blocks: 555985
Depends on: 657938
Keywords: html5
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

6 years ago
Depends on: 659248
(Assignee)

Comment 1

6 years ago
Created attachment 534921 [details] [diff] [review]
Patch v1.0

First Patch for the layout.
Not tested yet, but the compilation is ok.
Easy to share.
Looks like this patch also includes the DOM code.
Comment on attachment 534921 [details] [diff] [review]
Patch v1.0

Review of attachment 534921 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsMeterFrame.cpp
@@ +34,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

After speaking with Olli on IRC, we went to the conclusion that this header should be the same as nsProgressFrame.cpp but you should add your names after mine.

@@ +144,5 @@
> +
> +  nsIFrame* barFrame = mBarDiv->GetPrimaryFrame();
> +  NS_ASSERTION(barFrame, "The meter frame should have a child with a frame!");
> +
> +  // Bar Reflow

Please don't inline the bar reflow in ::Reflow but create a new method. That's going to be easier to read and that's what is usually done in the code base.

@@ +153,5 @@
> +  nscoord size = aReflowState.ComputedWidth() - 
> +			   	(reflowState.mComputedMargin.LeftRight() +
> +				reflowState.mComputedBorderPadding.LeftRight());
> +  size = NS_MAX(size, 0);
> +  reflowState.SetComputedWidth(size); 

I believe you don't except this to work.
Basically, you will have to do something similar to what is done in nsProgressFrame but min will have to be taken into account.

BTW, we could probably create a base class for both frames. But that should be a follow-up.

@@ +194,5 @@
> +{
> +  NS_ASSERTION(mBarDiv, "Meter bar div must exist!");
> +  
> +  if (aNameSpaceID == kNameSpaceID_None &&
> +      (aAttribute == nsGkAtoms::value || aAttribute == nsGkAtoms::max)) {

You will need to reflow if min, max or value changes.

@@ +218,5 @@
> +                    nsSize(0, 0));
> +
> +  nsSize autoSize;
> +  autoSize.height = autoSize.width = fontMet->Font().size; // 1em
> +  autoSize.width *= 10; // 10em

It should be 5em, see:
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-meter-element-0
(Assignee)

Comment 4

6 years ago
Created attachment 535115 [details] [diff] [review]
Patch v1.1

- Removing dom/content part from the patch
- Taking Mounir's comment into account
Attachment #534921 - Attachment is obsolete: true
You're adding a tab in layout/generic/nsQueryFrame.h. Please don't do that.
(Assignee)

Comment 6

6 years ago
Created attachment 535330 [details] [diff] [review]
Patch v1.2

- Removing tabs / windows ends of line (thanks to Ms2ger)
- Adding some forgotten stuff (about constructors)

No going on some tests.
Attachment #535115 - Attachment is obsolete: true
Comment on attachment 535330 [details] [diff] [review]
Patch v1.2

Review of attachment 535330 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/Makefile.in
@@ +72,5 @@
>  		nsGfxButtonControlFrame.cpp \
>  		nsGfxCheckboxControlFrame.cpp \
>  		nsGfxRadioControlFrame.cpp \
>  		nsProgressFrame.cpp \
> +        nsMeterFrame.cpp \

The indentation here is wrong.

::: layout/forms/nsMeterFrame.cpp
@@ +103,5 @@
> +  // Associate ::-moz-meter-bar pseudo-element to the anonymous child.
> +  nsCSSPseudoElements::Type pseudoType = nsCSSPseudoElements::ePseudo_mozMeterBar;
> +  nsRefPtr<nsStyleContext> newStyleContext = PresContext()->StyleSet()->
> +    ResolvePseudoElementStyle(mContent->AsElement(), pseudoType,
> +                              GetStyleContext());

I would prefer this to be added in a separate patch but I guess that's up to the reviewer.

@@ +172,5 @@
> +                             nsPresContext*           aPresContext,
> +                             const nsHTMLReflowState& aReflowState,
> +                             nsReflowStatus&          aStatus)
> +{
> +  bool vertical = GetStyleDisplay()->mOrient == NS_STYLE_ORIENT_VERTICAL;

Same thing here.

::: layout/style/forms.css
@@ +690,5 @@
>    background-color: #0064b4; /* blue */
>  }
>  
> +meter{
> +  -moz-appearance: meterbar;

We don't have 'meterbar'. IOW, that's going to do nothing. I would recommend to put that into comment and open a bug to add it.

@@ +699,5 @@
> +  -moz-border-top-colors: ThreeDShadow -moz-Dialog;
> +  -moz-border-right-colors: ThreeDHighlight -moz-Dialog;
> +  -moz-border-bottom-colors: ThreeDHighlight -moz-Dialog;
> +  -moz-border-left-colors: ThreeDShadow -moz-Dialog;
> +  background-color: -moz-Dialog;

Could you use the current <progress> background color, see:
https://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#662

@@ +708,5 @@
> +  display: inline-block ! important;
> +  float: none ! important;
> +  position: static ! important;
> +  overflow: visible ! important;
> +  -moz-box-sizing: border-box ! important;

You don't need this line.

@@ +710,5 @@
> +  position: static ! important;
> +  overflow: visible ! important;
> +  -moz-box-sizing: border-box ! important;
> +
> +  -moz-appearance: meterchunk;

Same thing than meterbar.

@@ +715,5 @@
> +  height: 100%;
> +  width: 100%;
> +  
> +  /* Default style in case of there is -moz-appearance: none; */
> +  background-color: ThreeDShadow;

I think we could use a kind of green here. In a next patch, we will have to change this depending on the <meter> state.
You are going to need tests for this patch. Feel free to ping me over IRC or to send me an email if you need any help to write them.
Assignee: nobody → Vincent.Lamotte
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 years ago
Created attachment 535399 [details] [diff] [review]
Patch v1.3

Taking Mounir's comment into account.
Attachment #535330 - Attachment is obsolete: true
Comment on attachment 535399 [details] [diff] [review]
Patch v1.3

Review of attachment 535399 [details] [diff] [review]:
-----------------------------------------------------------------

As soon as the tests are ready, you can ask for a review.

::: layout/style/forms.css
@@ +712,5 @@
> +  /* Related to the -moz-appearance : meterbar - Bug 659999
> +   *  position: static ! important;
> +   *  overflow: visible ! important; 	
> +   *  -moz-box-sizing: border-box ! important;		
> +   *  -moz-appearance: meterchunk;

Actually, only -moz-box-sizing was not useful. It's added to <progress> as a workaround for a bug on Windows.
You should keep the other lines and probably open a separate bug for meterchunk.
Keywords: student-project
Whiteboard: [mentor=volkmar]
(Assignee)

Comment 11

6 years ago
Created attachment 535616 [details] [diff] [review]
Patch v1.4

Minor changes
Attachment #535399 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 535636 [details] [diff] [review]
Patch v1.5

Basic layout "appears" to work.
Advance tests are coming soon.
Attachment #535616 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 660238
(Assignee)

Comment 13

6 years ago
Created attachment 536087 [details] [diff] [review]
Test v1.0

Tests for the basic layout.
Based on reftests of the progress element.
(Assignee)

Updated

6 years ago
Attachment #536087 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #535636 - Flags: review?(roc)
Comment on attachment 535636 [details] [diff] [review]
Patch v1.5

Review of attachment 535636 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsMeterFrame.cpp
@@ +79,5 @@
> +  NS_ASSERTION(!GetPrevContinuation(),
> +               "nsMeterFrame should not have continuations; if it does we "
> +               "need to call RegUnregAccessKey only for the first.");
> +  nsFormControlFrame::RegUnRegAccessKey(static_cast<nsIFrame*>(this), PR_FALSE);
> +  nsContentUtils::DestroyAnonymousContent(&mBarDiv);

We should probably refactor code so we don't need to explicitly write this boilerplate code. For example, we could have a special class that contains an nsCOMPtr<nsIContent> and automatically calls nsContentUtils::DestroyAnonymousContent. Or maybe nsFrame::DestroyFrom could check a state bit to see if there is anonymous content and if there is, call AppendAnonymousContentTo to get it and destroy it.

Also I think the access-key system could be reworked so that nsGenericHTMLElement takes care of all access-key registration/unregistration and frames wouldn't have to deal with it.

Obviously that work would happen in other bugs.

@@ +107,5 @@
> +                              GetStyleContext());
> +
> +  if (!aElements.AppendElement(ContentInfo(mBarDiv, newStyleContext))) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Hmm, again, we have a lot of duplicated code in the implementations of CreateAnonymousContent. We could probably have a utility method nsContentUtils::CreateAnonymousChild(nsIContent* aParent, nsIAtom* aTag, PRUint32 aNamespaceID, nsGenericElement* (* aCreator)(already_AddRefed<nsINodeInfo>, FromParser), nsCSSPseudoElements::Type aPseudoType, nsTArray<ContentInfo>& aElements) (where aPseudoType can be null)
... or something like that.

Again, fodder for another bug.

@@ +191,5 @@
> +  meterElement->GetValue(&value);  
> +  position = max-min;
> +  position = position != 0 ? (value-min) / position : 1;
> +
> +  size *= position;

Let's explicitly round, so use NSToCoordRound(size*position).

::: layout/style/forms.css
@@ +689,5 @@
>    /* Default style in case of there is -moz-appearance: none; */
>    background-color: #0064b4; /* blue */
>  }
>  
> +meter{

space before {
Attachment #535636 - Flags: review?(roc) → review+
By the way, I assume you considered having nsProgressFrame and nsMeterFrame be the same class? What would make that hard?
Attachment #536087 - Attachment is patch: true
Comment on attachment 536087 [details] [diff] [review]
Test v1.0

Review of attachment 536087 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536087 - Flags: review?(roc) → review+
Comment on attachment 535636 [details] [diff] [review]
Patch v1.5

Review of attachment 535636 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/forms.css
@@ +711,5 @@
> +  display: inline-block ! important;
> +  float: none ! important;
> +  position: static ! important;
> +  overflow: visible ! important; 
> +  /* Add a new moz-appearance : Bug ???????

bug 660232
(Assignee)

Comment 18

6 years ago
Created attachment 536324 [details] [diff] [review]
Patch v1.6

:roc & :volkmar changes.
Attachment #535636 - Attachment is obsolete: true

Updated

6 years ago
Blocks: 662251
Keywords: dev-doc-needed
Blocks: 663367
Hmm, you probably need to implement GetMinWidth and GetPrefWidth for meters.
I've open a bug for that. I have a patch but I think I forgot to attach it. Will do that.
https://hg.mozilla.org/mozilla-central/rev/1774f76e6867
https://hg.mozilla.org/mozilla-central/rev/30a518ac406f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.