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

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

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

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

- Removing tabs / windows ends of line (thanks to Ms2ger) - Adding some forgotten stuff (about constructors) No going on some tests.

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.

Taking Mounir's comment into account.

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.

Minor changes

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

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

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 {

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

Comment on attachment 536087 [details] [diff] [review] Test v1.0 Review of attachment 536087 [details] [diff] [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

:roc & :volkmar changes.

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