Bug 24733

Summary: Video controller doesn't respond to clicks after full-page-zoom
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Testcase
none
Patch, changelog
darin: review-
Revised patch
darin: review+
Patch
none
Patch with testcase koivisto: review+

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]
Testcase
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

r=me
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
<rdar://problem/6706994>
Comment 8 Simon Fraser (smfr) 2009-03-23 12:25:58 PDT
Created attachment 28861 [details]
Patch
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

r=me
Comment 11 Simon Fraser (smfr) 2009-03-23 13:40:18 PDT
http://trac.webkit.org/changeset/41918