Bug 24733 – Video controller doesn't respond to clicks after full-page-zoom

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

After zooming in, the built-in video controller doesn't respond to clicks any more.

[Simon Fraser (smfr)]

Created attachment 28816 [details]
Testcase

[Simon Fraser (smfr)]

Created attachment 28818 [details]
Patch, changelog

[Darin Adler]

Comment on attachment 28818 [details]
Patch, changelog

Great solution!

I’m not a big fan of the word “actual” here in the name of the new function, but I don’t have a better suggestion. I also don’t think “page” was great naming either, but that came straight from the DOM specification. The idea here is that it’s the point in the document’s view’s coordinate system, I think. Maybe that could lead to a better name?

I think Node::dispatchWheelEvent needs the code to handle this properly for wheel events. I don’t see anything in there dealing with zoom factor though, so I’m concerned it might already be wrong.

I think the code in MouseEvent::computePageLocation should be in MouseRelatedEvent, not MouseEvent.

> +    Frame* frame = view()->frame();
> +    if (frame) {
> +        float zoomFactor = frame->pageZoomFactor();
> +        setActualPageLocation(roundedIntPoint(FloatPoint(pageX() * zoomFactor, pageY() * zoomFactor)));
> +    }

Is it really OK to leave the page location as 0,0 if there’s no frame? Maybe it should instead act as if the zoom factor is 1 in that case instead of leaving it 0,0. I also think initCoordinates should set it to the same value it sets m_pageX and m_pageY to.

> +        // Page point in pixels, independent of zoom factor. c.f. pageX, pageY.

I don’t think “in pixels” is helpful in this comment.

> +    int adjustedPageX = pageX, adjustedPageY = pageY;

We normally don’t define two variables on a single line, even in a simple case like this one.

> +        if (evt->isMouseEvent() && evt->type() == eventNames().mousedownEvent && static_cast<MouseEvent*>(evt)->button() == LeftButton)
> +            slider->handleLeftMouseDownEvent(static_cast<MouseEvent*>(evt));

When I was doing some cleanup of slider in my own patch, one I probably won’t land at this time, I moved this code into RenderSlider’s forwardEvent function, because I think this qualifies as forwarding just as much as the mechanical forwarding. You should consider doing that rather than adding a new function.

> -    IntPoint point = IntPoint(mouseEvent->pageX(), mouseEvent->pageY());
> -    HitTestResult result(point);
> +    HitTestResult result(mouseEvent->actualPageLocation());
>  
> -    if (Frame* frame = event->target()->toNode()->document()->frame()) {
> -        float zoomFactor = frame->pageZoomFactor();
> -        point.setX(static_cast<int>(point.x() * zoomFactor));
> -        point.setY(static_cast<int>(point.y() * zoomFactor));
> -        result = frame->eventHandler()->hitTestResultAtPoint(point, false);
> -    }
> +    if (Frame* frame = event->target()->toNode()->document()->frame())
> +        result = frame->eventHandler()->hitTestResultAtPoint(mouseEvent->actualPageLocation(), false);

I think this is going to have an unwanted behavior change in the frame == 0 case so you should consider my suggestion above.

There are enough things that could be improved here that I’ll say review- for now.

[Simon Fraser (smfr)]

Created attachment 28825 [details]
Revised patch

[Darin Adler]

Comment on attachment 28825 [details]
Revised patch

r=me

[Simon Fraser (smfr)]

Comment on attachment 28825 [details]
Revised patch

Patch landed in http://trac.webkit.org/changeset/41899

There's more to do on the video controls shadow content, so keeping bug open.

[Simon Fraser (smfr)]

<rdar://problem/6706994>

[Simon Fraser (smfr)]

Created attachment 28861 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 28864 [details]
Patch with testcase

[Antti Koivisto]

Comment on attachment 28864 [details]
Patch with testcase

r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/41918