Bug 40217 – [Mac] <meter> elements should be rendered as level indicators.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Hajime Morrita]

Cocoa has NSLevelIndicator component, and <meter> should be rendered as that component on Mac.

[Hajime Morrita]

Created attachment 57993 [details]
patch v0

[Kent Tamura]

Comment on attachment 57993 [details]
patch v0

Would you split the patch, please?  I think "enabling ENABLE_METER_TAG on Mac" can be another patch.


WebCore/rendering/RenderMeter.h:36
 +      double min() const;
RenderTheme* may access HTMLMeterElement.  So these functions are not needed in RenderMeter.


WebCore/rendering/RenderThemeMac.mm:834
 +  RenderThemeMac::LevelType RenderThemeMac::levelTypeFor(const RenderMeter* renderMeter) const
This operation is not Mac-specific.  LevelType symbols should be HTML5-like (optimum, suboptimal, even-less-good) and the function should be in HTMLMeterElement.

[Hajime Morrita]

Created attachment 57997 [details]
patch v1

[Hajime Morrita]

Hi Kent-san, Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 57993 [details])
> Would you split the patch, please?  I think "enabling ENABLE_METER_TAG on Mac" can be another patch.
OK, I have splitted it out to Bug 40219.

> WebCore/rendering/RenderMeter.h:36
>  +      double min() const;
> RenderTheme* may access HTMLMeterElement.  So these functions are not needed in RenderMeter.
Fixed.

> WebCore/rendering/RenderThemeMac.mm:834
>  +  RenderThemeMac::LevelType RenderThemeMac::levelTypeFor(const RenderMeter* renderMeter) const
> This operation is not Mac-specific.  LevelType symbols should be HTML5-like (optimum, suboptimal, even-less-good) and the function should be in HTMLMeterElement.
Fixed.

[Kent Tamura]

Comment on attachment 57997 [details]
patch v1

Please add expectations for
 * fast/dom/HTMLMeterElement/meter-element.html
 * fast/dom/HTMLMeterElement/set-meter-properties.html
They are in platform/mac/Skipped for now.

If the height of a meter is larger than its width, WebKit with this patch draws ugly NSLevelIndicator, right?


LayoutTests/ChangeLog:1
 +  2010-06-06  MORITA Hajime  <[e-mail]>
This diff format will make webkit-patch work incorrectly.
We had better change the date.


LayoutTests/ChangeLog:5
 +          https://bugs.webkit.org/show_bug.cgi?id=40217
 +          [Mac] <meter> elements should be rendered as level indicators.
We usually write the 1-line description first, the bug URL then.


WebCore/rendering/RenderThemeMac.h:210
 +      NSLevelIndicatorStyle levelIndicatorStyleFor(ControlPart part) const;
The argument name "part" is not needed.


WebCore/rendering/RenderThemeMac.h:211
 +      NSLevelIndicatorCell* levelFor(const RenderMeter*) const;
nit: To call NSLevelIndicator(Cell) "level" is not so descriptive.  I prefer "levelIndicatorFor".


WebCore/rendering/RenderThemeMac.h:220
 +      mutable RetainPtr<NSLevelIndicatorCell> m_level;
ditto.  I prefer m_levelIndicator.


WebCore/rendering/RenderThemeMac.mm:865
 +      // we explictly control the color intead giving low and high value to NSLevelIndicatorCell as is.
typo: instead


WebCore/rendering/RenderThemeMac.mm:869
 +          [cell setWarningValue:value+1];
Add spaces around "+".

[Hajime Morrita]

Created attachment 58004 [details]
patch v2

[Hajime Morrita]

Hi Kent-san, thank you for reviewing again!

(In reply to comment #5)
> (From update of attachment 57997 [details])
> Please add expectations for
>  * fast/dom/HTMLMeterElement/meter-element.html
>  * fast/dom/HTMLMeterElement/set-meter-properties.html
> They are in platform/mac/Skipped for now.
> 
> If the height of a meter is larger than its width, WebKit with this patch draws ugly NSLevelIndicator, right?
Yes. It will draw tall NSLevelIndicator. 
I'd like to note that discrete-capacity indicator and rating indicator has constant height regardless
of css property. There are blank space with around these controls in that case.

> LayoutTests/ChangeLog:1
>  +  2010-06-06  MORITA Hajime  <[e-mail]>
> This diff format will make webkit-patch work incorrectly.
> We had better change the date.
Done.

> LayoutTests/ChangeLog:5
>  +          https://bugs.webkit.org/show_bug.cgi?id=40217
>  +          [Mac] <meter> elements should be rendered as level indicators.
> We usually write the 1-line description first, the bug URL then.
Done.

> 
> WebCore/rendering/RenderThemeMac.h:210
>  +      NSLevelIndicatorStyle levelIndicatorStyleFor(ControlPart part) const;
> The argument name "part" is not needed.
Done.

> WebCore/rendering/RenderThemeMac.h:211
>  +      NSLevelIndicatorCell* levelFor(const RenderMeter*) const;
> nit: To call NSLevelIndicator(Cell) "level" is not so descriptive.  I prefer "levelIndicatorFor".
Done.

> WebCore/rendering/RenderThemeMac.h:220
>  +      mutable RetainPtr<NSLevelIndicatorCell> m_level;
> ditto.  I prefer m_levelIndicator.
Done.

> WebCore/rendering/RenderThemeMac.mm:865
>  +      // we explictly control the color intead giving low and high value to NSLevelIndicatorCell as is.
> typo: instead
Done.

> WebCore/rendering/RenderThemeMac.mm:869
>  +          [cell setWarningValue:value+1];
> Add spaces around "+".
Done.

[Kent Tamura]

(In reply to comment #7)
> Hi Kent-san, thank you for reviewing again!
> 
> (In reply to comment #5)
> > (From update of attachment 57997 [details] [details])
> > Please add expectations for
> >  * fast/dom/HTMLMeterElement/meter-element.html
> >  * fast/dom/HTMLMeterElement/set-meter-properties.html
> > They are in platform/mac/Skipped for now.

No comments for this?

> > If the height of a meter is larger than its width, WebKit with this patch draws ugly NSLevelIndicator, right?
> Yes. It will draw tall NSLevelIndicator. 
> I'd like to note that discrete-capacity indicator and rating indicator has constant height regardless
> of css property. There are blank space with around these controls in that case.

So, what's your plan for vertical meters?
We need to support vertical meters.

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-meter-element-0
> When the element is taller than it is wide, it is expected to depict a vertical gauge,
>  with the minimum value on the bottom.

[Hajime Morrita]

Created attachment 58098 [details]
patch v3

[Hajime Morrita]

Hi, Kent-san, thank you for reviewing again!

> > (In reply to comment #5)
> > > (From update of attachment 57997 [details] [details] [details])
> > > Please add expectations for
> > >  * fast/dom/HTMLMeterElement/meter-element.html
> > >  * fast/dom/HTMLMeterElement/set-meter-properties.html
> > > They are in platform/mac/Skipped for now.
> 
> No comments for this?

Oops. I just missed it - Added expectations.
I also removed <meter> related tests from the mac skip list.
Because I'm going to resolve Bug 40219 before this one.


> So, what's your plan for vertical meters?
> We need to support vertical meters.

Hmm. So how about to use RenderTheme::paintMeter() to draw vertical gauses?
Updated patch does such.

[Kent Tamura]

Comment on attachment 58098 [details]
patch v3

(In reply to comment #10)
> > So, what's your plan for vertical meters?
> > We need to support vertical meters.
> 
> Hmm. So how about to use RenderTheme::paintMeter() to draw vertical gauses?

That's good.
Please file a bug about distinctive painting of optimum/suboptimal/even-less-good in RenderTheme::paintMeter().

[Hajime Morrita]

> Please file a bug about distinctive painting of optimum/suboptimal/even-less-good in RenderTheme::paintMeter().
OK, I've filed it to Bug 40280

[Hajime Morrita]

Committed r60822: <http://trac.webkit.org/changeset/60822>