Bug 40217 - [Mac] <meter> elements should be rendered as level indicators.
Summary: [Mac] <meter> elements should be rendered as level indicators.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 37074 40219
  Show dependency treegraph
 
Reported: 2010-06-06 19:24 PDT by Hajime Morrita
Modified: 2010-06-07 23:38 PDT (History)
2 users (show)

See Also:


Attachments
patch v0 (249.93 KB, patch)
2010-06-06 22:18 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1 (238.79 KB, patch)
2010-06-07 00:06 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (238.39 KB, patch)
2010-06-07 02:40 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v3 (244.29 KB, patch)
2010-06-07 18:52 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-06-06 19:24:05 PDT
Cocoa has NSLevelIndicator component, and <meter> should be rendered as that component on Mac.
Comment 1 Hajime Morrita 2010-06-06 22:18:32 PDT
Created attachment 57993 [details]
patch v0
Comment 2 Kent Tamura 2010-06-06 22:50:11 PDT
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.
Comment 3 Hajime Morrita 2010-06-07 00:06:46 PDT
Created attachment 57997 [details]
patch v1
Comment 4 Hajime Morrita 2010-06-07 00:08:15 PDT
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.
Comment 5 Kent Tamura 2010-06-07 00:51:45 PDT
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  <morrita@google.com>
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 "+".
Comment 6 Hajime Morrita 2010-06-07 02:40:41 PDT
Created attachment 58004 [details]
patch v2
Comment 7 Hajime Morrita 2010-06-07 02:46:01 PDT
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  <morrita@google.com>
> 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.
Comment 8 Kent Tamura 2010-06-07 02:52:25 PDT
(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.
Comment 9 Hajime Morrita 2010-06-07 18:52:56 PDT
Created attachment 58098 [details]
patch v3
Comment 10 Hajime Morrita 2010-06-07 18:57:12 PDT
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.
Comment 11 Kent Tamura 2010-06-07 19:10:35 PDT
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().
Comment 12 Hajime Morrita 2010-06-07 21:25:32 PDT
> Please file a bug about distinctive painting of optimum/suboptimal/even-less-good in RenderTheme::paintMeter().
OK, I've filed it to Bug 40280
Comment 13 Hajime Morrita 2010-06-07 23:26:39 PDT
Committed r60822: <http://trac.webkit.org/changeset/60822>