Bug 44907 - Refactor input[type=range] to use a more natural shadow DOM model.
Summary: Refactor input[type=range] to use a more natural shadow DOM model.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 46015 46595 48697 50971 50972 50973 52317 52464 53050
Blocks: 25648 48698 52937
  Show dependency treegraph
 
Reported: 2010-08-30 16:00 PDT by Dimitri Glazkov (Google)
Modified: 2011-04-30 09:27 PDT (History)
14 users (show)

See Also:


Attachments
Patch (39.23 KB, patch)
2010-08-30 16:01 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP: A bit more progress, still not done. (40.50 KB, patch)
2010-09-03 09:19 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Nearly there (55.88 KB, patch)
2010-09-10 16:37 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Capture state before breaking up into smaller patches (56.91 KB, patch)
2010-09-14 10:06 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP: Reconstruct based on prior patches. (40.92 KB, patch)
2010-12-10 15:06 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
WIP: Putting all pieces together for the slider. (20.97 KB, patch)
2011-01-11 16:41 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2010-08-30 16:00:58 PDT
Refactor input[type=range] to use a more natural shadow DOM model.
Comment 1 Dimitri Glazkov (Google) 2010-08-30 16:01:27 PDT
Created attachment 65974 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2010-08-30 16:05:25 PDT
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.
Comment 3 Dimitri Glazkov (Google) 2010-08-30 16:06:17 PDT
WDYT?
Comment 4 Adam Barth 2010-08-30 16:07:05 PDT
> 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"  :)
Comment 5 Dimitri Glazkov (Google) 2010-09-03 09:19:47 PDT
Created attachment 66508 [details]
WIP: A bit more progress, still not done.
Comment 6 Dimitri Glazkov (Google) 2010-09-10 16:37:01 PDT
Created attachment 67262 [details]
Nearly there
Comment 7 Dimitri Glazkov (Google) 2010-09-10 16:39:45 PDT
(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?
Comment 8 Darin Adler 2010-09-13 09:57:37 PDT
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.
Comment 9 Dimitri Glazkov (Google) 2010-09-13 13:18:41 PDT
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.
Comment 10 Dimitri Glazkov (Google) 2010-09-14 10:06:37 PDT
Created attachment 67571 [details]
Capture state before breaking up into smaller patches
Comment 11 Dimitri Glazkov (Google) 2010-09-14 10:10:26 PDT
(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.
Comment 12 Eric Seidel (no email) 2010-11-16 10:37:13 PST
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.)
Comment 13 Dimitri Glazkov (Google) 2010-11-16 11:43:42 PST
(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 :)
Comment 14 Dimitri Glazkov (Google) 2010-12-10 15:06:35 PST
Created attachment 76266 [details]
WIP: Reconstruct based on prior patches.
Comment 15 Dominic Cooney 2010-12-20 22:19:57 PST
I filed <https://bugs.webkit.org/show_bug.cgi?id=51379> for cleaning up keygen’s option elements.
Comment 16 Dimitri Glazkov (Google) 2011-01-11 16:41:44 PST
Created attachment 78623 [details]
WIP: Putting all pieces together for the slider.
Comment 17 Dimitri Glazkov (Google) 2011-04-13 10:34:23 PDT
The slider now uses new shadow DOM.