Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15040:
Any mouse click selects a drop down list item, when it shouldn't... (Closed)
|
Created:
12 years ago by Mohamed Mansour (USE mhm) Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAny mouse click selects a drop down list item, when it shouldn't
BUG=5634
(http://crbug.com/5634)
TEST=Tested both mouse clicks to check which one will select the item.
Patch Set 1 #
Total comments: 1
Patch Set 2 : '' #
Total comments: 2
Messages
Total messages: 28 (0 generated)
updated reviewers
(added one more reviewer that might know this stuff) Reviewers: I haven't tested the patch, but the requested behavior would match IE and Firefox on Windows. Dunno if this is the right place to fix or not.
> Dunno if this is the right place to fix or not. The method that executes after that is accepting the index (and only the index is passed to that, no way to find out what mouse event is triggered). I tested it and it works, but don't know if it works for everyone.
I'm not too familiar with this code, but this seems fine to me. Ideally we would show the regular right-click menu in this case, giving access to useful things like "Inspect Element". Do we? On 2008/12/18 18:06:39, Mohamed Mansour wrote: > > Dunno if this is the right place to fix or not. > > The method that executes after that is accepting the index (and only the index > is passed to that, no way to find out what mouse event is triggered). I tested > it and it works, but don't know if it works for everyone.
http://codereview.chromium.org/15040/diff/1/2 File webkit/port/platform/chromium/PopupMenuChromium.cpp (right): http://codereview.chromium.org/15040/diff/1/2#newcode550 Line 550: if (event.button() != LeftButton) What happens if you right-click the scrollbar? It seems like m_capturingScrollbar will never get reset. You might need this check in handleMouseDownEvent as well.
If I added many elements in the drop down, the scrollbar cannot be right clicked. But I did what you stated, placed it after first condition, as well as handleMouseDownEvent
http://codereview.chromium.org/15040/diff/206/6 File webkit/port/platform/chromium/PopupMenuChromium.cpp (right): http://codereview.chromium.org/15040/diff/206/6#newcode515 Line 515: if (event.button() != LeftButton) Since this is before the "abandon" step, does right-clicking outside the select no longer dismiss the select? I think it should.
http://codereview.chromium.org/15040/diff/206/6 File webkit/port/platform/chromium/PopupMenuChromium.cpp (right): http://codereview.chromium.org/15040/diff/206/6#newcode515 Line 515: if (event.button() != LeftButton) On 2008/12/18 22:34:35, Matt Perry wrote: > Since this is before the "abandon" step, does right-clicking outside the select > no longer dismiss the select? I think it should. If it's moved to after the abandon() step, it will have no effect. I don't think any check is needed in this function.
Alright, I am really lost what is needed now? Do we want to allow right clicking on the scrollbar? What is the problem exactly? The only difference that I see from this patch and without it is that right click is now disabled for that dropdown (popup)
Try the following with your latest patch: 1. open a select dropdown 2. right click outside the dropdown Does the dropdown dismiss? It should. If so, then we're good.
Yes, the dropdown dismisses once I right clicked outside. I tested the following: http://www.w3schools.com/html/tryit.asp?filename=tryhtml_select2
On 2008/12/19 02:09:35, Mohamed Mansour wrote: > Yes, the dropdown dismisses once I right clicked outside. I tested the > following: > http://www.w3schools.com/html/tryit.asp?filename=tryhtml_select2 Interesting. That suggests the if statement that does the abandon() is not necessary. In any case, LGTM.
handleMouseReleaseEvent and handleMouseDownEvent always returns true for isPointInBounds(event.pos()), BUT handleMouseMoveEvent captures it, so if I mouse out of that popup area, it knows its out of bounds. So should we actually remove those if statements for handleMouseDownEvent and handleMouseReleaseEvent ?
On 2008/12/19 02:11:50, Matt Perry wrote: > On 2008/12/19 02:09:35, Mohamed Mansour wrote: > > Yes, the dropdown dismisses once I right clicked outside. I tested the > > following: > > http://www.w3schools.com/html/tryit.asp?filename=tryhtml_select2 > > Interesting. That suggests the if statement that does the abandon() is not > necessary. > > In any case, LGTM. I don't think the popup menu will get a mousedown event if the click is not already within the bounds of the widget (some cursory tests on windows and linux appear to confirm this). I suspect we can go ahead and remove the isPointInBounds check from the mouse down handler.
On 2008/12/19 02:49:15, Mohamed Mansour wrote: > handleMouseReleaseEvent and handleMouseDownEvent always returns true for > isPointInBounds(event.pos()), BUT handleMouseMoveEvent captures it, so if I > mouse out of that popup area, it knows its out of bounds. > > So should we actually remove those if statements for handleMouseDownEvent and > handleMouseReleaseEvent ? Ah. What about this: 1. Open select dropdown. 2. Left-click and hold somewhere in the dropdown. 3. Drag somewhere out of the select. 4. Right-click (while still holding left-click). The dropdown should close. I believe that's the point of those if statements. What happens with your patch? Sorry for all the back and forth. The behavior here is pretty subtle.
No worries, I will do that check tonight, I had to redownload the whole source due to some failures at my end. I will let you know about the results tonight.
before anyone commits this, make sure you've tested it on all 3 platforms.
On 2008/12/19 18:55:58, Matt Perry wrote: > Ah. What about this: > 1. Open select dropdown. > 2. Left-click and hold somewhere in the dropdown. > 3. Drag somewhere out of the select. > 4. Right-click (while still holding left-click). > > The dropdown should close. I believe that's the point of those if statements. > What happens with your patch? It works fine, the popup closes as expected (on windows xp sp3) machine. I don't have other platforms to test.
On 2008/12/20 00:21:32, Mohamed Mansour wrote: > On 2008/12/19 18:55:58, Matt Perry wrote: > > Ah. What about this: > > 1. Open select dropdown. > > 2. Left-click and hold somewhere in the dropdown. > > 3. Drag somewhere out of the select. > > 4. Right-click (while still holding left-click). > > > > The dropdown should close. I believe that's the point of those if statements. > > What happens with your patch? > > It works fine, the popup closes as expected (on windows xp sp3) machine. > > I don't have other platforms to test. OK, then it sounds like the if statements aren't needed. Your patch is fine as is. I'd leave the (supposedly) useless if statements there, but if you're feeling brave/confident, you can take them out.
> OK, then it sounds like the if statements aren't needed. Your patch is fine as > is. I'd leave the (supposedly) useless if statements there, but if you're > feeling brave/confident, you can take them out. We could just keep them, not much processing power I guess :) BUT while testing this, I found a bug (I don't think its related, well I hope its not...) http://crbug.com/5753 , What do you think?
> We could just keep them, not much processing power I guess :) BUT while testing > this, I found a bug (I don't think its related, well I hope its not...) > http://crbug.com/5753 , What do you think? Its not related, I looked into, and I highly believe it is due to the skia rendering. Many windows are not rendering properly similar to the combobox. So is this LGTM?
No, I think that other platforms besides windows which cannot depend on the WM_ACTIVATE message to get rid of the popup will need this check. You should keep the isInBounds() check in the mousedown function, and should probably move it to the top of that function. e.g. Linux will need this check if http://codereview.chromium.org/14912/show goes through (currently, Linux doesn't abort drop downs at all). I don't know about mac. As a general note, editing code that affects all three platforms requires testing on all three platforms. If you do not have access to mac or linux then it will be the committer's responsibility (unfortunately that might make it slower to happen).
On 2008/12/23 04:01:57, Evan Stade wrote: > As a general note, editing code that affects all three platforms requires > testing on all three platforms. If you do not have access to mac or linux then > it will be the committer's responsibility (unfortunately that might make it > slower to happen). I understand Evan, I only have Linux and Windows, most likely I will buy a mac in late January. If someone else has time to test the patch, then go for it. Else, we could just wrap it around ifdef :x
Could anyone test this patch on mac / linux so we could commit/close it?
On 2009/01/07 04:29:58, Mohamed Mansour wrote: > Could anyone test this patch on mac / linux so we could commit/close it? This seems to work on linux (right clicking/middle clicking doesn't select the entry and the drop down stays open). Be aware that this file has moved to third_party/WebKit/WebCore/platform/chromium/ so you'll have to sync up and do 2 commits. It would also be awesome if you could come up with a layout test for this (might be possible with eventSendingController), but I would just do that in a separate change.
What's the status on this?
On 2009/03/27 18:53:12, pkasting wrote: > What's the status on this? This is no longer a fix in Chromium, it should be upstreamed to webkit since it is part of WebCore/platform/Chromium |
