Bug 50660 – Convert <progress> shadow DOM to a DOM-based shadow.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

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

[Hajime Morrita]

(In reply to comment #0)
> <progress> is holding its shadows as a member variables of  RenderProgress. 
> We could move them to HTMLMeterElement.
No, to HTMLProgressElement.

[Hajime Morrita]

Created attachment 80424 [details]
Patch

[Hajime Morrita]

Hi Dimitri, could you take a look?
I know this is just the first step of the shadow refresh and
I'll eliminate more renderer-to-dom dependency in following changes.

[Dimitri Glazkov (Google)]

Comment on attachment 80424 [details]
Patch

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

Ok. You probably should've put this patch into a separate bug, since it doesn't technically fix the bug yet -- and use this bug as a meta-bug :)

> Source/WebCore/html/HTMLProgressElement.cpp:40
> +class ProgressBarValueElement : public ShadowBlockElement {

Does it need to be ShadowBlockElement anymore? But I guess that's what the next patch is about.

[Hajime Morrita]

Committed r77107: <http://trac.webkit.org/changeset/77107>

[Hajime Morrita]

HI Dimitri, Thank you for reviewing!

(In reply to comment #4)
> (From update of attachment 80424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80424&action=review
> 
> Ok. You probably should've put this patch into a separate bug, since it doesn't technically fix the bug yet -- and use this bug as a meta-bug :)
Sure. I'll do so ... or introduce another bug as a tracking bug.

> 
> > Source/WebCore/html/HTMLProgressElement.cpp:40
> > +class ProgressBarValueElement : public ShadowBlockElement {
> 
> Does it need to be ShadowBlockElement anymore? But I guess that's what the next patch is about.
I'll do this when attacking <meter>. But I have a few item on <progress> side yet.

[Simon Fraser (smfr)]

I see some new test crashes after this commit:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r77108%20(27420)/results.html

[Simon Fraser (smfr)]

Patch was backed out via bug 53412.

[Hajime Morrita]

Created attachment 80629 [details]
Patch

[Hajime Morrita]

Simon, thank you for save the tree. I'm sorry for the breakage.
Dimitri, could you take a look again?

The difference from the last patch is ProgressBarValueElement::detach().
The last patch missed to unlink from host -> shadow reference, which caused the inconsistency.
(shadow -> host reference is unlinked by ShadowElement::detach()).

Thanks in advance.

[Dimitri Glazkov (Google)]

Comment on attachment 80629 [details]
Patch

Please make sure to run tests before landing.

[Hajime Morrita]

> Please make sure to run tests before landing.
Yes, I will. I'm really sorry for that.
The test was done only under dom/HTMLProgressElement/ at the last patch
and I was wrong.

[Hajime Morrita]

Committed r77211: <http://trac.webkit.org/changeset/77211>

[Erik Arvidsson]

Comment on attachment 80629 [details]
Patch

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

> Source/WebCore/rendering/RenderProgress.cpp:105
> +    if (!(valuePart()->renderer() && valuePart()->renderer()->style()->hasAppearance()))

Why does this need appearance? What happens if I do?

::-webkit-progress-bar-value {
  -webkit-appearance: none;
  background-color: red;
}

[Hajime Morrita]

(In reply to comment #14)
> (From update of attachment 80629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80629&action=review
> 
> > Source/WebCore/rendering/RenderProgress.cpp:105
> > +    if (!(valuePart()->renderer() && valuePart()->renderer()->style()->hasAppearance()))
> 
> Why does this need appearance? What happens if I do?
> 
> ::-webkit-progress-bar-value {
>   -webkit-appearance: none;
>   background-color: red;
> }
Shadow rendered even if "none" is given if host's appearance is set to "none".
But yes, it's confusing.... I'll revise the logic in coming patch(es).