Bug 24733 - Video controller doesn't respond to clicks after full-page-zoom
Summary: Video controller doesn't respond to clicks after full-page-zoom
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar
Depends on:
Reported: 2009-03-20 19:25 PDT by Simon Fraser (smfr)
Modified: 2009-03-23 13:40 PDT (History)
1 user (show)

See Also:

Testcase (356 bytes, text/html)
2009-03-20 20:00 PDT, Simon Fraser (smfr)
no flags Details
Patch, changelog (17.52 KB, patch)
2009-03-20 22:44 PDT, Simon Fraser (smfr)
darin: review-
Details | Formatted Diff | Diff
Revised patch (18.31 KB, patch)
2009-03-21 15:09 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2009-03-23 12:25 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch with testcase (10.04 KB, patch)
2009-03-23 13:32 PDT, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-03-20 19:25:03 PDT
After zooming in, the built-in video controller doesn't respond to clicks any more.
Comment 1 Simon Fraser (smfr) 2009-03-20 20:00:59 PDT
Created attachment 28816 [details]
Comment 2 Simon Fraser (smfr) 2009-03-20 22:44:27 PDT
Created attachment 28818 [details]
Patch, changelog
Comment 3 Darin Adler 2009-03-21 07:23:30 PDT
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.
Comment 4 Simon Fraser (smfr) 2009-03-21 15:09:49 PDT
Created attachment 28825 [details]
Revised patch
Comment 5 Darin Adler 2009-03-21 18:36:57 PDT
Comment on attachment 28825 [details]
Revised patch

Comment 6 Simon Fraser (smfr) 2009-03-22 11:03:07 PDT
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.
Comment 7 Simon Fraser (smfr) 2009-03-22 19:54:41 PDT
Comment 8 Simon Fraser (smfr) 2009-03-23 12:25:58 PDT
Created attachment 28861 [details]
Comment 9 Simon Fraser (smfr) 2009-03-23 13:32:18 PDT
Created attachment 28864 [details]
Patch with testcase
Comment 10 Antti Koivisto 2009-03-23 13:36:45 PDT
Comment on attachment 28864 [details]
Patch with testcase

Comment 11 Simon Fraser (smfr) 2009-03-23 13:40:18 PDT