Summary: | Convert <progress> shadow DOM to a DOM-based shadow. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dglazkov, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 53412 | ||||||||
Bug Blocks: | 48698, 53417 | ||||||||
Attachments: |
|
Description
Hajime Morrita
2010-12-07 18:21:11 PST
(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. Created attachment 80424 [details]
Patch
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. 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. Committed r77107: <http://trac.webkit.org/changeset/77107> 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. I see some new test crashes after this commit: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r77108%20(27420)/results.html Created attachment 80629 [details]
Patch
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. Comment on attachment 80629 [details]
Patch
Please make sure to run tests before landing.
> 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.
Committed r77211: <http://trac.webkit.org/changeset/77211> 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; } (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). |