Bug 50661 – Convert <meter> shadow DOM to a DOM-based shadow.

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

<meter> is holding its shadows as a member variables of  RenderMeter. 
We could move them to HTMLMeterElement.

[Hajime Morrita]

Created attachment 81576 [details]
Patch

[Hajime Morrita]

Hi dimitri, could you take a look?

[Kent Tamura]

Comment on attachment 81576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81576&action=review

Some drive-by comments.

> Source/WebCore/ChangeLog:22
> +        Need a short description and bug URL (OOPS!)

oops!

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22454
> +				0F3DD45012F5EA1B000D9190 /* ShadowBlur.h in Headers */,
> +				BCE4413412F748E2009B84B8 /* RenderCombineText.h in Headers */,
> +				BCE4413612F7490B009B84B8 /* FontWidthVariant.h in Headers */,
> +				14947FFE12F80CD200A0F631 /* DocumentOrderedMap.h in Headers */,
> +				97C471DC12F925BD0086354B /* ContentSecurityPolicy.h in Headers */,
> +				A7FE819C12FA677700850C1E /* ProgressBarValueElement.h in Headers */,

Do you want to add these files?

> Source/WebCore/html/shadow/MeterPartElement.h:53
> +    MeterPartElement(Document* document, PartType type) 

This should be private.

> Source/WebCore/html/shadow/MeterPartElement.h:76
> +inline const AtomicString* MeterPartElement::computeShadowPseudoId()

Do not define such large functions in a header file.

[Dimitri Glazkov (Google)]

Comment on attachment 81576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81576&action=review

I am really worried about the direction this patch has taken. You are introducing more custom layout, style calculation, and even new offsprings of methods we need to get rid of (updateFromElement). Please consider this as a plea to return back to basics. First, we should use attach/detach machinery. Second, I would love to get rid of custom positioning using position:absolute. It's almost like you're working _around_ the RenderBox/RenderBlock layout code, instead of using it to the fullest extent. Third, the MPD-stricken MeterPartElement needs to admit the truth and get split up into the multiple classes, each for one of the many entities it represents. You can do much better -- and I am confident you can do this! :)

> Source/WebCore/ChangeLog:10
> +        Introduced two shadow element class, and eliminte ShadowElement dependency.

"Introduces" and "eliminates".

> Source/WebCore/ChangeLog:11
> +        With these classes, the shadow tree has reshaped from the four-flat-children to four-children-with-a-parent.

is reshaped

> Source/WebCore/css/html.css:662
> +    position: absolute;

Absolutely positioning seems really gross here. Why are we doing this?

> Source/WebCore/dom/Node.h:571
> +    virtual void updateFromHost() {}

Noooooooo!!!

> Source/WebCore/html/HTMLMeterElement.cpp:75
> +    didElementStateChange();

You should not need this call. attach propagates down to the shadow DOM.

> Source/WebCore/html/HTMLMeterElement.cpp:215
> +void HTMLMeterElement::didElementStateChange()
> +{
> +    if (renderer())
> +        renderer()->updateFromElement();
> +    if (Node* root = shadowRoot()) {
> +        for (Node* node = root; node; node = node->traverseNextNode(root))
> +            node->updateFromHost();
> +    }
> +}

This should be completely unnecessary now.

> Source/WebCore/html/shadow/MeterPartElement.h:85
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdHorizontalValueSuboptimal, ("-webkit-meter-horizontal-suboptimal-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdHorizontalValueEvenLessGood, ("-webkit-meter-horizontal-even-less-good-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalBar, ("-webkit-meter-vertical-bar"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueOptimal, ("-webkit-meter-vertical-optimum-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueSuboptimal, ("-webkit-meter-vertical-suboptimal-value"));
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoIdVerticalValueEvenLessGood, ("-webkit-meter-vertical-even-less-good-value"));

This is gross too. If you want polymorphism, that's what subclassing is for.

[Hajime Morrita]

Kent-san, Dimitri, thank you for reviewing quickly!

> 
> > Source/WebCore/css/html.css:662
> > +    position: absolute;
> 
> Absolutely positioning seems really gross here. Why are we doing this?
I found this cause a tricky crash according to custom layout algorithm on related Render classes.
I'm considering to use normal layout code instead of custom one, with just rewriting RenderStyle values.
The patch will come later.

[Hajime Morrita]

Created attachment 88082 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 88082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88082&action=review

Two concerns:

1) Do we really need filling element? Can't the meter itself act as the filling?
2) creating subtree on attach seems wrong. We'll be adding DOM elements at the time when we're supposed to be populating the render tree.

> LayoutTests/ChangeLog:9
> +        Also updated meter-styles.html pixel result because the render tree is now layouted

layouted -> laid out

> Source/WebCore/ChangeLog:15
> +          -webkit-appeearance is "none" and the platform supports <meter>.

-webkit-appearance

> Source/WebCore/ChangeLog:18
> +          indicators are painted using the platform theme engine as same
> +          old.

as before.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:615
> +bool CSSMutableStyleDeclaration::setProperty(int propertyID, double value, CSSPrimitiveValue::UnitTypes unit, bool important, bool notifyChanged)
> +{
> +    CSSProperty property(propertyID, CSSPrimitiveValue::create(value, unit), important);
> +    setPropertyInternal(property);
> +    if (notifyChanged)
> +        setNeedsStyleRecalc();
> +    return true;
> +}

Do you need this change? Can you not just use

> Source/WebCore/html/shadow/MeterShadowElement.cpp:52
> +    getInlineStyleDecl()->setProperty(CSSPropertyWebkitBoxFlex, flex, CSSPrimitiveValue::CSS_NUMBER);

can be just style()->

[Dimitri Glazkov (Google)]

Here's what I am thinking: http://jsfiddle.net/8hpem/

[Hajime Morrita]

Created attachment 88117 [details]
Patch

[Hajime Morrita]

Hi Dimitri, thank you for lightning feedback!
I updated patch for address yours.


> 1) Do we really need filling element? Can't the meter itself act as the filling?
As you mentioned on the demo, I removed filling and use box-sizing and relative size specification
instead.

> 2) creating subtree on attach seems wrong. We'll be adding DOM elements at the time when we're supposed to be populating the render tree.
Sure. I moved shadow creation timing into HTMLMeterElement::create().

> 
> > LayoutTests/ChangeLog:9
> > +        Also updated meter-styles.html pixel result because the render tree is now layouted
> 
> layouted -> laid out
Fixed.

> 
> > Source/WebCore/ChangeLog:15
> > +          -webkit-appeearance is "none" and the platform supports <meter>.
> 
> -webkit-appearance
Fixed.

> 
> > Source/WebCore/ChangeLog:18
> > +          indicators are painted using the platform theme engine as same
> > +          old.
> 
> as before.
Fixed.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:615
> > +bool CSSMutableStyleDeclaration::setProperty(int propertyID, double value, CSSPrimitiveValue::UnitTypes unit, bool important, bool notifyChanged)
> > +{
> > +    CSSProperty property(propertyID, CSSPrimitiveValue::create(value, unit), important);
> > +    setPropertyInternal(property);
> > +    if (notifyChanged)
> > +        setNeedsStyleRecalc();
> > +    return true;
> > +}
> 
> Do you need this change? Can you not just use
> 
> > Source/WebCore/html/shadow/MeterShadowElement.cpp:52
> > +    getInlineStyleDecl()->setProperty(CSSPropertyWebkitBoxFlex, flex, CSSPrimitiveValue::CSS_NUMBER);
> 
> can be just style()->
Non-mutable CSSStyleDeclaration doesn't have typed property access. It has only string-based API.

[Dimitri Glazkov (Google)]

Comment on attachment 88117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88117&action=review

This is super-close! Thanks for working on this.

> LayoutTests/ChangeLog:10
> +        as normal flexboxes and its layout result is actually different,

This probably needs an update.

> Source/WebCore/html/HTMLMeterElement.cpp:52
> +    RefPtr<HTMLMeterElement> creating = adoptRef(new HTMLMeterElement(tagName, document, form));

creating -> meter

> Source/WebCore/html/HTMLMeterElement.h:61
> +    bool shouldShadowHaveRenderer() const;

You can now use rendererIsNeeded here.

> Source/WebCore/html/HTMLMeterElement.h:74
> +    void setupShadow();

Can we call it createShadowSubtree, for consistency? That's what I called it in InputType.

> Source/WebCore/html/shadow/MeterShadowElement.h:62
> +    static PassRefPtr<MeterBarElement> create(Document* document) { return adoptRef(new MeterBarElement(document)); }

Move to an inline definition right after the class decl. See http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/SliderThumbElement.h&l=76

> Source/WebCore/html/shadow/MeterShadowElement.h:73
> +    static PassRefPtr<MeterValueElement> create(Document* document) { return adoptRef(new MeterValueElement(document)); }

Ditto.

[WebKit Review Bot]

Attachment 88117 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8332101

[Hajime Morrita]

Created attachment 88132 [details]
Patch

[Hajime Morrita]

Dimitri, thank you for another turn!

(In reply to comment #11)
> 
> > LayoutTests/ChangeLog:10
> > +        as normal flexboxes and its layout result is actually different,
> 
> This probably needs an update.
Updated the ChangeLog description.

> 
> > Source/WebCore/html/HTMLMeterElement.cpp:52
> > +    RefPtr<HTMLMeterElement> creating = adoptRef(new HTMLMeterElement(tagName, document, form));
> 
> creating -> meter
renamed.

> 
> > Source/WebCore/html/HTMLMeterElement.h:61
> > +    bool shouldShadowHaveRenderer() const;
> 
> You can now use rendererIsNeeded here.
Good point. I moved this and related code to MeterShadowElement.

> 
> > Source/WebCore/html/HTMLMeterElement.h:74
> > +    void setupShadow();
> 
> Can we call it createShadowSubtree, for consistency? That's what I called it in InputType.
> 
Sure.

> > Source/WebCore/html/shadow/MeterShadowElement.h:62
> > +    static PassRefPtr<MeterBarElement> create(Document* document) { return adoptRef(new MeterBarElement(document)); }
> 
> Move to an inline definition right after the class decl. See http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/SliderThumbElement.h&l=76
> 
> > Source/WebCore/html/shadow/MeterShadowElement.h:73
> > +    static PassRefPtr<MeterValueElement> create(Document* document) { return adoptRef(new MeterValueElement(document)); }
> 
> Ditto.
Done.

[Dimitri Glazkov (Google)]

Comment on attachment 88132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88132&action=review

lovely, with one more tweak:

> Source/WebCore/html/shadow/MeterShadowElement.cpp:60
> +    return meterRenderer && meterRenderer->shouldHaveParts() && HTMLDivElement::rendererIsNeeded(style);

I think you can kill RenderMeter::shouldHaveParts altogether and call RenderTheme here directly:

return document()->page()->theme()->supportsMeter(style->appearance());

[Hajime Morrita]

> I think you can kill RenderMeter::shouldHaveParts altogether and call RenderTheme here directly:
> 
> return document()->page()->theme()->supportsMeter(style->appearance());
Okay, I'll do it before landing. Thanks for the polish!

[Hajime Morrita]

Committed r82880: <http://trac.webkit.org/changeset/82880>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82880 might have broken GTK Linux 32-bit Release

[Adrienne Walker]

Reverted r82880 for reason:

Meter elements not rendered in Chromium Linux tests

Committed r82889: <http://trac.webkit.org/changeset/82889>

[Adrienne Walker]

(In reply to comment #19)
> Reverted r82880 for reason:
> 
> Meter elements not rendered in Chromium Linux tests
> 
> Committed r82889: <http://trac.webkit.org/changeset/82889>

I saw your IRC message that you planned to rebaseline other tests, but the Linux tests looked truly incorrect, so I just rolled this out.  It also looks like the Windows test are wrong as well.

Here are the tests that were failing:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=fast/dom/HTMLMeterElement/meter-appearances-capacity.html,fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html,fast/dom/HTMLMeterElement/meter-boundary-values.html,fast/dom/HTMLMeterElement/meter-element.html,fast/dom/HTMLMeterElement/meter-optimums.html,fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html,fast/dom/HTMLMeterElement/meter-styles.html,fast/forms/input-appearance-spinbutton-disabled-readonly.html,fast/forms/input-appearance-spinbutton-layer.html,fast/forms/input-appearance-spinbutton-visibility.html,fast/speech/input-appearance-numberandspeech.html

[Hajime Morrita]

Created attachment 88161 [details]
My last minute change was wrong...

[Kent Tamura]

Comment on attachment 88161 [details]
My last minute change was wrong...

View in context: https://bugs.webkit.org/attachment.cgi?id=88161&action=review

> Tools/ChangeLog:11
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by Ojan Vafai.
> +
> +        [chromium] don't write .checksum files if a fallback platform has an embedded checksum
> +        https://bugs.webkit.org/show_bug.cgi?id=57783
> +
> +        * Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:
> +        * Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:
> +
>  2011-04-04  Kevin Ollivier  <[e-mail]>

What's this?

[Hajime Morrita]

> 
> What's this?
It looks I did a miserably worng git commit --amend...
I'm sorry for that, will fix shortly.

[Hajime Morrita]

Created attachment 88173 [details]
Patch

[Kent Tamura]

Comment on attachment 88173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review

The difference from r82880 is MeterShadowElement::rendererIsNeeded(), right?

> Source/WebCore/html/HTMLMeterElement.cpp:219
> +    return (value - min) / (max - min);

Is it possible to return Infinity?

[Hajime Morrita]

Kent-san, thank you for our help!

(In reply to comment #25)
> (From update of attachment 88173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review
>
> The difference from r82880 is MeterShadowElement::rendererIsNeeded(), right?
Yes, it should.

> 
> > Source/WebCore/html/HTMLMeterElement.cpp:219
> > +    return (value - min) / (max - min);
> 
> Is it possible to return Infinity?
Such case is guarded by previous statement.

[Kent Tamura]

Comment on attachment 88173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review

>>> Source/WebCore/html/HTMLMeterElement.cpp:219
>>> +    return (value - min) / (max - min);
>> 
>> Is it possible to return Infinity?
> 
> Such case is guarded by previous statement.

Really?
Suppose that max=2.2e-308, min=0, value=1.79e+308

[Kent Tamura]

(In reply to comment #27)
> (From update of attachment 88173 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88173&action=review
> 
> >>> Source/WebCore/html/HTMLMeterElement.cpp:219
> >>> +    return (value - min) / (max - min);
> >> 
> >> Is it possible to return Infinity?
> > 
> > Such case is guarded by previous statement.
> 
> Really?
> Suppose that max=2.2e-308, min=0, value=1.79e+308

Anyway, it's not new code of this patch.  It's ok to commit the patch.

[WebKit Review Bot]

Attachment 88173 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8334143

[Hajime Morrita]

> > Really?
> > Suppose that max=2.2e-308, min=0, value=1.79e+308
> 
value() is clamped by min() and max(). So it should work, in my understanding....

Just in case, I tested against following HTML, and it looks to work.

<html>
  <body>
    <meter min="0" max="2.2e-308" value="1.79e+308">
  <body>
</html>

And build failed... I lost a header inclusion :/

[Hajime Morrita]

> 
> And build failed... I lost a header inclusion :/

I'll fix this and land anyway.  Thank you for your feedback!

[Dimitri Glazkov (Google)]

This patch is cursed!!!

[Hajime Morrita]

Committed r82899: <http://trac.webkit.org/changeset/82899>

[Hajime Morrita]

Pray the commit cures us...

[WebKit Review Bot]

http://trac.webkit.org/changeset/82899 might have broken GTK Linux 32-bit Debug

[Adrienne Walker]

(In reply to comment #35)
> http://trac.webkit.org/changeset/82899 might have broken GTK Linux 32-bit Debug

This patch on its own broke a number of layout tests: 

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=fast/dom/HTMLMeterElement/meter-appearances-capacity.html,fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html,fast/dom/HTMLMeterElement/meter-boundary-values.html,fast/dom/HTMLMeterElement/meter-element.html,fast/dom/HTMLMeterElement/meter-optimums.html,fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html,fast/dom/HTMLMeterElement/meter-styles.html,fast/forms/input-appearance-spinbutton-disabled-readonly.html,fast/forms/input-appearance-spinbutton-layer.html,fast/forms/input-appearance-spinbutton-visibility.html,fast/speech/input-appearance-numberandspeech.html

I marked all of these as failing here: http://trac.webkit.org/changeset/82902/trunk/LayoutTests/platform/chromium/test_expectations.txt

Some of them should just be an easy rebaseline, but I am not sure about why the spinbutton tests were failing from this meter change.  So, I wanted somebody else's eye on it instead of just rebaselining the whole lot of them blindly.

[Kent Tamura]

(In reply to comment #36)
> Some of them should just be an easy rebaseline, but I am not sure about why the spinbutton tests were failing from this meter change.  So, I wanted somebody else's eye on it instead of just rebaselining the whole lot of them blindly.

A change for CSSSelector.cpp unexpectedly removed a case label :-)

[Kent Tamura]

(In reply to comment #37)
> A change for CSSSelector.cpp unexpectedly removed a case label :-)

Fixed: http://trac.webkit.org/changeset/82904

[WebKit Review Bot]

http://trac.webkit.org/changeset/82904 might have broken Qt Linux Release
The following tests are not passing:
fast/ruby/overhang-horizontal.html
fast/ruby/overhang-vertical.html

[Hajime Morrita]

(In reply to comment #38)
> (In reply to comment #37)
> > A change for CSSSelector.cpp unexpectedly removed a case label :-)
> 
> Fixed: http://trac.webkit.org/changeset/82904
OMG!!! thanks for fixing this....

[Kent Tamura]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=meter-element.html%2Cmeter-styles.html&showExpectations=true&showLargeExpectations=true&group=%40ToT%20-%20chromium.org

Chromium results look incorrect.

[Hajime Morrita]

(In reply to comment #41)
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=meter-element.html%2Cmeter-styles.html&showExpectations=true&showLargeExpectations=true&group=%40ToT%20-%20chromium.org
> 
> Chromium results look incorrect.
Woa, I'll take a look. Thank you for noticing this.

[Hajime Morrita]

> > 
> > Chromium results look incorrect.
> Woa, I'll take a look. Thank you for noticing this.
The test is wrong. too-thick borders and margins is hiding content area of meter.
I'll update the test.

[Hajime Morrita]

Will update as Bug 58149.

[Dimitri Glazkov (Google)]

Fixed, right?

[Hajime Morrita]

(In reply to comment #45)
> Fixed, right?
Right, I tend to forget to update metabugs....