<meter> should allow styling using CSS like <progress> does. It should have pseudo classes to - specify a style for the gauge level like optimum/suboptimal/even-less-good, and - specify a style for the value part and the background(bar) part
Created attachment 58109 [details] Mock HTML (In reply to comment #0) > <meter> should allow styling using CSS like <progress> does. > It should have pseudo classes to > - specify a style for the gauge level like optimum/suboptimal/even-less-good, and > - specify a style for the value part and the background(bar) part I think we should have 8 pseudo classes. - horizontal background - horizontal optimum value - horizontal suboptimal value - horizontal even-less-good value - vertical background - vertical optimum value - vertical suboptimal value - vertical even-less-good value
Created attachment 58632 [details] patch v0
Comment on attachment 58632 [details] patch v0 Initial comments: - Please update the base WebKit revision. Style bot and EWS are really helpful. - The default style of <meter> is not attractive :-)
Comment on attachment 58632 [details] patch v0 LayoutTests/ChangeLog:10 + - Disabled pixel tests for HTMLMeterElement is temporality disabled. This sentence looks curious. Maybe either of "disabled" is not needed? typo: temporality -> temporarily LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:13 + window.setTimeout(function(evt) { What's the intention of this setTimeout()? LayoutTests/platform/qt/Skipped:3289 + fast/dom/HTMLMeterElement/meter-element.html Because Qt has ENABLE_METER_TAG, we don't need to skip the test. we can: - Commit this change - Qt buildbot runs - Pick the Qt test result - Commit it WebCore/WebCore.xcodeproj/project.pbxproj:21781 + A7AA66D611C5ED6A001D8C8C /* RenderIndicator.cpp in Sources */, Should be sorted. WebCore/rendering/RenderMeter.cpp:66 + return IntRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), lround(width() - borderLeft() - paddingLeft() - borderRight() - paddingRight()), height() - borderTop() - paddingTop() - borderBottom() - paddingBottom()); We should have a test for this. e.g. meter { border-left-width: 100px; border-right-width: 1px; } and the meter value is just a half. WebCore/rendering/RenderTheme.cpp:236 + bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const Can we merge this into RenderTheme::isControlStyled()?
Created attachment 58843 [details] fix
Comment on attachment 58843 [details] fix I missed some point from the last review. will re-submit.
(In reply to comment #3) > (From update of attachment 58632 [details]) > Initial comments: > - Please update the base WebKit revision. Style bot and EWS are really helpful. Ah, I remember svn-apply doesn't support git binary diff. So the style bot and EWS don't work for your patch. Please make sure your patch doesn't have style errors before uploading.
Created attachment 58853 [details] patch v2
Hi Kent-san, thank you for reviewing! I updated the patch. (In reply to comment #4) > (From update of attachment 58632 [details]) > LayoutTests/ChangeLog:10 > + - Disabled pixel tests for HTMLMeterElement is temporality disabled. > This sentence looks curious. Maybe either of "disabled" is not needed? > typo: temporality -> temporarily Done (removed). > > > LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:13 > + window.setTimeout(function(evt) { > What's the intention of this setTimeout()? Removed. On writing it, I'd like to make sure that rendering is done, though I realized that it's useless. > > > LayoutTests/platform/qt/Skipped:3289 > + fast/dom/HTMLMeterElement/meter-element.html > Because Qt has ENABLE_METER_TAG, we don't need to skip the test. we can: > - Commit this change > - Qt buildbot runs > - Pick the Qt test result > - Commit it OK, remove the line from Skipped file. > > > WebCore/WebCore.xcodeproj/project.pbxproj:21781 > + A7AA66D611C5ED6A001D8C8C /* RenderIndicator.cpp in Sources */, > Should be sorted. Oops. I'll sort it before landing because sorting tend to cause a conflict. > WebCore/rendering/RenderMeter.cpp:66 > + return IntRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), lround(width() - borderLeft() - paddingLeft() - borderRight() - paddingRight()), height() - borderTop() - paddingTop() - borderBottom() - paddingBottom()); > We should have a test for this. e.g. > meter { > border-left-width: 100px; > border-right-width: 1px; > } > and the meter value is just a half. Added a test to meter-style.html. > WebCore/rendering/RenderTheme.cpp:236 > + bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const > Can we merge this into RenderTheme::isControlStyled()? No, It cannot. But I agree that canPaintControlsFor() is over-generalized. So I removed it and added supportsHorizontalMeter() instead.
(In reply to comment #9) > > WebCore/rendering/RenderTheme.cpp:236 > > + bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const > > Can we merge this into RenderTheme::isControlStyled()? > No, It cannot. > But I agree that canPaintControlsFor() is over-generalized. > So I removed it and added supportsHorizontalMeter() instead. Could you explain the reason why it can't?
(In reply to comment #10) > (In reply to comment #9) > > > WebCore/rendering/RenderTheme.cpp:236 > > > + bool RenderTheme::canPaintControlsFor(RenderObject* renderObject) const > > > Can we merge this into RenderTheme::isControlStyled()? > > > No, It cannot. > > But I agree that canPaintControlsFor() is over-generalized. > > So I removed it and added supportsHorizontalMeter() instead. > > Could you explain the reason why it can't? - We need a RenderObject to refer the layout result, but isControlStyled() doesn't have it. Because it is called before RenderObjects creation. - isControlStyle() requires extra arguments like BorderData which are not available for us. - RenderTheme::adjustStyle() rewrites RenderStyle::m_appearance based on the result of isControlStyled(). But we want to preserve the original appearance value always for <meter>. After all, isControlStyled() is based on the specified style, canPaintControlsFor() is based on the layout result.
(In reply to comment #11) > > Could you explain the reason why it can't? > > - We need a RenderObject to refer the layout result, but isControlStyled() doesn't have it. > Because it is called before RenderObjects creation. > - isControlStyle() requires extra arguments like BorderData which are not available for us. > - RenderTheme::adjustStyle() rewrites RenderStyle::m_appearance based on the result of isControlStyled(). > But we want to preserve the original appearance value always for <meter>. > > After all, isControlStyled() is based on the specified style, > canPaintControlsFor() is based on the layout result. I see. <meter> always has width/height information in its style, but they may be relative. e.g. height:50%. So we can't detect horizontal/vertical in adjustStyle()-isContrlStyled().
Comment on attachment 58853 [details] patch v2 LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:8 + window.setTimeout(function(evt) { Need a comment why setTimeou() is used. Do you want to confirm the value change updates the element style? if so, .offsetWidth may help to force repainting. document.getElementById("target1").offsetWidth; document.getElementById("target1").value = 90; ... document.getElementById("target1").offsetWidth; LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:19 + meter.styled { -webkit-appearance: none; } WebKit should reset -webkit-appearance automatically if an element has user-specified style, like <progress>. Ideally, if #id1::-webkit-meter-horizonta-bar { background-color: white; } is specified, -webkit-appearance for <meter id=id1> should be reset automatically. However, I don't have no idea how to implement it. WebCore/rendering/RenderIndicator.cpp:22 + #include "RenderIndicator.h" We had better enclose the code with #if ENABLE(PROGRESS_TAG) || ENABLE(METER_TAG) WebCore/rendering/RenderIndicator.h:24 + #if ENABLE(PROGRESS_TAG) ditto. WebCore/rendering/RenderIndicator.h:29 + //class ShadowBlockElement; This line should be removed. WebCore/rendering/RenderTheme.h:198 + virtual bool supportsHorizontalMeter() const { return false; } supportsHorizontalMeter() should have a ControlPart parameter because a platform might support just one or two of the three indicator appearances.
(In reply to comment #13) > However, I don't have no idea how to implement it. I meant "I don't have good idea how to implement it."
Created attachment 58962 [details] patch v3
Hi Kent-san, thank you for reviewing again! I updated the patch. > LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:8 > + window.setTimeout(function(evt) { > Need a comment why setTimeou() is used. > > Do you want to confirm the value change updates the element style? > if so, .offsetWidth may help to force repainting. Fixed to remove setTimeout() and force repaint by touching style object. > LayoutTests/fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html:19 > + meter.styled { -webkit-appearance: none; } > WebKit should reset -webkit-appearance automatically if an element has user-specified style, like <progress>. > Ideally, if > #id1::-webkit-meter-horizonta-bar { background-color: white; } > is specified, -webkit-appearance for <meter id=id1> should be reset automatically. Done. tests are also updated. > WebCore/rendering/RenderIndicator.cpp:22 > + #include "RenderIndicator.h" > We had better enclose the code with #if ENABLE(PROGRESS_TAG) || ENABLE(METER_TAG) Done. > WebCore/rendering/RenderIndicator.h:24 > + #if ENABLE(PROGRESS_TAG) > ditto. Doe. > WebCore/rendering/RenderIndicator.h:29 > + //class ShadowBlockElement; > This line should be removed. Done. > WebCore/rendering/RenderTheme.h:198 > + virtual bool supportsHorizontalMeter() const { return false; } > supportsHorizontalMeter() should have a ControlPart parameter because a platform might support just one or two of the three indicator appearances. Done.
Comment on attachment 58962 [details] patch v3 Almost OK. WebCore/rendering/RenderTheme.h:198 + virtual bool supportsHorizontalMeter(ControlPart) const { return false; } Other platforms might support themes for vertical meters. So supportsHorizontalMeter(ControlPart) should be supportsMeter(ControlPart, bool isHorizontal) WebCore/rendering/ShadowElement.cpp:96 + // No need this empty comment line WebCore/rendering/ShadowElement.cpp:101 + // ditto.
Created attachment 59055 [details] patch v4
Hi, thank you for reviewing! I updated the patch: > WebCore/rendering/RenderTheme.h:198 > + virtual bool supportsHorizontalMeter(ControlPart) const { return false; } > Other platforms might support themes for vertical meters. > So supportsHorizontalMeter(ControlPart) should be supportsMeter(ControlPart, bool isHorizontal) Done. > WebCore/rendering/ShadowElement.cpp:96 > + // > No need this empty comment line Done. > WebCore/rendering/ShadowElement.cpp:101 > + // > ditto. Done.
Comment on attachment 59055 [details] patch v4 Looks OK. Please commit this when the bots status are good. Watch test bots results and correct test expectations if problems occur. See [1] for Chromium tests. Sort project.pbxproj later. [1] http://build.chromium.org/buildbot/waterfall.fyi/waterfall?branch=&builder=Webkit+(webkit.org)&builder=Webkit+Linux+(webkit.org)&builder=Webkit+Mac+(webkit.org)&builder=Webkit.org+Builder
Committed r61376: <http://trac.webkit.org/changeset/61376>
http://trac.webkit.org/changeset/61376 might have broken Qt Linux Release
This broke a test on Gtk.
> This broke a test on Gtk. Which one? I couldn't figure out that. It seems a successive checkin overrides the waterfall status. I'm watching the tree a while, anyway.