Summary: | [Mac] <meter> elements should be rendered as level indicators. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||
Component: | Forms | Assignee: | Hajime Morrita <morrita> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | tkent, yael | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 37074, 40219 | ||||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-06-06 19:24:05 PDT
Created attachment 57993 [details]
patch v0
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.
Created attachment 57997 [details]
patch v1
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 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 "+". Created attachment 58004 [details]
patch v2
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. (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. Created attachment 58098 [details]
patch v3
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 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(). > Please file a bug about distinctive painting of optimum/suboptimal/even-less-good in RenderTheme::paintMeter(). OK, I've filed it to Bug 40280 Committed r60822: <http://trac.webkit.org/changeset/60822> |