Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558113002:
Drop-down closes via tap/touch again. (Closed)
DescriptionDrop-down closes via tap/touch again.
The drop-down list closes on a GestureTapDown, and the next GestureTap
cannot open the same list. The regression was because of the following
cl: https://codereview.chromium.org/2517253002/
BUG=670185, 569092
Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html
Committed: https://crrev.com/59dc4be5b2da772c3994506261f4044713198791
Cr-Commit-Position: refs/heads/master@{#437650}
Patch Set 1 #
Total comments: 11
Patch Set 2 : m_lastCancelledPopUp->m_lastHiddenPopUp #Patch Set 3 : Always reset the m_lastHiddenPopUp at the end of a GestureTap #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Description was changed from ========== Drop-down closes via tap/touch again. The drop-down lists closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html ========== to ========== Drop-down close via tap/touch again. The drop-down lists closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sahel@chromium.org changed reviewers: + bokan@chromium.org
Description was changed from ========== Drop-down close via tap/touch again. The drop-down lists closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html ========== to ========== Drop-down closes via tap/touch again. The drop-down list closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html ==========
Do gesture scrolls and taps inside the dropdown items go to a separate handler? i.e. Will we close the popup if we try to scroll an opened popup? https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:874: m_lastCancelledPagePopup = nullptr; This should be cleared even if we opened a different popup or none at all. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:877: Nit: remove line. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:918: m_client->cancelScheduledContentIntents(); For consistency, add a handleGestureEvent call here, break, and braces. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:925: NOTREACHED(); Nit: add braces here. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.h:673: RefPtr<WebPagePopupImpl> m_lastCancelledPagePopup; This should be called m_lastHiddenPagePopup.
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:874: m_lastCancelledPagePopup = nullptr; On 2016/12/08 16:28:35, bokan wrote: > This should be cleared even if we opened a different popup or none at all. A gestureTapDown will close a popup and store it in the lastHiddenPopUp, (null if no popup was open) there are three possible cases next: a) the next tap will try to open the same popup. This case it's prevented. b) the next tap will open nothing. In this case the next gestureTapDown will set the lastHiddenPopUp to null. The following tap will open a popup if needed. c) the next tap will open another popup. In this case the next gestureTapDown will close the new popup and update the lastHiddenPopUp. In fact, GestureTapDown will always happen before a gestureTap, and update/clear the lastHiddenPopUp. Clearing it in case of the attempt to open the same popup, or a tapCancel is not necessary. I can get rid of them if it makes the code simpler/less confusing. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:877: On 2016/12/08 16:28:35, bokan wrote: > Nit: remove line. Done. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:918: m_client->cancelScheduledContentIntents(); On 2016/12/08 16:28:34, bokan wrote: > For consistency, add a handleGestureEvent call here, break, and braces. Done. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:925: NOTREACHED(); On 2016/12/08 16:28:35, bokan wrote: > Nit: add braces here. Done. https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.h (right): https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.h:673: RefPtr<WebPagePopupImpl> m_lastCancelledPagePopup; On 2016/12/08 16:28:35, bokan wrote: > This should be called m_lastHiddenPagePopup. Done.
https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2558113002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebViewImpl.cpp:874: m_lastCancelledPagePopup = nullptr; On 2016/12/09 15:36:37, sahel wrote: > On 2016/12/08 16:28:35, bokan wrote: > > This should be cleared even if we opened a different popup or none at all. > > A gestureTapDown will close a popup and store it in the lastHiddenPopUp, (null > if no popup was open) there are three possible cases next: > > a) the next tap will try to open the same popup. This case it's prevented. > > b) the next tap will open nothing. In this case the next gestureTapDown will set > the lastHiddenPopUp to null. The following tap will open a popup if needed. > > c) the next tap will open another popup. In this case the next gestureTapDown > will close the new popup and update the lastHiddenPopUp. > > In fact, GestureTapDown will always happen before a gestureTap, and update/clear > the lastHiddenPopUp. Clearing it in case of the attempt to open the same popup, > or a tapCancel is not necessary. I can get rid of them if it makes the code > simpler/less confusing. Yep, I agree the logic is correct. But this value is used only to prevent the GestureTap from opening the menu hidden by its associated GestureTapDown. Once GestureTap is done you don't need it anymore so it seems more clear and less bug prone to reset it here. As code moves around and changes over time, it's better not to leave around dangling values unless you actually need them.
lgtm modulo my last comment though
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2558113002/#ps60001 (title: "Always reset the m_lastHiddenPopUp at the end of a GestureTap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481315539633040,
"parent_rev": "7298d9fab661df8db0afc95aef205a9ef7e4bd97", "commit_rev":
"f47102d5c0e2e49347c0e4970d334c99bdb77f6d"}
Message was sent while issue was closed.
Description was changed from ========== Drop-down closes via tap/touch again. The drop-down list closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html ========== to ========== Drop-down closes via tap/touch again. The drop-down list closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html Review-Url: https://codereview.chromium.org/2558113002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Drop-down closes via tap/touch again. The drop-down list closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html Review-Url: https://codereview.chromium.org/2558113002 ========== to ========== Drop-down closes via tap/touch again. The drop-down list closes on a GestureTapDown, and the next GestureTap cannot open the same list. The regression was because of the following cl: https://codereview.chromium.org/2517253002/ BUG=670185, 569092 Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html Committed: https://crrev.com/59dc4be5b2da772c3994506261f4044713198791 Cr-Commit-Position: refs/heads/master@{#437650} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/59dc4be5b2da772c3994506261f4044713198791 Cr-Commit-Position: refs/heads/master@{#437650} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
