Last Comment Bug 657953 - Implement the basic layout of the meter element
: Implement the basic layout of the meter element
Status: RESOLVED FIXED
[mentor=volkmar]
: dev-doc-needed, html5, student-project
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Vincent LAMOTTE
:
Mentors:
Depends on: 657938 659248
Blocks: 555985 663367 660238 662251
  Show dependency treegraph
 
Reported: 2011-05-18 09:16 PDT by Vincent LAMOTTE
Modified: 2012-06-06 06:19 PDT (History)
10 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (48.79 KB, patch)
2011-05-24 15:49 PDT, Vincent LAMOTTE
no flags Details | Diff | Review
Patch v1.1 (17.85 KB, patch)
2011-05-25 10:24 PDT, Vincent LAMOTTE
no flags Details | Diff | Review
Patch v1.2 (19.90 KB, patch)
2011-05-26 07:20 PDT, Vincent LAMOTTE
no flags Details | Diff | Review
Patch v1.3 (19.97 KB, patch)
2011-05-26 10:38 PDT, Vincent LAMOTTE
no flags Details | Diff | Review
Patch v1.4 (20.04 KB, patch)
2011-05-27 06:20 PDT, Vincent LAMOTTE
no flags Details | Diff | Review
Patch v1.5 (20.00 KB, patch)
2011-05-27 07:57 PDT, Vincent LAMOTTE
roc: review+
Details | Diff | Review
Test v1.0 (73.45 KB, patch)
2011-05-30 07:12 PDT, Vincent LAMOTTE
roc: review+
Details | Diff | Review
Patch v1.6 (20.02 KB, patch)
2011-05-31 09:38 PDT, Vincent LAMOTTE
no flags Details | Diff | Review

Description Vincent LAMOTTE 2011-05-18 09:16:20 PDT
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 2011-05-24 15:49:17 PDT
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.
Comment 2 :Ms2ger 2011-05-25 03:59:03 PDT
Looks like this patch also includes the DOM code.
Comment 3 Mounir Lamouri (:mounir) 2011-05-25 06:09:29 PDT
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 2011-05-25 10:24:09 PDT
Created attachment 535115 [details] [diff] [review]
Patch v1.1

- Removing dom/content part from the patch
- Taking Mounir's comment into account
Comment 5 :Ms2ger 2011-05-25 12:37:47 PDT
You're adding a tab in layout/generic/nsQueryFrame.h. Please don't do that.
Comment 6 Vincent LAMOTTE 2011-05-26 07:20:05 PDT
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.
Comment 7 Mounir Lamouri (:mounir) 2011-05-26 08:44:49 PDT
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) 2011-05-26 08:46:13 PDT
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 2011-05-26 10:38:55 PDT
Created attachment 535399 [details] [diff] [review]
Patch v1.3

Taking Mounir's comment into account.
Comment 10 Mounir Lamouri (:mounir) 2011-05-27 03:28:03 PDT
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 2011-05-27 06:20:24 PDT
Created attachment 535616 [details] [diff] [review]
Patch v1.4

Minor changes
Comment 12 Vincent LAMOTTE 2011-05-27 07:57:28 PDT
Created attachment 535636 [details] [diff] [review]
Patch v1.5

Basic layout "appears" to work.
Advance tests are coming soon.
Comment 13 Vincent LAMOTTE 2011-05-30 07:12:18 PDT
Created attachment 536087 [details] [diff] [review]
Test v1.0

Tests for the basic layout.
Based on reftests of the progress element.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-30 15:59:53 PDT
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) (Exited; email my personal email if necessary) 2011-05-30 16:00:37 PDT
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) (Exited; email my personal email if necessary) 2011-05-30 16:02:02 PDT
Comment on attachment 536087 [details] [diff] [review]
Test v1.0

Review of attachment 536087 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 17 Mounir Lamouri (:mounir) 2011-05-31 05:38:53 PDT
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 2011-05-31 09:38:41 PDT
Created attachment 536324 [details] [diff] [review]
Patch v1.6

:roc & :volkmar changes.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-18 20:58:25 PDT
Hmm, you probably need to implement GetMinWidth and GetPrefWidth for meters.
Comment 20 Mounir Lamouri (:mounir) 2011-09-18 22:31:19 PDT
I've open a bug for that. I have a patch but I think I forgot to attach it. Will do that.

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