Bug 44907 – Refactor input[type=range] to use a more natural shadow DOM model.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Refactor input[type=range] to use a more natural shadow DOM model.

[Dimitri Glazkov (Google)]

Created attachment 65974 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 65974 [details]
Patch

This is work in progress, not yet ready for review.

I looked at the way we hang a DOM node off of RenderObjects for our shadow DOM implementation, and decided it was evil.

So I started cooking a way to fix this. This (incomplete) patch highlights the approach: make shadow DOM nodes part of the standard DOM node lifecycle, and take advantage of the already existing DOM/layout functionality, thus avoiding things like forwardEvent, updateFromElement, etc.

[Dimitri Glazkov (Google)]

WDYT?

[Adam Barth]

> This is work in progress, not yet ready for review.

webkit-patch upload --no-review -m "work in progress"

That will avoid setting the review flag and name the patch "work in progress"  :)

[Dimitri Glazkov (Google)]

Created attachment 66508 [details]
WIP: A bit more progress, still not done.

[Dimitri Glazkov (Google)]

Created attachment 67262 [details]
Nearly there

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> Created an attachment (id=67262) [details]
> Nearly there

I swear this patch is going to grow only a tiny bit more. Things left to do:
* moving new classes out to separate files
* twiddling a bit more with event retargeting code to make things pretty
* writing a long-ass ChangeLog.

Can you please take a look and tell me if anything sticks out?

[Darin Adler]

Comment on attachment 67262 [details]
Nearly there

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

Looking good. No need for you to do this all in one big patch, though. You can break off bits and land them first.

> WebCore/css/CSSStyleSelector.cpp:1198
> +    // Check to see if an element wants to start style from a pre-defined pseudoId.
> +    // FIXME: This is horrid, because we resolve the style twice.
> +    RenderStyle* initialStyle = 0;
> +    if (e->stylePseudoId() != NOPSEUDO) {
> +        Node* parent = e->parentOrHostNode();
> +        // FIXME: This is a giant hack, needed to wire up Media timeline and volume
> +        //        elements.
> +        if (Node* shadowRoot = parent->shadowTreeRootNode())
> +            parent = shadowRoot->shadowParentNode();
> +        if (parent && parent->renderer()) {
> +            initialStyle = parent->renderer()->getCachedPseudoStyle(e->stylePseudoId());
> +            // Re-initialize m_element and m_styledElement that were dirtied by getCachedPseudoStyle call.
> +            // FIXME: Remove transient CSSStyleSelector members to eliminate the need
> +            // for this fixup.
> +            initElement(e);
> +        }
> +    }
> +    if (initialStyle)
> +        m_style = RenderStyle::clone(initialStyle);
> +    else
> +        m_style = RenderStyle::create();
Can you refactor this into a function?

> WebCore/css/mediaControls.css:93
> +    /* Unfortunately, a more appropriate setting of appearance is hard-coded in RenderSliderThumb::layout(),
> +       but we need to have an appearance of some sort to start painting. */
This comment is confusing. Maybe there’s a clearer way to word it.

> WebCore/dom/ContainerNode.cpp:1000
> +    Node* parentOrHostNode = c->parentOrHostNode();
> +    if (parentOrHostNode && parentOrHostNode->inDocument())
This seems like the kind of change we could land sooner in a separate round to make the final patch smaller.

> WebCore/dom/Element.cpp:793
> +    if (Node* shadowNode = shadow())
> +        shadowNode->insertedIntoDocument();
I’m worried that all of these calls will result in a noticeable slowdown. We won’t be able to land it like that!

> WebCore/dom/Element.h:311
> +    // FIXME: Make non-virtual.
> +    virtual Node* shadow();
I’d like to hear more about this; how it can be made non-virtual.

> WebCore/dom/Node.cpp:119
> +// FIXME: Move to a separate file.
> +EventAncestor::EventAncestor(ContainerNode* ancestor, Node* target)
It would be good to land this whole EventAncestor machinery first, separately. In fact, you could start by just creating the separate source file and then adding this class.

> WebCore/html/HTMLInputElement.cpp:2549
> -    if (inputType() == RANGE && renderer() && (evt->isMouseEvent() || evt->isDragEvent() || evt->isWheelEvent()))
> -        toRenderSlider(renderer())->forwardEvent(evt);
> +    if (inputType() == RANGE && renderer() && evt->isMouseEvent()) {
> +        MouseEvent* mouseEvent = static_cast<MouseEvent*>(evt);
> +        if (evt->type() == eventNames().mousedownEvent && mouseEvent->button() == LeftButton) {
> +            SliderThumbElement* thumb = toSliderThumbElement(shadow());
> +            ASSERT(thumb);
> +            thumb->dragFrom(mouseEvent->absoluteLocation());
> +            evt->setDefaultHandled();
> +        }
> +    }
This seems a little weak. Moving the specific code for SliderThumbElement here is a step backwards in factoring, so why do it?

> WebCore/html/HTMLInputElement.cpp:2976
> +// FIXME: Once most of the shadow DOM is handled this way, we should move this
> +//        to Element.
Please don’t break up these comments like this. A slightly longer line would be better.

A nice way to do this is might be to put this into ElementRareData.

> WebCore/html/HTMLInputElement.cpp:2983
> +    static ShadowRootMap* s_map = 0;
> +    if (!s_map)
> +        s_map = new ShadowRootMap();
> +    return s_map;
You should write this like this:

    static ShadowRootMap* s_map = new ShadowRootMap;
    return s_map;

No need for that if statement.

[Dimitri Glazkov (Google)]

Comment on attachment 67262 [details]
Nearly there

Thanks for looking at this, Darin! The comments below make sense when viewed as Formatted Diff. I replied to all yours :)

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

> WebCore/css/CSSStyleSelector.cpp:1198
> +        m_style = RenderStyle::create();
Sure thing. This would also make it more clear about which part of this needs to be rewritten away.

> WebCore/css/mediaControls.css:93
> +       but we need to have an appearance of some sort to start painting. */
Will do.

> WebCore/dom/Element.cpp:793
> +        shadowNode->insertedIntoDocument();
I can measure performance if you'd like, either Dromaeo or microbench it.

> WebCore/dom/Element.h:311
> +    virtual Node* shadow();
If the shadow static map (from below) is moved from HTMLInputElement to Element, then we don't need this to be virtual.

> WebCore/dom/Node.cpp:119
> +EventAncestor::EventAncestor(ContainerNode* ancestor, Node* target)
Sounds like a plan, I'll split up the patch.

> WebCore/html/HTMLInputElement.cpp:2549
> +    }
No, no, this is much better! :) At least, I think so -- all we're doing is getting the thumb and telling it to start dragging. It's nice and clean, no forwarding events or passing them through the render objects.

> WebCore/html/HTMLInputElement.cpp:2976
> +//        to Element.
Sorry :) Will fix.

> WebCore/html/HTMLInputElement.cpp:2983
> +    return s_map;
DOH.

[Dimitri Glazkov (Google)]

Created attachment 67571 [details]
Capture state before breaking up into smaller patches

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> Created an attachment (id=67571) [details]
> Capture state before breaking up into smaller patches

Now passes all tests. I'll be breaking out event ancestor stuff first, then we'll see what's left. In other news -- I killed eventParentNode! That's one of the hottest virtual calls in the event handling code.

[Eric Seidel (no email)]

If you're into fixing shadow doms... <keygen> needs a shadow dom to hold its <option> elements.  Right now they're just added to the DOM directly, which causes us to skip the html5 parsing tests on all platforms other than mac (since the current expected DOM is the one which appears for Mac.)

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> If you're into fixing shadow doms... <keygen> needs a shadow dom to hold its <option> elements.  Right now they're just added to the DOM directly, which causes us to skip the html5 parsing tests on all platforms other than mac (since the current expected DOM is the one which appears for Mac.)

This seems like a good thing to fix :)

[Dimitri Glazkov (Google)]

Created attachment 76266 [details]
WIP: Reconstruct based on prior patches.

[Dominic Cooney]

I filed <https://bugs.webkit.org/show_bug.cgi?id=51379> for cleaning up keygen’s option elements.

[Dimitri Glazkov (Google)]

Created attachment 78623 [details]
WIP: Putting all pieces together for the slider.

[Dimitri Glazkov (Google)]

The slider now uses new shadow DOM.