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+

Simon Fraser (smfr)
Reported 2009-03-20 19:25:03 PDT
After zooming in, the built-in video controller doesn't respond to clicks any more.
Attachments
Testcase (356 bytes, text/html)
2009-03-20 20:00 PDT, Simon Fraser (smfr)
no flags
Patch, changelog (17.52 KB, patch)
2009-03-20 22:44 PDT, Simon Fraser (smfr)
darin: review-
Revised patch (18.31 KB, patch)
2009-03-21 15:09 PDT, Simon Fraser (smfr)
darin: review+
Patch (7.81 KB, patch)
2009-03-23 12:25 PDT, Simon Fraser (smfr)
no flags
Patch with testcase (10.04 KB, patch)
2009-03-23 13:32 PDT, Simon Fraser (smfr)
koivisto: review+
Simon Fraser (smfr)
Comment 1 2009-03-20 20:00:59 PDT
Created attachment 28816 [details] Testcase
Simon Fraser (smfr)
Comment 2 2009-03-20 22:44:27 PDT
Created attachment 28818 [details] Patch, changelog
Darin Adler
Comment 3 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.
Simon Fraser (smfr)
Comment 4 2009-03-21 15:09:49 PDT
Created attachment 28825 [details] Revised patch
Darin Adler
Comment 5 2009-03-21 18:36:57 PDT
Comment on attachment 28825 [details] Revised patch r=me
Simon Fraser (smfr)
Comment 6 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.
Simon Fraser (smfr)
Comment 7 2009-03-22 19:54:41 PDT
Simon Fraser (smfr)
Comment 8 2009-03-23 12:25:58 PDT
Simon Fraser (smfr)
Comment 9 2009-03-23 13:32:18 PDT
Created attachment 28864 [details] Patch with testcase
Antti Koivisto
Comment 10 2009-03-23 13:36:45 PDT
Comment on attachment 28864 [details] Patch with testcase r=me
Simon Fraser (smfr)
Comment 11 2009-03-23 13:40:18 PDT
Note You need to log in before you can comment on or make changes to this bug.