657953 - Implement the basic layout of the meter element

Status: RESOLVED FIXED

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

Description

[Vincent LAMOTTE]

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

Comment 1

[Vincent LAMOTTE]

Attachment: Patch v1.0

First Patch for the layout.
Not tested yet, but the compilation is ok.
Easy to share.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like this patch also includes the DOM code.

Comment 3

[Mounir Lamouri (:mounir)]

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

Comment 4

[Vincent LAMOTTE]

Attachment: Patch v1.1

- Removing dom/content part from the patch
- Taking Mounir's comment into account

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You're adding a tab in layout/generic/nsQueryFrame.h. Please don't do that.

Comment 6

[Vincent LAMOTTE]

Attachment: Patch v1.2

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

No going on some tests.

Comment 7

[Mounir Lamouri (:mounir)]

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.

Comment 8

[Mounir Lamouri (:mounir)]

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.

Comment 9

[Vincent LAMOTTE]

Attachment: Patch v1.3

Taking Mounir's comment into account.

Comment 10

[Mounir Lamouri (:mounir)]

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.

Comment 11

[Vincent LAMOTTE]

Attachment: Patch v1.4

Minor changes

Comment 12

[Vincent LAMOTTE]

Attachment: Patch v1.5

Basic layout "appears" to work.
Advance tests are coming soon.

Comment 13

[Vincent LAMOTTE]

Attachment: Test v1.0

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 {

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

By the way, I assume you considered having nsProgressFrame and nsMeterFrame be the same class? What would make that hard?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 536087 [details] [diff] [review]
Test v1.0

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

Comment 17

[Mounir Lamouri (:mounir)]

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

Comment 18

[Vincent LAMOTTE]

Attachment: Patch v1.6

:roc & :volkmar changes.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, you probably need to implement GetMinWidth and GetPrefWidth for meters.

Comment 20

[Mounir Lamouri (:mounir)]

I've open a bug for that. I have a patch but I think I forgot to attach it. Will do that.

Comment 21

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/1774f76e6867
https://hg.mozilla.org/mozilla-central/rev/30a518ac406f